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

Allow to use some custom security provider in HTTP client #2299

Merged
merged 6 commits into from Mar 8, 2018

Conversation

Projects
None yet
2 participants
@mgtriffid
Contributor

mgtriffid commented Mar 8, 2018

Problem:

Dropwizard allows to configure JCE provider for server side, passing jceProvider property to Jetty. This allows to use some non-standard provider, like Conscrypt. But there is no such setting on client side.

Solution:

Add new field provider to TlsConfiguration and pass it to Apache HTTP client builder.

@@ -71,7 +71,8 @@ private SSLContext buildSslContext() throws SSLInitializationException {
final SSLContext sslContext;
try {
final SSLContextBuilder sslContextBuilder = new SSLContextBuilder();
sslContextBuilder.useProtocol(configuration.getProtocol());
sslContextBuilder.setProtocol(configuration.getProtocol());

This comment has been minimized.

@mgtriffid

mgtriffid Mar 8, 2018

Contributor

Method useProtocol is marked as deprecated, so changing to setProtocol.

Mikhail Gromov
@@ -1313,6 +1314,7 @@ Name Default Description
protocol TLSv1.2 The default protocol the client will attempt to use during the SSL Handshake.
See
`here <http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#SSLContext>`_ for more information.
provider (none) The name of the JCE provider to use on client side for cryptographic support.

This comment has been minimized.

@joschi

joschi Mar 8, 2018

Member

Could you please add some examples for valid values here, e. g. "BC" for BouncyCastle or Conscrypt?

@@ -1313,6 +1314,7 @@ Name Default Description
protocol TLSv1.2 The default protocol the client will attempt to use during the SSL Handshake.
See
`here <http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#SSLContext>`_ for more information.
provider (none) The name of the JCE provider to use on client side for cryptographic support (for example, SunJCE, Conscrypt, BC, etc).

This comment has been minimized.

@joschi

joschi Mar 8, 2018

Member

Are these really valid values for the SSLContextBuilder#setProvider() method?

And if so, why is "SUN" being used in the example above?

This comment has been minimized.

@mgtriffid

mgtriffid Mar 8, 2018

Contributor

SSLContextBuilder#setProvider() calls Security.getProvider() under the hood.

    public SSLContextBuilder setProvider(final String name) {
        this.provider = Security.getProvider(name);
        return this;
    }

Oracle Standard JDK comes with 10 providers, SUN and SunJCE among them. SUN is what Security.getProviders()[0].getName() returns on my JDK. Index 0 means it has the highest priority. That's why "SUN" in the example above. But I think now that it's better to specify default value as SunJSSE, as the provider which will be by default used with TLSv1.2 (in JDK 8) is in fact SunJSSE.
BC and Conscrypt will work if they were previously configured using Security.addProvider() or Security.insertProviderAt().

This comment has been minimized.

@joschi

joschi Mar 8, 2018

Member

Thanks for the explanation! 👍

Please add a link to https://docs.oracle.com/javase/8/docs/technotes/guides/security/SunProviders.html for a more detailed overview.

@@ -71,7 +71,8 @@ private SSLContext buildSslContext() throws SSLInitializationException {
final SSLContext sslContext;
try {
final SSLContextBuilder sslContextBuilder = new SSLContextBuilder();
sslContextBuilder.useProtocol(configuration.getProtocol());
sslContextBuilder.setProtocol(configuration.getProtocol());
sslContextBuilder.setProvider(configuration.getProvider());

This comment has been minimized.

@joschi

joschi Mar 8, 2018

Member

Would it make sense to only set the provider if configuration.getProvider() wasn't null?

This comment has been minimized.

@mgtriffid

mgtriffid Mar 8, 2018

Contributor

There is no desperate need in it, because Security.getProvider(null) doesn't throw NPE, but you're right, better to check. Updated.

@joschi

joschi approved these changes Mar 8, 2018

@joschi joschi merged commit 1f590cb into dropwizard:master Mar 8, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joschi joschi added this to the 1.3.0 milestone Mar 8, 2018

@joschi joschi added the improvement label Mar 8, 2018

@mgtriffid

This comment has been minimized.

Contributor

mgtriffid commented Mar 9, 2018

Thanks for such a quick merge, @joschi! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment