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

Clean up NettyClientUtils#createNettySslContext #1649

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

dwhjames
Copy link
Contributor

@dwhjames dwhjames commented Aug 3, 2022

Inserting a JDK javax.net.ssl.SSLContext into Netty originated in commit

/**
* INTERNAL API
*
* Given a Java [[SSLContext]], create a Netty [[SslContext]] that can be used to build
* a Netty HTTP/2 channel.
*/
@InternalApi
private def nettyHttp2SslContext(javaSslContext: SSLContext): SslContext = {
// 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.
// Create a Netty JdkSslContext object with all the correct ciphers, protocol settings, etc initialized.
val nettySslContext: JdkSslContext = GrpcSslContexts
.configure(GrpcSslContexts.forClient, SslProvider.JDK)
.build.asInstanceOf[JdkSslContext]
// Patch the SSLContext value inside the JdkSslContext object
val nettySslContextField: Field = classOf[JdkSslContext].getDeclaredField("sslContext")
nettySslContextField.setAccessible(true)
nettySslContextField.set(nettySslContext, javaSslContext)
nettySslContext
}

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

    // 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 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`
Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good find!

@raboof raboof merged commit b0ff79e into akka:main Aug 22, 2022
@dwhjames dwhjames deleted the netty-ssl-context branch August 23, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants