Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SHA256 signatures of the install script #8312

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

josegomezr
Copy link
Contributor

@josegomezr josegomezr commented Sep 7, 2023

Proposed Changes

Add SHA256 signatures of the install script.

Types of Changes

  • Security fix

Verification

sha256sum -c install.sh.sha256sum 

Testing

  • No functional change of k3s is needed.

Linked Issues

User-Facing Change

No functional change

- Add SHA256 signatures of the install script.

Further Comments

  • Allow install script verification. Downloading artifacts directly from 3rd parties in internet without verification poses a security issue. It's currently impossible to verify the integrity of the installer unless fetching it directly from GitHub. Please provide these signatures alongside the installer and make it available through get.k3s.io too.

@josegomezr josegomezr requested a review from a team as a code owner September 7, 2023 13:28
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Patch coverage has no change and project coverage change: +1.33% 🎉

Comparison is base (0d23cfe) 47.61% compared to head (7a0b206) 48.94%.
Report is 2 commits behind head on master.

❗ Current head 7a0b206 differs from pull request most recent head 7781b3d. Consider uploading reports for the commit 7781b3d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8312      +/-   ##
==========================================
+ Coverage   47.61%   48.94%   +1.33%     
==========================================
  Files         143      140       -3     
  Lines       14732    11817    -2915     
==========================================
- Hits         7014     5784    -1230     
+ Misses       6623     4921    -1702     
- Partials     1095     1112      +17     
Flag Coverage Δ
e2etests 48.94% <ø> (?)
inttests ?
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 131 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brandond
Copy link
Member

brandond commented Sep 7, 2023

I'm on the fence about this, as anyone running a curl | bash install script has already given up on security, but I guess this doesn't make things any worse.

Please add a test to the validate script to confirm that the checksums match the install script; to avoid accidentally updating the script without touching the checksums.

@josegomezr
Copy link
Contributor Author

@brandond Please have a look. I've added invocations to sha{256,384} in the ./scripts/validate script, I understood that the script is reached by all CI. Please correct me if I'm wrong.

And may I ask, how can the signatures be part of the release artifacts? what should I edit to have them there too?


As a side note:

As anyone running a curl | bash install script has already given up on security.

I partially agree, I see the comfort for a PoC on an isolated machine to do curl | sh and get your cluster up and running, but for a production setting a secure option should be advertised too so end users are aware.

It might be silly (and I'm not an expert in security at all) but I like the simple way composer.org offers to use their install guide:

image

Yes, you're downloading things from the wild internet, but you get to verify the integrity of the script with what the docs are telling you (a lot of oversimplification here, I grant that).

Another side-side note, a quick scan over the repo shows: that technique (curl pipe sh) is present in:

  • Dokerfile.dapper:

https://github.com/k3s-io/k3s/blob/af50e1b09687367979537b835505307a341be818/Dockerfile.dapper#L45C1-L47

  • Dokerfile.local:

    k3s/Dockerfile.local

    Lines 26 to 28 in af50e1b

    RUN if [ "$(go env GOARCH)" = "amd64" ]; then \
    curl -sL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s v1.51.2; \
    fi

in a Vagrantfile:

They don't seem to be a problem now, but are definitely potential attack surfaces, they should be closed in as time permits.

@brandond
Copy link
Member

They don't seem to be a problem now, but are definitely potential attack surfaces, they should be closed in as time permits.

That's actually the documented install method for both projects:

@josegomezr
Copy link
Contributor Author

josegomezr commented Sep 15, 2023

That's actually the documented install method for both projects:

They do are. That doesn't necessarily means it's a "safe" way to install, it's the same curl | sh technique without any sort of check.

I think we're both on the same page: the problem itself is not running sh, but rather not being able to verify that the downloaded script is supposed to be the correct script (and if possible, comes from the actual origin, but that needs GPG or something like that).

And what I'd like to achieve is to have that very minimum thing, a way to validate the integrity of the script that's running as the root user of a system and performing changes to the system.*


Tailscale offers packaged RPM's & DEB's with GPG signatures, so at least is possible to verify the package (which is done automagically by dnf/zypper/apt/etc).

And golangci-lint has a recommendation in their own install guide:

image

IMPORTANT: It's highly recommended installing a specific version of golangci-lint available on the releases page.

But then below has a snippet of code curl | sh-ing without verification 😅

Nevertheless, when following the releases page link, there's a checksums file to verify the integrity of the artifact downloaded.

^*: mistyped enter when writing the message, sorry for the noise.

scripts/validate Outdated Show resolved Hide resolved
- SHA256 Signature of the install script
- Added a sha256sum invocations in the validate script.

  These calls will validate that the install script signatures
  match. And when the script is changed the signatures must be
  recalculated as reported by the error message in sha256sum.

Signed-off-by: Jose D. Gomez R <[email protected]>
@josegomezr josegomezr changed the title Add SHA256/SHA384 signatures of the install script Add SHA256 signatures of the install script Sep 26, 2023
@josegomezr
Copy link
Contributor Author

Updated the title and description to match the current state (only sha256 signatures).

@brandond
Copy link
Member

how can the signatures be part of the release artifacts

Just to close the loop on this - they will not be release artifacts. The install script is served directly off the master branch; it is not part of the built release artifacts. If someone wants to use the checksum, they'll need to pull it down from github.

@foursixnine
Copy link

foursixnine commented Sep 26, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants