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

Feat: ssl_supported_protocols (TLSv1.3) + ssl_cipher_suites #198

Merged
merged 43 commits into from
Jun 6, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Feb 3, 2022

The intent here is to officially support TLS 1.3 with an option to limit the protocol (both in server/client mode) with:

  • ssl_supported_protocols => TLSv1.3

As well as being able to limit the TLS cipher suites used between client and server using:

  • ssl_cipher_suites => [ ... ]

Unfortunately to be able to support the feature a recent version of JRuby-OpenSSL is required and since the gem version is locked in 7.x and 8.0 versions we need to play some clever tricks to disable the >= 0.12.2 requirement -> tests than feature detect the version available and adjust accordingly.

@kares kares changed the title Feat: added ssl_supported_protocols (TLSv1.3) Feat: ssl_supported_protocols (TLSv1.3) + ssl_cipher_suites Mar 14, 2022
@kares kares marked this pull request as ready for review March 14, 2022 09:38
extra work that might not be worth the effort given Java 8 is EoL

# we depend on bouncycastle's bcpkix-jdk15on being on the class-path
s.add_runtime_dependency 'jruby-openssl', '>= 0.10.2'
s.add_runtime_dependency 'jruby-openssl', '>= 0.12.2' # 0.12 supports TLSv1.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is effectively breaking, preventing this minor-bumped feature from being installed on most Logstashes in the 7.x series where JOSSL is pinned, despite this PR otherwise doing a fair bit of work to ensure that the old functionality could run on those older Logstashes.

Can we continue to pin to >= 0.10.2, which will select JOSSL 0.12 on newer Logstashes and will allow installation on older Logstashes where the upgraded JOSSL is not available? If so, to prove that we can run the subset of behaviours we test for, can we include an old-JOSSL-pinned Logstash in the matrix (say: 7.17.2, which is pinned to ~> 0.11)

Copy link
Contributor Author

@kares kares Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is effectively breaking, preventing this minor-bumped feature from being installed on most Logstashes in the 7.x series where JOSSL is pinned,

The locking in LS was very unfortunate, esp. since it was a work-around for a build issue most of the time but took a while to actually look into, so the issue will be "breaking" on some 7.x versions (note that e.g. 6.8 did not do JOSSL locking) :

  • LS 7.9.0 "jruby-openssl", "~> 0.10" ✔️

  • LS 7.10.2 "jruby-openssl", "= 0.10.4"

  • LS 7.11.2 "jruby-openssl", "~> 0.10" ✔️

  • LS 7.12.1 "jruby-openssl", "~> 0.10" ✔️

  • LS 7.13.4 "jruby-openssl", "= 0.10.5"

  • LS 7.14.2 "jruby-openssl", "= 0.10.5"

  • LS 7.15.2 "jruby-openssl", "~> 0.11" ✔️

  • LS 7.16.x "jruby-openssl", "~> 0.11" ✔️

  • LS 7.17.0 "jruby-openssl", "~> 0.11" ✔️

  • LS 7.17.1 "jruby-openssl", "= 0.11.0"

  • LS 7.17.2 "jruby-openssl", "~> 0.11.0"

  • LS 7.17.3 "jruby-openssl", "~> 0.11.0"

  • LS 7.17.4 "jruby-openssl", "~> 0.11" ✔️ (future release)

  • LS 8.0.1 "jruby-openssl", "= 0.11.0"

  • LS 8.1.2 "jruby-openssl", "~> 0.11" ✔️

despite this PR otherwise doing a fair bit of work to ensure that the old functionality could run on those older Logstashes.

Right, we did some feature detection (although not that much) in tests but it's now been reverted 6c99df9 ... in order to not have the condition around the add_runtime_dependency (keeping the CI 🟢) - as suggested in #198 (comment)

Can we continue to pin to >= 0.10.2, which will select JOSSL 0.12 on newer Logstashes and will allow installation on older Logstashes where the upgraded JOSSL is not available? If so, to prove that we can run the subset of behaviours we test for, can we include an old-JOSSL-pinned Logstash in the matrix (say: 7.17.2, which is pinned to ~> 0.11)

Believe this has already been discussed: #198 (comment) ... we can not continue to use >= 0.10.2 as we need 2 features from 0.12.2 to provide the functionality in this PR. It's one thing that we do feature detection in tests (to work around pinned JOSSL) and another that we would need to document what features work depending on what version of logstash is used ...

@kares kares requested a review from yaauie April 27, 2022 15:40
@kares
Copy link
Contributor Author

kares commented Apr 28, 2022

@yaauie the plugin is updated to require LS >= 8.1 (which already has jruby-openssl 0.12.2)
this means we no longer need special checks for Java 8 (due TLSv1.3 support) thus all of the code has been removed.

Comment on lines 309 to 310
@ssl_context.min_version = min_max_version.first
@ssl_context.max_version = min_max_version.last
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned in the tcp output review, we can enable the exact list of supported protocols in the context instead of max/min, so that the behaviour is more consistent with the java context.

@kares kares requested review from yaauie and removed request for yaauie May 10, 2022 07:03
@kares kares requested review from jsvd and yaauie and removed request for yaauie May 30, 2022 06:47
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kares kares merged commit bca64db into logstash-plugins:main Jun 6, 2022
@AnandhkumarG
Copy link

AnandhkumarG commented Sep 14, 2022

Hi , Does this feature require Logstash 8.1.x as minimum? WIll this feature work in 7.17.x for example. Please let know if there is a compatiibility matrix for plugins versions-LS versions. Thanks in advance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logstash-input-tcp - disabling TLS < 1.2 does not work Parameter for SSL Version
5 participants