Skip to content

Conversation

@PiotrSikora
Copy link

@PiotrSikora PiotrSikora commented Oct 29, 2020

Fixes #15.

Signed-off-by: Piotr Sikora code@piotrsikora.dev

@PiotrSikora
Copy link
Author

@philwo PTAL

Fixes bazelbuild#15.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Author

Sample output:

2020/10/29 22:54:04 Downloading https://releases.bazel.build/3.7.0/release/bazel-3.7.0-linux-x86_64...
2020/10/29 22:54:06 Signed by Bazel Developer (Bazel APT repository key) <bazel-dev@googlegroups.com>

@philwo
Copy link
Member

philwo commented Nov 9, 2020

Thank you @PiotrSikora! I was on vacation - will look into this PR this week!

@PiotrSikora
Copy link
Author

@philwo friendly ping.

@philwo
Copy link
Member

philwo commented Dec 7, 2020

This looks really nice, thank you! It's great that we don't need to call gnupg to verify the signature and the code is very clean.

The only thing I'm wondering - what will happen when the embedded key expires? In the past this meant we had to extend the key and then everyone had to reimport the extended one into their GnuPG keychain (see https://groups.google.com/g/bazel-discuss/c/XzeKUSkMCDk/m/GiOj6ariEgAJ for some former discussion).

Would it mean that older Bazelisk releases will suddenly fail to fetch Bazel releases until we update the embedded key and users update to a newer version? 🤔

@PiotrSikora
Copy link
Author

The only thing I'm wondering - what will happen when the embedded key expires? In the past this meant we had to extend the key and then everyone had to reimport the extended one into their GnuPG keychain (see https://groups.google.com/g/bazel-discuss/c/XzeKUSkMCDk/m/GiOj6ariEgAJ for some former discussion).

Would it mean that older Bazelisk releases will suddenly fail to fetch Bazel releases until we update the embedded key and users update to a newer version? 🤔

Effectively, yes. They could always update Bazelisk to get the new key, or you could embed multiple public keys and do a rolling update, but that requires knowing the "next" key ahead of time.

A bit longer solution would be to redesign the signing infrastructure to use subkeys for signatures and rotate it periodically (see: https://www.gnupg.org/gph/en/manual/c481.html) and have the primary key without expiration date. It doesn't really serve anybody if the primary signing key is changed every year or two.

@philwo
Copy link
Member

philwo commented Dec 8, 2020

Thanks for the explanation!

Unfortunately we're deep in "I have no idea what I'm actually doing here, except copy & pasting various GnuPG command-lines from the internet and hope they will fix whatever is broken" territory when it comes to our code signing stuff and every time I have to extend the lifetime of the key, I just hope it doesn't break everything. 😬

I'll read up about GnuPG best practices in the next weeks, something I always wanted to do. If it's OK with you, I'd leave this PR open just a bit longer, while I figure out how we want to handle our signing key - from the code and feature side, this is very much great and ready to merge imho :) Thank you for this!

@PiotrSikora
Copy link
Author

Yeah, no rush.

@PiotrSikora
Copy link
Author

@philwo friendly ping 😄

@PiotrSikora
Copy link
Author

@philwo ping.

@jlisee
Copy link

jlisee commented Dec 17, 2021

As independent verification I rebased this patch against v.11.0 and it works:

2021/12/17 21:45:09 Downloading https://releases.bazel.build/4.2.2/rc1/bazel-4.2.2rc1-linux-x86_64...                                                                                                                                                                                                                         
2021/12/17 21:45:10 Signed by Bazel Developer (Bazel APT repository key) <bazel-dev@googlegroups.com>

The only real change was getClient().Get(signatureURL) to get(signatureUrl, "").

…signature

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Author

@philwo @fweikert could you take a look at this?

}

if len(keys) != 1 {
return "", fmt.Errorf("failed to load the embedded Verification Key")
Copy link

Choose a reason for hiding this comment

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

Thanks for putting this together, @PiotrSikora! We'd love to leverage this too.

Three ideas for handling an expired embedded key:

  1. Add an error message on failure (bonus points for specifically looking if the key is expir,ed) pointing users to look for a new release since the embedded verification key may be expired.
  2. Add a --no-verify flag to skip verification entirely 😅
  3. Add a --verification-signature-file flag as a "break glass" method to use a different key, which folks can use to pull from a fork.

Before adding those workarounds, it'd likely be better to avoid the complexity if possible! If there's anyone who won't be able to upgrade bazelisk to get a new key, it'd be great to hear about that use case! The only actual caveat I can think of is remembering to upgrade this tool every N years with the new key, but that's well worth the tradeoff to me! 🎉

@DemiMarie
Copy link

Could the verification key be replaced with one that does not expire?

@meteorcloudy meteorcloudy added the P3 We're not considering working on this, but happy to review a PR. (No assignee) label Jan 21, 2025
@PiotrSikora
Copy link
Author

@philwo @fweikert @meteorcloudy any chance this feature could be supported in one form or another? It still bothers me every time I see Bazelisk downloading another version of Bazel...

@meteorcloudy
Copy link
Member

I'm sorry, I'm afraid the team doesn't have capacity to review and support this feature for now.

Copy link
Member

@philwo philwo left a comment

Choose a reason for hiding this comment

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

I had a look at the code and think it's fine overall. My main worry would be about how we handle an expired key. It would be great if Bazelisk could then at least still verify Bazel versions that were released before the key expired. However, this doesn't seem easily possible with GnuPG-based code signing due to the lack of support for a trusted timestamping service.

That prompted me to check how long the current public key is still valid:

$ wget https://bazel.build/bazel-release.pub.gpg
$ gpg --show-keys bazel-release.pub.gpg 
pub   rsa4096 2016-05-24 [SC]
      71A1D0EFCFEB6281FD0437C93D5919B448457EE0
uid                      Bazel Developer (Bazel APT repository key) <bazel-dev@googlegroups.com>
sub   rsa4096 2016-05-24 [E]

Seems like it doesn't even have an expiry date? So, we could probably safely add this proposed verification and it should continue to work fine forever.

We would need to change the outdated embedded public key to the current one, though.

nonCandidateBaseURL = "https://storage.googleapis.com/bazel-builds/artifacts"
lastGreenBaseURL = "https://storage.googleapis.com/bazel-untrusted-builds/last_green_commit/"
verificationKey = `
-----BEGIN PGP PUBLIC KEY BLOCK-----
Copy link
Member

Choose a reason for hiding this comment

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

FYI, this key is the old one and expired in 2022. I saved this embedded public key into a file and checked it like this:

$ gpg --show-keys bazelkey.gpg 
pub   rsa4096 2016-05-24 [SC] [expired: 2022-05-24]
      71A1D0EFCFEB6281FD0437C93D5919B448457EE0
uid                      Bazel Developer (Bazel APT repository key) <bazel-dev@googlegroups.com>
sub   rsa4096 2016-05-24 [E] [expired: 2022-05-24]

gpg: WARNING: No valid encryption subkey left over.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally it would be nice if the key was in a separate file (https://pkg.go.dev/embed)

Copy link
Author

Choose a reason for hiding this comment

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

I've updated the embedded key to the one from https://bazel.build/bazel-release.pub.gpg using //go:embed as requested.

Having said that, the "expired" subkey that was included in the initial version of this PR still works and validates the recent releases, since the key itself is the same, see:

$ gpg --show-keys original.gpg 
pub   rsa4096 2016-05-24 [SC] [expired: 2022-05-24]
      71A1D0EFCFEB6281FD0437C93D5919B448457EE0
uid                      Bazel Developer (Bazel APT repository key) <bazel-dev@googlegroups.com>
sub   rsa4096 2016-05-24 [E] [expired: 2022-05-24]
$ gpg --show-keys bazel-release.pub.gpg
pub   rsa4096 2016-05-24 [SC]
      71A1D0EFCFEB6281FD0437C93D5919B448457EE0
uid                      Bazel Developer (Bazel APT repository key) <bazel-dev@googlegroups.com>
sub   rsa4096 2016-05-24 [E]

Copy link
Contributor

Choose a reason for hiding this comment

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

It works because the outdated and unmaintained golang.org/x/crypto/openpgp doesn't check the key expiration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that this PR is active again, I would suggest to switch to ProtonMail/go-crypto which is backwards compatible with x/crypto/openpgp, but maintained.

Copy link
Author

Choose a reason for hiding this comment

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

It works because the outdated and unmaintained golang.org/x/crypto/openpgp doesn't check the key expiration.

Right. My point was that the updated file contains the same public key as the original one embedded in the initial version of this PR, not a new one.

Considering that this PR is active again [...]

I don't believe this PR was ever inactive... It was waiting for review/approval from maintainers, which I requested again as recently as 5 days ago.

I would suggest to switch to ProtonMail/go-crypto which is backwards compatible with x/crypto/openpgp, but maintained.

Actually, it's not fully backwards compatible:

I'm fine with either option (the change is trivial: PiotrSikora@d4acc7e), but I'd like to hear from the maintainers which version they prefer (e.g. if importing ProtonMail's fork internally is going to be an issue and result in unnecessary delay in merging this PR) before updating it here. @philwo @fweikert any thoughts?

Copy link
Contributor

@valco1994 valco1994 Jan 28, 2026

Choose a reason for hiding this comment

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

Right. My point was that the updated file contains the same public key as the original one embedded in the initial version of this PR, not a new one.

Yes, thanks, it's a valuable observation.

I don't believe this PR was ever inactive... It was waiting for review/approval from maintainers, which I requested again as recently as 5 days ago.

I understand, but before your recent request there’s been no activity since 2022.

... previously resulted in some build issues with Bazel (e.g. google/go-github#2932).

But as far as I see at google/go-github#2935, they finally resolved issues and switched to github.com/ProtonMail/go-crypto/openpgp in 2023. So, I don't think that it can cause an issue in 2026.

Of course, the decision is up to maintainers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have created my own pull request #769 on top of this pull request.
The most valuable part of it relates to the implementation of features requested at #192 (comment).

I hope that your pull request will be accepted soon, and #769 will be considered after it.

Copy link
Author

Choose a reason for hiding this comment

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

I understand, but before your recent request there’s been no activity since 2022.

Correct, since there were no review requests for any changes.

But as far as I see at google/go-github#2935, they finally resolved issues and switched to github.com/ProtonMail/go-crypto/openpgp in 2023. So, I don't think that it can cause an issue in 2026.

That's not exactly the case. If you look at that PR, they've completely removed openpgp usage from the library, and instead added a pluggable Signer interface, with a sample usage in the commitpr example (which indeed uses ProtonMail's fork), so openpgp is no longer a dependency of go-github library.

I have created my own pull request #769 on top of this pull request. The most valuable part of it relates to the implementation of features requested at #192 (comment).

Maybe I'm misreading #192 (comment), but it reads to me as a list of potential options in case we need to support expired key (a moot point with the current public key) and/or add the ability to disable signature verification for the official releases, but even @Yasumoto himself suggested that we should avoid adding the complexity unless necessary, and no one requested them to be added.

Note that the signature verification is enabled for: official releases (including release candidates and the rolling versions), as well as Bash & Fish completion scripts, but fetching Bazel releases from other sources (e.g. using BAZELISK_BASE_URL or BAZELISK_FORMAT_URL) will skip signature verification, since those builds are not expected to be signed by the official keys, which should be enough to allow custom builds to work with Bazelisk without any changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not exactly the case. If you look at that PR, they've completely removed openpgp usage from the library, and instead added a pluggable Signer interface, with a sample usage in the commitpr example (which indeed uses ProtonMail's fork), so openpgp is no longer a dependency of go-github library.

My fault, I didn't look carefully. Thank you for pointing it out!
So, the question regarding possible cross-compilation issues should be investigated deeper.

Maybe I'm misreading #192 (comment), but it reads to me as a list of potential options in case we need to support expired key ...

The major reason mentioned by Yasumoto, is indeed the handling of the case with an expired key. And I think that it's still a serious reason even if the current key has no expiration date set. We can't be sure that the key will never be compromised and revoked, or changed due to some other reasons. The inability to skip authenticity verification or use an alternative key will make bazelisk inoperable in this case.

But he also mentions

method to use a different key, which folks can use to pull from a fork.

And it looks like a good reason for enabling authenticity verification for forks. But it can only work if there is a way to specify an alternative verification key too.

@valco1994
Copy link
Contributor

valco1994 commented Jan 26, 2026

I took this PR as the base for my PR #769. I extended and reworked it quite drastically to handle additional options and expired keys especially, but preserved the original approach (and the corresponding commit with precision up to conflict resolution) of @PiotrSikora.

@philwo, @meteorcloudy, @fweikert, please, review it.

…signature

Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Signed-off-by: Piotr Sikora <code@piotrsikora.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 We're not considering working on this, but happy to review a PR. (No assignee)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Signature verification support

8 participants