Filter out CA PrivateKeyEntry when creating a KeyManager #73807

BigPandaToo · 2021-06-06T19:10:28Z

In 8.0, with security on by default, we store the HTTP
layer CA PrivateKeyEntry in the http.ssl keystore (along
with the node certificate) so that it is available in our
Enrollment API transport actions.
When loading a keystore, the current behavior is that the
X509ExtendedKeyManager will iterate through the PrivateKeyEntry
objects and will return the first key/certificate that satisfies
the requirements of the client and the server configuration,
and lacks any additional logic/filters.
We need the KeyManager to deterministically pick the node
certificate/key in all cases as this is the intended entry to be
used for TLS on the HTTP layer.
This change introduces filtering when creating the in-memory
keystore the KeyManager is loaded with, so that it will not
include PrivateKeyEntry objects when:

there are more than 1 PrivateKeyEntry objects in the keystore
The leaf certificate associated with the PrivateKeyEntry is a
CA certificate

Related: #75097

BigPandaToo · 2021-06-06T19:14:54Z

@elasticmachine update branch

BigPandaToo · 2021-06-08T16:03:52Z

@elasticmachine update branch

BigPandaToo · 2021-06-08T20:40:49Z

@elasticmachine update branch

BigPandaToo · 2021-06-16T14:28:33Z

@elasticmachine update branch

BigPandaToo · 2021-06-18T15:29:22Z

@elasticmachine update branch

BigPandaToo · 2021-06-23T11:15:49Z

@elasticmachine update branch

BigPandaToo · 2021-06-24T13:34:02Z

@elasticmachine update branch

jkakavas · 2021-07-07T11:53:30Z

@BigPandaToo I looked into the failing tests and these are actually bugs in the enroll node and enroll kibana APIs that we missed originally because of lack of integration tests. I fixed them here instead of opening a new PR, let me know if you have any concerns with that

jkakavas

Can we also please change the PR title to something more appropriate and add a PR description that explains why we're adding this ?

client/rest-high-level/qa/ssl-enabled/build.gradle

jkakavas · 2021-07-07T12:04:32Z

client/rest-high-level/qa/ssl-enabled/build.gradle

+  setting 'xpack.security.transport.ssl.enabled', 'true'
+  setting 'xpack.security.http.ssl.keystore.path', 'httpCa.p12'
+  setting 'xpack.security.transport.ssl.keystore.path', 'transport.p12'
+  setting 'xpack.security.transport.ssl.verification_mode', 'none'


Why do we need verification mode to be none for transport ?

Changed it to certificate. The reason I didn't leave it to be full - this transport cert doesn't have SAN (which is required for the full verification mode). I think it is orthogonal to these test purposes.

Changed it to certificate.

This is what we will be configuring nodes with too, so it makes sense to have this to certificate instead of none.

The reason I didn't leave it to be full - this transport cert doesn't have SAN (which is required for the full verification mode).I think it is orthogonal to these test purposes.

We are using this cluster for Enrollment IT and our enrollment process will generate configuration that sets the transport verification mode to certificate so it's in our best interest to keep the test cluster config as close to what users will have as possible

client/rest-high-level/qa/ssl-enabled/build.gradle

...t-high-level/qa/ssl-enabled/src/javaRestTest/java/org/elasticsearch/client/EnrollmentIT.java

...java/org/elasticsearch/xpack/security/action/enrollment/TransportKibanaEnrollmentAction.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfig.java

BigPandaToo · 2021-07-08T02:33:23Z

@elasticmachine update branch

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfig.java

jkakavas · 2021-07-08T05:53:50Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfigTests.java

+        assertThat(http_key, notNullValue());
+        assertThat(aliases.length, equalTo(1));
+        assertThat(aliases[0], equalTo("http"));
+        assertThat(certificates.length, equalTo(2));


I think we can remove this assertion, it doesn't test something that we are interested in ( the certificate chain length of the non-ca certificate we happened to use in the test ) - same as below

Just making sure we haven't deleted something we didn't mean to (like it happened before the last chage)

This the certificate chain length of a single certificate chain of a single PrivateKeyEntry. ( final X509Certificate[] certificates = keyManager.getCertificateChain("http"); )

We wouldn't have deleted one of the certificates in that chain before the last change.

I still think we don't need this, but won't block the PR on this

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfigTests.java

jkakavas · 2021-07-08T05:57:12Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfigTests.java

+        assertThat(aliases[0], equalTo("ca"));
+        assertThat(certificates.length, equalTo(1));
+    }
+


maybe quickly add one that contains a keystore with private key entries and trusted certificate entries and check the behavior there too ?

What do you mean? A keystore with multiple not ca private key? Making sure nothing being deleted?

A keystore with a TrustedCertificate entry, a CA PrivateKeyEntry and a non-ca PrivateKeyEntry and make sure that we removed the CA PrivateKeyEntry only

I believe that's what httpCa.p12 is in testCreateKeyManagerFromPKCS12ContainingCA

...rity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommandTests.java

jkakavas

Thanks for the iteration @BigPandaToo . Let's fix the keystore checking for multiple entries and a couple minor things and it'd be good to be merged

jkakavas · 2021-07-08T06:19:05Z

Suggestion for PR Title : "Filter out CA PrivateKeyEntry when creating a KeyManager"

Suggestion for the description: ( It'd be nice to have diffs and suggested text as with code :/ but we don't so I add this below , feel free to pick any of the wording you might like )

In 8.0, with security on by default, we store ca
Private Key Entry used for node and Kibana
enrollment in http.sll keystore. This  keystore is
used for TLS. The current behavior is that the
KeyManager will iterate through the private key
entry and will return the first key/certificate it sees
fits, which lacking any additional logic/filters is any
of the PrivateKeyEntry. If it picks up ca Private
Key instead of the node' one TLS handshake will
fail. The solution is, when creating an in-memory
key store from filter our ca Private Key entry if

    this is http key store;
    there are more than 1 alises in the store (cover
    the case when ca is the node certificate).

----->

In 8.0, with security on by default, we store the HTTP
layer CA PrivateKeyEntry in the  http.ssl keystore (along
with the node certificate) so that it is available in our 
Enrollment API transport actions. 
When loading a keystore, the current behavior is that the
X509ExtendedKeyManager will iterate through the PrivateKeyEntry
objects and will return the first key/certificate that satisfies
the requirements of the client and the server configuration, 
and lacks any additional logic/filters.
We need the KeyManager to deterministically pick the node
certificate/key in all cases as this is the intended entry to be
used for TLS on the HTTP layer. 
This change introduces filtering when creating the in-memory
keystore the KeyManager is loaded with, so that it will not 
include PrivateKeyEntry objects when: 

- there are more than 1 PrivateKeyEntry objects in the keystore 
- The leaf certificate associated with the PrivateKeyEntry is a
CA certificate

jkakavas

I have a couple of outstanding comments for your consideration but I don't want to hold the PR for more. See if you can address these if you agree, LGTM otherwise.

jkakavas · 2021-07-09T04:22:02Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfigTests.java

+            keyStore.load(in, keyStorePassword.getChars());
+        }
+        List<String> aliases = new ArrayList<>();
+        for (String s : Collections.list(keyStore.aliases())) {


we should not be filtering here. We should check that the aliases of all entries are 3 so that we can verify below that we do not remove non PrivateKeyEntry entries

Handle a case of multiple keys in the same keystore.

8f4d1c8

BigPandaToo marked this pull request as draft June 6, 2021 19:10

elasticmachine and others added 5 commits June 6, 2021 14:14

Merge branch 'master' into Sobd_CA

de1cf4d

Handle a case of multiple keys in the same keystore.

dacef10

Handle a case of multiple keys in the same keystore.

f38429e

Handle a case of multiple keys in the same keystore.

52f8ae6

Handle a case of multiple keys in the same keystore.

03db8e2

elasticmachine and others added 4 commits June 8, 2021 11:03

Merge branch 'master' into Sobd_CA

b345f2b

Handle a case of multiple keys in the same keystore.

6a741f9

Handle a case of multiple keys in the same keystore.

a8fbc74

Handle a case of multiple keys in the same keystore.

3a0983b

Merge branch 'master' into Sobd_CA

62047cd

Merge branch 'master' into Sobd_CA

7a40b6a

elasticmachine and others added 4 commits June 18, 2021 10:29

Merge branch 'master' into Sobd_CA

cbd0623

Handle the case of multiple keys in the same keystore

d408fa9

Handle the case of multiple keys in the same keystore

6b4e3a6

Handle a case of multiple keys in the same keystore

2c4ff7d

elasticmachine and others added 2 commits June 23, 2021 06:15

Merge branch 'master' into Sobd_CA

7467e99

Handle the case of multiple keys in the same keystore

b2fdcb6

elasticmachine and others added 4 commits June 24, 2021 08:34

Merge branch 'master' into Sobd_CA

2e24fd6

Handle the case of multiple keys in the same keystore

b6c5a91

Handle the case of multiple keys in the same keystore

d2bf651

Handle the case of multiple keys in the same keystore

03edb70

Kibana Enrollment request doesn't have a body anymore

896a922

Fix test expected values

115732f

jkakavas requested changes Jul 7, 2021

View reviewed changes

BigPandaToo mentioned this pull request Jul 8, 2021

Only filter out CA certificates for xpack.security.http.ssl keystore #75097

Closed

Handle the case of multiple keys in the same keystore

16813e3

BigPandaToo changed the title ~~Handle a case of multiple keys in the same keystore.~~ Security on by default: handle a situation when a keystore that is used for HTTPS contains more than one PrivateKeyEntry Jul 8, 2021

BigPandaToo requested a review from jkakavas July 8, 2021 02:08

Handle the case of multiple keys in the same keystore

1aed5c4

elasticmachine and others added 2 commits July 7, 2021 22:33

Merge branch 'master' into Sobd_CA

480390c

Handle the case of multiple keys in the same keystore

90a8001

jkakavas reviewed Jul 8, 2021

View reviewed changes

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfig.java Show resolved Hide resolved

jkakavas reviewed Jul 8, 2021

View reviewed changes

Handle the case of multiple keys in the same keystore

4733a06

jkakavas reviewed Jul 8, 2021

View reviewed changes

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/StoreKeyConfigTests.java Outdated Show resolved Hide resolved

jkakavas reviewed Jul 8, 2021

View reviewed changes

...rity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlMetadataCommandTests.java Show resolved Hide resolved

jkakavas requested changes Jul 8, 2021

View reviewed changes

Handle the case of multiple keys in the same keystore

beaee05

BigPandaToo changed the title ~~Security on by default: handle a situation when a keystore that is used for HTTPS contains more than one PrivateKeyEntry~~ Filter out CA PrivateKeyEntry when creating a KeyManager Jul 8, 2021

BigPandaToo requested a review from jkakavas July 8, 2021 07:59

jkakavas approved these changes Jul 8, 2021

View reviewed changes

BigPandaToo merged commit 063a1f2 into elastic:master Jul 8, 2021

jkakavas reviewed Jul 9, 2021

View reviewed changes

jkakavas mentioned this pull request Aug 13, 2021

Enroll Kibana API uses Service Accounts #76370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter out CA PrivateKeyEntry when creating a KeyManager #73807

Filter out CA PrivateKeyEntry when creating a KeyManager #73807

BigPandaToo commented Jun 6, 2021 •

edited

Loading

BigPandaToo commented Jun 6, 2021

BigPandaToo commented Jun 8, 2021

BigPandaToo commented Jun 8, 2021

BigPandaToo commented Jun 16, 2021

BigPandaToo commented Jun 18, 2021

BigPandaToo commented Jun 23, 2021

BigPandaToo commented Jun 24, 2021

jkakavas commented Jul 7, 2021

jkakavas left a comment

jkakavas Jul 7, 2021

BigPandaToo Jul 8, 2021

jkakavas Jul 8, 2021

BigPandaToo commented Jul 8, 2021

jkakavas Jul 8, 2021 •

edited

Loading

BigPandaToo Jul 8, 2021 •

edited

Loading

jkakavas Jul 8, 2021

jkakavas Jul 8, 2021

jkakavas Jul 8, 2021

BigPandaToo Jul 8, 2021

jkakavas Jul 8, 2021

BigPandaToo Jul 8, 2021

jkakavas left a comment

jkakavas commented Jul 8, 2021

jkakavas left a comment

jkakavas Jul 9, 2021

Filter out CA PrivateKeyEntry when creating a KeyManager #73807

Filter out CA PrivateKeyEntry when creating a KeyManager #73807

Conversation

BigPandaToo commented Jun 6, 2021 • edited Loading

BigPandaToo commented Jun 6, 2021

BigPandaToo commented Jun 8, 2021

BigPandaToo commented Jun 8, 2021

BigPandaToo commented Jun 16, 2021

BigPandaToo commented Jun 18, 2021

BigPandaToo commented Jun 23, 2021

BigPandaToo commented Jun 24, 2021

jkakavas commented Jul 7, 2021

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigPandaToo commented Jul 8, 2021

jkakavas Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

BigPandaToo Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas left a comment

Choose a reason for hiding this comment

jkakavas commented Jul 8, 2021

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigPandaToo commented Jun 6, 2021 •

edited

Loading

jkakavas Jul 8, 2021 •

edited

Loading

BigPandaToo Jul 8, 2021 •

edited

Loading