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

Enable hostname verification in client SSL config #275

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

bclozel
Copy link
Member

@bclozel bclozel commented Feb 7, 2018

This is a fix for gh-222.

Netty client doesn't enable hostname verification in its client SSL Context by default (see this warning).

I've just tested that change against badssl.com - but I'm willing to improve that PR in any way you'd like.

In order to make other tests pass, I had to use Netty's InsecureTrustManagerFactory.INSTANCE and I don't know if this is the right solution vs. using registering the self signed cert as previously. The self signed cert is generated with "example.com" as a domain. It looks like hostname verification will resolve the real cert associated with "example.com" and won't find a valid cert path between the generated and the real one.

It looks like Netty itself is doing this when testing SSL-related scenarios.

Netty HTTP client's `SSLContext` has an underlying `SSLEngine` that
doesn't have hostname verification enabled by default. This feature is
relying on JDK7+ API.

Since Reactor Netty is JDK8+, we can safely enable this by default and
remove this code once Netty has moved to JDK8 as a baseline.

Closes: reactorgh-222
@codecov-io
Copy link

Codecov Report

Merging #275 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #275      +/-   ##
============================================
+ Coverage     67.55%   67.64%   +0.09%     
- Complexity     1008     1011       +3     
============================================
  Files            73       73              
  Lines          4262     4268       +6     
  Branches        605      605              
============================================
+ Hits           2879     2887       +8     
+ Misses         1020     1015       -5     
- Partials        363      366       +3
Impacted Files Coverage Δ Complexity Δ
...n/java/reactor/ipc/netty/options/NettyOptions.java 72.8% <ø> (ø) 24 <0> (ø) ⬇️
...actor/ipc/netty/http/client/HttpClientOptions.java 85.96% <100%> (+1.65%) 23 <1> (+1) ⬆️
...tor/ipc/netty/channel/CloseableContextHandler.java 52.63% <0%> (-5.27%) 8% <0%> (-1%)
...ctor/ipc/netty/resources/DefaultPoolResources.java 74.64% <0%> (-4.23%) 8% <0%> (-1%)
.../ipc/netty/http/client/MonoHttpClientResponse.java 77.02% <0%> (-4.06%) 6% <0%> (ø)
...or/ipc/netty/http/client/HttpClientOperations.java 54.15% <0%> (-1.61%) 71% <0%> (-2%)
...a/reactor/ipc/netty/channel/ChannelOperations.java 81.95% <0%> (-1.51%) 47% <0%> (-1%)
...or/ipc/netty/channel/ChannelOperationsHandler.java 63.63% <0%> (+0.69%) 57% <0%> (+1%) ⬆️
...java/reactor/ipc/netty/channel/ContextHandler.java 73.52% <0%> (+1.96%) 27% <0%> (+1%) ⬆️
.../ipc/netty/channel/PooledClientContextHandler.java 63.41% <0%> (+4.87%) 26% <0%> (+2%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1acd4b...8dcb1b6. Read the comment docs.

@smaldini smaldini merged commit 519b2b3 into reactor:master Feb 7, 2018
@smaldini smaldini added this to the 0.7.4.RELEASE milestone Feb 7, 2018
@smaldini smaldini added the type/bug A general bug label Feb 7, 2018
@unlimitedsola
Copy link

unlimitedsola commented Sep 16, 2018

but how can I disable it? @bclozel
I mean, only disable the hostname verification, not the entire trust manager.

@violetagg
Copy link
Member

@unlimitedsola In 0.8.x you can provide SslHandler configuration:

reactor.netty.http.client.HttpClient#secure(java.util.function.Consumer<? super reactor.netty.tcp.SslProvider.SslContextSpec>)
...
sslContextSpec
    .sslContext(SslContextBuilder)
    .defaultConfiguration(SslProvider.DefaultConfigurationType)
    .handlerConfigurator(handler -> {
        SSLEngine sslEngine = handler.engine();
        SSLParameters sslParameters = sslEngine.getSSLParameters();
        // extend/remove ssl params
        sslEngine.setSSLParameters(sslParameters);
    });

@unlimitedsola
Copy link

@violetagg Thanks for your explanation, I'm wondering if Spring Boot 2.0.5.RELEASE compatible with reactor-netty 0.8.x? if not, is there any estimated time of being available?

@violetagg
Copy link
Member

Thanks for your explanation, I'm wondering if Spring Boot 2.0.5.RELEASE compatible with reactor-netty 0.8.x? if not, is there any estimated time of being available?

No Spring Boot 2.0.5.RELEASE is not compatible with Reactor Netty 0.8.x
Spring Boot 2.1.x will be compatible with Reactor Netty 0.8.x. Spring Boot 2.1.0.M4 is scheduled for 24.09 (https://github.com/spring-projects/spring-boot/milestones)

If you need this for Reactor Netty 0.7.x please create and enhancement issue.

@unlimitedsola
Copy link

unlimitedsola commented Sep 17, 2018

@violetagg I see, I'll wait for Spring Boot 2.1.0 coming out. Thank you again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants