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

Rewrite PGPKeyConverter classes to fix support for X448,Ed448,X25519,… #1663

Merged
merged 3 commits into from
May 23, 2024

Conversation

vanitasvitae
Copy link
Contributor

@vanitasvitae vanitasvitae commented May 14, 2024

…Ed25519 keys

Obsoletes #1658

This patch

  • Adds support for X448 in PGPKeyConverter.implGetKdfParameters()
  • Adds support for legacy X448 in BcPGPKeyConverter
  • Adds support for legacy Ed448 in BcPGPKeyConverter.getPrivateBCPGKey()
  • Rewrites BcPGPKeyConverter.getPublicBCPGKey() to detect key types based on algorithm ID
    • Fixes proper return types
      • legacy Ed448: Ed448PublicBCPGKey -> EdDSAPublicBCPGKey
      • legacy X448: X448PublicBCPGKey -> ECDHPublicBCPGKey
  • JcaPGPKeyConverter.getPGPPublicKey():
    • Add detection of legacy X25519 using OID 1.3.101.110
    • Add support for legacy X448
    • Add support for legacy Ed448
  • JcaPGPKeyConverter.getPublicKey():
    • Add support for legacy X448
    • Add support for legacy Ed448
  • Rewrite JcaPGPKeyConverter.getPublicBCPGKey() to detect key types based on algorithm ID
    • Add support for converting XDHPublicKeyImpl keys
    • Add support for legacy Ed448
    • Add support for legacy X448
  • Fixes X25519, X448 keys (using the new dedicated PublicKeyAlgorithmTags) to use native encoding for the private key material
  • Fixes generation of legacy Ed448 keys (using PublicKeyAlgorithmTags.EDDSA_LEGACY together with an Ed448 key (BC15EdDSAPublicKey passed into JcaPGPKeyConverter) caused PGPException (unknown key class))
  • Fixes conversion of legacy Ed25519 keys (if setProvider() is not called on JcaPGPKeyConverter)
    • BcPGPKeyPair of Ed25519 key + ECDH algorithm tag cannot be converted to JcaPGPKeyPair
      • EdDSAPublicKeyImpl is not recognized in JcaPGPKeyConverter.getPublicBCPGKey() -> PGPException("unknown key class")
  • Fixes generation of legacy X448 keys (JcaPGPKeyPair, PublicKeyAlgorithmTags.ECDH together with an X448 key)
    • BC11XDHPublicKey is not recognized in JcaPGPKeyConverter.getPublicBCPGKey()
  • Fixes conversion of legacy X25519 keys (BcPGPKeyPair with PublicKeyAlgorithmTags.EDDSA_LEGACY and an XDHPublicKeyImpl cannot be converted to a JcaPGPKeyPair) (if setProvider() is not called on JcaPGPKeyConverter)
    • JcaPGPKeyConverter.getPublicBCPGKey() does not recognize XDHPublicKeyImpl

Further, this patch adds comments to make it easier to distinguish the different branches for different key types.

With the patch, X25519, X448, Ed25519, Ed448 keys can be properly converted between BC, JCA whether or not JcaPGPKeyConverter.setProvider(new BouncyCastleProvider()) is called.

@vanitasvitae
Copy link
Contributor Author

I noticed, that crypto-refresh-13 states that X25519 keys using the dedicated algorithm ID not only do not use MPI encoding, but also do not encode the key in reverse order.
I'm working on getting this fixed, but if I just remove reverse calls, my tests break :/ This will take a while.

@vanitasvitae
Copy link
Contributor Author

I think I got it. The reverse coding only affects secret key material. I was mostly focussing on public key material during my investigations.

@vanitasvitae
Copy link
Contributor Author

vanitasvitae commented May 21, 2024

I rebased the PR into two commits.
The first one demonstrates that the current key conversion implementation is broken by adding tests.
The second one contains the patch fixing the tests.

Edit: This PR also fixes #1584

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.

None yet

2 participants