-
Notifications
You must be signed in to change notification settings - Fork 274
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
base: main
Are you sure you want to change the base?
Allow skipping hot reload dn validation #4752
Conversation
…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 <[email protected]>
…or new initTestCluster params Signed-off-by: Paris Larkins <[email protected]>
ff40ae2
to
c757db7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4752 +/- ##
==========================================
+ Coverage 65.22% 67.97% +2.75%
==========================================
Files 318 310 -8
Lines 22327 20932 -1395
Branches 3591 3318 -273
==========================================
- Hits 14562 14228 -334
+ Misses 5968 4955 -1013
+ Partials 1797 1749 -48
|
Signed-off-by: Paris Larkins <[email protected]>
70a6154
to
18ded5c
Compare
There was a problem hiding this 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
src/test/java/org/opensearch/security/ssl/SecuritySSLReloadCertsActionTests.java
Show resolved
Hide resolved
Signed-off-by: Paris Larkins <[email protected]>
src/main/java/org/opensearch/security/ssl/OpenSearchSecuritySSLPlugin.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/ssl/SecuritySSLReloadCertsActionTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 <[email protected]>
@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. |
String updateKey = (Objects.equals(updateChannel, "http")) ? HTTP_CERTIFICATES_LIST_KEY : TRANSPORT_CERTIFICATES_LIST_KEY; | ||
String oldKey = (Objects.equals(updateChannel, "http")) ? TRANSPORT_CERTIFICATES_LIST_KEY : HTTP_CERTIFICATES_LIST_KEY; | ||
final var updatedCertDetailsResponse = DefaultObjectMapper.objectMapper.createObjectNode(); | ||
updatedCertDetailsResponse.set(updateKey, buildCertsInfoNode(NEW_CA_NODE_CERT_DETAILS)); | ||
updatedCertDetailsResponse.set(oldKey, buildCertsInfoNode(NODE_CERT_DETAILS)); | ||
return updatedCertDetailsResponse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got kind of confused reading this helper function - can we refactor this for readability? I thought that this was just to help with the dn of either the http or transport certs, but why do we have keys for both http and transport (not just one or the other)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @derek-ho, agreed! I have refactored this and the existing getUpdatedCertDetailsExpectedResponse
into a simpler method. Both http and transport keys are required because the expected JSON response returned here is compared with the response from a GET
to _opendistro/_security/api/ssl/certs
, which includes both http and transport certs:
{
"http_certificates_list": [
{
"issuer_dn": "CN=Example Com Inc. Signing CA,OU=Example Com Inc. Signing CA,O=Example Com Inc.,DC=example,DC=com",
"subject_dn": "CN=node-1.example.com,OU=SSL,O=Test,L=Test,C=DE",
"san": "[[0, [2.5.4.3, node-1.example.com]], [2, localhost], [2, node-1.example.com], [7, 127.0.0.1], [8, 1.2.3.4.5.5]]",
"not_before": "2023-04-14T13:22:53Z",
"not_after": "2033-04-11T13:22:53Z"
}
],
"transport_certificates_list": [
{
"issuer_dn": "CN=Example Com Inc. Signing CA,OU=Example Com Inc. Signing CA,O=Example Com Inc.,DC=example,DC=com",
"subject_dn": "CN=node-1.example.com,OU=SSL,O=Test,L=Test,C=DE",
"san": "[[0, [2.5.4.3, node-1.example.com]], [2, localhost], [2, node-1.example.com], [7, 127.0.0.1], [8, 1.2.3.4.5.5]]",
"not_before": "2023-04-14T13:23:00Z",
"not_after": "2033-04-11T13:23:00Z"
}
]
}
This way we can verify that the cert for only one channel was updated.
…tUpdatedCertDetailsExpectedResponse helpers into simplified getCertDetailsExpectedResponse Signed-off-by: Paris Larkins <[email protected]>
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.
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 tofalse
.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.