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?

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.

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) <ftpmaster@ubuntu.com>

pub   rsa4096 2012-05-11 [SC]
      843938DF228D22F7B3742BC0D94AA3F0EFE21092
uid           [ unknown] Ubuntu CD Image Automatic Signing Key (2012) <cdimage@ubuntu.com>

pub   rsa4096 2018-09-17 [SC]
      F6ECB3762474EDA9D21B7022871920D1991BC93C
uid           [ unknown] Ubuntu Archive Automatic Signing Key (2018) <ftpmaster@ubuntu.com>

$ 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 <ftpmaster@ubuntu.com>
sub   elg2048 2004-09-12 [E]

pub   dsa1024 2004-12-30 [SC]
      C5986B4F1257FFA86632CBA746181433FBB75451
uid           [ unknown] Ubuntu CD Image Automatic Signing Key <cdimage@ubuntu.com>

$ 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

1. Use openpgp/packet.PublicKey
2. 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.

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