Fail closed when no core dump encryption public key is found

2 files changed, 18 insertions, 22 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java
index bfc4c09cf9e..98ca77bc66f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java
@@ -180,23 +180,21 @@ public class CoredumpHandler {
         return coreEncryptionPublicKeyIdFlag.with(FetchVector.Dimension.NODE_TYPE, context.nodeType().name()).value();
     }
 
-    static OutputStream maybeWrapWithEncryption(OutputStream wrappedStream, Optional<SecretSharedKey> sharedCoreKey) {
-        return sharedCoreKey
-                .map(key -> key.makeEncryptionCipher().wrapOutputStream(wrappedStream))
-                .orElse(wrappedStream);
+    static OutputStream wrapWithEncryption(OutputStream wrappedStream, SecretSharedKey sharedCoreKey) {
+        return sharedCoreKey.makeEncryptionCipher().wrapOutputStream(wrappedStream);
     }
 
     /**
      * Compresses and, if a key is provided, encrypts core file (and deletes the uncompressed core), then moves
      * the entire core dump processing directory to {@link #doneCoredumpsPath} for archive
      */
-    private void finishProcessing(NodeAgentContext context, ContainerPath coredumpDirectory, Optional<SecretSharedKey> sharedCoreKey) {
+    private void finishProcessing(NodeAgentContext context, ContainerPath coredumpDirectory, SecretSharedKey sharedCoreKey) {
         ContainerPath coreFile = findCoredumpFileInProcessingDirectory(coredumpDirectory);
-        String extension = COMPRESSED_EXTENSION + (sharedCoreKey.isPresent() ? ENCRYPTED_EXTENSION : "");
+        String extension = COMPRESSED_EXTENSION + ENCRYPTED_EXTENSION;
         ContainerPath compressedCoreFile = coreFile.resolveSibling(coreFile.getFileName() + extension);
 
         try (ZstdCompressingInputStream zcis = new ZstdCompressingInputStream(Files.newInputStream(coreFile));
-             OutputStream fos = maybeWrapWithEncryption(Files.newOutputStream(compressedCoreFile), sharedCoreKey)) {
+             OutputStream fos = wrapWithEncryption(Files.newOutputStream(compressedCoreFile), sharedCoreKey)) {
             zcis.transferTo(fos);
         } catch (IOException e) {
             throw new UncheckedIOException(e);
@@ -287,11 +285,12 @@ public class CoredumpHandler {
         dockerImage.ifPresent(metadata::setDockerImage);
         dockerImage.flatMap(DockerImage::tag).ifPresent(metadata::setVespaVersion);
         dockerImage.ifPresent(metadata::setDockerImage);
-        Optional<SecretSharedKey> sharedCoreKey = Optional.of(corePublicKeyFlagValue(context))
+        SecretSharedKey sharedCoreKey = Optional.of(corePublicKeyFlagValue(context))
                 .filter(k -> !k.isEmpty())
                 .map(KeyId::ofString)
-                .flatMap(secretSharedKeySupplier::create);
-        sharedCoreKey.map(key -> key.sealedSharedKey().toTokenString()).ifPresent(metadata::setDecryptionToken);
+                .flatMap(secretSharedKeySupplier::create)
+                .orElseThrow(() -> new IllegalStateException("No core dump encryption key provided"));
+        metadata.setDecryptionToken(sharedCoreKey.sealedSharedKey().toTokenString());
 
         String coreDumpId = coreDumpDirectory.getFileName().toString();
         cores.report(context.hostname(), coreDumpId, metadata);
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
index 33d785eb04e..573e5b1ed20 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
@@ -196,15 +196,14 @@ public class CoredumpHandlerTest {
         });
     }
 
-    void do_process_single_coredump_test(String expectedCoreFileName, boolean encrypt) throws IOException {
+    void do_process_single_coredump_test(String expectedCoreFileName) throws IOException {
         ContainerPath coredumpDirectory = context.paths().of("/path/to/coredump/proccessing/id-123");
         Files.createDirectories(coredumpDirectory);
         Files.write(coredumpDirectory.resolve("metadata2.json"), "{\"test-metadata\":{}}".getBytes());
         Files.createFile(coredumpDirectory.resolve("dump_bash.core.431"));
         assertFolderContents(coredumpDirectory, "metadata2.json", "dump_bash.core.431");
         CoreDumpMetadata expectedMetadata = new CoreDumpMetadata();
-        if (encrypt)
-            expectedMetadata.setDecryptionToken("131Q0MMF3hBuMVnXg1WnSFexZGrcwa9ZhfHlegLNwPIN6hQJnBxq5srLf3aZbYdlRVE");
+        expectedMetadata.setDecryptionToken("131Q0MMF3hBuMVnXg1WnSFexZGrcwa9ZhfHlegLNwPIN6hQJnBxq5srLf3aZbYdlRVE");
 
         coredumpHandler.processAndReportSingleCoreDump(context, coredumpDirectory, Optional.empty());
         verify(coreCollector, never()).collect(any(), any());
@@ -215,31 +214,29 @@ public class CoredumpHandlerTest {
     }
 
     @Test
-    void process_single_coredump_test_without_encryption() throws IOException {
-        do_process_single_coredump_test("dump_bash.core.431.zst", false);
+    void processing_single_coredump_test_without_encryption_throws() throws IOException {
+        assertThrows(IllegalStateException.class, () -> do_process_single_coredump_test("dump_bash.core.431.zst"));
     }
 
     @Test
     void process_single_coredump_test_with_encryption() throws IOException {
         flagSource.withStringFlag(Flags.CORE_ENCRYPTION_PUBLIC_KEY_ID.id(), "bar-key");
         when(secretSharedKeySupplier.create(KeyId.ofString("bar-key"))).thenReturn(Optional.of(makeFixedSecretSharedKey()));
-        do_process_single_coredump_test("dump_bash.core.431.zst.enc", true);
+        do_process_single_coredump_test("dump_bash.core.431.zst.enc");
     }
 
-    // TODO fail closed instead of open
     @Test
-    void encryption_disabled_when_no_public_key_set_in_feature_flag() throws IOException {
+    void processing_throws_when_no_public_key_set_in_feature_flag() throws IOException {
         flagSource.withStringFlag(Flags.CORE_ENCRYPTION_PUBLIC_KEY_ID.id(), ""); // empty -> not set
         verify(secretSharedKeySupplier, never()).create(any());
-        do_process_single_coredump_test("dump_bash.core.431.zst", false); // No .enc suffix; not encrypted
+        assertThrows(IllegalStateException.class, () -> do_process_single_coredump_test("dump_bash.core.431.zst"));
     }
 
-    // TODO fail closed instead of open
     @Test
-    void encryption_disabled_when_no_key_returned_for_key_id_specified_by_feature_flag() throws IOException {
+    void processing_throws_when_no_key_returned_for_key_id_specified_by_feature_flag() throws IOException {
         flagSource.withStringFlag(Flags.CORE_ENCRYPTION_PUBLIC_KEY_ID.id(), "baz-key");
         when(secretSharedKeySupplier.create(KeyId.ofString("baz-key"))).thenReturn(Optional.empty());
-        do_process_single_coredump_test("dump_bash.core.431.zst", false); // No .enc suffix; not encrypted
+        assertThrows(IllegalStateException.class, () -> do_process_single_coredump_test("dump_bash.core.431.zst"));
     }
 
     @Test