fix: Add TrustManagerFactory workaround for Conscrypt bug #1993
Conversation
|
This is adding an adapter so that if a customer's JVM has Conscrypt, then the connector will work around this Conscrypt bug and continue to work correctly. I will add some logic so that the connector only uses the workaround if Conscript is enabled. |
enocom
changed the title
core/src/main/java/com/google/cloud/sql/core/ConscryptWorkaroundTrustManagerFactory.java
Show resolved
Hide resolved
| Provider prov = ctx.getProvider(); | ||
| log.info("TLS Provider: {}", prov.getName()); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| // handle exception |
There was a problem hiding this comment.
Rethrow exception.
| @@ -0,0 +1,13 @@ | |||
| security.provider.1=Conscrypt | |||
| security.provider.2=SUN | |||
There was a problem hiding this comment.
Should we be setting all of these here or just a few?
There was a problem hiding this comment.
They should all be here, but Conscrypt should come first so it gets loaded first.
There was a problem hiding this comment.
Why do we need this big file? Is everything really necessary here?
There was a problem hiding this comment.
This file fully replaces the built-in JVM security properties. So we need to set all of the default security properties, not just the few properties that are relevant to prioritizing the Conscrypt JCE library.
There was a problem hiding this comment.
Could we say that in a comment in the file? Future maintainers are going to struggle to understand why this is here otherwise. Also, do we need all the extra whitespace below?
There was a problem hiding this comment.
I added some comments to the file to clarify.
hessjcg
force-pushed
the
gh-1983-fix
branch
5 times, most recently
from
6b4dfde
to
31a72fa
Compare
| @@ -229,5 +229,23 @@ | |||
| </plugin> | |||
| </plugins> | |||
| </build> | |||
| <profiles> | |||
| <profile> | |||
| <id>google-conscript</id> | |||
There was a problem hiding this comment.
This profile allows you to run the Postgres JDBC integration tests using Conscrypt crypto.
| @@ -0,0 +1,13 @@ | |||
| security.provider.1=Conscrypt | |||
| security.provider.2=SUN | |||
There was a problem hiding this comment.
Why do we need this big file? Is everything really necessary here?
| // Note: This is a workaround for Conscrypt bug #1033 | ||
| // Conscrypt is the JCE provider on some Google Cloud runtimes like DataProc. | ||
| // https://github.com/google/conscrypt/issues/1033 | ||
| if (ConscryptWorkaroundTrustManagerFactory.isWorkaroundNeeded()) { |
There was a problem hiding this comment.
How do these lines relate to ConscryptWorkaroundTrustManagerFactorySpi? Do we need both?
There was a problem hiding this comment.
In the JVM, we need to implement 3 classes to make sure that we are capturing all of the TrustManager instances created by the default Java Crypto provider and wrapping them with the ConscryptWorkaroundTrustManager:
class ConscryptWorkaroundTrustManagerFactory extends TrustManagerFactory- has a bunch of final methods that delegate to aTrustManagerFactorySpi.class ConscryptWorkaroundTrustManagerFactorySpi implements TrustManagerFactorySpi- can actually intercept and delegate calls related to trust managers and wrap them withConscryptWorkaroundTrustManagerConscryptWorkaroundTrustManager- the workaround for the Conscrypt bug.
There was a problem hiding this comment.
Got it. Thanks. Personally, this context would be useful to me in the future so I'd love to see this capture in the commit message or in a comment somewhere.
There was a problem hiding this comment.
I added this extra info to the ConscryptWorkaroundTrustManagerFactory class javadoc.
| // Note: This is a workaround for Conscrypt bug #1033 | ||
| // Conscrypt is the JCE provider on some Google Cloud runtimes like DataProc. | ||
| // https://github.com/google/conscrypt/issues/1033 | ||
| if (ConscryptWorkaroundTrustManagerFactory.isWorkaroundNeeded()) { |
There was a problem hiding this comment.
Got it. Thanks. Personally, this context would be useful to me in the future so I'd love to see this capture in the commit message or in a comment somewhere.
| @@ -0,0 +1,13 @@ | |||
| security.provider.1=Conscrypt | |||
| security.provider.2=SUN | |||
There was a problem hiding this comment.
Could we say that in a comment in the file? Future maintainers are going to struggle to understand why this is here otherwise. Also, do we need all the extra whitespace below?
|
I added comments for clarity. |
This is a workaround for an underlying bug in the Google Conscrypt crypto library
google/conscrypt #1033.
The root cause is that the Conscrypt and OpenJDK X509 certificate libraries sometimes interpret the AuthType
field differently: Conscrypt finds 'GENERIC' auth type when OpenJDK finds 'UNKNOWN' auth type. This causes certificate validation to fail.
The workaround implemented here is to add a delegate
TrustManagerthat replaces 'GENERIC' auth type with 'UNKNOWN' auth type so that the Conscrypt crypto plays nice with the JDK crypto. See comment on #1033.I manually tested this on a modified JVM that used Conscrypt as it's primary crypto library. The integration tests passed. I have not found a good way to make this test part of the test suite.
Fixes #1983