chore: [wip] PQC POC 2#13203
Conversation
TAG=agy CONV=0ade5891-3c8d-4e27-a240-b1a8cd6a0b0c
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Post-Quantum Cryptography (PQC) support across the Java SDK by integrating Bouncy Castle and configuring PQC-hardened SSL contexts for gRPC and HTTP/JSON transports. Key changes include the addition of a pqc-test module, reflection-based configuration for Netty channel builders, and updates to OAuth2Utils and InstantiatingHttpJsonChannelProvider. Feedback identifies issues with potential initialization errors in OAuth2Utils, the use of a SNAPSHOT dependency version, and performance overhead from repeated reflection lookups. Suggestions were also provided for improving exception handling, ensuring logic consistency in transport creation, and cleaning up test provider registrations.
…ranch chore/pqc-poc-2 TAG=agy CONV=0ade5891-3c8d-4e27-a240-b1a8cd6a0b0c
TAG=agy CONV=0ade5891-3c8d-4e27-a240-b1a8cd6a0b0c
This commit addresses the review feedback by implementing: 1. Graceful fallback to default NetHttpTransport with WARNING logging in OAuth2Utils static initializer. 2. warning-level exception logging in InstantiatingGrpcChannelProvider. 3. Caching of shaded/unshaded gRPC Netty OpenSSL reflection lookups inside thread-safe static OpenSslReflectionHolder to remove runtime overhead. 4. Reverting createHttpTransport in InstantiatingHttpJsonChannelProvider to return null when mTLS is not active, as default transport is already PQC-hardened. 5. Clean unregistration of BC and BCJSSE security providers in integration test server/client teardown. TAG=agy
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements Post-Quantum Cryptography (PQC) support by hardening HTTP and gRPC transports with Bouncy Castle JSSE. Key changes include updating OAuth2Utils to use a PQC-hardened transport, adding Bouncy Castle dependencies to GAX modules, and introducing reflection-based configuration for gRPC channels to support hybrid PQC key exchange. A new integration test module, pqc-test, is also introduced. Feedback highlights the use of a -SNAPSHOT dependency version in the parent POM, code duplication and brittle string-based class checks in the gRPC reflection logic, and unused imports.
| <javax.annotation-api.version>1.3.2</javax.annotation-api.version> | ||
| <grpc.version>1.81.0</grpc.version> | ||
| <google.http-client.version>2.1.0</google.http-client.version> | ||
| <google.http-client.version>2.1.1-SNAPSHOT</google.http-client.version> |
There was a problem hiding this comment.
Updating google.http-client.version to a -SNAPSHOT version in a parent POM is generally discouraged for release-track projects. This can lead to unstable builds and dependency resolution issues in environments without access to the specific snapshot repository. If this is necessary for the POC, please ensure it is reverted or replaced with a stable version before merging.
| private ManagedChannelBuilder<?> applyPqcConfiguration(ManagedChannelBuilder<?> builder) { | ||
| // Configure the PQ and classical hybrid named groups: | ||
| // 1. X25519MLKEM768 (codepoint 4588): Hybrid classical (X25519) + post-quantum (ML-KEM-768) key exchange. | ||
| // Provides defense-in-depth: if ML-KEM is compromised, security reverts to classical strength of X25519. | ||
| // 2. MLKEM768 (codepoint 1896): Pure post-quantum key exchange using ML-KEM-768. | ||
| // 3. X25519 (codepoint 29): Classical elliptic curve Diffie-Hellman key exchange, used as a fallback. | ||
| String[] hybridGroups = new String[] {"X25519MLKEM768", "MLKEM768", "X25519"}; | ||
| String builderClassName = builder.getClass().getName(); | ||
| boolean isShaded = "io.grpc.netty.shaded.io.grpc.netty.NettyChannelBuilder".equals(builderClassName); | ||
| boolean isUnshaded = "io.grpc.netty.NettyChannelBuilder".equals(builderClassName); | ||
|
|
||
| if (isShaded && OpenSslReflectionHolder.SHADED_AVAILABLE) { | ||
| try { | ||
| Object sslContextBuilder = OpenSslReflectionHolder.SHADED_FOR_CLIENT.invoke(null); | ||
| OpenSslReflectionHolder.SHADED_OPTION_METHOD.invoke( | ||
| sslContextBuilder, OpenSslReflectionHolder.SHADED_GROUPS_OPTION, (Object) hybridGroups); | ||
| Object sslContext = OpenSslReflectionHolder.SHADED_BUILD_METHOD.invoke(sslContextBuilder); | ||
| OpenSslReflectionHolder.SHADED_SSL_CONTEXT_METHOD.invoke(builder, sslContext); | ||
| return builder; | ||
| } catch (java.lang.reflect.InvocationTargetException | IllegalAccessException | RuntimeException e) { | ||
| LOG.log(Level.WARNING, "Failed to configure shaded PQC transport fallback", e); | ||
| } | ||
| } else if (isUnshaded && OpenSslReflectionHolder.UNSHADED_AVAILABLE) { | ||
| try { | ||
| Object sslContextBuilder = OpenSslReflectionHolder.UNSHADED_FOR_CLIENT.invoke(null); | ||
| OpenSslReflectionHolder.UNSHADED_OPTION_METHOD.invoke( | ||
| sslContextBuilder, OpenSslReflectionHolder.UNSHADED_GROUPS_OPTION, (Object) hybridGroups); | ||
| Object sslContext = OpenSslReflectionHolder.UNSHADED_BUILD_METHOD.invoke(sslContextBuilder); | ||
| OpenSslReflectionHolder.UNSHADED_SSL_CONTEXT_METHOD.invoke(builder, sslContext); | ||
| return builder; | ||
| } catch (java.lang.reflect.InvocationTargetException | IllegalAccessException | RuntimeException e) { | ||
| LOG.log(Level.WARNING, "Failed to configure unshaded PQC transport fallback", e); | ||
| } | ||
| } | ||
| return builder; | ||
| } |
There was a problem hiding this comment.
The applyPqcConfiguration method contains significant code duplication between the shaded and unshaded Netty configuration paths. Additionally, checking the builder class name via string comparison (lines 938-939) is brittle and may fail if the class is renamed or wrapped.
Consider refactoring OpenSslReflectionHolder to store two instances of a helper class (e.g., NettyReflectionMetadata) that encapsulates the reflected methods and classes for each variant. You can then use Class.isInstance() for a more robust type check and a single helper method to apply the configuration.
References
- If code is duplicated and needs to be shared, move it to a separate helper/utility class.
| import javax.net.ssl.SSLContext; | ||
| import java.security.NoSuchAlgorithmException; |
There was a problem hiding this comment.
The imports for javax.net.ssl.SSLContext and java.security.NoSuchAlgorithmException are unused in this file and should be removed.
1 participant
No description provided.