From 8214f91b9edb47a848ee42f1d4d293c1e08d4fa2 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 15 Jan 2020 15:14:09 +1100 Subject: [PATCH 1/5] Encrypt generated cert with AES Replace DES with AES to align with modern encryption standards Resolves: #50843 --- .../org/elasticsearch/xpack/security/cli/CertificateTool.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java index 4ae1f313ce152..ad5cfd5e05b1d 100644 --- a/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java +++ b/x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java @@ -922,7 +922,7 @@ static Collection parseFile(Path file) throws Exception } static PEMEncryptor getEncrypter(char[] password) { - return new JcePEMEncryptorBuilder("DES-EDE3-CBC").setProvider(BC_PROV).build(password); + return new JcePEMEncryptorBuilder("AES-128-CBC").setProvider(BC_PROV).build(password); } private static T withPassword(String description, char[] password, Terminal terminal, From f5226686f389236fd41782e5c8dbb5c91cfe344f Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 15 Jan 2020 16:47:00 +1100 Subject: [PATCH 2/5] Add test to ensure AES is in use --- .../xpack/security/cli/CertificateToolTests.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java index 6845edbdc6b38..9de9d3809a873 100644 --- a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java +++ b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java @@ -21,6 +21,7 @@ import org.bouncycastle.asn1.x509.GeneralName; import org.bouncycastle.asn1.x509.GeneralNames; import org.bouncycastle.cert.X509CertificateHolder; +import org.bouncycastle.openssl.PEMDecryptorProvider; import org.bouncycastle.openssl.PEMEncryptedKeyPair; import org.bouncycastle.openssl.PEMParser; import org.bouncycastle.pkcs.PKCS10CertificationRequest; @@ -50,6 +51,7 @@ import org.hamcrest.Matchers; import org.junit.After; import org.junit.BeforeClass; +import org.mockito.Mockito; import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.TrustManagerFactory; @@ -349,6 +351,14 @@ public void testGeneratingSignedPemCertificates() throws Exception { PEMParser pemParser = new PEMParser(reader); Object parsed = pemParser.readObject(); assertThat(parsed, instanceOf(PEMEncryptedKeyPair.class)); + // Verify we are using AES encryption + final PEMDecryptorProvider pemDecryptorProvider = Mockito.mock(PEMDecryptorProvider.class); + try { + ((PEMEncryptedKeyPair) parsed).decryptKeyPair(pemDecryptorProvider); + } catch (Exception e) {} + finally { + Mockito.verify(pemDecryptorProvider).get("AES-128-CBC"); + } char[] zeroChars = new char[caInfo.password.length]; Arrays.fill(zeroChars, (char) 0); assertArrayEquals(zeroChars, caInfo.password); @@ -368,7 +378,9 @@ public void testGeneratingSignedPemCertificates() throws Exception { assertTrue(Files.exists(zipRoot.resolve(filename))); final Path cert = zipRoot.resolve(filename + "/" + filename + ".crt"); assertTrue(Files.exists(cert)); - assertTrue(Files.exists(zipRoot.resolve(filename + "/" + filename + ".key"))); + Path keyFile = zipRoot.resolve(filename + "/" + filename + ".key"); + assertTrue(Files.exists(keyFile)); + assertTrue(Files.readString(keyFile).contains("DEK-Info: AES-128-CBC")); final Path p12 = zipRoot.resolve(filename + "/" + filename + ".p12"); try (InputStream input = Files.newInputStream(cert)) { X509Certificate certificate = readX509Certificate(input); From e743710a8aa7c32925e5340f02171e72d27d871a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 15 Jan 2020 17:08:17 +1100 Subject: [PATCH 3/5] Fix key content check failure due to randomness --- .../xpack/security/cli/CertificateToolTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java index 9de9d3809a873..ec586b1c54a64 100644 --- a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java +++ b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java @@ -380,7 +380,9 @@ public void testGeneratingSignedPemCertificates() throws Exception { assertTrue(Files.exists(cert)); Path keyFile = zipRoot.resolve(filename + "/" + filename + ".key"); assertTrue(Files.exists(keyFile)); - assertTrue(Files.readString(keyFile).contains("DEK-Info: AES-128-CBC")); + if (keyPassword != null) { + assertTrue(Files.readString(keyFile).contains("DEK-Info: AES-128-CBC")); + } final Path p12 = zipRoot.resolve(filename + "/" + filename + ".p12"); try (InputStream input = Files.newInputStream(cert)) { X509Certificate certificate = readX509Certificate(input); From 340b623fab6aeea0728de570168a0817c886e67e Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 15 Jan 2020 18:16:19 +1100 Subject: [PATCH 4/5] Document the empty catch block --- .../xpack/security/cli/CertificateToolTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java index ec586b1c54a64..cf67fc62aa738 100644 --- a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java +++ b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java @@ -355,7 +355,9 @@ public void testGeneratingSignedPemCertificates() throws Exception { final PEMDecryptorProvider pemDecryptorProvider = Mockito.mock(PEMDecryptorProvider.class); try { ((PEMEncryptedKeyPair) parsed).decryptKeyPair(pemDecryptorProvider); - } catch (Exception e) {} + } catch (Exception e) { + // Catch error thrown by the empty mock, we are only interested in the argument passed in + } finally { Mockito.verify(pemDecryptorProvider).get("AES-128-CBC"); } From 34a605a40b21818e5a623ef03a4b0525844f3451 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 15 Jan 2020 19:06:48 +1100 Subject: [PATCH 5/5] Add else block to complete text content test --- .../elasticsearch/xpack/security/cli/CertificateToolTests.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java index cf67fc62aa738..21a8440a7003c 100644 --- a/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java +++ b/x-pack/plugin/security/cli/src/test/java/org/elasticsearch/xpack/security/cli/CertificateToolTests.java @@ -384,6 +384,8 @@ public void testGeneratingSignedPemCertificates() throws Exception { assertTrue(Files.exists(keyFile)); if (keyPassword != null) { assertTrue(Files.readString(keyFile).contains("DEK-Info: AES-128-CBC")); + } else { + assertFalse(Files.readString(keyFile).contains("DEK-Info:")); } final Path p12 = zipRoot.resolve(filename + "/" + filename + ".p12"); try (InputStream input = Files.newInputStream(cert)) {