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

NettyWebServer uses a deprecated constructor for JdkSslContext #519

Closed
pejomstd opened this issue Mar 20, 2019 · 8 comments · Fixed by #530
Closed

NettyWebServer uses a deprecated constructor for JdkSslContext #519

pejomstd opened this issue Mar 20, 2019 · 8 comments · Fixed by #530
Assignees
Labels
enhancement New feature or request webserver

Comments

@pejomstd
Copy link

pejomstd commented Mar 20, 2019

Environment Details

  • Helidon Version: 1.0.1
  • Helidon SE
  • JDK version: irrelevant
  • OS: irrelevant
  • Docker version (if applicable): irrelevant

Problem Description

We would like to use Helidon, but with a restricted TLS version (1.2). Helidon does not allow this, and also uses a deprecated constructor of JdkSslContext in NettyWebServer.java.
Please change to use the non deprecated constructor, and also make it configurable which protocols to enable / use

Steps to reproduce

"Step by step instructions to reproduce the problem"

Start WebServer,
do openssl s_client -connect localhost:port -tls1, this should fail if setup with "TLSv1.2"

@kallekarlberg
Copy link

Should this not be considered a bug?

@romain-grecourt
Copy link
Contributor

Helidon 1.0.1 uses Netty 4.1.30.Final, the JdkSslContext constructor used in NettyWebServer is not deprecated in 4.1.30.Final, but is deprecated in 4.1.31.Final.

I wouldn't call this a bug, it feels more like a request for enhancement.

@tomas-langer @spericas thoughts ?

@kallekarlberg
Copy link

Ok, you don't have to call it a bug :-)

However this prevents us from using helidon in production :-( as security auditors will find this and say: "please disable TLS 1.0 on port X" And currenly we can't as fiddling in java.security jre/jdk files is not a viable option to use

@pejomstd
Copy link
Author

pejomstd commented Mar 22, 2019

I would suggest,
upgrade to Netty 4.1.31.Final or later,

Change NettyWebserver.java

From:

sslContext = new JdkSslContext(soConfig.ssl(), false, ClientAuth.NONE);

to

sslContext = new JdkSslContext(
	sslContext, 
	isClient, 
	null, 
	IdentityCipherSuiteFilter.INSTANCE,
    JdkDefaultApplicationProtocolNegotiator.INSTANCE, 
	clientAuth, 
	config.enabledSslProtocols(), 
	false
	);

And support for adding protocol(s) in ServerConfiguration like:

ServerConfiguration serverConfiguration = ServerConfiguration.builder()
                .bindAddress(bindAddress)
                .port(1234)
                .ssl(sslContext)
.enabledSslProtocols("TLSv1.2") // vararg String...

And use this when spawning up

@romain-grecourt romain-grecourt added the enhancement New feature or request label Mar 22, 2019
@romain-grecourt romain-grecourt self-assigned this Mar 22, 2019
romain-grecourt added a commit that referenced this issue Mar 22, 2019
Provide configuration for enabled SSL protocols.
Upgrade netty to 4.1.34
@pejomstd
Copy link
Author

Any chance this will be released soon?

@romain-grecourt
Copy link
Contributor

This will be part of the next release, most likely next week or so.

romain-grecourt added a commit that referenced this issue Apr 16, 2019
* Fixes #519
* Upgrade netty to 4.1.34
* Provide configuration for enabled SSL protocols.
* Add configuration mapping for ssl 'ssl-protocols'
* Update unit test to test the new configuration attribute
@pejomstd
Copy link
Author

seems like the suggested implementation was not kept when moving this to #530.

It now requires to load a config object just to get this feature, as it seems hidden deep insite ServerConfigurationBuilder and no obvious way to do this porgrammatically. Or did I miss something?

@romain-grecourt
Copy link
Contributor

Most of the suggested implementation was kept, except the attribute changed from String[] to a Set<String> and config mapping was added.

However I forgot to add a new method to ServerConfiguration.Builder (oops), this is a bug that will be fixed promptly.

Basically this means that in v1.0.3 you can't set the SSL enabled protocols for the default socket programmatically.

You can do it with configuration like this:

server:
  port: 8080
  host: 0.0.0.0
  ssl-protocols:
    - "TLSv1.2"
  ssl:
    private-key:
      keystore-resource-path: "xxx"
      keystore-passphrase: "xxx"

You can also configure an additional socket for SSL, programmatically or by config:

ServerConfiguration serverConfig = ServerConfiguration.builder()
        .addSocket("secure", SocketConfiguration.builder()
                .port(8081)
                .enabledSSlProtocols("TLSv1.2"))
        .build(); 
server:
  port: 8080
  host: 0.0.0.0
  sockets:
    secure:
      port: 8082
      host: 0.0.0.0
      ssl-protocols:
        - "TLSv1.2"
      ssl:
        private-key:
          keystore-resource-path: "xxx"
          keystore-passphrase: "xxx"

Note the inconsistency in the configuration with "ssl-protocols" and "ssl". We will re-work the SSL configuration sometime soon to address that and the way to configure the SSL enabled protocols will change then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request webserver
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants