-
Notifications
You must be signed in to change notification settings - Fork 66
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: TLSv1.3 support #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! Thanks for adding the SSL testing ❤️
Tests are failing on 6.8
with
Java::JavaxNetSsl::SSLHandshakeException:
No appropriate protocol (protocol is disabled or cipher suites are inappropriate)
when using a modern Java8 JVM that supports TLS1.3, such as OpenJDK Runtime Environment (Temurin)(build 1.8.0_322-b06)
on my local box, plus 1.8.0_282-b08
in the testing environment.
my bad - did not realize the TLS 1.3 detection won't do, due Manticore itself (used for testing) on Java 8 without the added a different way to detect whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff!
Only thought here is whether this should be a minor
update, rather than patch
, as it is adding a new feature plus a tiny comment nit
CHANGELOG.md
Outdated
@@ -1,3 +1,6 @@ | |||
## 3.4.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is an added feature, rather than a bug fix, does it make sense to make this a minor rather than patch release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine by me, thought the changes are fully backwards compatible but it makes sense ... updated
src/main/java/org/logstash/plugins/inputs/http/util/SslSimpleBuilder.java
Outdated
Show resolved
Hide resolved
…uilder.java Co-authored-by: Rob Bavey <[email protected]>
Implements TLS 1.3 support in the plugin, using existing
tls_min_version
/tls_max_version
options.The plugin was lacking TLS testing thus it's been added.
NOTE: a follow-up minor release is planned, where the protocol configuration will be changed to
ssl_supported_protocols => ...
and thetls_min_version
/tls_max_version
options are going to be deprecated.But for now this is worth shipping as a patch release.