Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenJSSE Platform support #5369

Merged
merged 12 commits into from Aug 18, 2019

Conversation

@yschimke
Copy link
Collaborator

commented Aug 14, 2019

Support for HTTP/2 with https://github.com/openjsse/openjsse

Caveats

  1. Tests based on MockWebserver (on JDK8) won't work without larger changes
  2. Unclear what the overall status is of the project is, and edge cases like cipher selections on older VMs may need testing.
@yschimke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

This seems cool, but could be too early for us.

yschimke added 2 commits Aug 14, 2019
@yschimke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

n.b. Failures relate to weird mix of jdk8 and TLSv1.3. Not a bug in this PR per se, just something to sidestep more carefully than we are currently.

e.g. We may want to use Platform for this call ->

val factory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm())

Caused by: java.lang.IllegalArgumentException: TLSv1.3
	at sun.security.ssl.ProtocolVersion.valueOf(ProtocolVersion.java:187)
	at sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:206)
	at sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:124)
	at org.openjsse.sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(CertificateMessage.java:1308)
	at org.openjsse.sun.security.ssl.CertificateMessage$T13CertificateConsumer.onConsumeCertificate(CertificateMessage.java:1199)
	at org.openjsse.sun.security.ssl.CertificateMessage$T13CertificateConsumer.consume(CertificateMessage.java:1146)
@yschimke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

image

@yschimke yschimke requested a review from swankjesse Aug 14, 2019

@yschimke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 16, 2019

@swankjesse Getting MockWebserver working with openjsse is a bigger effort. Options

  1. We could test twice. Basic client only tests on JDK8, test client+server with JDK11?
  2. Fix MockWebserver completely to handle openjsse on JDK8.
yschimke added 4 commits Aug 16, 2019
Fix
@swankjesse

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

What’s difficult about getting MockWebServer working with OpenJSSE?

@swankjesse
Copy link
Member

left a comment

This is great.

private val provider: Provider = org.openjsse.net.ssl.OpenJSSE()

override fun newSSLContext(): SSLContext =
SSLContext.getInstance("TLSv1.3", provider)

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 18, 2019

Member

is this TLSv1.3+ ? We also wanna support TLSv1.2 right?

This comment has been minimized.

Copy link
@yschimke

yschimke Aug 18, 2019

Author Collaborator

It does https://github.com/openjsse/openjsse/blob/82abe9803bbae50428be27189ab85b48d20f96bd/src/main/java/org/openjsse/sun/security/ssl/SSLContextImpl.java#L808

But in JDKs, supported vs defaulted often diverge so on other OpenJDK/Oracle/Android platforms we have had to select TLSv1.2 specifically or we might get held back.


public override fun trustManager(sslSocketFactory: SSLSocketFactory): X509TrustManager? =
throw UnsupportedOperationException(
"clientBuilder.sslSocketFactory(SSLSocketFactory) not supported with OpenJSSE")

This comment has been minimized.

Copy link
@swankjesse
sslParameters.applicationProtocols = names.toTypedArray()
}

sslSocket.sslParameters = sslParameters

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 18, 2019

Member

what does this do?

This comment has been minimized.

Copy link
@yschimke

yschimke Aug 18, 2019

Author Collaborator

javax.net.ssl.SSLSocket creates a new params object when you use the getter, so to apply you specifically need to call the setter.

This comment has been minimized.

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 18, 2019

Member

Got it. APIs that mix mutable and accessors are hard to reason about.

override fun getSelectedProtocol(sslSocket: SSLSocket): String? =
if (sslSocket is org.openjsse.javax.net.ssl.SSLSocket) {
when (val protocol = sslSocket.applicationProtocol) {
null, "" -> null

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 18, 2019

Member

does it actually return both of these values? ick

This comment has been minimized.

Copy link
@yschimke

yschimke Aug 18, 2019

Author Collaborator

I don't make the rules...

     * @return null if it has not yet been determined if application
     *         protocols might be used for this connection, an empty
     *         {@code String} if application protocols values will not
     *         be used, or a non-empty application protocol {@code String}
     *         if a value was successfully negotiated.
@yschimke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 18, 2019

What’s difficult about getting MockWebServer working with OpenJSSE?

A mixture of JVM singletons (TlsUtils.localhost), provider setup timing, strict code in the JDK that throws exceptions if it sees the 1.3 version being selected, assumptions in MockWebServer e.g. it doesn't use the Platform SSLContext API. So it's a separate larger task to fix it up. Work we should do, but doesn't block using this, and we can already start testing this as a client only where it appears to mostly just work.

@yschimke

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 18, 2019

Raised #5380 to fix up MockWebserver. But not blocking this as we can test thoroughly with client only to flush out bugs before we bake the 4.2 cake.

@yschimke yschimke merged commit 7cf508a into square:master Aug 18, 2019

3 checks passed

ci/circleci: checkjdk8 Your tests passed on CircleCI!
Details
ci/circleci: testjdk11 Your tests passed on CircleCI!
Details
ci/circleci: testopenjsse Your tests passed on CircleCI!
Details
@swankjesse

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Thanks!

@yschimke yschimke deleted the yschimke:openjsse_platform branch Sep 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.