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

Update prysm.sh to include slasher and sig verify #5543

Merged
merged 38 commits into from
Apr 23, 2020

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Apr 20, 2020

Key changes:

  • Only download the binary for the process you are attempting to run.
  • Add slasher option
  • Verify sha256 of binary
  • Verify signature of binary

prestonvanloon and others added 26 commits April 20, 2020 11:15
@prestonvanloon prestonvanloon changed the title prysm.sh - Add gpg detached signature checks Update prysm.sh to include slasher and sig verify Apr 22, 2020
@Yoldark34
Copy link

Hello, is it normal that gpg is telling the RSA key "0AE0051D647BA3C1A917AF4072E33E4DF1A5036E" is not public ?

I had to do "gpg --recv-keys 0AE0051D647BA3C1A917AF4072E33E4DF1A5036E". If it's normal it may be usefull to ask the key before trying to check the signature or at least display the command the user has to run in order to get the missing key.

Best regards.

@Yoldark34
Copy link

Not that is my business but IMO you should have made 2 separate pullRequest for the commit c2bfe24, you are adding two separate functionality in one PR.

@prestonvanloon
Copy link
Member Author

@Yoldark34 Thanks for the feedback. In general, I think that these features are relatively small in scope and shouldn't be much of a burden on reviewers to review in a single PR. With that said, I probably still should have split this into multiple PRs as I have listed 4 key changes. If a reviewer feels strongly about this, I can split it up.

@prestonvanloon prestonvanloon marked this pull request as ready for review April 22, 2020 18:55
@prestonvanloon prestonvanloon requested a review from a team as a code owner April 22, 2020 18:55
@farazdagi
Copy link
Contributor

farazdagi commented Apr 22, 2020

I am getting "sha256sum is not available. Not verifying integrity of downloaded binary." error, am I expected to download sth manually?

image

@Yoldark34
Copy link

Yoldark34 commented Apr 22, 2020

I don't think it is a good idea for a script to install third party software so, yes, you have to install it yourself

@prestonvanloon is it possible to ask Y/N question prior to launch the program without the checks if there is any possible ? just to be sure people are aware of launching unverified program, and also prevent them to launch it if they don't want.

@prestonvanloon
Copy link
Member Author

Sounds like mac has shasum installed by default, we can use shasum -a 256 do achieve the same goal.

With regards to the public key, I think i'll just make it part of the prysm.sh file so it doesn't need to retrieve from a key server, which could be offline or compromised.

@Yoldark34, I'll give it a try. If Y/N input is too difficult to do, maybe we can refuse to run an unverified binary unless there is SKIP_PRYSM_VERIFY=1 or something along those lines that requires user input to skip a failed verification.

with the specific version, as desired. Example: USE_PRYSM_VERSION=v1.0.0-alpha.5 \
If you must wish to continue running an unverified binary, specific the \
environment variable PRYSM_ALLOW_UNVERIFIED_BINARIES=1"
exit 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice thanks :)

Copy link
Contributor

@farazdagi farazdagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-tested locally, works ok

@rauljordan rauljordan merged commit a6a2ad4 into master Apr 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the verify-sig-prysm-sh branch April 23, 2020 14:57
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