Skip to content

Conversation

edmocosta
Copy link
Contributor

@edmocosta edmocosta commented Dec 7, 2023

  • Deprecated ssl_enable in favor of ssl_enabled
  • Deprecated ssl_cert in favor of ssl_certificate
  • Deprecated ssl_verify in favor of ssl_client_authentication when mode is server
  • Deprecated ssl_verify in favor of ssl_verification_mode when mode is client
  • Added ssl_cipher_suites configuration and common validations
  • Fixed the server mode when SSL is enabled

Closes elastic/logstash#14926

@edmocosta edmocosta force-pushed the standardize_ssl_settings branch from 28f62d3 to e672955 Compare December 7, 2023 16:43
Fixed server mode with SSL enabled
@edmocosta edmocosta force-pushed the standardize_ssl_settings branch from f89e9f6 to 91008fd Compare December 7, 2023 17:41
@edmocosta edmocosta changed the title [WIP] Standardized SSL settings Standardized SSL settings Dec 7, 2023
@edmocosta edmocosta marked this pull request as ready for review December 7, 2023 20:20
Copy link
Contributor

@kaisecheng kaisecheng left a comment

Choose a reason for hiding this comment

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

Docs and the actual change look great. I have tested the settings manually with self-signed cert with tcp-input.

For the docs, do you think we can add a table or example config to demonstrate the available settings for server mode and client mode? When I read from the beginning, It is not easy to spot that server mode uses ssl_client_authentication, while client mode uses ssl_verification_mode.

For the test, do we have test cover mTLS with ssl_certificate_authorities? It will be great to have it to get more confident to this plugin

@edmocosta
Copy link
Contributor Author

Thanks for reviewing @kaisecheng! :)

This plugin has almost no test coverage for the server mode and for the already existing SSL settings. I've added a bunch of them but couldn't cover it all on this PR, which main focus was standardizing the existing settings.
Adding full coverage + integration tests would take a considerable amount of time, which, IMO, should be spent on this unifying issue.

I've added new tests to ensure the OpenSSL cert_store is being configured properly, it's the minimum requirement to make ssl_certificate_authorities work on both modes.

Regarding the docs, the only settings that are specific per mode are the ssl_client_authentication and ssl_verification_mode, all the other settings are shared between them. I can definitely add a table if you think it's necessary, but IMO, the note on those settings description should be enough to identify in which mode it should be used. (I've fixed the ssl_verification_mode one description, it was missing the NOTE formatter)

Copy link
Contributor

@kaisecheng kaisecheng left a comment

Choose a reason for hiding this comment

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

agree we can cover the tests in another PR and not block this feature.
Awesome work for standardisation. LGTM

@edmocosta edmocosta merged commit a725dcb into logstash-plugins:main Dec 19, 2023
@edmocosta edmocosta deleted the standardize_ssl_settings branch December 19, 2023 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants