-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Filter out CA PrivateKeyEntry when creating a KeyManager #73807
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
Changes from all commits
8f4d1c8
de1cf4d
dacef10
f38429e
52f8ae6
03db8e2
b345f2b
6a741f9
a8fbc74
3a0983b
62047cd
7a40b6a
cbd0623
d408fa9
6b4e3a6
2c4ff7d
7467e99
b2fdcb6
2e24fd6
b6c5a91
d2bf651
03edb70
168d417
0a12420
ded0c1b
7ef6884
628d6ba
d80d443
7b30f62
896a922
115732f
16813e3
1aed5c4
480390c
90a8001
4733a06
beaee05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import org.elasticsearch.gradle.internal.test.RestIntegTestTask | ||
| import org.elasticsearch.gradle.internal.info.BuildParams | ||
|
|
||
| apply plugin: 'elasticsearch.java-rest-test' | ||
| dependencies { | ||
| javaRestTestImplementation(testArtifact(project(':client:rest-high-level'))) | ||
| } | ||
|
|
||
| tasks.matching{ it.name == "javaRestTest" }.configureEach { | ||
| onlyIf { BuildParams.inFipsJvm == false} | ||
| systemProperty 'tests.rest.cluster.username', System.getProperty('tests.rest.cluster.username', 'test_user') | ||
| systemProperty 'tests.rest.cluster.password', System.getProperty('tests.rest.cluster.password', 'test-user-password') | ||
| } | ||
|
|
||
| testClusters.matching { it.name == 'javaRestTest' }.configureEach { | ||
| testDistribution = 'DEFAULT' | ||
| numberOfNodes = 2 | ||
| setting 'xpack.license.self_generated.type', 'trial' | ||
| setting 'xpack.security.enabled', 'true' | ||
| setting 'xpack.security.authc.token.enabled', 'true' | ||
| setting 'xpack.security.authc.api_key.enabled', 'true' | ||
|
|
||
| extraConfigFile 'httpCa.p12', file('./src/javaRestTest/resources/httpCa.p12') | ||
| extraConfigFile 'transport.p12', file('./src/javaRestTest/resources/transport.p12') | ||
|
|
||
| // TBD: sync these settings (which options are set) with the ones we will be generating in #74868 | ||
| setting 'xpack.security.http.ssl.enabled', 'true' | ||
jkakavas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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', 'certificate' | ||
|
|
||
|
|
||
| keystore 'xpack.security.http.ssl.keystore.secure_password', 'password' | ||
| keystore 'xpack.security.transport.ssl.keystore.secure_password', 'password' | ||
| user username: 'admin_user', password: 'admin-password', role: 'superuser' | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
| * in compliance with, at your election, the Elastic License 2.0 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| package org.elasticsearch.client; | ||
|
|
||
| import org.elasticsearch.client.security.KibanaEnrollmentResponse; | ||
| import org.elasticsearch.client.security.NodeEnrollmentResponse; | ||
| import org.elasticsearch.common.settings.SecureString; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
| import org.elasticsearch.core.PathUtils; | ||
| import org.junit.AfterClass; | ||
| import org.junit.BeforeClass; | ||
|
|
||
| import java.io.FileNotFoundException; | ||
| import java.net.URL; | ||
| import java.nio.file.Path; | ||
| import java.util.List; | ||
|
|
||
| import static org.hamcrest.Matchers.endsWith; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.hamcrest.Matchers.notNullValue; | ||
|
|
||
| public class EnrollmentIT extends ESRestHighLevelClientTestCase { | ||
| private static Path httpTrustStore; | ||
|
|
||
| @BeforeClass | ||
| public static void findTrustStore() throws Exception { | ||
| final URL resource = EnrollmentIT.class.getResource("/httpCa.p12"); | ||
| if (resource == null) { | ||
| throw new FileNotFoundException("Cannot find classpath resource /httpCa.p12"); | ||
| } | ||
| httpTrustStore = PathUtils.get(resource.toURI()); | ||
| } | ||
|
|
||
| @AfterClass | ||
| public static void cleanupStatics() { | ||
| httpTrustStore = null; | ||
| } | ||
|
|
||
| @Override | ||
| protected String getProtocol() { | ||
| return "https"; | ||
| } | ||
|
|
||
| @Override | ||
| protected Settings restClientSettings() { | ||
| String token = basicAuthHeaderValue("admin_user", new SecureString("admin-password".toCharArray())); | ||
| return Settings.builder() | ||
| .put(ThreadContext.PREFIX + ".Authorization", token) | ||
| .put(TRUSTSTORE_PATH, httpTrustStore) | ||
| .put(TRUSTSTORE_PASSWORD, "password") | ||
| .build(); | ||
| } | ||
|
|
||
| public void testEnrollNode() throws Exception { | ||
| final NodeEnrollmentResponse nodeEnrollmentResponse = | ||
| execute(highLevelClient().security()::enrollNode, highLevelClient().security()::enrollNodeAsync, RequestOptions.DEFAULT); | ||
| assertThat(nodeEnrollmentResponse, notNullValue()); | ||
| assertThat(nodeEnrollmentResponse.getHttpCaKey(), endsWith("K2S3vidA=")); | ||
| assertThat(nodeEnrollmentResponse.getHttpCaCert(), endsWith("LfkRjirc=")); | ||
| assertThat(nodeEnrollmentResponse.getTransportKey(), endsWith("1I-r8vOQ==")); | ||
| assertThat(nodeEnrollmentResponse.getTransportCert(), endsWith("OpTdtgJo=")); | ||
| List<String> nodesAddresses = nodeEnrollmentResponse.getNodesAddresses(); | ||
| assertThat(nodesAddresses.size(), equalTo(2)); | ||
jkakavas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| public void testEnrollKibana() throws Exception { | ||
| KibanaEnrollmentResponse kibanaResponse = | ||
| execute(highLevelClient().security()::enrollKibana, highLevelClient().security()::enrollKibanaAsync, RequestOptions.DEFAULT); | ||
| assertThat(kibanaResponse, notNullValue()); | ||
| assertThat(kibanaResponse.getHttpCa() | ||
| , endsWith("brcNC5xq6YE7C4_06nH7F6le4kE4Uo6c9fpkl4ehOxQxndNLn462tFF-8VBA8IftJ1PPWzqGxLsCTzM6p6w8sa-XhgNYglLfkRjirc=")); | ||
| assertNotNull(kibanaResponse.getPassword()); | ||
| assertThat(kibanaResponse.getPassword().toString().length(), equalTo(14)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,15 @@ | |
| import javax.net.ssl.TrustManagerFactory; | ||
| import javax.net.ssl.X509ExtendedKeyManager; | ||
|
|
||
| import java.io.InputStream; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.security.KeyStore; | ||
| import java.security.PrivateKey; | ||
| import java.security.cert.X509Certificate; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| import static org.elasticsearch.test.TestMatchers.throwableWithMessage; | ||
| import static org.hamcrest.Matchers.containsString; | ||
|
|
@@ -52,6 +60,52 @@ public void testKeyStorePathCanBeEmptyForPkcs11() throws Exception { | |
| assertThat(ee.getCause().getMessage(), containsString("PKCS11 not found")); | ||
| } | ||
|
|
||
| public void testCreateKeyManagerFromPKCS12ContainingCA() throws Exception { | ||
| assumeFalse("Can't run in a FIPS JVM", inFipsJvm()); | ||
| final Settings settings = Settings.builder().put("path.home", createTempDir()).build(); | ||
| final Path path = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/httpCa.p12"); | ||
| final SecureString keyStorePassword = new SecureString("password".toCharArray()); | ||
| final StoreKeyConfig keyConfig = new StoreKeyConfig(path.toString(), "PKCS12", keyStorePassword, keyStorePassword, | ||
| KeyManagerFactory.getDefaultAlgorithm(), TrustManagerFactory.getDefaultAlgorithm()); | ||
| KeyStore keyStore = KeyStore.getInstance("PKCS12"); | ||
| try (InputStream in = Files.newInputStream(path)) { | ||
| keyStore.load(in, keyStorePassword.getChars()); | ||
| } | ||
| List<String> aliases = new ArrayList<>(); | ||
| for (String s : Collections.list(keyStore.aliases())) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| if (keyStore.isKeyEntry(s)) { | ||
| aliases.add(s); | ||
| } | ||
| } | ||
| assertThat(aliases.size(), equalTo(2)); | ||
| final X509ExtendedKeyManager keyManager = keyConfig.createKeyManager(TestEnvironment.newEnvironment(settings)); | ||
| for (String alias : aliases) { | ||
| PrivateKey key = keyManager.getPrivateKey(alias); | ||
| assertTrue(key == null || alias.equals("http")); | ||
| } | ||
| final String[] new_aliases = keyManager.getServerAliases("RSA", null); | ||
| final X509Certificate[] certificates = keyManager.getCertificateChain("http"); | ||
| assertThat(new_aliases.length, equalTo(1)); | ||
| assertThat(certificates.length, equalTo(2)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just making sure we haven't deleted something we didn't mean to (like it happened before the last chage)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This the certificate chain length of a single certificate chain of a single PrivateKeyEntry. ( We wouldn't have deleted one of the certificates in that chain before the last change.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think we don't need this, but won't block the PR on this |
||
| } | ||
|
|
||
| public void testCreateKeyManagerFromPKCS12ContainingCAOnly() throws Exception { | ||
| assumeFalse("Can't run in a FIPS JVM", inFipsJvm()); | ||
| final Settings settings = Settings.builder().put("path.home", createTempDir()).build(); | ||
| final String path = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/ca.p12").toString(); | ||
| final SecureString keyStorePassword = new SecureString("password".toCharArray()); | ||
| final StoreKeyConfig keyConfig = new StoreKeyConfig(path, "PKCS12", keyStorePassword, keyStorePassword, | ||
| KeyManagerFactory.getDefaultAlgorithm(), TrustManagerFactory.getDefaultAlgorithm()); | ||
| final X509ExtendedKeyManager keyManager = keyConfig.createKeyManager(TestEnvironment.newEnvironment(settings)); | ||
| final PrivateKey ca_key = keyManager.getPrivateKey("ca"); | ||
| final String[] aliases = keyManager.getServerAliases("RSA", null); | ||
| final X509Certificate[] certificates = keyManager.getCertificateChain("ca"); | ||
| assertThat(ca_key, notNullValue()); | ||
| assertThat(aliases.length, equalTo(1)); | ||
| assertThat(aliases[0], equalTo("ca")); | ||
| assertThat(certificates.length, equalTo(1)); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe quickly add one that contains a keystore with private key entries and trusted certificate entries and check the behavior there too ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean? A keystore with multiple not ca private key? Making sure nothing being deleted?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A keystore with a TrustedCertificate entry, a CA PrivateKeyEntry and a non-ca PrivateKeyEntry and make sure that we removed the CA PrivateKeyEntry only
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe that's what httpCa.p12 is in |
||
| private void tryReadPrivateKeyFromKeyStore(String type, String extension) { | ||
| final Settings settings = Settings.builder().put("path.home", createTempDir()).build(); | ||
| final String path = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode" + extension).toString(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.