Add Signature.IssuerKeyVersion field

[andrewgdotcom]

• exposes version field from IssuerFingerprint subpacket
• remove redundant nil checks

(closes #296)

[twiss]

Thanks for the PR! However I wouldn't put exposing a new field in the category of "minor fixes" 😅

Also, if we set a new field when reading signature packets, we should also set it when creating them, such as here:

go-crypto/openpgp/v2/write.go

Line 913 in 681b4e5

IssuerFingerprint: signer.Fingerprint,

(among other places).

And, we should also take it into account when writing the signature here, so that it roundtrips correctly:

go-crypto/openpgp/packet/signature.go

Line 1395 in 681b4e5

contents := append([]uint8{uint8(issuer.Version)}, sig.IssuerFingerprint...)

And finally we could then remove the issuer parameter from buildSubpackets which was probably a mistake to begin with:

go-crypto/openpgp/packet/signature.go

Line 1257 in 681b4e5

func (sig *Signature) buildSubpackets(issuer PublicKey, config *Config) (subpackets []outputSubpacket, err error) {

[andrewgdotcom]

It would be nice if there was a "versioned fingerprint" type that we could use instead of having separate fields, but that would either require a breaking api change, or even more redundancy...

[twiss]

Thanks!

After looking at the code again I think IssuerKeyVersion is less clunky and makes more sense than IssuerFingerprintVersion. I changed it locally with some other tiny cleanup, but can't push to the branch. Would you mind allowing maintainers to push to the branch? Otherwise if you prefer I can also make a new PR.

[andrewgdotcom]

Would you mind allowing maintainers to push to the branch?

I don't have that option, it's only available in personal branches, not org branches. If you want to PR into my branch, or just take my branch and mangle it, feel free :-)

[andrewgdotcom]

Scratch the above, I've given you write access to my fork.

[twiss]

That also works, thanks!

I've pushed the change, lmk if it still looks reasonable to you and then I'll merge.

[twiss]

Actually, re-reading this, this part is not correct because it assumes the signing key version matches the one-pass signature version, which is unfortunately not necessarily the case: https://www.rfc-editor.org/rfc/rfc9580.html#signed-message-versions

I'm somewhat wondering whether we need this field in SignatureCandidate or should drop it for now, since we don't have this information here. Do you have an opinion?

[andrewgdotcom]

Oof, good catch. I'm inclined to say that if we don't have the info, we should just drop it. Will need to remove the corresponding check further down... leave it with me.

[andrewgdotcom]

The implicit logic elsewhere is that if IssuerFingerprint is nil, IssuerKeyVersion is 0 - so maybe the most self-consistent (and future-proof) thing to do is set this to ops.Version IFF ops.Version is >= 6, and 0 otherwise?

[andrewgdotcom]

I've made the IFF change now, as it mirrors the equivalent logic in newSignatureCandidateFromSignature. I've also made a couple of driveby fixes to SignatureCandidate.validate().

[twiss]

I'm not sure if it's guaranteed to be future proof because if we define key version 7 without defining one-pass signature version 7; we would have an issuer fingerprint without a known issuer key version (which arguably is not ideal and so we should have included an issuer key version field). Arguably we should then not do that but.. that's to be decided in the future 🙃

[andrewgdotcom]

Well, the definition of ops v6 assumes the fingerprint length is fixed, so we're already taking that risk... 😁 I think lockstep version bumps of key, sig and ops should be standard practice in future, even if there are no wire format changes in some of the packets (we did the same thing in v2->v3)

[twiss]

Yeah, that's true. However, in the absence of a field on the wire, what are we actually checking with this field here?

We're setting the issuer key version to 6 if the OPS version is 6, and then later we're checking the OPS version against the signature version and the (assumed) OPS issuer key version against the signature's issuer key version. So, in a very roundabout way, we're checking that if the OPS version is 6, the signature version and issuer key version field must also be 6. However, we're still not actually checking the issuer key version field against the verification key version.

I think it would be more straightforward to just check the signature's issuer key version field against the verification key version, and also check that the signature version and signing key version correspond properly as per Table 27 (i.e. #309). Then we don't have to carry this extra field around based on various assumptions.

[andrewgdotcom]

Fair enough, I've reverted this (and also simplified some of the test logic further)

[twiss]

Thanks!