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

Adding capability to hot reload ssl certificates #238

Merged

Conversation

debjanibnrj
Copy link
Contributor

@debjanibnrj debjanibnrj commented Feb 19, 2020

    * Added api PUT /_opendistro/_security/sslcerts/reload which reinitializes keystore
    * Added api GET /_opendistro/_security/nodecerts which gets transport public key information
    * Updated DefaultODSKeyStore to update, set and get transport layer certificates
    * Added unit tests

Description:

This feature allows super admin users to "hot reload" their expired SSL certificates without restarting their clusters. This API is very sensitive so currently it only allows users to replace their expired certificates with valid certificates issued with the same Issuer/Subject DN and SAN.

This API assumes that new certificates are in the same location specified by the security configurations in elasticsearch.yml (https://opendistro.github.io/for-elasticsearch-docs/docs/security-configuration/tls/) and the same TLS configuration values hold for the new certificates.

To enable this feature add the following property to your elasticsearch.yml file -

opendistro_security.ssl_cert_reload_enabled: true

This property is disabled by default.


API Reference:

PUT /_opendistro/_security/api/ssl/{certType}/reloadcerts
Description: This API updates SSL transport and http certificate information and is accessible by super admins only.

Request:

  • PUT /_opendistro/_security/api/ssl/transport/reloadcerts
  • PUT /_opendistro/_security/api/ssl/http/reloadcerts

Response:

 { "message": "successfully updated transport certs"}

GET /_opendistro/_security/api/ssl/certs
Description: This API returns SSL transport and http certificate information and is accessible by super admins only.

Request:

  • GET /_opendistro/_security/api/ssl/certs

Response:

 {
   "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":"[localhost]",
         "not_before":"2020-02-17T16:19:25Z",
         "not_after":"2022-02-16T16:19:25Z"
      }
   ],
   "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":"[localhost]",
         "not_before":"2020-02-17T16:19:25Z",
         "not_after":"2022-02-16T16:19:25Z"
      }
   ]
}

@debjanibnrj debjanibnrj force-pushed the hotreload branch 2 times, most recently from 9b26676 to 954e428 Compare February 24, 2020 23:42
@debjanibnrj
Copy link
Contributor Author

Tests failed due to flakes -

2020-02-25T00:08:46.0177126Z [INFO] 
2020-02-25T00:08:46.0184110Z [WARNING] Flakes: 
2020-02-25T00:08:46.0198049Z [WARNING] com.amazon.opendistroforelasticsearch.security.auditlog.sink.SinkProviderTLSTest.testTlsConfigurationNoFallback(com.amazon.opendistroforelasticsearch.security.auditlog.sink.SinkProviderTLSTest)
2020-02-25T00:08:46.0210834Z [ERROR]   Run 1: SinkProviderTLSTest.testTlsConfigurationNoFallback:66 » Bind Address already i...
2020-02-25T00:08:46.0223358Z [ERROR]   Run 2: SinkProviderTLSTest.testTlsConfigurationNoFallback:66 » Bind Address already i...
2020-02-25T00:08:46.0253227Z [ERROR]   Run 3: SinkProviderTLSTest.testTlsConfigurationNoFallback:66 » Bind Address already i...
2020-02-25T00:08:46.0254027Z [ERROR]   Run 4: SinkProviderTLSTest.testTlsConfigurationNoFallback:66 » Bind Address already i...
2020-02-25T00:08:46.0254322Z [INFO]   Run 5: PASS
2020-02-25T00:08:46.0254512Z [INFO] 
2020-02-25T00:08:46.0254784Z [WARNING] com.amazon.opendistroforelasticsearch.security.auditlog.sink.WebhookAuditLogTest.httpsTestPemContentEndpoint(com.amazon.opendistroforelasticsearch.security.auditlog.sink.WebhookAuditLogTest)
2020-02-25T00:08:46.0255486Z [ERROR]   Run 1: WebhookAuditLogTest.httpsTestPemContentEndpoint:670 » Bind Address already in ...
2020-02-25T00:08:46.0255814Z [INFO]   Run 2: PASS
2020-02-25T00:08:46.0256015Z [INFO]   Run 3: PASS
2020-02-25T00:08:46.0256203Z [INFO] 
2020-02-25T00:08:46.0256419Z [WARNING] com.amazon.opendistroforelasticsearch.security.auditlog.sink.WebhookAuditLogTest.postGetHttpTest(com.amazon.opendistroforelasticsearch.security.auditlog.sink.WebhookAuditLogTest)
2020-02-25T00:08:46.0259376Z [ERROR]   Run 1: WebhookAuditLogTest.postGetHttpTest:232 » Bind Address already in use (Bind fa...
2020-02-25T00:08:46.0260068Z [ERROR]   Run 2: WebhookAuditLogTest.postGetHttpTest:232 » Bind Address already in use (Bind fa...
2020-02-25T00:08:46.0263453Z [ERROR]   Run 3: WebhookAuditLogTest.postGetHttpTest:232 » Bind Address already in use (Bind fa...
2020-02-25T00:08:46.0264629Z [ERROR]   Run 4: WebhookAuditLogTest.postGetHttpTest:232 » Bind Address already in use (Bind fa...
2020-02-25T00:08:46.0264786Z [INFO]   Run 5: PASS
2020-02-25T00:08:46.0264879Z [INFO] 
2020-02-25T00:08:46.0266763Z [INFO] 
2020-02-25T00:08:46.0284075Z [WARNING] Tests run: 775, Failures: 0, Errors: 0, Skipped: 27, Flakes: 3```

@hardik-k-shah
Copy link
Member

Also, let's take a review from @aetter and @elfisher for API and config naming.

@debjanibnrj
Copy link
Contributor Author

debjanibnrj commented Feb 28, 2020

Comment on lines 349 to 354
final X509Certificate[] transportKeystoreCerts = new X509Certificate[]{ PemKeyReader.loadCertificateFromFile(pemCertFilePath) };

validateNewTransportCerts(transportKeystoreCerts);
setTransportSSLCerts(transportKeystoreCerts);
Copy link

Choose a reason for hiding this comment

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

Needs review

} catch (final Exception e1) {
builder = channel.newBuilder();
builder.startObject();
builder.field("error", e1.toString());
Copy link

Choose a reason for hiding this comment

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

Can we test some exceptions to see what information are we exposing ? (Similar for the other action)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following test cases return the exception if an Invalid DN is set and if the reload property is not there in the elasticsearch.yml config (https://github.com/opendistro-for-elasticsearch/security/pull/238/files#diff-714682d00b2af7282796d8efc9812f56R173-R198). I'll add one more in there in case the reloaded certificate has an invalid expiry date.

Let me know if there is any specific test that comes to your mind.

@@ -633,7 +633,7 @@ public void testNodeClientSSL() throws Exception {

@Test
public void testTransportClientSSLFail() throws Exception {
thrown.expect(NoNodeAvailableException.class);
thrown.expect(IllegalStateException.class);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier we were allowing transport client creation with an empty truststore file. I added a stronger validation for truststore validation so right now we will prevent transport client and OpenDistroPlugin will not load, hence cause this IllegalStateException

        * Added api PUT /_opendistro/_security/ssl/{certType}/reloadcerts which reinitializes http or transport keystore
        * Added api GET /_opendistro/_security/ssl/certs which returns public key details for http and transport certificates
        * Updated DefaultODSKeyStore to update, set and get http and transport layer certificates
        * Added unit tests
@debjanibnrj debjanibnrj merged commit 24fab5e into opensearch-project:opendistro-1.1 Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants