fix: ensure the EdDSA key generation produces a 32 bytes public key

[nikschul]

What this PR changes/adds

This PR fixes an edge case in the Ed25519 public key encoding process, specifically within the encodeX (EdECPoint point) method. It introduces a normalization step that guarantees the extracted Y-coordinate is strictly 32 bytes long before we reverse the array, apply the isXOdd parity bit, and encode it to Base64URL.

Why it does that

Previously, we relied directly on point.getY().toByteArray(). Because BigInteger dynamically sizes its output based on the mathematical value of the number, it introduces two unpredictable behaviors for cryptography:

• Dropped leading zeros: If the Y-coordinate happens to be small enough, BigInteger drops the leading zero byte, returning a 31-byte array. This resulted in truncated 42-character Base64URL strings (instead of the required 43 characters).

• Sign extension bytes: If the highest bit of a 32-byte number is 1, BigInteger prepends an extra 0x00 byte to keep the number positive, returning a 33-byte array.

Because the array length was dynamic, our isXOdd mask (bytes[bytes.length - 1] ^= mask) was occasionally being applied to the wrong byte index. Forcing a strict 32-byte array ensures standard-compliant Ed25519 JWKs and guarantees the parity bit is always applied to the correct index.

Further notes

• Added a normalization block to handle stripping extra sign bytes or padding missing zero bytes.

• The isXOdd mask index is now safely hardcoded to bytes[31] since the array size is guaranteed.

• Two tests are added, to test for both edge cases with 31 byte and 33 byte keys

Linked Issue(s)

Closes #4445
Closes Identity Hub Issue #913

[github-actions]

We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contributors manual, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.

[paullatzelsperger]

looks good, minor nits aside.

[jimmarino]

Doing a quick drive-by, can't this all be simplified by using PublicKey.getEncoded() and then parsing it with SubjectPublicKeyInfo to get the bytes from there?

var encoded = publicKey.getEncoded();
var spki =  SubjectPublicKeyInfo.getInstance(ASN1Primitive.fromByteArray(encoded));
var bytes = spki.getPublicKeyData().getBytes();

[paullatzelsperger]

there seems to be no test for the length violation now, which was the original trigger for the PR.

[paullatzelsperger]

the code is now more than 2 years old, maybe Nimbus now has a way to convert JDK types (PrivateKey and PublicKey) into Nimbus types? @nikschul could you investigate this?

[nikschul]

the code is now more than 2 years old, maybe Nimbus now has a way to convert JDK types (PrivateKey and PublicKey) into Nimbus types? @nikschul could you investigate this?

as far as I can see, there is no java.security.interfaces.EdECPublicKey constructor to the Nimbus OctetKeyPair

the code is now more than 2 years old, maybe Nimbus now has a way to convert JDK types (PrivateKey and PublicKey) into Nimbus types? @nikschul could you investigate this?

@paullatzelsperger
Even in the newest version of Nimbus JOSE+JWT(10.8) there is no constructor for an OctetKeyPair from a java.security.interfaces.EdECPublicKey https://javadoc.io/doc/com.nimbusds/nimbus-jose-jwt/latest/com/nimbusds/jose/jwk/OctetKeyPair.html

This is likely because converting an EdECPublicKey to the raw bytes required by the JWK OKP format is non-trivial: the JDK represents the public key point's Y coordinate as a BigInteger, which is big-endian and strips leading zeros, whereas Ed25519 requires a fixed 32-byte little-endian encoding. Thus, a naive conversion would silently produce incorrect key material in some cases.

[nikschul]

there seems to be no test for the length violation now, which was the original trigger for the PR.

I have added two more tests for the faulty keys with 31 bytes and 33 bytes

[ndr-brt]

all good implementation wise, just added some cosmetic comments.

This will close also #4445, could you please link it to this PR?

[ndr-brt]

nit: superfluous comment (to what's already mentioned in the exception message)

[ndr-brt]

Since this test class is getting bigger, could you please group the tests related to createJwt in a nested dedicated subclass? That will make the tests easier to browse

[paullatzelsperger]

The two tests createJwk_withLeadingZeroInXcoordinate_preservesAll32Bytes and createJwk_edDsaKey_handles33ByteYcoordinate_stripsLeadingZero don't actually test what their name suggests, since the mocked public key's getEncoded() method always returns an empty byte array.
This always causes an IllegalArgumentException to be raised by the converter, since the spki variable is null. Therefore, those tests only verify that an exception is raised if the encoded bytes are empty.

[jimmarino]

@nikschul Can you please investigate the approach I highlighted with @paullatzelsperger? It will avoid a lot of the issues we are seeing in the conversion and tests.

[nikschul]

@nikschul Can you please investigate the approach I highlighted with @paullatzelsperger? It will avoid a lot of the issues we are seeing in the conversion and tests.

@jimmarino
I am not exactly sure which approach you are referirng to. If you are talking about this approach:

Doing a quick drive-by, can't this all be simplified by using PublicKey.getEncoded() and then parsing it with SubjectPublicKeyInfo to get the bytes from there?

var encoded = publicKey.getEncoded();
var spki =  SubjectPublicKeyInfo.getInstance(ASN1Primitive.fromByteArray(encoded));
var bytes = spki.getPublicKeyData().getBytes();

please take a look at 455-461 of the CryptoConverter.java, in which already included your suggestion.

Or are you referring to a different approach?

[paullatzelsperger]

I'm sorry to be a nit-picker, but it seems we have arrived in a place where the original byte-length problem isn't a problem anymore, or cannot be replicated, because we are using a different (higher-level) API internally, that foregoes the BigInteger problem altogether.
This becomes apparent where we have to use reflection to test an internal class, which is usually a big smell.

Regarding the testing, I think we should do a complete mind-reset, after which we arrive at roughly the following test cases for the createJwk method:

• Ed25519 key pair is valid
• key pair does not contain a public key
• public key contains garbage bytes (could be parameterized test)
• public key contains an empty or null byte array

Also, please do resolve comments if you have fixed them in code, this makes it much easier to digest

Note: please do still regard my comment in the CryptoConverter.java class.

[nikschul]

I'm sorry to be a nit-picker, but it seems we have arrived in a place where the original byte-length problem isn't a problem anymore, or cannot be replicated, because we are using a different (higher-level) API internally, that foregoes the BigInteger problem altogether. This becomes apparent where we have to use reflection to test an internal class, which is usually a big smell.

Regarding the testing, I think we should do a complete mind-reset, after which we arrive at roughly the following test cases for the createJwk method:

* Ed25519 key pair is valid

* key pair does not contain a public key

* public key contains garbage bytes (could be parameterized test)

* public key contains an empty or null byte array

Also, please do resolve comments if you have fixed them in code, this makes it much easier to digest

Note: please do still regard my comment in the CryptoConverter.java class.

I've updated the test cases accordingly.