Respect --pass option in certutil csr mode (#109834)

tvernum · web-flow · commit 07296d596a1d · 2024-06-18T14:42:03.000+10:00
elasticsearch-certutil csr generates a private key and a certificate signing request (CSR) file. It has always accepted the "--pass" command line option, but ignore it and always generated an unencrypted private key. This commit fixes the utility so the --pass option is respected and the private key is encrypted. Backport of: #106105
diff --git a/docs/changelog/106105.yaml b/docs/changelog/106105.yaml
@@ -0,0 +1,5 @@
+pr: 106105
+summary: Respect --pass option in certutil csr mode
+area: TLS
+type: bug
+issues: []
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
@@ -589,6 +589,29 @@ static void writePkcs12(
                 }
             });
         }
+
+        protected void writePemPrivateKey(
+            Terminal terminal,
+            OptionSet options,
+            ZipOutputStream outputStream,
+            JcaPEMWriter pemWriter,
+            String keyFileName,
+            PrivateKey privateKey
+        ) throws IOException {
+            final boolean usePassword = useOutputPassword(options);
+            final char[] outputPassword = getOutputPassword(options);
+            outputStream.putNextEntry(new ZipEntry(keyFileName));
+            if (usePassword) {
+                withPassword(keyFileName, outputPassword, terminal, true, password -> {
+                    pemWriter.writeObject(privateKey, getEncrypter(password));
+                    return null;
+                });
+            } else {
+                pemWriter.writeObject(privateKey);
+            }
+            pemWriter.flush();
+            outputStream.closeEntry();
+        }
     }
 
     static class SigningRequestCommand extends CertificateCommand {
@@ -620,9 +643,7 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
             terminal.println("");
 
             final Path output = resolveOutputPath(terminal, options, DEFAULT_CSR_ZIP);
-            final int keySize = getKeySize(options);
-            Collection<CertificateInformation> certificateInformations = getCertificateInformationList(terminal, options);
-            generateAndWriteCsrs(output, keySize, certificateInformations);
+            generateAndWriteCsrs(terminal, options, output);
 
             terminal.println("");
             terminal.println("Certificate signing requests have been written to " + output);
@@ -638,12 +659,25 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
             terminal.println("follow the SSL configuration instructions in the product guide.");
         }
 
+        // For testing
+        void generateAndWriteCsrs(Terminal terminal, OptionSet options, Path output) throws Exception {
+            final int keySize = getKeySize(options);
+            Collection<CertificateInformation> certificateInformations = getCertificateInformationList(terminal, options);
+            generateAndWriteCsrs(terminal, options, output, keySize, certificateInformations);
+        }
+
         /**
          * Generates certificate signing requests and writes them out to the specified file in zip format
          *
          * @param certInfo the details to use in the certificate signing requests
          */
-        void generateAndWriteCsrs(Path output, int keySize, Collection<CertificateInformation> certInfo) throws Exception {
+        void generateAndWriteCsrs(
+            Terminal terminal,
+            OptionSet options,
+            Path output,
+            int keySize,
+            Collection<CertificateInformation> certInfo
+        ) throws Exception {
             fullyWriteZipFile(output, (outputStream, pemWriter) -> {
                 for (CertificateInformation certificateInformation : certInfo) {
                     KeyPair keyPair = CertGenUtils.generateKeyPair(keySize);
@@ -666,10 +700,14 @@ void generateAndWriteCsrs(Path output, int keySize, Collection<CertificateInform
                     outputStream.closeEntry();
 
                     // write private key
-                    outputStream.putNextEntry(new ZipEntry(dirName + certificateInformation.name.filename + ".key"));
-                    pemWriter.writeObject(keyPair.getPrivate());
-                    pemWriter.flush();
-                    outputStream.closeEntry();
+                    super.writePemPrivateKey(
+                        terminal,
+                        options,
+                        outputStream,
+                        pemWriter,
+                        dirName + certificateInformation.name.filename + ".key",
+                        keyPair.getPrivate()
+                    );
                 }
             });
         }
@@ -805,10 +843,8 @@ void generateAndWriteSignedCertificates(
 
             final int keySize = getKeySize(options);
             final int days = getDays(options);
-            final char[] outputPassword = super.getOutputPassword(options);
             if (writeZipFile) {
                 final boolean usePem = usePemFormat(options);
-                final boolean usePassword = super.useOutputPassword(options);
                 fullyWriteZipFile(output, (outputStream, pemWriter) -> {
                     // write out the CA info first if it was generated
                     if (caInfo != null && caInfo.generated) {
@@ -838,20 +874,10 @@ void generateAndWriteSignedCertificates(
                             outputStream.closeEntry();
 
                             // write private key
-                            final String keyFileName = entryBase + ".key";
-                            outputStream.putNextEntry(new ZipEntry(keyFileName));
-                            if (usePassword) {
-                                withPassword(keyFileName, outputPassword, terminal, true, password -> {
-                                    pemWriter.writeObject(pair.key, getEncrypter(password));
-                                    return null;
-                                });
-                            } else {
-                                pemWriter.writeObject(pair.key);
-                            }
-                            pemWriter.flush();
-                            outputStream.closeEntry();
+                            writePemPrivateKey(terminal, options, outputStream, pemWriter, entryBase + ".key", pair.key);
                         } else {
                             final String fileName = entryBase + ".p12";
+                            final char[] outputPassword = super.getOutputPassword(options);
                             outputStream.putNextEntry(new ZipEntry(fileName));
                             writePkcs12(
                                 fileName,
@@ -868,6 +894,7 @@ void generateAndWriteSignedCertificates(
                 });
             } else {
                 assert certs.size() == 1;
+                final char[] outputPassword = super.getOutputPassword(options);
                 CertificateInformation certificateInformation = certs.iterator().next();
                 CertificateAndKey pair = generateCertificateAndKey(certificateInformation, caInfo, keySize, days);
                 fullyWriteFile(
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
@@ -26,7 +26,9 @@
 import org.bouncycastle.cert.X509CertificateHolder;
 import org.bouncycastle.openssl.PEMDecryptorProvider;
 import org.bouncycastle.openssl.PEMEncryptedKeyPair;
+import org.bouncycastle.openssl.PEMKeyPair;
 import org.bouncycastle.openssl.PEMParser;
+import org.bouncycastle.openssl.bc.BcPEMDecryptorProvider;
 import org.bouncycastle.pkcs.PKCS10CertificationRequest;
 import org.elasticsearch.cli.MockTerminal;
 import org.elasticsearch.cli.Terminal;
@@ -59,6 +61,7 @@
 import java.io.Reader;
 import java.net.InetAddress;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.FileSystem;
 import java.nio.file.FileSystems;
@@ -74,6 +77,7 @@
 import java.security.cert.X509Certificate;
 import java.security.interfaces.RSAKey;
 import java.time.temporal.ChronoUnit;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -85,6 +89,7 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import javax.net.ssl.KeyManagerFactory;
 import javax.net.ssl.TrustManagerFactory;
@@ -257,17 +262,35 @@ public void testParsingFileWithInvalidDetails() throws Exception {
         assertThat(terminal.getErrorOutput(), containsString("could not be converted to a valid DN"));
     }
 
-    public void testGeneratingCsr() throws Exception {
+    public void testGeneratingCsrFromInstancesFile() throws Exception {
         Path tempDir = initTempDir();
         Path outputFile = tempDir.resolve("out.zip");
+        MockTerminal terminal = new MockTerminal();
+        final List<String> args = new ArrayList<>();
+
         Path instanceFile = writeInstancesTo(tempDir.resolve("instances.yml"));
         Collection<CertificateInformation> certInfos = CertificateTool.parseFile(instanceFile);
         assertEquals(4, certInfos.size());
 
         assertFalse(Files.exists(outputFile));
         int keySize = randomFrom(1024, 2048);
 
-        new CertificateTool.SigningRequestCommand().generateAndWriteCsrs(outputFile, keySize, certInfos);
+        final boolean encrypt = randomBoolean();
+        final String password = encrypt ? randomAlphaOfLengthBetween(8, 12) : null;
+        if (encrypt) {
+            args.add("--pass");
+            if (randomBoolean()) {
+                args.add(password);
+            } else {
+                for (Object ignore : certInfos) {
+                    terminal.addSecretInput(password);
+                }
+            }
+        }
+
+        final CertificateTool.SigningRequestCommand command = new CertificateTool.SigningRequestCommand();
+        final OptionSet options = command.getParser().parse(Strings.toStringArray(args));
+        command.generateAndWriteCsrs(terminal, options, outputFile, keySize, certInfos);
         assertTrue(Files.exists(outputFile));
 
         Set<PosixFilePermission> perms = Files.getPosixFilePermissions(outputFile);
@@ -284,7 +307,6 @@ public void testGeneratingCsr() throws Exception {
             assertTrue(Files.exists(zipRoot.resolve(filename)));
             final Path csr = zipRoot.resolve(filename + "/" + filename + ".csr");
             assertTrue(Files.exists(csr));
-            assertTrue(Files.exists(zipRoot.resolve(filename + "/" + filename + ".key")));
             PKCS10CertificationRequest request = readCertificateRequest(csr);
             assertEquals(certInfo.name.x500Principal.getName(), request.getSubject().toString());
             Attribute[] extensionsReq = request.getAttributes(PKCSObjectIdentifiers.pkcs_9_at_extensionRequest);
@@ -296,7 +318,82 @@ public void testGeneratingCsr() throws Exception {
             } else {
                 assertEquals(0, extensionsReq.length);
             }
+
+            final Path keyPath = zipRoot.resolve(filename + "/" + filename + ".key");
+            assertTrue(Files.exists(keyPath));
+            PEMKeyPair key = readPrivateKey(keyPath, password);
+            assertNotNull(key);
+        }
+    }
+
+    public void testGeneratingCsrFromCommandLineParameters() throws Exception {
+        Path tempDir = initTempDir();
+        Path outputFile = tempDir.resolve("out.zip");
+        MockTerminal terminal = new MockTerminal();
+        final List<String> args = new ArrayList<>();
+
+        final int keySize = randomFrom(1024, 2048);
+        args.add("--keysize");
+        args.add(String.valueOf(keySize));
+
+        final String name = randomAlphaOfLengthBetween(4, 16);
+        args.add("--name");
+        args.add(name);
+
+        final List<String> dns = randomList(0, 4, () -> randomAlphaOfLengthBetween(4, 8) + "." + randomAlphaOfLengthBetween(2, 5));
+        dns.stream().map(s -> "--dns=" + s).forEach(args::add);
+        final List<String> ip = randomList(
+            0,
+            2,
+            () -> Stream.generate(() -> randomIntBetween(10, 250)).limit(4).map(String::valueOf).collect(Collectors.joining("."))
+        );
+        ip.stream().map(s -> "--ip=" + s).forEach(args::add);
+
+        final boolean encrypt = randomBoolean();
+        final String password = encrypt ? randomAlphaOfLengthBetween(8, 12) : null;
+        if (encrypt) {
+            args.add("--pass");
+            if (randomBoolean()) {
+                args.add(password);
+            } else {
+                terminal.addSecretInput(password);
+            }
+        }
+
+        final CertificateTool.SigningRequestCommand command = new CertificateTool.SigningRequestCommand();
+        final OptionSet options = command.getParser().parse(Strings.toStringArray(args));
+        command.generateAndWriteCsrs(terminal, options, outputFile);
+        assertTrue(Files.exists(outputFile));
+
+        Set<PosixFilePermission> perms = Files.getPosixFilePermissions(outputFile);
+        assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_READ));
+        assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_WRITE));
+        assertEquals(perms.toString(), 2, perms.size());
+
+        final Path zipRoot = getRootPathOfZip(outputFile);
+
+        assertFalse(Files.exists(zipRoot.resolve("ca")));
+        assertTrue(Files.exists(zipRoot.resolve(name)));
+        final Path csr = zipRoot.resolve(name + "/" + name + ".csr");
+        assertTrue(Files.exists(csr));
+
+        PKCS10CertificationRequest request = readCertificateRequest(csr);
+        assertEquals("CN=" + name, request.getSubject().toString());
+
+        Attribute[] extensionsReq = request.getAttributes(PKCSObjectIdentifiers.pkcs_9_at_extensionRequest);
+        if (dns.size() > 0 || ip.size() > 0) {
+            assertEquals(1, extensionsReq.length);
+            Extensions extensions = Extensions.getInstance(extensionsReq[0].getAttributeValues()[0]);
+            GeneralNames subjAltNames = GeneralNames.fromExtensions(extensions, Extension.subjectAlternativeName);
+            assertSubjAltNames(subjAltNames, ip, dns);
+        } else {
+            assertEquals(0, extensionsReq.length);
         }
+
+        final Path keyPath = zipRoot.resolve(name + "/" + name + ".key");
+        assertTrue(Files.exists(keyPath));
+        PEMKeyPair key = readPrivateKey(keyPath, password);
+        assertNotNull(key);
     }
 
     public void testGeneratingSignedPemCertificates() throws Exception {
@@ -968,19 +1065,6 @@ private int getDurationInDays(X509Certificate cert) {
         return (int) ChronoUnit.DAYS.between(cert.getNotBefore().toInstant(), cert.getNotAfter().toInstant());
     }
 
-    private void assertSubjAltNames(Certificate certificate, String ip, String dns) throws Exception {
-        final X509CertificateHolder holder = new X509CertificateHolder(certificate.getEncoded());
-        final GeneralNames names = GeneralNames.fromExtensions(holder.getExtensions(), Extension.subjectAlternativeName);
-        final CertificateInformation certInfo = new CertificateInformation(
-            "n",
-            "n",
-            Collections.singletonList(ip),
-            Collections.singletonList(dns),
-            Collections.emptyList()
-        );
-        assertSubjAltNames(names, certInfo);
-    }
-
     /**
      * Checks whether there are keys in {@code keyStore} that are trusted by {@code trustStore}.
      */
@@ -1014,13 +1098,39 @@ private PKCS10CertificationRequest readCertificateRequest(Path path) throws Exce
         }
     }
 
+    private PEMKeyPair readPrivateKey(Path path, String password) throws Exception {
+        try (Reader reader = Files.newBufferedReader(path); PEMParser pemParser = new PEMParser(reader)) {
+            Object object = pemParser.readObject();
+            if (password == null) {
+                assertThat(object, instanceOf(PEMKeyPair.class));
+                return (PEMKeyPair) object;
+            } else {
+                assertThat(object, instanceOf(PEMEncryptedKeyPair.class));
+                final PEMEncryptedKeyPair encryptedKeyPair = (PEMEncryptedKeyPair) object;
+                assertThat(encryptedKeyPair.getDekAlgName(), is("AES-128-CBC"));
+                return encryptedKeyPair.decryptKeyPair(new BcPEMDecryptorProvider(password.toCharArray()));
+            }
+        }
+    }
+
     private X509Certificate readX509Certificate(InputStream input) throws Exception {
         List<Certificate> list = CertParsingUtils.readCertificates(input);
         assertEquals(1, list.size());
         assertThat(list.get(0), instanceOf(X509Certificate.class));
         return (X509Certificate) list.get(0);
     }
 
+    private void assertSubjAltNames(Certificate certificate, String ip, String dns) throws Exception {
+        final X509CertificateHolder holder = new X509CertificateHolder(certificate.getEncoded());
+        final GeneralNames names = GeneralNames.fromExtensions(holder.getExtensions(), Extension.subjectAlternativeName);
+        assertSubjAltNames(names, Collections.singletonList(ip), Collections.singletonList(dns));
+    }
+
+    private void assertSubjAltNames(GeneralNames generalNames, List<String> ip, List<String> dns) throws Exception {
+        final CertificateInformation certInfo = new CertificateInformation("n", "n", ip, dns, Collections.emptyList());
+        assertSubjAltNames(generalNames, certInfo);
+    }
+
     private void assertSubjAltNames(GeneralNames subjAltNames, CertificateInformation certInfo) throws Exception {
         final int expectedCount = certInfo.ipAddresses.size() + certInfo.dnsNames.size() + certInfo.commonNames.size();
         assertEquals(expectedCount, subjAltNames.getNames().length);
@@ -1102,6 +1212,11 @@ private static Path resolvePath(String path) {
         return PathUtils.get(path).toAbsolutePath();
     }
 
+    private static Path getRootPathOfZip(Path pemZip) throws IOException, URISyntaxException {
+        FileSystem zipFS = FileSystems.newFileSystem(new URI("jar:" + pemZip.toUri()), Collections.emptyMap());
+        return zipFS.getPath("/");
+    }
+
     private String generateCA(Path caFile, MockTerminal terminal, Environment env) throws Exception {
         final int caKeySize = randomIntBetween(4, 8) * 512;
         final int days = randomIntBetween(7, 1500);
