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

Add disableSniHostCheck property to TLS and SSL configuration classes #1080

Closed
6 tasks done
sleberknight opened this issue Nov 22, 2023 · 1 comment · Fixed by #1086
Closed
6 tasks done

Add disableSniHostCheck property to TLS and SSL configuration classes #1080

sleberknight opened this issue Nov 22, 2023 · 1 comment · Fixed by #1086
Assignees
Labels
enhancement A request for change or improvement to an existing feature
Milestone

Comments

@sleberknight
Copy link
Member

sleberknight commented Nov 22, 2023

Dropwizard 3.0 added the disableSniHostCheck option with a default value of false, meaning it enables SNI checking by default. While this is good, we need to support services that cannot yet enable strict SNI checking.

So, we need to add this property to our own TLS and SSL configuration classes. The default should, like Dropwizard, be false (so strict security is the default), but also permit changing it to true (so SNI checking is disabled).

  • Add disableSniHostCheck to TlsContextConfiguration
  • Add disableSniHostCheck to SSLContextConfiguration
  • Add disableSniHostCheck to SimpleSSLContextFactory (1)
  • Ensure the conversion methods (toTlsContextConfiguration and toSslContextConfiguration) include the disableSniHostCheck property
  • Mention in the TlsContextConfiguration#toDropwizardTlsConfiguration method that the disableSniHostCheck property does not exist in Dropwizard's TlsConfiguration class, so it is ignored.
  • Mention in the TlsContextConfiguration#fromDropwizardTlsConfiguration method that the disableSniHostCheck property does not exist in Dropwizard's TlsConfiguration class, so we can't set a value from it, and that we are defaulting it to false (which enables SNI checking) because that is the more secure option.

I think these are all the things that must be changed, but when implementing there might be more. In that case, they should be added to the above list.

Notes:

  1. SimpleSSLContextFactory has telescoping constructors in addition to a builder, so we need to decide whether to add disableSniHostCheck only to the builder, or to also add yet another ugly constructor. We could also choose to deprecate those constructors, since we really want people using the builder anyway.
@sleberknight
Copy link
Member Author

For now, I added a new constructor, which becomes the "all-args" constructor. Deprecating and removal of these telescoping constructors should be done separately, if and when we think it makes sense.

sleberknight added a commit that referenced this issue Nov 25, 2023
* Add disableSniHostCheck to TlsContextConfiguration,
  SSLContextConfiguration, and SimpleSSLContextFactory
* Update SSLContextConfiguration#toSimpleSSLContextFactory factory
  method to provide disableSniHostCheck to SimpleSSLContextFactory
* Update SSLContextConfiguration#toTlsContextConfiguration factory
  method to provide disableSniHostCheck to TlsContextConfiguration
* Update javadoc of the conversion functions in TlsContextConfiguration
  to explain how disableSniHostCheck is handled (since it does
  not exist in Dropwizard TlsConfiguration)
* Update TlsContextConfiguration#toSslContextConfiguration to
  provide disableSniHostCheck to SSLContextConfiguration
* Add new all-args constructor to SimpleSSLContextFactory
* Clean up duplicative code in SimpleSSLContextFactory by extracting
  several private helper methods
* Change SimpleSSLContextFactory#configuration method to be
  public, and to return unmodifiable map.
* Change tests with lots of assertions to use assertAll
* Minor grammatical fixes in javadocs and comments

Closes #1080
Closes #1085
@sleberknight sleberknight added the in progress A task that is actively being worked on label Nov 25, 2023
terezivy pushed a commit that referenced this issue Nov 25, 2023
…#1086)

* Add disableSniHostCheck to TlsContextConfiguration,
  SSLContextConfiguration, and SimpleSSLContextFactory
* Update SSLContextConfiguration#toSimpleSSLContextFactory factory
  method to provide disableSniHostCheck to SimpleSSLContextFactory
* Update SSLContextConfiguration#toTlsContextConfiguration factory
  method to provide disableSniHostCheck to TlsContextConfiguration
* Update javadoc of the conversion functions in TlsContextConfiguration
  to explain how disableSniHostCheck is handled (since it does
  not exist in Dropwizard TlsConfiguration)
* Update TlsContextConfiguration#toSslContextConfiguration to
  provide disableSniHostCheck to SSLContextConfiguration
* Add new all-args constructor to SimpleSSLContextFactory
* Clean up duplicative code in SimpleSSLContextFactory by extracting
  several private helper methods
* Change SimpleSSLContextFactory#configuration method to be
  public, and to return unmodifiable map.
* Change tests with lots of assertions to use assertAll
* Minor grammatical fixes in javadocs and comments

Closes #1080
Closes #1085
@sleberknight sleberknight removed the in progress A task that is actively being worked on label Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for change or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant