Ensure certificate verification by using a singleton and defaults.

[wsargent]

Fixes #352

[slandelle]

What's the benefit of this singleton over the static class?

What are the benefits of this SingletonHolder pattern (first time I see it)? Why not an enum?

[wsargent]

singleton holder is a thread safe lazy initialization pattern: https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
After going through an issue where I needed to patch static methods http://tersesystems.com/2014/03/02/monkeypatching-java-classes/ I much prefer singletons because at least you can swap out one reference and have it done with.

[slandelle]

OK, thanks :)

[slandelle]

I just tried to merge and got the following test errors:

Caused by: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
    at sun.security.provider.certpath.SunCertPathBuilder.engineBuild(SunCertPathBuilder.java:196)
    at java.security.cert.CertPathBuilder.build(CertPathBuilder.java:268)
    at sun.security.validator.PKIXValidator.doBuild(PKIXValidator.java:380)
    at sun.security.validator.PKIXValidator.engineValidate(PKIXValidator.java:292)
    at sun.security.validator.Validator.validate(Validator.java:260)
    at sun.security.ssl.X509TrustManagerImpl.validate(X509TrustManagerImpl.java:326)
    at sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:283)
    at sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:138)
    at sun.security.ssl.ClientHandshaker.serverCertificate(ClientHandshaker.java:1328)
    at sun.security.ssl.ClientHandshaker.processMessage(ClientHandshaker.java:153)
    at sun.security.ssl.Handshaker.processLoop(Handshaker.java:868)
    at sun.security.ssl.Handshaker$1.run(Handshaker.java:808)
    at sun.security.ssl.Handshaker$1.run(Handshaker.java:806)
    at java.security.AccessController.doPrivileged(Native Method)
    at sun.security.ssl.Handshaker$DelegatedTask.run(Handshaker.java:1227)

Failed tests:
  GrizzlyFeedableBodyGeneratorTest.testNonBlockingFeederOverSSLMultipleThreads:125->doNonBlockingFeeder:363 expected:<200> but was:<0>
  GrizzlyFeedableBodyGeneratorTest.testSimpleFeederOverSSLMultipleThreads:115->doSimpleFeeder:227 expected:<200> but was:<0>
  GrizzlyHttpToHttpsRedirectTest>HttpToHttpsRedirectTest.runAllSequentiallyBecauseNotThreadSafe:90->HttpToHttpsRedirectTest.httpToHttpsRedirect:102 » Execution
  GrizzlyProxyTunnelingTest>ProxyTunnellingTest.testConfigProxy:126 » Execution ...
  GrizzlyProxyTunnelingTest>ProxyTunnellingTest.testRequestProxy:98 » Execution ...
  GrizzlyProxyTunnelingTest>ProxyTunnellingTest.testSimpleAHCConfigProxy:143 » Execution
  NettyHttpToHttpsRedirectTest.runAllSequentiallyBecauseNotThreadSafe » Execution
  NettyProxyTunnellingTest.testConfigProxy » Execution javax.net.ssl.SSLHandshak...
  NettyProxyTunnellingTest.testRequestProxy » Execution javax.net.ssl.SSLHandsha...
  NettyProxyTunnellingTest.testSimpleAHCConfigProxy » Execution javax.net.ssl.SS...
  NettyProxyTunnellingTest.echoText » Execution javax.net.ssl.SSLHandshakeExcept...

@wsargent Did you run the tests successfully before submitting?

[slandelle]

@wsargent OK, I feel stupid: that's just that the tests use a self-signed certificate.

With your PR, in order to have AHC work with servers using self signed certificates is to expect the users to create a SSLContext with a "trust all" TrustManager and either set the Default SSLContext or pass it in AHC config. Do I get it right?

If so, this is way too much a constraint, we should ship such a permissive TrustManager and have an option for enabling/disabling.

How do you deal with this in Play?

[wsargent]

From memory, but I think the way I did it in Play was to add a self signed certificate to a trust manager explicitly in the test.

[slandelle]

But that means that Play WS doesn't work with self signed certificates, like it's usually the case on dev/test platforms?!

[slandelle]

Ha, OK, if acceptAnyCertificate is let to true, you leave AHC as is with current LooseTrustManager.

[slandelle]

So honestly, you're PR was excessive: if you completely remove AHC LooseTrustManager like you did, you're shooting yourself in the foot and will have to reimplement it in Play. :)

[slandelle]

@wsargent I've cherry-picked your commit in #572 and introduced an acceptAnyCertificate config parameter (same name as in Play):

• if false (default), use SSLContext.getDefault
• if true, use a loose TrustManager

WDYT?

[slandelle]

@jfarcand @oleksiys WDYT?

[wsargent]

So honestly, you're PR was excessive: if you completely remove AHC LooseTrustManager like you did, you're shooting yourself in the foot and will have to reimplement it in Play. :)

By design. playframework/playframework#2855