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

High Level OpenPGP v6 Key Generation #1857

Closed
wants to merge 53 commits into from

Conversation

vanitasvitae
Copy link
Contributor

Hey!
In this PR, I'm working on a high-level API for OpenPGP key generation (v6 only for now).
Let me know, what you think of the design sketched out in OpenPGPV6KeyGenerator :)

I tried to keep the API straight-forward, but still allow modifications of the (hashed) signature subpacket areas by the use of callbacks which the user can hook into, to modify the signature subpackets prior to signature generation.

The PR also adds key generator classes for individual keys, such that the risk of making mistakes during key generation (e.g. using wrong/weak parameters) is minimized.

I used PGPainless as orientation for the API design, but kept the generation API a bit slimmer for now. Let me know what you think of the direction the API is heading.

@vanitasvitae vanitasvitae marked this pull request as draft October 3, 2024 09:58
@vanitasvitae
Copy link
Contributor Author

I'm contemplating whether to transform the OpenPGPV6KeyGenerator into a general OpenPGPKeyGenerator class, where the user can specify the key version number.

There are some non-trivial differences between v4 and v6, most notably the fact that v6 stores preferences in a Direct-Key signature, while v4 prefers user-id certification signatures (at least if you want to be compatible to GnuPG).

@vanitasvitae vanitasvitae force-pushed the keyGeneration branch 3 times, most recently from 911179b to 96f9e0e Compare October 17, 2024 11:13
@vanitasvitae vanitasvitae marked this pull request as ready for review October 17, 2024 14:06
@vanitasvitae
Copy link
Contributor Author

Ready for review. Let me know if I can make your job easier by squashing or splitting this up into smaller PRs :)

@vanitasvitae
Copy link
Contributor Author

One thing I dislike about the current API design:
PGPKeyPair by default contains a primary key. Ideally I'd like to deal with an untyped key pair, whose type is determined when the key pair is added to a key ring.
Of course, you could manually convert between key types by converting the public key packet into a PublicSubkeyPacket object, and I even added PGPPublicKey.asSubkey() for this purpose, but that's still something the user needs to know.

An additional issue with the current API is, that PBESecretKeyEncryptor instances that use AEAD rely on the public key's packet tag to derive the KEK (key encapsulation key), so the key type has to be properly set the moment the PBESecretKeyEncryptor is instantiated.

I'm not sure, if there is a clean way to refactor the current API, circumsailing these rough edges in a backwards-compatible way.

@ligefeiBouncycastle
Copy link
Collaborator

@vanitasvitae Thank you for your contribution to this project.

Please note that due to compatibility constraints, the existing APIs cannot be modified.

I have a few questions about this PR:

Are there any relevant standards (e.g., RFC 9580) related to OpenPGPV6KeyGenerator?
Why is PBESecretKeyEncryptorFactory implemented as an abstract class instead of an interface?
Why are OpenPGPV6KeyGenerator and related classes located in the openpgp/api package rather than the openpgp/operator package?

@vanitasvitae
Copy link
Contributor Author

Hey @ligefeiBouncycastle !
Thank you for the brief review :)

I'm aware that breaking changes in the existing API shall be kept to an absolute minimum. Where possible, I try to avoid changing method signatures in favor of adding new alternative methods. I think I succeeded in keeping the existing API intact as should be evident by the unaltered unit tests.

The OpenPGPV6KeyGenerator class provides a high-level API for generating OpenPGP v6 keys that follow the guidance from RFC9580. Currently, this class also contains methods for generating RSA keys, which I will likely remove again, as RSA keys are deprecated in RFC9580. I mostly added these methods to broaden the surface for testing.

PBESecretKeyEncryptorFactory can indeed be changed to an interface, thanks for noticing this oversight :)

I decided to place the new classes into /openpgp/api/, as I believe they present a different layer compared to the code in /openpgp/.
My understanding of the codebase is the following:
Code in /bcpg/ contains packet marshalling/parsing code without a logic backend.
The existing classes in /openpgp/ provide a "mid-level" API on top of /bcpg/, where the user has a lot of flexibility, as they need to construct generators and streams from the ground up. This layer allows the user to implement key generation, encryption, signing, decryption etc. in exactly the way they need, but also requires a substantial amount background knowledge and understanding of the OpenPGP protocol.

The new classes in /openpgp/api/ on the other hand strive to provide a high-level, ready to use API on top of the /openpgp/ classes. The goal of this layer is to provide the user with an opinionated API that comes preconfigured and does the right thing by default without the need for much boilerplate code. The idea is that the smallest amount of user-written code should produce the most generic artifacts, which are most compatible with other implementations.

Let me know if you have any more feedback and ideas on how to further improve this PR :)
Paul

@vanitasvitae
Copy link
Contributor Author

Note: It might make sense to split this PR up into smaller chunks (e.g. separate out all changes to the /openpgp/ package).
In #1911 I introduced new OpenPGPCertificate/OpenPGPKey classes, which present a high-level wrapper around PGPPublicKeyRing/PGPSecretKeyRing. Perhaps it makes sense to change OpenPGPV6KeyGenerator to return these types instead of PGPSecretKeyRing.

@ligefeiBouncycastle
Copy link
Collaborator

ligefeiBouncycastle commented Nov 28, 2024

@vanitasvitae Thank you for your response. It would be great if #1911 could be split into smaller, more manageable parts.

Please don't forget AEADProtectedPGPSecretKeyTest. reencryptKeyJca() function

@vanitasvitae
Copy link
Contributor Author

Please don't forget AEADProtectedPGPSecretKeyTest. reencryptKeyJca() function

Good call!

@vanitasvitae vanitasvitae marked this pull request as draft November 28, 2024 14:42
@vanitasvitae
Copy link
Contributor Author

I'm splitting this PR up into two parts, one focussed on changes in /openpgp/, /openpgp/operator/ and /bcpg/, and finally another one adding the new /openpgp/api/ classes.

@vanitasvitae
Copy link
Contributor Author

This PR appears to now be part of the main branch.

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.

2 participants