Verify release signatures #102

woky · 2023-10-20T09:12:41Z

This commit builds on the previous commit that introduces support for
OpenPGP keyrings to verify signatures of archive release files. Previously,
we used the Release file of each configured suite. With this commit, we
fetch the InRelease file instead. This file is in clearsign format[1] and
contains the Release file and its signature[2].

[1] https://www.gnupg.org/gph/en/manual/x135.html
[2] https://www.chiark.greenend.org.uk/~cjwatson/blog/no-more-hash-sum-mismatch-errors.html

This commit extends the chisel release with keyring definitions. Keyrings are defined in ASCII armored format in the top-level public-keys property by name. Keyrings are referenced by name in the public-keys list property in archive definitions. An example of the extended chisel release file is at the bottom. This commit uses the newly added github.com/ProtonMail/go-crypto/openpgp package dependency[1]. This package is a maintained fork of the deprecated golang.org/x/crypto/openpgp package[2][3]. [1] https://github.com/ProtonMail/go-crypto [2] https://pkg.go.dev/golang.org/x/crypto/openpgp [3] https://golang.org/issue/44226 Example chisel.yaml: format: chisel-v1 archives: ubuntu: version: 22.04 components: [main, universe] suites: [jammy, jammy-updates, jammy-security] public-keys: [ubuntu] ubuntu-fips: version: 22.04 pro: fips components: [main] suites: [jammy] public-keys: [ubuntu-fips] public-keys: ubuntu: | -----BEGIN PGP PUBLIC KEY BLOCK----- mQINBFzZxGABEADSWmX0+K//0cosKPyr5m1ewmwWKjRo/KBPTyR8icHhbBWfFd8T DtYggvQHPU0YnKRcWits0et8JqSgZttNa28s7SaSUTBzfgzFJZgULAi/4i8u8TUj +KH2zSoUX55NKC9aozba1cR66jM6O/BHXK5YoZzTpmiY1AHlIWAJ9s6cCClhnYMR ... E+SWDGxtgwixyPziL56UavL/eeYJWeS/WqvGzZzsAtgSujFVLKWyUaRi0NvYW3h/ I50Tzj0Pkm8GtgvP2UqAWvy+iRpeUQ2ji0Nc =j6+P -----END PGP PUBLIC KEY BLOCK----- ubuntu-fips: | -----BEGIN PGP PUBLIC KEY BLOCK----- mQINBE+tgXgBEADfiL1KNFHT4H4Dw0OR9LemR8ebsFl+b9E44IpGhgWYDufj0gaM /UJ1Ti3bHfRT39VVZ6cv1P4mQy0bnAKFbYz/wo+GhzjBWtn6dThYv7n+KL8bptSC Xgg1a6en8dCCIA/pwtS2Ut/g4Eu6Z467dvYNlMgCqvg+prKIrXf5ibio48j3AFvd ... mguPI1KLfnVnXnsT5JYMbG2DCLHI/OIvnpRq8v955glZ5L9aq8bNnOwC2BK6MVUs pbJRpGLQ29hbeH8jnRPOPQ+Sbwa2C8/ZSoBa/L6JGl5RDaOLQ1w= =6Bkw -----END PGP PUBLIC KEY BLOCK-----

This commit builds on the previous commit that introduces support for OpenPGP keyrings to verify signatures of archive release files. Previously, we used the Release file of each configured suite. With this commit, we fetch the InRelease file instead. This file is in clearsign format[1] and contains the Release file and its signature[2]. [1] https://www.gnupg.org/gph/en/manual/x135.html [2] https://www.chiark.greenend.org.uk/~cjwatson/blog/no-more-hash-sum-mismatch-errors.html

woky · 2023-10-20T09:13:36Z

This PR depends on #100

The CI will pass once chisel-releases contains valid Ubuntu keyrings. The PRs in chisel-releases are: https://github.com/canonical/chisel-releases/pulls?q=is%3Apr+is%3Aopen+%22Add+Ubuntu+archive+signing+keys%22+in%3Atitle

niemeyer

Looks quite good, thanks. Only superficial comments.

internal/archive/archive.go

niemeyer · 2023-10-20T14:31:34Z

internal/archive/archive.go

+	for _, keyring := range index.archive.options.Keyrings {
+		_, err = block.VerifySignature(keyring, nil)
+		if err == nil || !errors.Is(err, pgperrors.ErrUnknownIssuer) {
+			break


I imagined that our intent here would be that, when we have a list of keys, any one of them being a valid verifier would suffice, but the above seems to do otherwise. Not only that, but it seems to change behavior depending on the order of the public keys as well. What's the idea there?

I think it works as you think it should no matter how are the keys ordered.

Pasting the whole block in question for full context:

err := pgperrors.ErrUnknownIssuer for _, keyring := range index.archive.options.Keyrings { _, err = block.VerifySignature(keyring, nil) if err == nil || !errors.Is(err, pgperrors.ErrUnknownIssuer) { break } } if err != nil { return nil, fmt.Errorf("signature verification failed: %w", err) }

If the archive defines no keys, we fail to verify. (err is initially set to pgperrors.ErrUnknownIssuer.)

If a key in a keyring successfully verifies the signature, then err is nil, and we break and succeed.

If no key in a keyring verifies the signature, IOW if err is pgperrors.ErrUnknownIssuer, we continue with the next keyring.

If any other error occurs during the signature verification, we fail immediately. Here's the list of possible errors: https://github.com/ProtonMail/go-crypto/blob/main/openpgp/errors/errors.go

Am I missing something obvious?

Just realized, that order can indeed matter for fatal errors, i.e. if one keyring has revoked keys, we can get ErrKeyRevoked and fail immediately, even though next keyring would verify it just fine. In that case, should we continue?

I'd propose that it succeeds if there's at least one valid key, issuing warnings for those that don't work.

What if one of the keyrings contain key revocations? We'd have to go through them all. But then, what if a release is signed by key A, keyring k1 contains public key A and keyring k2 contains public key B and revocation for key A? Which of these keyrings do we use?

niemeyer · 2023-10-20T14:37:33Z

internal/archive/archive.go

 func (index *ubuntuIndex) fetchRelease() error {
 	logf("Fetching %s %s %s suite details...", index.label, index.version, index.suite)
-	reader, err := index.fetch("Release", "", fetchDefault)
+	reader, err := index.fetchVerifyReleaseFile()


How about organizing this as:

fetchRelease => Just fetches the InRelease file, returns reader, err

verifyRelease => Takes reader from fetchRelease, returns reader, err

parseRelease => Calls fetchRelease, verifyRelease, sets index.release as appropriate.

Applied with one small variation that instead of passing around a reader, a byte array is passed, because clearsign.Decode() expects []byte. If I passed around reader, I'd have to read it fully in verifyRelease and then create byte reader for parseRelease. This way, network IO is isolated in fetchRelease. What do you think? If you like your suggestion better, I'm going to apply it verbatim.

internal/setup/setup.go

niemeyer · 2023-10-20T14:56:40Z

internal/setup/setup.go

@@ -30,6 +31,7 @@ type Archive struct {
 	Version    string
 	Suites     []string
 	Components []string
+	Keyrings   []openpgp.KeyRing


PublicKeys

"keyring" is the container, which holds public keys, private keys, or whatever else the format supports. For us, these keyrings hold the public side of the keys that signed the archive.

Updated, but I don't think this is proper naming.

IMHO the property in YAML is misnamed because keyring is what it actually holds. It's list of packets that is turned into openpgp.KeyRing instance which we use for signature verification and it can contain public keys but also revoked keys.

FWIW it can also contain just a private key, which I use for testing here https://github.com/canonical/chisel/pull/102/files/0dd312696da0c165a5d9c42a2312174e48f663e8#diff-dffb7bab06af83e76768dcafc44e1f9b291acf0b33fd280dc65b970a0bba6ed2R119. I use this keyring to create signatures in testarchive and I also pass it to what is now called PublicKeys.

Example:

$ gpg --no-default-keyring --keyring /usr/share/keyrings/ubuntu-archive-keyring.gpg -k /usr/share/keyrings/ubuntu-archive-keyring.gpg ---------------------------------------------- pub rsa4096 2012-05-11 [SC] 790BC7277767219C42C86F933B4FE6ACC0B21F32 uid [ unknown] Ubuntu Archive Automatic Signing Key (2012) <[email protected]> pub rsa4096 2012-05-11 [SC] 843938DF228D22F7B3742BC0D94AA3F0EFE21092 uid [ unknown] Ubuntu CD Image Automatic Signing Key (2012) <[email protected]> pub rsa4096 2018-09-17 [SC] F6ECB3762474EDA9D21B7022871920D1991BC93C uid [ unknown] Ubuntu Archive Automatic Signing Key (2018) <[email protected]> $ gpg --no-default-keyring --keyring /usr/share/keyrings/ubuntu-archive-keyring.gpg --export --armor -----BEGIN PGP PUBLIC KEY BLOCK----- mQINBE+tgXgBEADfiL1KNFHT4H4Dw0OR9LemR8ebsFl+b9E44IpGhgWYDufj0gaM /UJ1Ti3bHfRT39VVZ6cv1P4mQy0bnAKFbYz/wo+GhzjBWtn6dThYv7n+KL8bptSC ... E+SWDGxtgwixyPziL56UavL/eeYJWeS/WqvGzZzsAtgSujFVLKWyUaRi0NvYW3h/ I50Tzj0Pkm8GtgvP2UqAWvy+iRpeUQ2ji0Nc =j6+P -----END PGP PUBLIC KEY BLOCK-----

$ gpg --no-default-keyring --keyring /usr/share/keyrings/ubuntu-archive-removed-keys.gpg -k /usr/share/keyrings/ubuntu-archive-removed-keys.gpg --------------------------------------------------- pub dsa1024 2004-09-12 [SC] 630239CC130E1A7FD81A27B140976EAF437D05B5 uid [ unknown] Ubuntu Archive Automatic Signing Key <[email protected]> sub elg2048 2004-09-12 [E] pub dsa1024 2004-12-30 [SC] C5986B4F1257FFA86632CBA746181433FBB75451 uid [ unknown] Ubuntu CD Image Automatic Signing Key <[email protected]> $ gpg --no-default-keyring --keyring /usr/share/keyrings/ubuntu-archive-removed-keys.gpg --export --armor -----BEGIN PGP PUBLIC KEY BLOCK----- mQGiBEFEnz8RBAC7LstGsKD7McXZgd58oN68KquARLBl6rjA2vdhwl77KkPPOr3O YeSBH/voUsqausJfDNuTNivOfwceDe50lbhq52ODj4Mx9Jg+4aHn9fmRkIk41i2J ... HcRaDJqO6YL9tsvinm+qciUo3vQVQzF0pptOx49Z0gAHYNU7PULHpvA= =f4jO -----END PGP PUBLIC KEY BLOCK-----

Thanks for the change @woky. In this case, despite both being accurate, I do agree that PublicKeys is a better name as it is more explicit about what that variable expects. As @niemeyer said, a Keyring is a container of keys, but in our case, we are restricting that container to only have public keys, and thus it is something much narrower than a keyring.

Being verbose about it will help with readability.

we are restricting that container to only have public keys

We are not though. See my comments about testing.

EDIT: FYI, the openpgp package doesn't care whether the armored block type is PGP PUBLIC KEY BLOCK or PGP PRIVATE KEY BLOCK (it only checks whether it's either of those two). It just takes the list of things inside and adds them to a keyring.

Sure. Our point is that the chisel release configuration (yaml) expects a list of public keys, and while a collection of public keys can be a keyring, a keyring might not be solely composed of public keys

Here is some trivial code to process an armored public key:

https://gist.github.com/niemeyer/a4c221dff9fd166500db80d4ce9f817a

I'm closing this PR for now as the conversation is not being productive here.

@cjdcordeiro Let's please coordinate on how to continue this work.

Getting a public key from keyring is easy. Using it to verify clearsigned message requires bits from private openpgp code base. And as I said above, trying every key public key against a signature is not the way signatures are supposed to be verified. The keyring abstractions solves this fact. If we used use raw public keys, we'd also need to copy the part that filters those that have given issuer ID matching that of the signature. That's another part of openpgp code we'd need to copy.

So I have conflicting guidance

Use openpgp/packet.PublicKey

We're not going to fork openpgp

We can't do both at the same time.

Getting a public key from keyring is easy.

Everything is easy, code wise. I have code at hand to verify the signature which is just as simple. But this is not the problem anymore. We need someone that is willing to understanding the underlying rationale, instead of me coding the feature with small incomplete snippets while trying to covey an idea here.

Would you mind sharing that code? AFAU, it requires us to copy these blocks of code from the openpgp library:

https://github.com/ProtonMail/go-crypto/blob/afb1ddc0824c/openpgp/clearsign/clearsign.go#L408-L421

https://github.com/ProtonMail/go-crypto/blob/afb1ddc0824c/openpgp/clearsign/clearsign.go#L446-L464

https://github.com/ProtonMail/go-crypto/blob/afb1ddc0824c/openpgp/read.go#L480-L499

IOW, there's no built-in support to verify clearsigned message against a raw public key, we need to copy bits of the library implementation.

I would love to be proven wrong so we could move forward.

I've tried to give example implementation using only packet.PublicKey another go and it turns out, we'd have to copy more code from the library to be correct. The keys have flags and we also need to filter keys that can sign (verify) messages: https://github.com/ProtonMail/go-crypto/blob/afb1ddc0824ce0052d72ac0d6917f144a1207424/openpgp/keys.go#L338-L366 This information is stored in openpgp.Key instance in SelfSignature field. So this keyring machinery would need to be copied to setup.go.

niemeyer · 2023-10-20T15:00:17Z

internal/setup/setup.go

@@ -414,6 +418,16 @@ func parseRelease(baseDir, filePath string, data []byte) (*Release, error) {
 		return nil, fmt.Errorf("%s: no archives defined", fileName)
 	}

+	keyringsByName := make(map[string]openpgp.KeyRing, 0)


I won't repeat this again, but let's please review the PR and update the terminology with some nice var naming (short but neat) so when we're speaking of the content of those fields we refer to them as the archive public keys. If this sounds strange, imagine an age variable of type int. We'd refer to it as the "age" rather than the "int".

I've updated the naming where it makes sense. For instance, in archive_test.go, where I use the same keyring for creating and verifying signatures, I think it doesn't make sense to call it PublicKeys. That's just a suggestion though, I don't mind changing it everywhere.

I think that makes sense @woky ...if your test key is more than a public key, then it shouldn't be called PublicKey

woky · 2023-10-23T12:08:08Z

Unfortunately, it appears that clearsign.Block is stateful (it uses io.Reader for Body), so running block.VerifySignature() for the second time will read empty body, and hence the signature will be invalid. So if the first keyring fails to verify the signature, the remaining keyrings will fail too. I'm going to fix that and add a test for that. Converted to draft for now.

woky · 2023-10-24T23:34:49Z

I've fixed the issue. Variables/functions/comments/messages are updated to refer to "public keys" instead of keyrings. I retained the openpgp.Keyring type. See my comment about it here please: #102 (comment)

woky added 2 commits October 20, 2023 11:10

woky requested review from cjdcordeiro, niemeyer and rebornplusplus October 20, 2023 09:12

woky added the Blocked Waiting for something external label Oct 20, 2023

woky mentioned this pull request Oct 20, 2023

Support OpenPGP keyrings in release #100

Merged

cjdcordeiro added Priority Look at me first and removed Blocked Waiting for something external labels Oct 20, 2023

niemeyer requested changes Oct 20, 2023

View reviewed changes

niemeyer added the Reviewed Supposedly ready for tuning or merging label Oct 20, 2023

woky added 3 commits October 23, 2023 03:12

fixup! Verify release signatures

a13f54f

fixup! Verify release signatures

c2966e0

fixup! Verify release signatures

c74cfe0

cjdcordeiro removed the Reviewed Supposedly ready for tuning or merging label Oct 23, 2023

woky marked this pull request as draft October 23, 2023 12:05

woky removed the Priority Look at me first label Oct 23, 2023

woky marked this pull request as ready for review October 24, 2023 23:38

woky assigned niemeyer Oct 24, 2023

woky requested a review from niemeyer October 24, 2023 23:38

woky added the Priority Look at me first label Oct 24, 2023

woky added 2 commits October 25, 2023 02:05

fixup! Verify release signatures

b7fec5b

Merge remote-tracking branch 'upstream/main' into pgp-verify

a2cf662

woky force-pushed the pgp-verify branch from 3570386 to a2cf662 Compare October 25, 2023 00:05

niemeyer closed this Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify release signatures #102

Verify release signatures #102

woky commented Oct 20, 2023

woky commented Oct 20, 2023 •

edited

Loading

niemeyer left a comment

niemeyer Oct 20, 2023

woky Oct 23, 2023

woky Oct 23, 2023

cjdcordeiro Oct 23, 2023

woky Oct 23, 2023 •

edited

Loading

niemeyer Oct 20, 2023

woky Oct 23, 2023 •

edited

Loading

niemeyer Oct 20, 2023

woky Oct 23, 2023

cjdcordeiro Oct 23, 2023

woky Oct 23, 2023 •

edited

Loading

cjdcordeiro Oct 23, 2023

niemeyer Oct 25, 2023

woky Oct 25, 2023 •

edited

Loading

niemeyer Oct 25, 2023

woky Oct 25, 2023 •

edited

Loading

woky Oct 25, 2023

niemeyer Oct 20, 2023 •

edited

Loading

woky Oct 23, 2023 •

edited

Loading

cjdcordeiro Oct 23, 2023

woky commented Oct 23, 2023 •

edited

Loading

woky commented Oct 24, 2023

Verify release signatures #102

Verify release signatures #102

Conversation

woky commented Oct 20, 2023

woky commented Oct 20, 2023 • edited Loading

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

woky Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky commented Oct 23, 2023 • edited Loading

woky commented Oct 24, 2023

woky commented Oct 20, 2023 •

edited

Loading

woky Oct 23, 2023 •

edited

Loading

woky Oct 23, 2023 •

edited

Loading

woky Oct 23, 2023 •

edited

Loading

woky Oct 25, 2023 •

edited

Loading

woky Oct 25, 2023 •

edited

Loading

niemeyer Oct 20, 2023 •

edited

Loading

woky Oct 23, 2023 •

edited

Loading

woky commented Oct 23, 2023 •

edited

Loading