-
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
Re-introduce (but discourage) withSslContext #979
Re-introduce (but discourage) withSslContext #979
Conversation
builder.sslContext((tm match { | ||
case None => context | ||
case Some(trustManager) => context.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.
Hm, this gives precedence to SSLContext
over TrustManager
. I naïvely expected the opposite would be preferred but I have no argument backing the idea.
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.
I'll throw an error when both are set.
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`
Refs #978