-
Notifications
You must be signed in to change notification settings - Fork 124
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
Support JDK 1.8.0_252 #964
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you proved this approach would work! 🙇🏼♂️
.map(javaCtx => builder.negotiationType(NegotiationType.TLS).sslContext(nettyHttp2SslContext(javaCtx))) | ||
builder = settings.trustManager | ||
.map(trustManager => { | ||
val nettySslContext = GrpcSslContexts.forClient().trustManager(trustManager).build() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, we'd also invoke .keyManager
on the builder here...
nettySslContextField.set(nettySslContext, javaSslContext) | ||
|
||
nettySslContext | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So good to get rid of this!
new DefaultKeyManagerFactoryWrapper(sslConfigSettings.keyManagerConfig.algorithm), | ||
new DefaultTrustManagerFactoryWrapper(sslConfigSettings.trustManagerConfig.algorithm)).build() | ||
sslContext | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can still use this code but, instead of returning SSLContext
return a trustManager
and a keyManager
build using ssl-config
's Default{Key,Trust}ManagerFactoryWrapper
.
1ce1f80
to
daf5b0a
Compare
@raboof WDYT about |
This makes it possible to leverage upstreams' OpenSSL support, which is especially useful now that the JDK implementation doesn't work with JDK 1.8.0-252
Not all scripted tests work because they don't have the right resolvers, but I verified gen-scala-server/00-interop works with 1.8.0_252 with this change.
e795097
to
b5b50ce
Compare
Sounds useful to add, but let's merge this first |
Inserting a JDK `javax.net.ssl.SSLContext` into Netty originated in commit https://github.com/richdougherty/akka-grpc/blob/aa05239c6cddcb20dfa0770e8e8e7649e3bbaaef/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala#L59-L82 in PR akka#266 It was removed in PR akka#964 to address Issue akka#946 It was returned in PR akka#979 to address Issue akka#978 --- Original comment was ```scala // FIXME: Create a JdkSslContext using a normal constructor. Need to work out sensible values for all args first. // In the meantime, use a Netty SslContextBuild to create a JdkSslContext, then use reflection to patch the // object's internal SSLContext. It's not pretty, but it gets something working for now. ``` --- This commit addresses the original `FIXME` comment, and avoids using deprecated constructors on `io.grpc.netty.shaded.io.netty.handler.ssl.JdkSslContext`
Inserting a JDK `javax.net.ssl.SSLContext` into Netty originated in commit https://github.com/richdougherty/akka-grpc/blob/aa05239c6cddcb20dfa0770e8e8e7649e3bbaaef/runtime/src/main/scala/akka/grpc/internal/NettyClientUtils.scala#L59-L82 in PR #266 It was removed in PR #964 to address Issue #946 It was returned in PR #979 to address Issue #978 --- Original comment was ```scala // FIXME: Create a JdkSslContext using a normal constructor. Need to work out sensible values for all args first. // In the meantime, use a Netty SslContextBuild to create a JdkSslContext, then use reflection to patch the // object's internal SSLContext. It's not pretty, but it gets something working for now. ``` --- This commit addresses the original `FIXME` comment, and avoids using deprecated constructors on `io.grpc.netty.shaded.io.netty.handler.ssl.JdkSslContext`
References #946