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.

Sign up for GitHub

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

Jump to bottom

TLS Hostname verification is not performed #222

Closed
imgx64 opened this issue Nov 19, 2017 · 5 comments
Closed

TLS Hostname verification is not performed #222

imgx64 opened this issue Nov 19, 2017 · 5 comments
Labels
for/stackoverflow Questions are best asked on SO or Gitter
Milestone
0.7.4.RELEASE

Comments

@imgx64
Copy link

imgx64 commented Nov 19, 2017

Expected behavior

When connecting to a server and the server returns a valid certificate that doesn't correspond to its DNS name, the connection is accepted. For example: https://wrong.host.badssl.com/

Actual behavior

Hostname verification should be done according to RFC6125 and the connection should be terminated.

Steps to reproduce

HttpClient.create().get("https://wrong.host.badssl.com/").block();

Reactor Netty version

0.7.1.RELEASE

JVM version (e.g. java -version)

1.8.0_152

OS version (e.g. uname -a)

Windows 10 version 1703
@smaldini smaldini added this to the 0.7.x Backlog milestone Nov 20, 2017
@smaldini smaldini added the for/stackoverflow Questions are best asked on SO or Gitter label Nov 20, 2017
@mbhave mbhave mentioned this issue Jan 24, 2018
Closed
@philwebb
Copy link

Isn't this one a bug, and quite important? Someone could do a man-in-the-middle attack. Should it not be targeted to 0.7.x?

@smaldini smaldini modified the milestones: 0.8.x Backlog, 0.7.4.RELEASE Jan 25, 2018
bclozel added a commit to bclozel/reactor-netty that referenced this issue Feb 7, 2018
@bclozel
Enable hostname verification in client SSL config
7101ec7 
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
@bclozel bclozel mentioned this issue Feb 7, 2018
Merged
bclozel added a commit to bclozel/reactor-netty that referenced this issue Feb 7, 2018
@bclozel
Enable hostname verification in client SSL config
8dcb1b6 
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
@bclozel bclozel mentioned this issue Feb 7, 2018
Merged
smaldini pushed a commit that referenced this issue Feb 7, 2018
@bclozel @smaldini
Enable hostname verification in client SSL config
519b2b3 
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: gh-222
@philwebb
Copy link

philwebb commented Feb 9, 2018

I'm not sure if it's related, but I've just seen this error on Spring Boot

[ERROR] Tests run: 19, Failures: 0, Errors: 1, Skipped: 1, Time elapsed: 1.823 s <<< FAILURE! - in org.springframework.boot.web.embedded.tomcat.TomcatReactiveWebServerFactoryTests
[ERROR] sslNeedsClientAuthenticationSucceedsWithClientCertificate(org.springframework.boot.web.embedded.tomcat.TomcatReactiveWebServerFactoryTests)  Time elapsed: 0.159 s  <<< ERROR!
reactor.core.Exceptions$ReactiveException: javax.net.ssl.SSLException: SSLEngine closed already
	at reactor.core.Exceptions.propagate(Exceptions.java:326)
	at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:87)
	at reactor.core.publisher.Mono.block(Mono.java:1162)
	at org.springframework.boot.web.reactive.server.AbstractReactiveWebServerFactoryTests.testClientAuthSuccess(AbstractReactiveWebServerFactoryTests.java:202)
	at org.springframework.boot.web.reactive.server.AbstractReactiveWebServerFactoryTests.sslNeedsClientAuthenticationSucceedsWithClientCertificate(AbstractReactiveWebServerFactoryTests.java:213)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.springframework.boot.testsupport.rule.OutputCapture$1.evaluate(OutputCapture.java:57)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
	at org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:239)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:369)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:275)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:239)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:160)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:373)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:334)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:119)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:407)
	Suppressed: java.lang.Exception: #block terminated with an error
		at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:89)
		... 33 more
Caused by: javax.net.ssl.SSLException: SSLEngine closed already
	at io.netty.handler.ssl.SslHandler.wrap(...)(Unknown Source)
[INFO] Running org.springframework.boot.web.embedded.tomcat.TomcatServletWebServerFactoryTests

@mbhave
Copy link

mbhave commented Feb 10, 2018

I don't think Spring Boot has moved to reactor-netty snapshots yet so it's probably not the changes that went into this commit that caused this.

@smaldini
Copy link
Contributor

smaldini commented Feb 10, 2018 •
edited
Loading

It could be related if it was the case, @bclozel applied these changes in our own tests otherwise the ssl context wasn't validating self signed certificates: https://github.com/reactor/reactor-netty/pull/275/files#diff-5ff61b23da46920cd9646f1b51d12622R678

@violetagg
Copy link
Member

violetagg commented Feb 12, 2018 •
edited
Loading

Spring Boot is still using Bismuth-SR5 so it is not the change for this issue that caused the test failure
https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-dependencies/pom.xml#L130

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
for/stackoverflow Questions are best asked on SO or Gitter
Projects
None yet
Milestone
0.7.4.RELEASE
Development

No branches or pull requests
5 participants
@smaldini @imgx64 @philwebb @violetagg @mbhave