Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse signature subpacket type 4 (exportable cert flag) #200

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

dedekind125
Copy link
Contributor

@dedekind125 dedekind125 commented Mar 8, 2024

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
Copy link
Contributor

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

@dedekind125
Copy link
Contributor Author

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!

@dedekind125 dedekind125 changed the title Add support for signature subpacket type 4 (recoverable flag) Add support for signature subpacket type 4 (exportable cert flag) Mar 8, 2024
@twiss
Copy link
Member

twiss commented Mar 8, 2024

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
Copy link
Contributor

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

@andrewgdotcom
Copy link
Contributor

@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 😄)

openpgp/packet/signature.go Outdated Show resolved Hide resolved
@twiss
Copy link
Member

twiss commented Mar 10, 2024

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
Copy link
Contributor Author

dedekind125 commented Mar 11, 2024

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
Copy link
Member

twiss commented Mar 11, 2024

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
Copy link
Contributor Author

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 dedekind125 changed the title Add support for signature subpacket type 4 (exportable cert flag) Handle signature subpacket type 4 (exportable cert flag) Mar 12, 2024
@dedekind125 dedekind125 requested a review from twiss March 13, 2024 09:06
@dedekind125
Copy link
Contributor Author

Hey @twiss, any updates on merging this PR?

@twiss
Copy link
Member

twiss commented Apr 8, 2024

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 dedekind125 changed the title Handle signature subpacket type 4 (exportable cert flag) Parse signature subpacket type 4 (exportable cert flag) Apr 9, 2024
@dedekind125
Copy link
Contributor Author

dedekind125 commented Apr 9, 2024

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.

Copy link
Member

@twiss twiss left a comment

Choose a reason for hiding this comment

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

👍 Thanks!

@twiss twiss merged commit f4e1152 into ProtonMail:main Apr 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants