Skip to content

Add InsecureGenerateNonCriticalKeyFlags option to generate non-critical key flags signature subpackets#291

Merged
twiss merged 8 commits intoProtonMail:mainfrom
KSerrania:kserrania/add-option-remove-critical-from-subpackets
Jul 8, 2025
Merged

Add InsecureGenerateNonCriticalKeyFlags option to generate non-critical key flags signature subpackets#291
twiss merged 8 commits intoProtonMail:mainfrom
KSerrania:kserrania/add-option-remove-critical-from-subpackets

Conversation

@KSerrania
Copy link
Copy Markdown
Contributor

@KSerrania KSerrania commented Jul 7, 2025 •
edited
Loading

Overview

Adds a new option to openpgp/packet.Config, InsecureGenerateNonCriticalKeyFlags, to force the Sign method to not add the critical flags to the key flags subpacket.

Use-case

At Datadog, we have a setup where we have an RSA private key stored in a secrets manager, and we use this library to craft a GPG key from it, in order to sign deb and rpm packages, and to export the public GPG key to let customers import it in their systems.

The exported public key's signature packet contains two critical subpackets (signature date, and key flags), which are always set to true by the Sign method.

gpg --list-packets output 
$ gpg --list-packets /mount/public-key.asc
# off=0 ctb=c6 tag=6 hlen=3 plen=397 new-ctb
:public key packet:
   version 4, algo 1, created 1749600000, expires 0
   pkey[0]: [3072 bits]
   pkey[1]: [17 bits]
   keyid: AD7E54B36DF4CD19
# off=400 ctb=cd tag=13 hlen=2 plen=50 new-ctb
:user ID packet: "Datadog Pacakges (Testing) <package@datadoghq.com>"
# off=452 ctb=c2 tag=2 hlen=3 plen=453 new-ctb
:signature packet: algo 1, keyid AD7E54B36DF4CD19
   version 4, created 1751893082, md5len 0, sigclass 0x13
   digest algo 8, begin of digest 97 b5
   critical hashed subpkt 2 len 4 (sig created 2025-07-07)
   hashed subpkt 11 len 1 (pref-sym-algos: 7)
   hashed subpkt 16 len 8 (issuer key ID AD7E54B36DF4CD19)
   hashed subpkt 21 len 1 (pref-hash-algos: 8)
   hashed subpkt 22 len 1 (pref-zip-algos: 0)
   hashed subpkt 25 len 1 (primary user ID)
   critical hashed subpkt 27 len 1 (key flags: 03)
   hashed subpkt 30 len 1 (features: 01)
   hashed subpkt 33 len 21 (issuer fpr v4 BF8CBF16F1F5EA4EE6C44CEBAD7E54B36DF4CD19)
   data: [3072 bits]

These critical subpackets are not accepted by some versions of rpm, for instance the version included in OpenSUSE Leap 15.1 to 15.4 (rpm 4.14.3-150400.59.3.1). OpenSUSE versions 15.5 and 15.6 (rpm 4.14.3-150400.59.16.1) do not have this issue.

For this particular case, with further testing we determined that what trips rpm up is the critical subpacket on the key flags subpacket.

rpm --import test in OpenSUSE Leap 15.4 
$ docker run -v "$PWD:/mount" -it opensuse/leap:15.4 bash
01c10ff07d30:/ # gpg --list-packets /mount/public-key.asc
gpg: keybox '/root/.gnupg/pubring.kbx' created
# off=0 ctb=c6 tag=6 hlen=3 plen=397 new-ctb
:public key packet:
	version 4, algo 1, created 1749600000, expires 0
	pkey[0]: [3072 bits]
	pkey[1]: [17 bits]
	keyid: AD7E54B36DF4CD19
# off=400 ctb=cd tag=13 hlen=2 plen=50 new-ctb
:user ID packet: "Datadog Pacakges (Testing) <package@datadoghq.com>"
# off=452 ctb=c2 tag=2 hlen=3 plen=453 new-ctb
:signature packet: algo 1, keyid AD7E54B36DF4CD19
	version 4, created 1751893082, md5len 0, sigclass 0x13
	digest algo 8, begin of digest 97 b5
	critical hashed subpkt 2 len 4 (sig created 2025-07-07)
	hashed subpkt 11 len 1 (pref-sym-algos: 7)
	hashed subpkt 16 len 8 (issuer key ID AD7E54B36DF4CD19)
	hashed subpkt 21 len 1 (pref-hash-algos: 8)
	hashed subpkt 22 len 1 (pref-zip-algos: 0)
	hashed subpkt 25 len 1 (primary user ID)
	critical hashed subpkt 27 len 1 (key flags: 03)
	hashed subpkt 30 len 1 (features: 01)
	hashed subpkt 33 len 21 (issuer fpr v4 BF8CBF16F1F5EA4EE6C44CEBAD7E54B36DF4CD19)
	data: [3072 bits]
01c10ff07d30:/ # gpg --import /mount/public-key.asc
gpg: /root/.gnupg/trustdb.gpg: trustdb created
gpg: key AD7E54B36DF4CD19: public key "Datadog Pacakges (Testing) <package@datadoghq.com>" imported
gpg: Total number processed: 1
gpg:               imported: 1
01c10ff07d30:/ # rpm --import /mount/public-key.asc
error: /mount/public-key.asc: key 1 import failed.

Since we need to continue supporting these OS versions, we'd like to add opt-in configuration to force the Sign method to not add the critical subpackets to the key flags subpacket of a signature. In our case, we'd activate this option when adding a user id to the key to ensure the self-signature does not contain the offending critical subpacket.

We currently use a fork to work around this issue, but, if possible, we'd rather continue using the upstream library.

Testing

Tested the exported public key output with the option set to true (the key flag subpacket is not critical) and false (all expected subpackets are critical).
Added unit test for the option.

Questions

Should this option be prefixed with Insecure?

@andrewgdotcom
Copy link
Copy Markdown
Contributor

andrewgdotcom commented Jul 7, 2025

Signature Creation Time (amongst others) SHOULD be marked as critical (*). It's overkill to remove the critical bit from it, just because there is an interop issue with criticality of a different subpacket.

(*) https://datatracker.ietf.org/doc/html/rfc9580#section-5.2.3.11

@wiktor-k
Copy link
Copy Markdown

wiktor-k commented Jul 7, 2025

we determined that what trips rpm up is the critical subpacket on the key flags subpacket.

Maybe it'd be simpler to just not add the critical bit there? 🤔

@KSerrania
Copy link
Copy Markdown
Contributor Author

KSerrania commented Jul 7, 2025

I see, thanks for the review. Would it be fine to have a DisableKeyFlagCriticalSubpacket option to only remove the critical bit from that particular subpacket then? If yes, I'll amend the PR.

@KSerrania KSerrania changed the title Add DisableCriticalSubpackets option to remove critical bit from signature subpackets Add DisableKeyFlagCriticalSubpacket option to remove critical bit from signature subpackets Jul 7, 2025
@KSerrania KSerrania changed the title Add DisableKeyFlagCriticalSubpacket option to remove critical bit from signature subpackets Add DisableKeyFlagCriticalSubpacket option to remove critical bit from key flags signature subpacket Jul 7, 2025
@KSerrania
Copy link
Copy Markdown
Contributor Author

KSerrania commented Jul 7, 2025 •
edited
Loading

I updated the PR and description to only affect the key flags subpacket.

@wiktor-k
Copy link
Copy Markdown

wiktor-k commented Jul 7, 2025

I think it looks good now 👌

Note though, that I'm rather a random bystander, not anyone from the Proton team so my ack means nothing 😅

@twiss
Copy link
Copy Markdown
Collaborator

twiss commented Jul 8, 2025

Hi 👋 Thanks for the PR! It mostly looks good to me.

Should this option be prefixed with Insecure?

I think arguably so, because an implementation that doesn't support the key flags subpacket may end up using the signing key for encryption (which in turn might cause the keyholder to need to use InsecureAllowDecryptionWithSigningKeys).

How about we call this flag InsecureGenerateNonCriticalKeyFlags?

@KSerrania
Copy link
Copy Markdown
Contributor Author

KSerrania commented Jul 8, 2025

Hi @twiss, thanks for the review!

How about we call this flag InsecureGenerateNonCriticalKeyFlags?

Sounds good to me, I'll make the changes right away.

@KSerrania KSerrania changed the title Add DisableKeyFlagCriticalSubpacket option to remove critical bit from key flags signature subpacket Add InsecureGenerateNonCriticalKeyFlags option to remove critical bit from key flags signature subpacket Jul 8, 2025
twiss
twiss reviewed Jul 8, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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! Some nitpicks below.

KSerrania and others added 2 commits July 8, 2025 14:25
@KSerrania @twiss
Apply suggestions from code review
ce529db 
Co-authored-by: Daniel Huigens <d.huigens@protonmail.com>
@KSerrania
Reorder Config struct
4c7c9a1
twiss
twiss approved these changes Jul 8, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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 changed the title Add InsecureGenerateNonCriticalKeyFlags option to remove critical bit from key flags signature subpacket Add InsecureGenerateNonCriticalKeyFlags option to generate non-critical key flags signature subpackets Jul 8, 2025
@twiss twiss merged commit 0906643 into ProtonMail:main Jul 8, 2025
9 checks passed
@KSerrania KSerrania mentioned this pull request Jul 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@twiss twiss twiss approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KSerrania @andrewgdotcom @wiktor-k @twiss