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

Standardize SSL settings #470

Merged
merged 4 commits into from
Apr 5, 2023

Conversation

edmocosta
Copy link
Contributor

@edmocosta edmocosta commented Mar 28, 2023

What this PR does?

Added the following SSL settings
ssl_enabled: Enable/disable SSL
ssl_client_authentication: Controls the server’s behavior in regard to requesting a certificate from client connections

Reviewed and deprecated the following SSL settings to comply with Logstash's naming convention
ssl in favor of ssl_enabled
ssl_verify_mode in favor of ssl_client_authentication

Other changes

  • Using ssl_verify_mode should keep the current behavior as it is. Once upgraded to ssl_client_authentication, it will validate and reject configurations with ssl_certificate_authorities set and ssl_client_authentication = > none. The current ssl_verify_mode behavior is to silently ignore the none value and use force_peer/required.

  • Fixed ssl_peer_metadata when ssl_enabled => false: Standardize SSL settings #470 (comment)

  • Added tests for the new and existing SslContextBuilder methods.

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


Closes elastic/logstash#14925
Closes #399

@edmocosta edmocosta force-pushed the standardize_ssl_settings branch from 1dacba8 to 481994e Compare March 30, 2023 16:19
@edmocosta edmocosta force-pushed the standardize_ssl_settings branch from 481994e to c0af983 Compare March 30, 2023 16:40
@edmocosta edmocosta changed the title [WIP] Standardize and add SSL settings Standardize SSL settings Mar 30, 2023
@edmocosta edmocosta marked this pull request as ready for review March 30, 2023 16:41
@roaksoax roaksoax requested review from andsel and jsvd March 30, 2023 17:11
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

Overall seems good, but left some comments for improvement.

lib/logstash/inputs/beats.rb Show resolved Hide resolved
lib/logstash/inputs/beats.rb Outdated Show resolved Hide resolved
lib/logstash/inputs/beats.rb Outdated Show resolved Hide resolved
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
src/main/java/org/logstash/netty/SslContextBuilder.java Outdated Show resolved Hide resolved

import static org.hamcrest.Matchers.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid * import switch to explicit list

Copy link
Contributor

Choose a reason for hiding this comment

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

same for the following import of such kind

@edmocosta edmocosta force-pushed the standardize_ssl_settings branch from b2a56ef to 19e21ba Compare March 31, 2023 14:58
@edmocosta edmocosta requested a review from andsel April 3, 2023 07:40
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

@edmocosta edmocosta merged commit b6ea0a2 into logstash-plugins:main Apr 5, 2023
@edmocosta edmocosta deleted the standardize_ssl_settings branch April 5, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants