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

SignaturePacket and OnePassSignaturePacket parsing of version 6 packets #1634

Closed
wants to merge 10 commits into from

Conversation

vanitasvitae
Copy link
Contributor

This PR is a rewrite of #1409

Here, I focus solely on SignaturePacket and OnePassSignaturePacket.

For SignaturePacket, I added support for parsing and encoding version 6 packets. I decided to split up the parsing of different versions into separate methods to improve readability. Let me know if you'd rather like a larger constructor body instead.

For OnePassSignaturePacket, I added parsing and encoding of the salt and fingerprint fields.

While testing parsing of the raw signature value, I noticed that OperatorBcTest creates test keys with using the Ed448, Ed25519 public key algorithm tags: https://github.com/bcgit/bc-java/blob/main/pg/src/test/java/org/bouncycastle/openpgp/test/OperatorBcTest.java#L384
However, the signature generation code that adds the binding signatures still uses MPInteger encoding for the raw signature. MPI encoding however shall only be used with EDDSA_LEGACY (22), not Ed25519 (27) or Ed448 (28), which do use octet strings:
https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-13.html#name-public-key-algorithms

Currently, I do work around this by fixing the parser logic for v4,v5 signatures created by Ed25519, Ed448 keys to use the old parsing method which uses MPIntegers to encode the signature value. For Version 6 signatures I use the new method of parsing into fixed length octet strings.

Ultimately, this behaviour should correct itself once signature calculation (and verification) is properly implemented using octet strings for Ed448, Ed25519.

Interop with LibrePGP is a bit of a pain. Crypto-Refresh states that the size of the salt MUST correspond to the values specified by the hash algorithm table (https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-13.html#name-version-4-and-6-signature-p) and even states that signatures which do not satisfy this criterion are to be considered malformed (https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-13.html#name-malformed-and-unknown-signa).

LibrePGP on the other hand defines salt as optional, so we cannot enforce this on the packet parser level.

Further, C-R pins compatible version of OnePassSignature and Signature packets (https://www.ietf.org/archive/id/draft-ietf-openpgp-crypto-refresh-13.html#name-packet-versions-in-signatur) while LibrePGP does not even specify, what version of the OPS packet is to be used with v5 signatures.

In general: Do you want me to open PRs around v6 against the main branch, or would you prefer me to target some v6 feature branch which can later be merged into main in one go?
Also, let me know if you want me to squash the commits :)

@vanitasvitae
Copy link
Contributor Author

I fixed the Ed25519, Ed448 encoding issues and now the OperatorBcTest succeeds again without requiring further changes :)

I also moved out some unrelated changes to keep this PR small :)

@dghgit
Copy link
Contributor

dghgit commented May 1, 2024

So, small PR's against the main branch is the way to go. One other thing, would you try and stick with the BC way of formatting code in general - it will keep things easier to control as changes in "code style" end up being mixed with actual changes (edit: actually it appears you've already worked this out and done this, just saw the new commits, appreciated). Thanks!

I'll start merging this one now.

@dghgit
Copy link
Contributor

dghgit commented May 1, 2024

Merged with minor changes. Thanks!

@dghgit dghgit closed this May 1, 2024
hubot pushed a commit that referenced this pull request May 1, 2024
@vanitasvitae
Copy link
Contributor Author

Is it sufficient to make sure gradle check passes, or are there some more checks I can perform to ensure the code conforms to BC standards?

@dghgit
Copy link
Contributor

dghgit commented May 1, 2024

The gradle check should be enough - if there's anything that turns out to be missing we should be able to add it.

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