Parse signature subpacket type 4 (exportable cert flag)

[dedekind125]

Recently I had to parse a PGP key with a signature that contained a critical subpacket of type 4.

Similarly to #138 and #139, this PR:

• Handles the Exportable Certification subpacket on signatures per RFC 4880 5.2.3.11.
• Parses the signature when the subpacket indicates that the signature is exportable (this is also the default behavior if the subpacket is absent)
• Returns a UnsupportedError for non-exportable signatures (subpacket has the value false). This is done to avoid introducing a breaking change as extra work needs to be done to support this.

Data used for test case:

echo -n "c2c07404130102001e050263fde196050903c3b880021b26030b0309021601041508090a028401000a09101bf1d93a68a8b208342b07ff6f59f5a882d148c481b877b434271b2e844e0dd783d9eb5534aac51170024382089cf07194a1835c72177d677a7ce04a614c1f85ccf5972d08ebabdfefefbe9d0f2b9f0a0a010c0889d9ab43ec99ccaddf76f7a96d91c49256ae23078a22469fd2a3d1d2ccfb30eb4f137e8c893731163e8f7aa18abb6a72ebfbab71ac8946f991d0a10d0293bc275183f67567c709bdd7e035d16c3cb2d14565b8baccaf721a3e1ed59385fc4b248648bdf7072a07ed693caf9179ea980fa8f89bef9c6819870c0074aa0419bf80e073863fe4cfe144a3083586d05f3ce8277d891dc11aa157dd133ac8d2dab4e8095affe6f3d3be673d2392c9177102374f51c56199dd3c05" | xxd -r -p | gpg --list-packets
# off=0 ctb=c2 tag=2 hlen=3 plen=308 new-ctb
:signature packet: algo 1, keyid 1BF1D93A68A8B208
	version 4, created 1677582742, md5len 0, sigclass 0x13
	digest algo 2, begin of digest 34 2b
	hashed subpkt 2 len 4 (sig created 2023-02-28)
	hashed subpkt 9 len 4 (key expires after 2y1d0h0m)
	hashed subpkt 27 len 1 (key flags: 26)
	hashed subpkt 11 len 2 (pref-sym-algos: 3 9)
	hashed subpkt 22 len 1 (pref-zip-algos: 1)
	hashed subpkt 21 len 3 (pref-hash-algos: 8 9 10)
	critical hashed subpkt 4 len 1 (exportable)
	subpkt 16 len 8 (issuer key ID 1BF1D93A68A8B208)

[andrewgdotcom]

Subpacket 0x04 is "Exportable", while RFC 4880 5.2.3.12 describes 0x07 "Revocable". There is no "Recoverable" subpacket.

[dedekind125]

Hey @andrewgdotcom, sorry for the confusion.

Indeed, I am not used to reading RFC's. I changed the naming to "Exportable". Let me know, if that is okay now!

[twiss]

Hi 👋 Thanks for the PR! However, two comments, one low-level and one high-level:

First, this PR only implements parsing, not serialization of the subpacket, so if you set sig.Exportable and then sign the signature, it won't have the subpacket. It would probably be good to add that.

Then, the spec talks about removing certifications with exportable=0 when exporting or importing them. However, we don't have such concepts in go-crypto, since it's a library; it just implements parsing and serializing, and we don't know what the intended purpose is. @andrewgdotcom Do you have an opinion on how this flag should be handled by the library, if at all?

[andrewgdotcom]

@twiss A library should be able to CRUD such "user intent" subpackets. Beyond that I think it's entirely an application issue.

[andrewgdotcom]

@twiss Subpacket types 23, 24 and 31 are also conspicuously missing. It would be nice to also have these, particularly for future keyserver work but also because it should be safe in general to parse e.g. a sig with a critical "keyserver preferences" subpacket (if someone was crazy enough to do that). I'd be happy to help out with that.

(On the other hand, lack of support for type 7 is a welcome unfeature, see https://gitlab.com/dkg/openpgp-revocation/-/issues/5 😄)

[twiss]

A library should be able to CRUD such "user intent" subpackets. Beyond that I think it's entirely an application issue.

I think it's a bit unfortunate that, to comply with the spec, the application will have to manually go through the certifications, check this flag, and remove them if it's present and false (when relevant). Perhaps we could make some kind of helper functions for importing and exporting keys, here or in GopenPGP.

This PR is also technically speaking a breaking change in the sense that, currently, we reject keys with unexportable certificates, which at least indicates that there's some issue. But I guess even if we add a high-level API that breaking change in the low-level API isn't really avoidable, so maybe it's OK.

Subpacket types 23, 24 and 31 are also conspicuously missing. It would be nice to also have these, particularly for future keyserver work but also because it should be safe in general to parse e.g. a sig with a critical "keyserver preferences" subpacket (if someone was crazy enough to do that). I'd be happy to help out with that.

Sounds good :)

[dedekind125]

This PR is also technically speaking a breaking change in the sense that, currently, we reject keys with unexportable certificates, which at least indicates that there's some issue. But I guess even if we add a high-level API that breaking change in the low-level API isn't really avoidable, so maybe it's OK.

An alternative for this MR is to provide functionality for parsing the subpacket and throw an error in case the flag is set to false. Then this is not a breaking change and at the same time the library can support parsing keys where the "exportable" subpacket is explicitly set to true.

[twiss]

Yeah, that could work. I guess we might then eventually also want a config option saying whether we should accept or ignore unexportable signatures.

As an aside, coming back to the original case, I'm actually a bit confused why someone would create a critical subpacket marking the signature as exportable (which is the default anyway, so it shouldn't really be critical to explicitly say so). Do you happen to know what software generated this key?

[dedekind125]

As an aside, coming back to the original case, I'm actually a bit confused why someone would create a critical subpacket marking the signature as exportable (which is the default anyway, so it shouldn't really be critical to explicitly say so). Do you happen to know what software generated this key?

Yeah, I agree, it does make much sense to explicitly mark the signature certs as exportable. This key came from a 3rd party source, so unfortunately I do not know how it was generated. I will make the changes to just parse the subpacket then, and throw an error if its false if that would make the MR merge-able.

[dedekind125]

Hey @twiss, any updates on merging this PR?

[twiss]

Hey 👋 Apologies for the long delay.
In the current state of the PR, if you set sig.Exportable = false, and then sign it, the subpacket won't be included, which seems unexpected. So I would either still include the generation code, or remove the property entirely (and only read the subpacket during parsing), so that it can't be set.

[dedekind125]

Hey 👋 Apologies for the long delay. In the current state of the PR, if you set sig.Exportable = false, and then sign it, the subpacket won't be included, which seems unexpected. So I would either still include the generation code, or remove the property entirely (and only read the subpacket during parsing), so that it can't be set.

Makes sense, I removed the property from the signature all-together. Basically, now we can just parse the subpacket - if its set to true. I kept the error in case the subpacket is set to false, since parsing it may entail that the library support keys with unexportable certificates.

[twiss]

👍 Thanks!