chore: [WIP] PQC for TLS proof of concept#12970
Conversation
diegomarquezp
changed the title
![gemini-code-assist[bot]](https://avatars.github.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 for gRPC and HTTP/JSON transports by integrating Conscrypt and enforcing specific TLS 1.3 named groups. It also adds a new pqc-test module to verify connectivity against PQC-enforcing servers. The review feedback identifies several critical issues: the modification of global JVM state (security providers and system properties) within library methods which can cause side effects for other application components, a missing dependency in the gax-grpc module, and a regression that breaks compatibility with non-Netty gRPC transports.
| import java.security.GeneralSecurityException; | ||
| import java.security.KeyStore; | ||
| import java.security.Security; | ||
| import org.conscrypt.Conscrypt; |
There was a problem hiding this comment.
The import of org.conscrypt.Conscrypt will likely cause a compilation error as the conscrypt-openjdk-uber dependency is missing from the gax-grpc module's POM file (it was only added to gax-httpjson).
| Security.insertProviderAt(Conscrypt.newProvider(), 1); | ||
|
|
||
| // Force the use of the PQC algorithm group. | ||
| System.setProperty("jdk.tls.namedGroups", "X25519MLKEM768"); |
There was a problem hiding this comment.
Modifying global JVM state by calling Security.insertProviderAt and setting the jdk.tls.namedGroups system property inside a library method is problematic. These changes affect all TLS connections in the entire application, not just those created by this provider. This can lead to unexpected behavior or failures in other parts of the system that are not PQC-compatible. Consider making this configuration opt-in and scoped to the specific channel if possible.
| } else { | ||
| throw new IllegalStateException("Expected NettyChannelBuilder but got " + builder.getClass().getName()); | ||
| } |
There was a problem hiding this comment.
Throwing an IllegalStateException when the builder is not a NettyChannelBuilder breaks compatibility with other gRPC transport implementations (e.g., OkHttpChannelBuilder or InProcessChannelBuilder). Since applyPqcConfiguration is called by default, this change effectively prevents the use of any non-Netty transport. The PQC configuration should be applied gracefully or only when the transport supports it.
| java.security.Security.insertProviderAt(org.conscrypt.Conscrypt.newProvider(), 1); | ||
| System.setProperty("jdk.tls.namedGroups", "X25519MLKEM768"); |
There was a problem hiding this comment.
Modifying global security providers and system properties here affects the entire JVM. This should be avoided in library code to prevent side effects on other components.
| java.security.Security.insertProviderAt(org.conscrypt.Conscrypt.newProvider(), 1); | ||
| System.setProperty("jdk.tls.namedGroups", "X25519MLKEM768"); |
There was a problem hiding this comment.
Modifying global JVM state within a static factory method is a side effect that can impact other parts of the application. This configuration should be localized or made optional.
1 participant
No description provided.