search

ProtonMail / go-crypto

Fork of go/x/crypto, providing an up-to-date OpenPGP implementation
https://pkg.go.dev/github.com/ProtonMail/go-crypto
BSD 3-Clause "New" or "Revised" License
328 stars 99 forks source link

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

Closed dedekind125 closed 4 months ago

dedekind125 commented 5 months ago

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

Similarly to https://github.com/ProtonMail/go-crypto/pull/138 and https://github.com/ProtonMail/go-crypto/pull/139, this PR:

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 commented 5 months ago

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

dedekind125 commented 5 months ago

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 commented 5 months ago

Hi :wave: 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 commented 5 months ago

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

andrewgdotcom commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

Hey @twiss, any updates on merging this PR?

twiss commented 4 months ago

Hey :wave: 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 commented 4 months ago

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.