feat(gax-httpjson): Add Post Quantum Cryptography (PQC) Support by default via Conscrypt#13853
feat(gax-httpjson): Add Post Quantum Cryptography (PQC) Support by default via Conscrypt#13853lqiu96 wants to merge 32 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces Post-Quantum Cryptography (PQC) TLS negotiation for HTTP/JSON clients by integrating Conscrypt as a security provider and adding corresponding integration tests. The reviewer feedback focuses on improving the robustness and architecture of this implementation. Specifically, the reviewer recommends removing the undesirable coupling from the core HTTP module to the transport-specific module by defining the PQC groups locally. Additionally, the reviewer suggests wrapping the Conscrypt named groups configuration in a defensive try-catch block and separating the builder creation from the build calls to prevent genuine configuration or mTLS errors from being incorrectly swallowed as Conscrypt loading failures.
| try { | ||
| return new NetHttpTransport.Builder() | ||
| .setSecurityProvider(Conscrypt.newProvider()) | ||
| .setSslSocketConfigurator( | ||
| socket -> { | ||
| if (Conscrypt.isConscrypt(socket)) { | ||
| Conscrypt.setNamedGroups( | ||
| socket, InstantiatingHttpJsonChannelProvider.DEFAULT_PQC_GROUPS); | ||
| } | ||
| }) | ||
| .build(); | ||
| } catch (Throwable t) { | ||
| LOG.log( | ||
| Level.FINE, "Conscrypt native libraries not available. Falling back to JDK TLS.", t); | ||
| return new NetHttpTransport.Builder().build(); | ||
| } |
There was a problem hiding this comment.
Refactor the transport builder initialization to:
- Use the locally defined
PQC_GROUPSinstead of referencingInstantiatingHttpJsonChannelProvider.DEFAULT_PQC_GROUPS. - Wrap the
Conscrypt.setNamedGroupscall in a defensivetry-catchblock to prevent runtime connection failures if an older or incompatible version of Conscrypt is present on the classpath. - Separate the builder creation from the
build()call so that any potential configuration/build exceptions are not incorrectly swallowed and logged as Conscrypt native library loading failures.
NetHttpTransport.Builder builder = new NetHttpTransport.Builder();
try {
builder.setSecurityProvider(Conscrypt.newProvider())
.setSslSocketConfigurator(
socket -> {
if (Conscrypt.isConscrypt(socket)) {
try {
Conscrypt.setNamedGroups(socket, PQC_GROUPS);
} catch (Exception e) {
LOG.log(Level.WARNING, "Failed to set PQC named groups on Conscrypt socket", e);
}
}
});
} catch (Throwable t) {
LOG.log(
Level.FINE, "Conscrypt native libraries not available. Falling back to JDK TLS.", t);
}
return builder.build();References
- Avoid inverting layering by making lower-level channel implementations depend on higher-level RPC wrappers. If code is duplicated and needs to be shared, move it to a separate helper/utility class.
| try { | ||
| NetHttpTransport.Builder builder = | ||
| new NetHttpTransport.Builder() | ||
| .setSecurityProvider(Conscrypt.newProvider()) | ||
| .setSslSocketConfigurator( | ||
| socket -> { | ||
| if (Conscrypt.isConscrypt(socket)) { | ||
| Conscrypt.setNamedGroups(socket, DEFAULT_PQC_GROUPS); | ||
| } | ||
| }); | ||
| return configureMtls(builder).build(); | ||
| } catch (Throwable t) { | ||
| LOG.log(Level.FINE, "Conscrypt native libraries not available. Falling back to JDK TLS.", t); | ||
| return configureMtls(new NetHttpTransport.Builder()).build(); | ||
| } |
There was a problem hiding this comment.
Refactor the transport builder initialization to:
- Wrap the
Conscrypt.setNamedGroupscall in a defensivetry-catchblock to prevent runtime connection failures if an older or incompatible version of Conscrypt is present on the classpath. - Separate the builder creation from the
configureMtls(builder).build()call. This ensures that any genuine mTLS configuration errors (e.g., key store issues) are correctly propagated to the caller instead of being caught, logged as Conscrypt native library loading failures, and retried.
NetHttpTransport.Builder builder = new NetHttpTransport.Builder();
try {
builder.setSecurityProvider(Conscrypt.newProvider())
.setSslSocketConfigurator(
socket -> {
if (Conscrypt.isConscrypt(socket)) {
try {
Conscrypt.setNamedGroups(socket, DEFAULT_PQC_GROUPS);
} catch (Exception e) {
LOG.log(Level.WARNING, "Failed to set PQC named groups on Conscrypt socket", e);
}
}
});
} catch (Throwable t) {
LOG.log(Level.FINE, "Conscrypt native libraries not available. Falling back to JDK TLS.", t);
}
return configureMtls(builder).build();| import com.google.api.gax.core.GaxProperties; | ||
| import com.google.api.gax.httpjson.HttpHeadersUtils; | ||
| import com.google.api.gax.httpjson.HttpJsonStatusCode; | ||
| import com.google.api.gax.httpjson.InstantiatingHttpJsonChannelProvider; |
There was a problem hiding this comment.
This import is unnecessary and introduces an undesirable coupling from the core HTTP module (google-cloud-core-http) to the transport-specific module (gax-httpjson). We can remove this import by defining the PQC groups array locally within this class.
References
- Avoid inverting layering by making lower-level channel implementations depend on higher-level RPC wrappers. If code is duplicated and needs to be shared, move it to a separate helper/utility class.
|
|
||
| public static class DefaultHttpTransportFactory implements HttpTransportFactory { | ||
|
|
||
| private static final Logger LOG = Logger.getLogger(DefaultHttpTransportFactory.class.getName()); |
There was a problem hiding this comment.
Define the PQC groups array locally to avoid coupling with gax-httpjson.
| private static final Logger LOG = Logger.getLogger(DefaultHttpTransportFactory.class.getName()); | |
| private static final Logger LOG = Logger.getLogger(DefaultHttpTransportFactory.class.getName()); | |
| private static final String[] PQC_GROUPS = | |
| new String[] {"X25519MLKEM768", "SecP256r1MLKEM768", "X25519"}; |
References
- Avoid inverting layering by making lower-level channel implementations depend on higher-level RPC wrappers. If code is duplicated and needs to be shared, move it to a separate helper/utility class.
There was a problem hiding this comment.
Is java-core already dependent on gax-httpjson?
There was a problem hiding this comment.
This comment was from an older change didn't seem to want to import InstantiatingHttpJsonChannelProvider for constants. Moved it all to a Util class (HttpJsonTransportUtils) that can be shared between the two instead.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Post-Quantum Cryptography (PQC) TLS negotiation support for HTTP/JSON clients by registering Conscrypt as the security provider and configuring default PQC named groups. The feedback suggests utilizing lazy initialization and caching for the Conscrypt security provider in both InstantiatingHttpJsonChannelProvider and HttpTransportOptions to prevent the performance and memory overhead of instantiating a new provider on every transport creation.
| public static final String[] DEFAULT_PQC_GROUPS = | ||
| new String[] {"X25519MLKEM768", "SecP256r1MLKEM768", "X25519"}; |
There was a problem hiding this comment.
Prefer lazy initialization and caching for resource-intensive objects like the Conscrypt security provider. Creating a new Provider instance on every transport creation can lead to significant performance overhead and high memory usage.
We can introduce a static lazy holder class to cache the Conscrypt provider instance safely, ensuring it is only initialized once and does not cause class-loading issues if Conscrypt is not present on the classpath.
public static final String[] DEFAULT_PQC_GROUPS =
new String[] {"X25519MLKEM768", "SecP256r1MLKEM768", "X25519"};
private static class ConscryptProviderHolder {
private static final java.security.Provider INSTANCE = createProvider();
private static java.security.Provider createProvider() {
try {
return Conscrypt.newProvider();
} catch (Throwable t) {
LOG.log(
Level.FINE,
"Conscrypt native libraries not available. Falling back to JDK TLS.",
t);
return null;
}
}
}References
- Prefer lazy initialization over eager initialization for resource-intensive objects (such as CharsetEncoder) if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.
| NetHttpTransport.Builder builder = new NetHttpTransport.Builder(); | ||
| try { | ||
| builder | ||
| .setSecurityProvider(Conscrypt.newProvider()) | ||
| .setSslSocketConfigurator( | ||
| socket -> { | ||
| if (Conscrypt.isConscrypt(socket)) { | ||
| try { | ||
| Conscrypt.setNamedGroups(socket, DEFAULT_PQC_GROUPS); | ||
| } catch (Throwable t) { | ||
| // Catch runtime socket configuration errors (e.g. version mismatch or JNI | ||
| // error) | ||
| // and fall back to Conscrypt's default TLS groups without failing the request. | ||
| LOG.log( | ||
| Level.FINE, | ||
| "Failed to set PQC named groups on Conscrypt socket. Falling back to" | ||
| + " Conscrypt default TLS groups.", | ||
| t); | ||
| } | ||
| } | ||
| }); | ||
| } catch (Throwable t) { | ||
| LOG.log(Level.FINE, "Conscrypt native libraries not available. Falling back to JDK TLS.", t); | ||
| builder = new NetHttpTransport.Builder(); | ||
| } |
There was a problem hiding this comment.
Use the cached Conscrypt provider instance from ConscryptProviderHolder instead of creating a new provider instance on every call to createHttpTransport(). This avoids unnecessary performance and memory overhead.
NetHttpTransport.Builder builder = new NetHttpTransport.Builder();
java.security.Provider conscryptProvider = ConscryptProviderHolder.INSTANCE;
if (conscryptProvider != null) {
builder
.setSecurityProvider(conscryptProvider)
.setSslSocketConfigurator(
socket -> {
if (Conscrypt.isConscrypt(socket)) {
try {
Conscrypt.setNamedGroups(socket, DEFAULT_PQC_GROUPS);
} catch (Throwable t) {
// Catch runtime socket configuration errors (e.g. version mismatch or JNI
// error)
// and fall back to Conscrypt's default TLS groups without failing the request.
LOG.log(
Level.FINE,
"Failed to set PQC named groups on Conscrypt socket. Falling back to"
+ " Conscrypt default TLS groups.",
t);
}
}
});
}References
- Prefer lazy initialization over eager initialization for resource-intensive objects (such as CharsetEncoder) if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.
| private static final String[] PQC_GROUPS = | ||
| new String[] {"X25519MLKEM768", "SecP256r1MLKEM768", "X25519"}; | ||
| private static final HttpTransportFactory INSTANCE = new DefaultHttpTransportFactory(); |
There was a problem hiding this comment.
Prefer lazy initialization and caching for resource-intensive objects like the Conscrypt security provider. Creating a new Provider instance on every transport creation can lead to significant performance overhead and high memory usage.
We can introduce a static lazy holder class to cache the Conscrypt provider instance safely, ensuring it is only initialized once and does not cause class-loading issues if Conscrypt is not present on the classpath.
private static final String[] PQC_GROUPS =
new String[] {"X25519MLKEM768", "SecP256r1MLKEM768", "X25519"};
private static final HttpTransportFactory INSTANCE = new DefaultHttpTransportFactory();
private static class ConscryptProviderHolder {
private static final java.security.Provider INSTANCE = createProvider();
private static java.security.Provider createProvider() {
try {
return Conscrypt.newProvider();
} catch (Throwable t) {
LOG.log(
Level.FINE,
"Conscrypt native libraries not available. Falling back to JDK TLS.",
t);
return null;
}
}
}References
- Prefer lazy initialization over eager initialization for resource-intensive objects (such as CharsetEncoder) if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.
| NetHttpTransport.Builder builder = new NetHttpTransport.Builder(); | ||
| try { | ||
| builder | ||
| .setSecurityProvider(Conscrypt.newProvider()) | ||
| .setSslSocketConfigurator( | ||
| socket -> { | ||
| if (Conscrypt.isConscrypt(socket)) { | ||
| try { | ||
| Conscrypt.setNamedGroups(socket, PQC_GROUPS); | ||
| } catch (Throwable t) { | ||
| // Catch runtime socket configuration errors (e.g. version mismatch or JNI | ||
| // error) | ||
| // and fall back to Conscrypt's default TLS groups without failing the | ||
| // request. | ||
| LOG.log( | ||
| Level.FINE, | ||
| "Failed to set PQC named groups on Conscrypt socket. Falling back to" | ||
| + " Conscrypt default TLS groups.", | ||
| t); | ||
| } | ||
| } | ||
| }); | ||
| } catch (Throwable t) { | ||
| LOG.log( | ||
| Level.FINE, "Conscrypt native libraries not available. Falling back to JDK TLS.", t); | ||
| builder = new NetHttpTransport.Builder(); | ||
| } |
There was a problem hiding this comment.
Use the cached Conscrypt provider instance from ConscryptProviderHolder instead of creating a new provider instance on every call to create(). This avoids unnecessary performance and memory overhead.
NetHttpTransport.Builder builder = new NetHttpTransport.Builder();
java.security.Provider conscryptProvider = ConscryptProviderHolder.INSTANCE;
if (conscryptProvider != null) {
builder
.setSecurityProvider(conscryptProvider)
.setSslSocketConfigurator(
socket -> {
if (Conscrypt.isConscrypt(socket)) {
try {
Conscrypt.setNamedGroups(socket, PQC_GROUPS);
} catch (Throwable t) {
// Catch runtime socket configuration errors (e.g. version mismatch or JNI
// error)
// and fall back to Conscrypt's default TLS groups without failing the
// request.
LOG.log(
Level.FINE,
"Failed to set PQC named groups on Conscrypt socket. Falling back to"
+ " Conscrypt default TLS groups.",
t);
}
}
});
}References
- Prefer lazy initialization over eager initialization for resource-intensive objects (such as CharsetEncoder) if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.
218f3e6 to
1a08829
Compare
| } | ||
| try { | ||
| Conscrypt.setNamedGroups(socket, DEFAULT_PQC_GROUPS); | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
Are there known checked exceptions? If not, catching Exception is fine but catching Throwable might hide larger problems. e.g. Even if a version mismatch is ignored here, it might blow up in other places.
| * | ||
| * <p>Returns {@code null} on failure so that transport creation can fall back to default JDK TLS, | ||
| * ensuring that setting Conscrypt as the default security provider does not cause breaking | ||
| * failures for customers running on environments where Conscrypt is unsupported or unavailable. |
There was a problem hiding this comment.
where Conscrypt is unsupported
What error would we run into?
|
|
||
| public static class DefaultHttpTransportFactory implements HttpTransportFactory { | ||
|
|
||
| private static final Logger LOG = Logger.getLogger(DefaultHttpTransportFactory.class.getName()); |
There was a problem hiding this comment.
Is java-core already dependent on gax-httpjson?
| * standard TLS 1.3 endpoints if ML-KEM is not negotiated. | ||
| * </ul> | ||
| */ | ||
| public static final String[] DEFAULT_PQC_GROUPS = |
There was a problem hiding this comment.
Does this need to be public?
There was a problem hiding this comment.
removed visibility (public was a remnant from an older change)
…qc-verification sample module
…streamline Conscrypt try-fallback
…rator in try block and remove draft Javadoc
…h blocks in GAX and Core HTTP
…pt is unavailable
…annelProviderTest
…n GAX and Core HTTP
…lProviderTest and HttpTransportOptionsTest
…t exception and restore HttpTransportOptionsTest to main
…ryptPqcConfiguratorHelper
…t classical non-PQC test case
…socket configurator
…Conscrypt log level to WARNING
…scrypt PQC fallback
…rHolder and comment catch block
…tProviderHolder Javadoc
…tcp readiness polling loop
…snapshot repositories
…on for Java 8 compatibility
…n pqc-httpjson-support
…uantumCryptography without constructing custom transport
b27c1ca to
50e0e64
Compare
… configureConscryptSecurityProvider
|
|


No description provided.