-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Validate SSL settings at parse time #49196
Conversation
Pinging @elastic/es-core-features (:Core/Features/Monitoring) |
@elasticmachine update branch |
@rjernst, this is the last of the settings validation PRs if you or someone else from your team would like to review it. |
@danhermann Perhaps I am misunderstanding this change. I thought the point was to add validators to the Setting instances, yet this seems to still do validation much later? Is the problem the "ssl" setting is a group setting, and thus we don't try to look at the settings underneath it until later? |
@rjernst, the problem with adding the validation to |
@elasticmachine update branch |
@rjernst, is this approach to validation acceptable to you or do I need to revisit it? |
There are two issues I see here:
|
@rjernst, thank you for the additional info. Let me add a few details around the issues you raised above.
The validation that's happening there is a check that none of the SSL settings involve secure settings since they are not reloadable and so may not be dynamically changed. If there are no secure settings, the I believe the warning about http with user/pass is still logged at the same point as before.
Fully validating the SSL settings requires an instance of That said, there are three scenarios I can think of in which invalid values could be provided for these settings:
I thought it might be acceptable given that it prevents the first scenario, the second scenario is already handled, and the third scenario would be possible only when upgrading from previous ES versions and after recovery would subsequently be prevented. Let me know if you think I missed another option here that would be preferable. |
merge conflict between base and head |
@elasticmachine update branch |
@rjernst, do any of the comments I made above address either of the issues you raised? |
Dan and I spoke offline and agreed the validation should be simpler if each SSL setting is an affix setting instead of pulling them all together as a group setting. I'm ok with this PR going in if there is a compelling need to get this validation in. @danhermann Do you think we should get this in or close it in favor of the cleanup work described? |
Yes, I'll close this one out in favor of separate affix settings for the SSL settings. |
@rjernst I agree with the your earlier comment, however, after discussion with @danhermann, ideally these SSL settings would be re-usable and validate-able affix settings which would require a decent investment. This PR as-is provides protection from invalid cluster state and would like to get this in sooner then later. Any objections to re-open and merge this, and log a follow up issue ? |
I'm fine with that. |
@elasticmachine update branch |
@elasticmachine update branch |
Provides parse-time validation for monitoring SSL/TLS settings as described in #47711.