diff --git a/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateCreator.java b/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateCreator.java index 8b8b6e8..add5193 100644 --- a/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateCreator.java +++ b/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateCreator.java @@ -1,6 +1,8 @@ package de.mlessmann.certassist.openssl; import static de.mlessmann.certassist.Constants.CERTASSIST_TMP_PREFIX; +import static java.util.concurrent.TimeUnit.*; +import static org.slf4j.LoggerFactory.getLogger; import de.mlessmann.certassist.DeleteRecursiveFileVisitor; import de.mlessmann.certassist.ExecutableResolver; @@ -15,7 +17,9 @@ import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.List; import java.util.Optional; +import java.util.UUID; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Matcher; @@ -24,10 +28,12 @@ import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; import org.springframework.lang.NonNull; import org.springframework.stereotype.Service; import org.springframework.util.CollectionUtils; import org.zeroturnaround.exec.ProcessExecutor; +import org.zeroturnaround.exec.ProcessResult; import org.zeroturnaround.exec.StartedProcess; import org.zeroturnaround.exec.stream.slf4j.Slf4jStream; @@ -36,6 +42,7 @@ import org.zeroturnaround.exec.stream.slf4j.Slf4jStream; @Slf4j public class OpenSSLCertificateCreator { + private static final Logger openSSLLogger = getLogger("OpenSSL-Logger"); public static final String OPENSSL_CERT_SUBJECT_TEMPLATE = "/C=ISO-COUNTRY/ST=STATE/L=LOCALITY/O=ORGANIZATION/CN=COMMON-NAME"; private static final String CSR_EXT_TEMPLATE = @@ -50,6 +57,8 @@ public class OpenSSLCertificateCreator { private static final Pattern FINGERPRINT_EXTRACTOR = Pattern.compile( "^(?[0-9a-zA-Z]+) (?i)Fingerprint(?-i)=(?[a-z:A-Z0-9]+)" ); + private static final String OSSL_ENV_KEY_PW = "KEY_PASS"; + private static final String OSSL_ARG_KEY_PW = "env:" + OSSL_ENV_KEY_PW; private final AtomicBoolean versionLogged = new AtomicBoolean(false); private final ExecutableResolver executableResolver; @@ -74,9 +83,27 @@ public class OpenSSLCertificateCreator { return certSubject; } + private static void killIfActive(StartedProcess process) { + if (process == null) { + return; + } + Process sysProc = process.getProcess(); + if (!sysProc.isAlive()) { + return; + } + + try { + log.debug("Process is still alive. Asking politely for it to destroy itself and waiting on exit for 2s"); + sysProc.destroy(); + sysProc.waitFor(2_000, MILLISECONDS); + } catch (InterruptedException e) { + log.debug("Interrupted while waiting for process to terminate. Registering forceful termination onExit", e); + Runtime.getRuntime().addShutdownHook(new Thread(sysProc::destroyForcibly)); + } + } + @NonNull - public OpenSSLCertificateResult createCertificate(CertificateRequest request) - throws CommandLineOperationException, InterruptedException { + public OpenSSLCertificateResult createCertificate(CertificateRequest request) throws CommandLineOperationException { Path tmpDir; try { tmpDir = Files.createTempDirectory(CERTASSIST_TMP_PREFIX); @@ -84,28 +111,28 @@ public class OpenSSLCertificateCreator { throw new CommandLineOperationException("Could not create temporary directory for certificate creation", e); } - String certPassword = passwordProvider.generateNewPassword(); - Path keyFile = createKeyfile(request, tmpDir.resolve("certificate.key"), certPassword); + String keyPassphrase = passwordProvider.generateNewPassword(); + Path keyFile = createKeyfile(request, tmpDir.resolve("certificate.key"), keyPassphrase); if ( request.getType() == RequestType.ROOT_AUTHORITY || request.getType() == RequestType.STANDALONE_CERTIFICATE ) { - Path certificate = createCertificate(request, keyFile, tmpDir.resolve("certificate.crt"), certPassword); + Path certificate = createCertificate(request, keyFile, tmpDir.resolve("certificate.crt"), keyPassphrase); String fingerprint = getCertificateFingerprint(certificate); - passwordProvider.setPasswordFor(fingerprint, certPassword); + passwordProvider.setPasswordFor(fingerprint, keyPassphrase); return new OpenSSLCertificateResult(tmpDir, certificate, keyFile, certificate, fingerprint); } try (var certAuthority = certificateProvider.requestCertificateUsage(request.getTrustingAuthority())) { - Path unsignedCert = createSigningRequest(request, keyFile, tmpDir.resolve("child.csr"), certPassword); + Path signingRequest = createSigningRequest(request, keyFile, tmpDir.resolve("child.csr"), keyPassphrase); Path signedCert = signCertificate( request, certAuthority.certificatePath(), certAuthority.certificateKeyPath(), - unsignedCert, - certPassword + passwordProvider.getPasswordFor(certAuthority.fingerprint()), + signingRequest ); String fingerprint = getCertificateFingerprint(signedCert); - passwordProvider.setPasswordFor(fingerprint, certPassword); + passwordProvider.setPasswordFor(fingerprint, keyPassphrase); Path fullchain = tmpDir.resolve("fullchain.pem"); try { @@ -123,48 +150,53 @@ public class OpenSSLCertificateCreator { } private Path createKeyfile(CertificateRequest request, Path outFile, String filePassword) - throws CommandLineOperationException, InterruptedException { + throws CommandLineOperationException { Path keyFile = outFile.toAbsolutePath(); - log.atDebug().log("Writing new certificate key to {}", keyFile); + log.debug("Writing new certificate key to {}", keyFile); + StartedProcess keygenProc = null; try { - StartedProcess keygenProc = new ProcessExecutor() + keygenProc = + new ProcessExecutor() .command( resolveOpenSSL(), "genrsa", "-out", keyFile.toString(), + "-aes256", "-passout", - "env:KEY_PASS", + OSSL_ARG_KEY_PW, Integer.toString(request.getRequestedKeyLength()) ) - .environment("KEY_PASS", filePassword) - .redirectOutput(Slf4jStream.ofCaller().asDebug()) - .redirectError(Slf4jStream.ofCaller().asError()) + .environment(OSSL_ENV_KEY_PW, filePassword) + .redirectOutput(Slf4jStream.of(openSSLLogger).asDebug()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); - keygenProc.getFuture().get(); - } catch (IOException e) { + keygenProc.getFuture().get(3, MINUTES); + } catch (IOException | ExecutionException | InterruptedException | TimeoutException e) { throw new CommandLineOperationException("Failure running OpenSSL keygen command.", e); - } catch (ExecutionException e) { - throw new RuntimeException(e); + } finally { + killIfActive(keygenProc); } return keyFile; } - private Path createCertificate(CertificateRequest request, Path keyFile, Path outFile, String certPassword) - throws CommandLineOperationException, InterruptedException { - log.atDebug().log("Writing new certificate file {}", outFile); + private Path createCertificate(CertificateRequest request, Path keyFile, Path outFile, String keyPassphrase) + throws CommandLineOperationException { + log.debug("Writing new certificate file {}", outFile); String certSubject = buildSubjectArg(request); + StartedProcess certGenProc = null; try { - StartedProcess certGenProc = new ProcessExecutor() + certGenProc = + new ProcessExecutor() .command( resolveOpenSSL(), "req", "-x509", "-new", "-passin", - "env:KEY_PASS", + OSSL_ARG_KEY_PW, "-key", keyFile.toString(), "-sha256", @@ -172,58 +204,56 @@ public class OpenSSLCertificateCreator { Integer.toString(request.getRequestedValidityDays()), "-out", outFile.toString(), - "-passout", - "env:KEY_PASS", "-utf8", "-subj", certSubject ) - .environment("KEY_PASS", certPassword) - .redirectOutput(Slf4jStream.ofCaller().asDebug()) - .redirectError(Slf4jStream.ofCaller().asError()) + .environment(OSSL_ENV_KEY_PW, keyPassphrase) + .redirectOutput(Slf4jStream.of(openSSLLogger).asDebug()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); - certGenProc.getFuture().get(); - } catch (IOException e) { + certGenProc.getFuture().get(30, SECONDS); + } catch (IOException | ExecutionException | InterruptedException | TimeoutException e) { throw new CommandLineOperationException("Failure running OpenSSL req command.", e); - } catch (ExecutionException e) { - throw new RuntimeException(e); + } finally { + killIfActive(certGenProc); } return outFile; } private Path createSigningRequest(CertificateRequest request, Path keyFile, Path outFile, String certPassword) - throws CommandLineOperationException, InterruptedException { + throws CommandLineOperationException { log.atDebug().log("Writing new certificate signing request file {}", outFile); String certSubject = buildSubjectArg(request); + StartedProcess certGenProc = null; try { - StartedProcess certGenProc = new ProcessExecutor() + certGenProc = + new ProcessExecutor() .command( resolveOpenSSL(), "req", "-new", "-passin", - "env:KEY_PASS", + OSSL_ARG_KEY_PW, "-key", keyFile.toString(), "-sha256", "-out", outFile.toString(), - "-passout", - "env:KEY_PASS", "-utf8", "-subj", certSubject ) - .environment("KEY_PASS", certPassword) - .redirectOutput(Slf4jStream.ofCaller().asDebug()) - .redirectError(Slf4jStream.ofCaller().asError()) + .environment(OSSL_ENV_KEY_PW, certPassword) + .redirectOutput(Slf4jStream.of(openSSLLogger).asDebug()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); - certGenProc.getFuture().get(); - } catch (IOException e) { + certGenProc.getFuture().get(30, SECONDS); + } catch (IOException | ExecutionException | InterruptedException | TimeoutException e) { throw new CommandLineOperationException("Failure running OpenSSL req command.", e); - } catch (ExecutionException e) { - throw new RuntimeException(e); + } finally { + killIfActive(certGenProc); } return outFile; } @@ -242,6 +272,7 @@ public class OpenSSLCertificateCreator { } Path tmpDir = null; + StartedProcess verifyCommand = null; try { Path tempTrustedBundle; if (trustedCAs.size() == 1) { @@ -260,16 +291,18 @@ public class OpenSSLCertificateCreator { } } - StartedProcess verifyCommand = new ProcessExecutor() + verifyCommand = + new ProcessExecutor() .command(resolveOpenSSL(), "verify", "-CAfile", tempTrustedBundle.toString(), fullChainFile.toString()) - .redirectOutput(Slf4jStream.ofCaller().asError()) - .redirectError(Slf4jStream.ofCaller().asError()) + .redirectOutput(Slf4jStream.of(openSSLLogger).asError()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); - var verifyResult = verifyCommand.getFuture().get(); + var verifyResult = verifyCommand.getFuture().get(30, SECONDS); return verifyResult.getExitValue() == 0; - } catch (IOException | InterruptedException | ExecutionException e) { + } catch (IOException | InterruptedException | ExecutionException | TimeoutException e) { throw new RuntimeException(e); } finally { + killIfActive(verifyCommand); if (tmpDir != null) { try { Files.walkFileTree(tmpDir, new DeleteRecursiveFileVisitor()); @@ -281,15 +314,62 @@ public class OpenSSLCertificateCreator { } } + /** + * Checks whether the provided key file is encrypted using a passphrase + */ + public boolean isKeyEncrypted(@NonNull Path keyFile) throws CommandLineOperationException { + // If the key is not encrypted, any passphrase will work -> so generate a random one to check. + String passphrase = UUID.randomUUID().toString(); + boolean firstPass = verifyKeyPassphrase(keyFile, passphrase); + if (firstPass) { + // Try with another random passphrase in case we randomly got lucky guessing the passphrase the first time. + passphrase = UUID.randomUUID().toString(); + return !verifyKeyPassphrase(keyFile, passphrase); + } + return true; + } + + /** + * Verifies a passphrase against a provided key. + * @implNote Due to the implementation of the OpenSSL cli, any password will be valid for unencrypted keys. (Check with {@link #isKeyEncrypted(Path).) + */ + public boolean verifyKeyPassphrase(@NonNull Path keyFile, @NonNull String passphrase) + throws CommandLineOperationException { + StartedProcess verifyCommand = null; + try { + verifyCommand = + new ProcessExecutor() + .command( + resolveOpenSSL(), + "rsa", + "-check", + "-in", + keyFile.toString(), + "-passin", + "pass:" + passphrase, + "-noout" + ) + .redirectOutput(Slf4jStream.of(openSSLLogger).asError()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) + .start(); + var verifyResult = verifyCommand.getFuture().get(30, SECONDS); + return verifyResult.getExitValue() == 0; + } catch (IOException | InterruptedException | ExecutionException | TimeoutException e) { + throw new CommandLineOperationException("Failed to verify key encryption", e); + } finally { + killIfActive(verifyCommand); + } + } + private Path signCertificate( CertificateRequest request, Path caCert, Path caKey, - Path csrFile, - String certPassword - ) throws CommandLineOperationException, InterruptedException { + String caKeyPassphrase, + Path csrFile + ) throws CommandLineOperationException { Path outFile = csrFile.resolveSibling(csrFile.getFileName().toString().replace(".csr", ".crt")); - log.atDebug().log("Writing new signed certificate file {}", outFile); + log.debug("Writing new signed certificate file {}", outFile); Path extFile = csrFile.resolveSibling(csrFile.getFileName().toString().replace(".csr", ".ext")); try { @@ -322,8 +402,10 @@ public class OpenSSLCertificateCreator { throw new RuntimeException(e); } + StartedProcess certGenProc = null; try { - StartedProcess certGenProc = new ProcessExecutor() + certGenProc = + new ProcessExecutor() .command( resolveOpenSSL(), "x509", @@ -337,32 +419,42 @@ public class OpenSSLCertificateCreator { "-CAkey", caKey.toString(), "-CAcreateserial", + "-passin", + OSSL_ARG_KEY_PW, "-out", outFile.toString(), "-extfile", extFile.toString() ) - .redirectOutput(Slf4jStream.ofCaller().asDebug()) - .redirectError(Slf4jStream.ofCaller().asError()) + .environment(OSSL_ENV_KEY_PW, caKeyPassphrase) + .redirectOutput(Slf4jStream.of(openSSLLogger).asDebug()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); - certGenProc.getFuture().get(); - } catch (IOException e) { + ProcessResult result = certGenProc.getFuture().get(30, SECONDS); + // Check exit code + if (result.getExitValue() != 0) { + throw new CommandLineOperationException( + "Failed to sign certificate. Exit code: " + result.getExitValue() + ); + } + } catch (IOException | TimeoutException | ExecutionException | InterruptedException e) { throw new CommandLineOperationException("Failure running OpenSSL x509 command.", e); - } catch (ExecutionException e) { - throw new RuntimeException(e); + } finally { + killIfActive(certGenProc); } return outFile; } - public String getCertificateFingerprint(Path certificate) - throws CommandLineOperationException, InterruptedException { + public String getCertificateFingerprint(Path certificate) throws CommandLineOperationException { + StartedProcess fingerprintProc = null; try { - StartedProcess fingerprintProc = new ProcessExecutor() + fingerprintProc = + new ProcessExecutor() .command(resolveOpenSSL(), "x509", "-in", certificate.toString(), "-noout", "-fingerprint") .readOutput(true) - .redirectError(Slf4jStream.ofCaller().asError()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); - var fingerprintResult = fingerprintProc.getFuture().get(); + var fingerprintResult = fingerprintProc.getFuture().get(30, SECONDS); String output = fingerprintResult.getOutput().getUTF8(); if (fingerprintResult.getExitValue() != 0) { @@ -389,14 +481,18 @@ public class OpenSSLCertificateCreator { ); } return "%s;%s".formatted(algorithm, fingerprint); - } catch (IOException | ExecutionException e) { + } catch (IOException | ExecutionException | TimeoutException | InterruptedException e) { throw new RuntimeException(e); + } finally { + killIfActive(fingerprintProc); } } - public CertificateRequest getCertificateInfo(Path path) throws CommandLineOperationException, InterruptedException { + public CertificateRequest getCertificateInfo(Path path) throws CommandLineOperationException { + StartedProcess infoProc = null; try { - StartedProcess infoProc = new ProcessExecutor() + infoProc = + new ProcessExecutor() .command( resolveOpenSSL(), "x509", @@ -421,9 +517,9 @@ public class OpenSSLCertificateCreator { "lname" ) .readOutput(true) - .redirectError(Slf4jStream.ofCaller().asError()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); - var infoResult = infoProc.getFuture().get(); + var infoResult = infoProc.getFuture().get(30, SECONDS); String output = infoResult.getOutput().getUTF8(); if (infoResult.getExitValue() != 0) { log.debug("Certificate info command output:\n{}", output); @@ -432,7 +528,7 @@ public class OpenSSLCertificateCreator { ); } return getCertificateInfo(output.lines().toArray(String[]::new)); - } catch (IOException | ExecutionException e) { + } catch (IOException | ExecutionException | InterruptedException | TimeoutException e) { throw new RuntimeException(e); } } @@ -445,7 +541,7 @@ public class OpenSSLCertificateCreator { StartedProcess versionProc = new ProcessExecutor() .command(path, "version") .readOutput(true) - .redirectError(Slf4jStream.ofCaller().asError()) + .redirectError(Slf4jStream.of(openSSLLogger).asError()) .start(); var versionResult = versionProc.getFuture().get(); if (versionResult.getExitValue() != 0) { diff --git a/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateResult.java b/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateResult.java index bbca3f5..17d1721 100644 --- a/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateResult.java +++ b/src/main/java/de/mlessmann/certassist/openssl/OpenSSLCertificateResult.java @@ -41,12 +41,19 @@ public class OpenSSLCertificateResult implements CertificateUsage { @Override public void close() { + cleanupDir(true); + } + + private void cleanupDir(boolean retryOnExit) { try { log.info("Cleaning up temporary output directory {}", tmpDir); Files.walkFileTree(tmpDir, Set.of(), Integer.MAX_VALUE, new DeleteRecursiveFileVisitor()); Files.deleteIfExists(tmpDir); } catch (IOException e) { - log.error("Failed to clean up temporary output directory {}!", tmpDir, e); + log.error("Failed to clean up temporary output directory {}! (retry={})", tmpDir, retryOnExit, e); + if (retryOnExit) { + Runtime.getRuntime().addShutdownHook(new Thread(() -> cleanupDir(false))); + } } } } diff --git a/src/main/java/de/mlessmann/certassist/service/CertificateCreationService.java b/src/main/java/de/mlessmann/certassist/service/CertificateCreationService.java index bc1265f..429efe3 100644 --- a/src/main/java/de/mlessmann/certassist/service/CertificateCreationService.java +++ b/src/main/java/de/mlessmann/certassist/service/CertificateCreationService.java @@ -32,9 +32,6 @@ public class CertificateCreationService { ) { certificate.setPrivateKey(Files.readAllBytes(certificateCreatorResult.certificateKeyPath())); certificate.setCert(Files.readAllBytes(certificateCreatorResult.certificatePath())); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new IllegalStateException("Interrupted exception", e); } catch (CommandLineOperationException | IOException e) { throw new IllegalStateException("Failed to create certificate!", e); } @@ -81,8 +78,6 @@ public class CertificateCreationService { return certificateRepository.save(entity); } catch (CommandLineOperationException | IOException e) { throw new RuntimeException("Unable to import certificate", e); - } catch (InterruptedException e) { - throw new RuntimeException(e); } } diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 115f1fb..cacc878 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -8,11 +8,11 @@ password=admin spring.jpa.database-platform=org.hibernate.community.dialect.SQLiteDialect #TODO: Use flyway for db setup hibernate.hbm2ddl.auto=update -hibernate.show_sql=true -hibernate.format_sql=true +#hibernate.show_sql=true +#hibernate.format_sql=true # Logging logging.level.root=INFO logging.level.de.mlessmann.certassist=DEBUG -logging.level.org.sqlite=TRACE -logging.level.org.hibernate=DEBUG \ No newline at end of file +#logging.level.org.sqlite=TRACE +#logging.level.org.hibernate=DEBUG \ No newline at end of file diff --git a/src/test/java/de/mlessmann/certassist/TestOpenSSLCertificateCreator.java b/src/test/java/de/mlessmann/certassist/TestOpenSSLCertificateCreator.java index bf5de81..c39a807 100644 --- a/src/test/java/de/mlessmann/certassist/TestOpenSSLCertificateCreator.java +++ b/src/test/java/de/mlessmann/certassist/TestOpenSSLCertificateCreator.java @@ -9,12 +9,23 @@ import java.nio.file.Path; import java.util.Objects; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +@SpringBootTest class TestOpenSSLCertificateCreator { public static final String TEST_CERT_PASSPHRASE = "ABC-123"; public static final Path TEST_CERT_PATH = Path.of("src/test/resources/openssl"); - private CertificatePasswordProvider passwordProvider; + public static final String ERR_NOT_ENCRYPTED = "Private key not encrypted"; + public static final String ERR_VERIFY_FAILED = "Certificate verification failed"; + + @Autowired + OpenSSLCertificateCreator openSSLCertificateCreator; + + @MockBean + CertificatePasswordProvider passwordProvider; @BeforeEach void setUp() { @@ -50,8 +61,11 @@ class TestOpenSSLCertificateCreator { try (var cert = certificateCreator.createCertificate(certRequest)) { assertThat(certificateCreator.verifyCertificate(cert.certificatePath(), cert.certificatePath())) - .isEqualTo(true); - System.out.println("Certificate created: " + cert); + .withFailMessage(ERR_VERIFY_FAILED) + .isTrue(); + assertThat(certificateCreator.isKeyEncrypted(cert.certificateKeyPath())) + .withFailMessage(ERR_NOT_ENCRYPTED) + .isTrue(); CertificateRequest childRequest = CertificateRequest .builder() @@ -73,12 +87,15 @@ class TestOpenSSLCertificateCreator { doNothing().when(spiedCert).close(); when(certificateProvider.requestCertificateUsage(cert.fingerprint())).thenReturn(spiedCert); try (var childCert = certificateCreator.createCertificate(childRequest)) { - System.out.println("Child certificate created: " + childCert); Path fullchain = childCert.fullchainPath(); assertThat( - certificateCreator.verifyCertificate(cert.certificatePath(), Objects.requireNonNull(fullchain)) + certificateCreator.verifyCertificate(Objects.requireNonNull(fullchain), cert.certificatePath()) ) - .isEqualTo(true); + .withFailMessage(ERR_VERIFY_FAILED) + .isTrue(); + assertThat(certificateCreator.isKeyEncrypted(childCert.certificateKeyPath())) + .withFailMessage(ERR_NOT_ENCRYPTED) + .isTrue(); } } }