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

Allow skipping hot reload dn validation #4752

Conversation

parislarkins
Copy link
Contributor

@parislarkins parislarkins commented Sep 24, 2024

Description

This change adds two new boolean settings for the Security plugin:

  • plugins.security.ssl.http.enforce_cert_reload_dn_verification
  • plugins.security.ssl.transport.enforce_cert_reload_dn_verification

Both options will default to True, which will maintain the current behaviour where performing a hot reload requires the Distinguished Names (IssuerDN, SubjectDN, and SAN) in the new certificate to match the current certificate.

When set to False, the Distinguished Names validation will be skipped when hot reloading the respective certificate (HTTP or Transport).

The current behaviour is preventing us from hot-reloading new Let's Encrypt certificates that were signed by a different intermediate CA than the original certificate, meaning we need to perform a rolling restart of each of our clusters in order to rotate the certificates. According to Let's Encrypt, one of their root CAs is expected to expire as soon as 2030 (https://letsencrypt.org/certificates/), so an approach that allowed different intermediate CAs but still rejected changed root CAs will still require us to perform a rolling restart for our clusters when the root CAs are inevitably rotated.

We do not believe adding the ability to disable this validation for hot-reloads creates any added security risk, as long as it is properly considered considering organisational context. There is no similar validation performed when changing certificates by restarting OpenSearch, and an actor that could trigger a hot-reload (requiring them to modify the certificate files on the OpenSearch node) would also likely be able to bypass the validation by restarting OpenSearch.

For example, in our operational context only the super admin user can trigger a hot-reload. The certificate for the super admin user is stored only on the OpenSearch nodes, so any malicious actor who had the super admin certificate and ability to modify the OpenSearch certificate files would also have the ability to restart OpenSearch. This makes the validation performed when triggering a hot-reload irrelevant, as any malicious actor could simply restart OpenSearch instead.

  • Category: New feature

Issues Resolved

Testing

Unit tests have been added that cover reloading certificates to a certificate signed by a different Certificate Authority (different root and signing CA), and validating that this is rejected when the new settings are set to true, and accepted when the settings are set to false.

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…ification and plugins.security.ssl.transport.enforce_cert_reload_dn_verification, to control whether DN validation should be performed when hot reloading certificates

Signed-off-by: Paris Larkins <paris.larkins@instaclustr.com>
…or new initTestCluster params

Signed-off-by: Paris Larkins <paris.larkins@instaclustr.com>
@parislarkins parislarkins force-pushed the allow-skipping-hot-reload-dn-validation branch from ff40ae2 to c757db7 Compare September 24, 2024 05:11
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.97%. Comparing base (4f2e689) to head (d74905f).
Report is 55 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4752      +/-   ##
==========================================
+ Coverage   65.22%   70.97%   +5.75%     
==========================================
  Files         318      310       -8     
  Lines       22327    20946    -1381     
  Branches     3591     3326     -265     
==========================================
+ Hits        14562    14867     +305     
+ Misses       5968     4329    -1639     
+ Partials     1797     1750      -47     
Files with missing lines Coverage Δ
...ensearch/security/ssl/DefaultSecurityKeyStore.java 68.57% <100.00%> (+1.90%) ⬆️
...arch/security/ssl/OpenSearchSecuritySSLPlugin.java 86.53% <100.00%> (+0.22%) ⬆️
...ensearch/security/ssl/util/SSLConfigConstants.java 77.77% <ø> (ø)

... and 112 files with indirect coverage changes

Signed-off-by: Paris Larkins <paris.larkins@netapp.com>
@parislarkins parislarkins force-pushed the allow-skipping-hot-reload-dn-validation branch from 70a6154 to 18ded5c Compare September 25, 2024 23:48
Copy link
Contributor

@shikharj05 shikharj05 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

Signed-off-by: Paris Larkins <paris.larkins@netapp.com>
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thank you @parislarkins , this change looks reasonable to me.

What happens if you disable dn validation and change the dn of a node, but the dn is not in the nodes_dn list?

…behaviour test cases

Signed-off-by: Paris Larkins <paris.larkins@netapp.com>
@parislarkins
Copy link
Contributor Author

Thank you @parislarkins , this change looks reasonable to me.

What happens if you disable dn validation and change the dn of a node, but the dn is not in the nodes_dn list?

@cwperks I have not tested this specifically, but I expect the node would no longer be able to connect to the remote cluster. As I understand it, this is the same behaviour that would happen if the cert was changed by restarting OpenSearch as well.

cwperks
cwperks previously approved these changes Oct 3, 2024
@cwperks cwperks added the backport 2.x backport to 2.x branch label Oct 4, 2024
…tUpdatedCertDetailsExpectedResponse helpers into simplified getCertDetailsExpectedResponse

Signed-off-by: Paris Larkins <paris.larkins@netapp.com>
Copy link
Collaborator

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

Thanks

@cwperks cwperks merged commit c8cacf1 into opensearch-project:main Oct 23, 2024
42 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 23, 2024
Signed-off-by: Paris Larkins <paris.larkins@instaclustr.com>
Signed-off-by: Paris Larkins <paris.larkins@netapp.com>
(cherry picked from commit c8cacf1)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Exception when hot-reloading Let's Encrypt certs issued by alternate intermediate CAs
4 participants