Add support for setting io.netty.handler.ssl.SslContext
#734
Conversation
| * To achieve this, specify {@link SslProvider#OPENSSL} | ||
| * {@linkplain SslContextBuilder#sslProvider(SslProvider) TLS/SSL protocol provider} via the tuner. | ||
| * Note that doing so adds a runtime dependency on | ||
| * <a href="https://netty.io/wiki/forked-tomcat-native.html">netty-tcnative</a>, which you must satisfy. |
There was a problem hiding this comment.
There are multiple artifacts that may be used to satisfy this dependency. They differ by the target OS, ISA, native dependencies which are linked either dynamically or statically. As a result, I don't think we can specify this dependency in the published POM as an optional runtime dependency.
driver-core/src/main/com/mongodb/connection/netty/NettyStreamFactoryFactory.java
Outdated
Show resolved
Hide resolved
| dependencies { | ||
| api project(path: ':bson', configuration: 'default') | ||
|
|
||
| implementation "com.github.jnr:jnr-unixsocket:$jnrUnixsocketVersion", optional | ||
| api "io.netty:netty-buffer:$nettyVersion", optional | ||
| api "io.netty:netty-transport:$nettyVersion", optional | ||
| implementation "io.netty:netty-handler:$nettyVersion", optional | ||
| api "io.netty:netty-handler:$nettyVersion", optional |
There was a problem hiding this comment.
This changes the dependency in the published POM from optional runtime to optional compile time.
This may be used as a convenient way to utilize OpenSSL as an alternative to the TLS/SSL protocol implementation in a JDK. See also https://netty.io/wiki/requirements-for-4.x.html#tls-with-openssl. JAVA-4177
driver-core/src/main/com/mongodb/connection/netty/NettyStreamFactoryFactory.java
Outdated
Show resolved
Hide resolved
| public Builder nettySslContext(final SslContext nettySslContext) { | ||
| this.nettySslContext = notNull("nettySslContext", nettySslContext); | ||
| isTrueArgument("nettySslContext must be client-side", nettySslContext.isClient()); | ||
| isTrueArgument("nettySslContext must not be reference counted", |
There was a problem hiding this comment.
Should this requirement be in the api docs?
There was a problem hiding this comment.
It is there, formulated as "Only {@link SslProvider#JDK} and {@link SslProvider#OPENSSL} TLS/SSL protocol providers are supported." The constraint is implemented as is because checking the provided context against a blacklist is shorter and saves us from relying on specific classes implementing allowed values of SslProvider (if Netty starts implementing a forbidden value with a different class - not a big deal, our check becomes useless; but if Netty starts implementing an allowed value with a different class - our users will not be able to use it).
There was a problem hiding this comment.
I changed the error message for this check to "sslContext must use either SslProvider.JDK or SslProvider.OPENSSL TLS/SSL protocol provider".
Done.
| * | ||
| * @since 4.3 | ||
| */ | ||
| public Builder nettySslContext(final SslContext nettySslContext) { |
There was a problem hiding this comment.
Naming convention question: Why nettySslContext and not just SslContext?
There was a problem hiding this comment.
I wanted to emphasize that Netty SslContext is not of type javax.net.ssl.SSLContext, however I was not sure whether it is a good idea to make names longer just for that purpose. I interpret your question as a sign that it was not a good idea.
Removed the netty prefix.
Done.
| if (streamType.equals("netty")) { | ||
| return new NettyStreamFactory(getSocketSettings(), getSslSettings()); | ||
| } else if (streamType.equals("nio2")) { | ||
| StreamFactoryFactory overriddenStreamMetafactory = getOverriddenStreamFactoryFactory(); |
There was a problem hiding this comment.
Naming convention question: why Metafactory and not FactoryFactory?
There was a problem hiding this comment.
Thought I would use "metafactory" as this is what should have been used instead of "factoryFactory". Replaced with "factoryFactory".
Done.
| if (nettyStreamFactoryFactory == null) { | ||
| nettyStreamFactoryFactory = NettyStreamFactoryFactory.builder().build(); | ||
| if (nettyStreamFactoryFactory == null) { | ||
| if (streamType.equals("netty")) { |
There was a problem hiding this comment.
this double if statement could be joined.
JAVA-4177
This may be used as a convenient way to utilize OpenSSL as an alternative to the TLS/SSL protocol implementation in a JDK.
JAVA-4177