Skip to content

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

Merged
hubot merged 3 commits into
bcgit:mainfrom
pgpainless:fixKeyConversion
May 23, 2024
Merged

Rewrite PGPKeyConverter classes to fix support for X448,Ed448,X25519,…#1663
hubot merged 3 commits into
bcgit:mainfrom
pgpainless:fixKeyConversion

Conversation

@vanitasvitae

@vanitasvitae vanitasvitae commented May 14, 2024

Copy link
Copy Markdown
Contributor

…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
Copy Markdown
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
Copy Markdown
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

vanitasvitae commented May 21, 2024

Copy link
Copy Markdown
Contributor Author

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.

2 participants