DSPX-1165: don't use jce namespaces
#269
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mkleene, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on refactoring the ECKeyPair class within the SDK. The changes involve updating the internal representation of key pairs, reducing explicit dependencies on the Bouncy Castle provider name in API calls, and revising the methods for handling compressed EC public keys. Related changes include updating imports in NanoTDF.java and TDF.java to use standard Java security interfaces and removing a test case corresponding to the removed X.509 functionality.
Highlights
- ECKeyPair Refactoring: The
ECKeyPairclass has been refactored to store the public and private keys directly asECPublicKeyandECPrivateKeyinterfaces instead of a genericKeyPairobject. These fields are now also marked asfinal. - Reduced Explicit Provider Usage: Explicit references to the Bouncy Castle provider name ("BC") have been removed from many
getInstancecalls for cryptographic algorithms (KeyPairGeneratorSpi,KeyFactory,KeyAgreement,Signature), relying instead on the provider being correctly added to the security list. - EC Public Key Handling Updates: The logic for compressing EC public keys has been moved into a new private helper method (
getCompressedECPublicKey), and the method for creating a public key from a compressed EC point (publicKeyFromECPoint) has been refactored to use standard Java security APIs where possible. - Removed X.509 Certificate Functionality: The static method
getPEMPublicKeyFromX509Cert, which extracted a public key from an X.509 certificate PEM string, has been removed from theECKeyPairclass.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request primarily refactors the ECKeyPair class to use standard Java security interfaces for EC keys (java.security.interfaces.ECPublicKey, java.security.interfaces.ECPrivateKey) instead of Bouncy Castle specific ones. It also removes explicit Bouncy Castle provider specifications in many JCA calls, relying on the statically registered provider. Several methods related to key generation, compression, and conversion have been updated. The tests have been adjusted accordingly, though one assertion has been commented out.
The PR title "save this" and the empty description are not informative and should be improved for better context and traceability.
As no specific style guide was provided, this review adheres to common Java best practices and the Google Java Style Guide principles for readability and maintainability.
| // String publicKey = ECKeyPair.publicKeyFromECPoint(ecPoint, | ||
| // ECKeyPair.NanoTDFECCurve.SECP256R1.toString()); | ||
| // assertArrayEquals(ECKeys.sdkPublicKey.toCharArray(), publicKey.toCharArray()); |
There was a problem hiding this comment.
These lines testing ECKeyPair.publicKeyFromECPoint have been commented out. If this functionality is still expected to work after the refactoring of publicKeyFromECPoint, the test should be updated and re-enabled. Commenting out tests reduces coverage and can hide regressions.
If the test was commented out due to difficulties in comparing PEM strings (e.g., whitespace/line ending issues), consider a more robust comparison method. For example, parse both PEM strings back to ECPublicKey objects and compare their encoded forms (getEncoded()) or relevant parameters.
| import org.bouncycastle.asn1.x9.X9ECParameters; | ||
| import org.bouncycastle.asn1.x9.X9ECPoint; | ||
| import org.bouncycastle.crypto.digests.SHA256Digest; | ||
| import org.bouncycastle.crypto.generators.HKDFBytesGenerator; | ||
| import org.bouncycastle.crypto.params.HKDFParameters; | ||
| import org.bouncycastle.jcajce.provider.asymmetric.ec.KeyPairGeneratorSpi; | ||
| import org.bouncycastle.jce.ECNamedCurveTable; | ||
| import org.bouncycastle.jce.interfaces.ECPrivateKey; | ||
| import org.bouncycastle.jce.interfaces.ECPublicKey; | ||
| import org.bouncycastle.jcajce.util.ECKeyUtil; | ||
| import org.bouncycastle.jce.provider.BouncyCastleProvider; | ||
| import org.bouncycastle.jce.spec.ECNamedCurveParameterSpec; | ||
| import org.bouncycastle.math.ec.ECCurve; | ||
| import org.bouncycastle.math.ec.ECPoint; | ||
| import org.bouncycastle.openssl.PEMException; | ||
| import org.bouncycastle.openssl.PEMParser; | ||
| import org.bouncycastle.openssl.jcajce.JcaPEMKeyConverter; | ||
| import org.bouncycastle.pqc.jcajce.provider.util.KeyUtil; |
There was a problem hiding this comment.
There are a few unused imports in this block. Consider removing them to keep the import section clean and reduce clutter.
Specifically, the following imports appear to be unused in the current version of the file:
org.bouncycastle.asn1.x9.X9ECParametersorg.bouncycastle.jcajce.util.ECKeyUtilorg.bouncycastle.pqc.jcajce.provider.util.KeyUtil
| ECParameterSpec ecParameterSpec = algorithmParameters.getParameterSpec(ECParameterSpec.class); | ||
|
|
||
| ECPublicKeySpec spec = new ECPublicKeySpec(jpoint, ecParameterSpec); | ||
| KeyFactory keyFactory = KeyFactory.getInstance("ECDSA"); |
There was a problem hiding this comment.
The standard algorithm name for KeyFactory when dealing with Elliptic Curve keys is "EC". Using "ECDSA" here might be incorrect or rely on provider-specific behavior, as "ECDSA" is typically an algorithm for Signature objects. It's recommended to use "EC" for consistency with AlgorithmParameters.getInstance("EC") used earlier in this method and for better adherence to standard JCA algorithm names.
| KeyFactory keyFactory = KeyFactory.getInstance("ECDSA"); | |
| KeyFactory keyFactory = KeyFactory.getInstance("EC"); |
|
Right now the PEM formatting is wrong because without an
ECNamedCurveParameterSpecwe serialize the PEMusing explicit curve parameters instead of an OID. But this seems to only be used internally for tests so we
might be able to find another way of guaranteeing that things are consistent internally.