Standardize and add SSL settings #1118

edmocosta · 2023-02-28T15:54:16Z

What this PR does?

Added the following SSL settings:
ssl_truststore_type: The format of the truststore file
ssl_keystore_type: The format of the keystore file
ssl_certificate: OpenSSL-style X.509 certificate file to authenticate the client
ssl_key: OpenSSL-style RSA private key that corresponds to the ssl_certificate
ssl_cipher_suites: The list of cipher suites

Reviewed and deprecated the following SSL settings to comply with Logstash's naming convention:
ssl in favor of ssl_enabled
cacert in favor of ssl_certificate_authorities
keystore in favor of ssl_keystore_path
keystore_password in favor of ssl_keystore_password
truststore in favor of ssl_truststore_path
truststore_password in favor of ssl_truststore_password
ssl_certificate_verification in favor of ssl_verification_mode

The behavior standardization across plugins, such as the accepted certificate formats, default values, etc will be tackled in future PRs.

Closes elastic/logstash#14924
Closes #404
Closes #672
Closes #676

andsel

First review run.
Left some comments to evaluate.

andsel · 2023-03-09T12:58:51Z

docs/index.asciidoc

 [id="plugins-{type}s-{plugin}-keystore"]
 ===== `keystore`
+deprecated[8.8.0, Replaced by <<plugins-{type}s-{plugin}-ssl_keystore_path>>]



I think should refer to the plugin's version (11.14.10). Given such big change, I would think to bump at 12.0.0.
This comment is valid for all the following deprecated[8.8.0, changes

Yes, I initially thought about it as well but given it is backward compatible I ended up increasing only the minor 11.14.0. Do you think we should bump at 12.0.0? Even if it does not break/change any existing behavior?

Backwards compatible changes do not require a major bump. This is additive, and all configurations that work under 11.13.x will continue to work without modification in 11.14.0.

docs/index.asciidoc

lib/logstash/outputs/elasticsearch/http_client_builder.rb

spec/unit/outputs/elasticsearch_spec.rb

yaauie · 2023-03-10T02:36:55Z

lib/logstash/outputs/elasticsearch/http_client_builder.rb

+        ssl_options[:client_key] = ssl_key
+      elsif !!ssl_certificate || !!ssl_key
+        raise(LogStash::ConfigurationError, 'You must set both "ssl_certificate" and "ssl_key" for client authentication')
      end


Message is a bit unclear (you don't actually have to set either, because you can use a keystore), and catches two distinct cases:

using ssl_certificate requires ssl_key AND

using ssl_key requires ssl_certificate

I prefer declaring it in a way that centers the single config that "activates" the feature, in this case ssl_certificate:

using ssl_certificate requires an ssl_key

an ssl_certificate is required when using an ssl_key

yaauie · 2023-03-10T02:41:01Z

docs/index.asciidoc

 | <<plugins-{type}s-{plugin}-sniffing_path>> |<<string,string>>|No
-| <<plugins-{type}s-{plugin}-ssl>> |<<boolean,boolean>>|No
-| <<plugins-{type}s-{plugin}-ssl_certificate_verification>> |<<boolean,boolean>>|No
+| <<plugins-{type}s-{plugin}-ssl>> |<<boolean,boolean>>|__Deprecated__


I would prefer not to leave the deprecated settings in this table, and perhaps to have a separate section with them and their detailed blocks, so that we can reduce the noise new users of the plugin encounter while still ensuring that the deprecated options are still discoverable to those who are looking for documentation for options currently in their pipeline configs.

…ttings into a new section

andsel

LGTM

karenzone · 2023-03-21T21:25:57Z

CHANGELOG.md

@@ -1,3 +1,19 @@
+## 11.14.0
+ - Added SSL settings for: [#1115](https://github.com/logstash-plugins/logstash-output-elasticsearch/pull/1115)
+   - `ssl_truststore_type`: The format of the truststore file


Is this PR in the changelog correct? Shouldn't it be a link to this PR (#1118)?

Yes, you're right, it should be #1118. I'll submit a fix! Thanks for the heads up!

Thanks, @edmo. Remember that the link will be wrong for any release notes that include 11.14.0, and will require a manual fix. :-)

karenzone · 2023-03-21T21:34:12Z

docs/index.asciidoc

 Elasticsearch documentation.

+[id="plugins-{type}s-{plugin}-deprecated-options"]
+==== Elasticsearch Output Deprecated Configuration Options


Deviation from the way we typically handle deprecations in plugins. (See https://www.elastic.co/guide/en/logstash/current/plugins-inputs-beats.html#plugins-inputs-beats-options.)

Standards can change over time, but the change needs to be deliberate and communicated clearly.

Nevermind. I see where this came from: #1118 (comment).

This is definitely better for new users, but isn't very nice to our existing users who are scanning an alphabetical list or descriptions for a specific setting in their config.

whyyouwannaknow · 2024-08-09T09:11:46Z

Hello,

I might not post in the correct issue but we saw a deprecation on the setting ssl_certificate_verification which we replaced by ssl_verification_mode.
The thing is that this new setting ssl_verification_mode only supports the value full or none but not the value certificate.

This value certificate should be implemented in the output ElasticSearch of a Logstash pipeline as it is more secured then putting none instead.

Could this be implemented in a future update? Shoud we open a separated issue for this?

Thank you.

Regards,

edmocosta · 2024-08-09T09:34:17Z

Hi @whyyouwannaknow!

Thank you for reaching out! Unfortunately, the underline HTTP library this plugin uses does not fully support the certificate mode, that's why it only accepts the full & none options. Please note that the previous configuration also didn't support it, being true => full and false => none.

We're working on standardising the SSL/TLS configs and features across plugins (elastic/logstash#14905), and that project's second phase also includes adding support to the certificate mode.

Thank you!

Deprecated and added new SSL settings

953def7

roaksoax added the status:work-in-progress label Feb 28, 2023

Fixing typo on the setting name

6f3828b

roaksoax assigned edmocosta Feb 28, 2023

Added missing SSL settings

03e6d71

edmocosta force-pushed the standardize_ssl_settings branch from 96eb956 to 03e6d71 Compare March 2, 2023 12:52

edmocosta changed the title ~~[WIP] Standardize SSL settings~~ Standardize SSL settings Mar 2, 2023

edmocosta changed the title ~~Standardize SSL settings~~ Standardize and add SSL settings Mar 2, 2023

edmocosta marked this pull request as ready for review March 2, 2023 14:14

edmocosta added status:needs-review and removed status:work-in-progress labels Mar 2, 2023

edmocosta added 2 commits March 2, 2023 18:57

[ci skip] Fixing docs

0059d64

Moved setup_ssl_params call to the register method

6814686

roaksoax requested a review from andsel March 9, 2023 02:51

andsel requested changes Mar 9, 2023

View reviewed changes

roaksoax added status:changes-requested and removed status:needs-review labels Mar 9, 2023

andsel mentioned this pull request Mar 9, 2023

Standardize and add SSL settings logstash-plugins/logstash-filter-elasticsearch#168

Merged

Apply PR suggestions

ef9bc18

yaauie reviewed Mar 10, 2023

View reviewed changes

edmocosta added 2 commits March 10, 2023 09:55

Fixing style guide for raise exceptions

e71f9a7

Improving ssl_certificate/ssl_key validation and moving deprecated se…

185cbe3

…ttings into a new section

edmocosta force-pushed the standardize_ssl_settings branch from 82ed184 to 185cbe3 Compare March 10, 2023 10:35

edmocosta mentioned this pull request Mar 10, 2023

Standardize and add SSL settings logstash-plugins/logstash-input-elasticsearch#185

Merged

edmocosta requested a review from andsel March 10, 2023 10:59

andsel approved these changes Mar 10, 2023

View reviewed changes

roaksoax added status:approved and removed status:changes-requested labels Mar 10, 2023

edmocosta merged commit cfe44f3 into logstash-plugins:main Mar 10, 2023

edmocosta deleted the standardize_ssl_settings branch March 10, 2023 16:51

roaksoax removed the status:approved label Mar 10, 2023

karenzone reviewed Mar 21, 2023

View reviewed changes

edmocosta mentioned this pull request Mar 22, 2023

Fix 11.14.0 CHANGELOG PR link #1122

Merged

yaauie mentioned this pull request May 30, 2023

BREAKING: ssl_certificate_verification => true uses deprecated verifier since v11.14.0 #1138

Closed

yaauie mentioned this pull request Jun 12, 2023

normalized SSL config not applied when LS core uses private-API build_client directly #1141

Closed

edmocosta mentioned this pull request Jun 22, 2023

Support TLS certificate & key pair #998

Closed

robbavey mentioned this pull request Jul 14, 2023

Add further documentation for Logstash volumes elastic/cloud-on-k8s#7022

Merged

itay-nvn-nv mentioned this pull request Apr 15, 2024

SSL settings changed and not documented #1178

Closed

donoghuc mentioned this pull request Dec 9, 2024

Remove deprecated SSL settings from Elasticsearch output #1190

Closed

7 tasks

Standardize and add SSL settings #1118

Standardize and add SSL settings #1118

Uh oh!

Conversation

edmocosta commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does?

Uh oh!

andsel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaauie Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andsel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karenzone Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whyyouwannaknow commented Aug 9, 2024

Uh oh!

edmocosta commented Aug 9, 2024

Uh oh!

Uh oh!

edmocosta commented Feb 28, 2023 •

edited

Loading

yaauie Mar 10, 2023 •

edited

Loading

karenzone Mar 21, 2023 •

edited

Loading