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: add support for TLSv1.3 #447

Merged
merged 7 commits into from
Mar 21, 2022
Merged

Feat: add support for TLSv1.3 #447

merged 7 commits into from
Mar 21, 2022

Conversation

tsaarni
Copy link
Contributor

@tsaarni tsaarni commented Mar 10, 2022

This change adds support for TLSv1.3.

Updates elastic/logstash#10494

Signed-off-by: Tero Saarni [email protected]

Open questions:

  • JCE limited strength jurisdiction ciphers are not updated by this PR. Find out if TLSv1.3 ciphers can be added there.
  • Is Java 8 compatibility needed? If not, then TLS_CHACHA20_POLY1305_SHA256 can be added to TLSv1.3 ciphers.
  • Decide if the cipher auto-detection for CHA-CHA stays, if yes, describe it in the docs.

Signed-off-by: Tero Saarni <[email protected]>
@tsaarni
Copy link
Contributor Author

tsaarni commented Mar 14, 2022

@kares wrote (originally from elastic/logstash#10494 (comment))

@tsaarni thanks for the effort, whatever progress you're able to do I can built upon, what might server as an example to some extent is what we did in TCP input ... as to how much existing Beats option we'll deprecate in favor of a naming similar to the TCP input added options is undecided at this point.

For now I have updated currently existing options tls_max_version and cipher_suites and did not change/deprecate them to align with TCP input's ssl_supported_protocols and ssl_cipher_suites . Would this be acceptable approach at this point?

By the way: it is curious we will be stuck with ssl_ in so many projects, even if SSL protocol was deprecated long time ago :-)

One test job failed but maybe it is flake since the error is Error response from daemon: manifest for docker.elastic.co/logstash/logstash:8.1.1-SNAPSHOT not found: manifest unknown: manifest unknown

Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

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

👍 on track - left a few comments on code changes

I am planning to revisit this with some kind of a TLSv1.3 "happy path" test

src/main/java/org/logstash/netty/SslContextBuilder.java Outdated Show resolved Hide resolved
src/main/java/org/logstash/netty/SslContextBuilder.java Outdated Show resolved Hide resolved
@kares
Copy link
Contributor

kares commented Mar 15, 2022

JCE limited strength jurisdiction ciphers are not updated by this PR. Find out if TLSv1.3 ciphers can be added there.

hopefully we do not need to care about that part anymore - will need to double check. fine to leave it as is for now.

Is Java 8 compatibility needed? If not, then TLS_CHACHA20_POLY1305_SHA256 can be added to TLSv1.3 ciphers.

unfortunately Java 8 compat is still needed - wonder if we should do some kind of 'light' feature detection of the cipher being available ...

@tsaarni
Copy link
Contributor Author

tsaarni commented Mar 15, 2022

I am planning to revisit this with some kind of a TLSv1.3 "happy path" test

I went ahead and added a happy path test in integration suite, but let me know if this is something you were thinking. I noticed that suite had old filebeat that did not support TLSv1.3 so I needed to upgraded it. Went for latest but 7.x would be OK as well.

Also small change in spec/support/file_helpers.rb: Filebeat was not happy with too wide config file permissions when executed in my machine. The files created via write_to_tmp_file() have now forced 0600 permissions, not depending on system's umask to be set accordingly.

@tsaarni
Copy link
Contributor Author

tsaarni commented Mar 16, 2022

@kares
Copy link
Contributor

kares commented Mar 16, 2022

I went ahead and added a happy path test in integration suite, but let me know if this is something you were thinking. I noticed that suite had old filebeat that did not support TLSv1.3 so I needed to upgraded it. Went for latest but 7.x would be OK as well.

💯 - great stuff Tero, regarding 7.x it would be better to have the older version as the plugin tries to maintain backwards compatibility 7.8 was supported a few months back but any Beats from 7.8 - 7.10 range that has TLS 1.3 and works would be for the best. I do not mind having 8.x as well but in that case we would need to take things further and parameterize the version of Beats we're testing against (and have different CI targets).

Also small change in spec/support/file_helpers.rb: Filebeat was not happy with too wide config file permissions when executed in my machine. The files created via write_to_tmp_file() have now forced 0600 permissions, not depending on system's umask to be set accordingly.

fine by me to have narrower configuration - hopefully it still works with 7.x

@tsaarni
Copy link
Contributor Author

tsaarni commented Mar 16, 2022

regarding 7.x it would be better to have the older version as the plugin tries to maintain backwards compatibility 7.8 was supported a few months back but any Beats from 7.8 - 7.10 range that has TLS 1.3 and works would be for the best.

Filebeat 7.6.0 was the first one with TLS 1.3. It seemed to work, except for the errors due to bundler problem elastic/logstash#13890.

@kares
Copy link
Contributor

kares commented Mar 17, 2022

We just pushed a plugin update with a similar code base: logstash-plugins/logstash-input-http#146

There's a couple of things that are handled slightly differently (loading the cipher lists ahead of time), could you take a look and bump the version number and include a CHANGELOG entry please?

@kares kares mentioned this pull request Mar 17, 2022
40 tasks
@tsaarni
Copy link
Contributor Author

tsaarni commented Mar 18, 2022

There's a couple of things that are handled slightly differently (loading the cipher lists ahead of time), could you take a look and bump the version number and include a CHANGELOG entry please?

Sure! I have now copied the default/supported ciphers handling from logstash-plugins/logstash-input-http#146.

I stepped minor version number like it seemed to be done in HTTP input plugin as well.

Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

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

👍 looking great, thanks again for the contribution!

Planning to run some manual testing with Beats next week and will release afterwards unless I bump into any hiccups.

Copy link
Contributor

@kares kares left a comment

Choose a reason for hiding this comment

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

Tested with Beats (7.16) and all seems to be working:

  • when TLS 1.3 was forced on Beats' end things already work even wout this PR (as expected)
  • when tls_min_version => 1.3 was able to get Beats to work when ssl.supported_protocols: [TLSv1.3] (or simply when ssl.supported_protocols is not set)
  • with tls_min_version => 1.3 and ssl.supported_protocols: [TLSv1.2] Beats failed as expected

@kares kares changed the title Add support for TLSv1.3 Feat: add support for TLSv1.3 Mar 21, 2022
@kares kares merged commit 837b784 into logstash-plugins:main Mar 21, 2022
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.

3 participants