Merge pull request #26823 from vespa-engine/hmusum/fail-deployment-id-certificate-file-is-missing

Fail deployment if no certificate file exists in application package

4 files changed, 60 insertions, 27 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java
index 4a8bc3cd09a..3ebaebf680a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackage.java
@@ -1,15 +1,10 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.application.pkg;
 
-import com.google.common.hash.Funnel;
-import com.google.common.hash.HashFunction;
 import com.google.common.hash.Hasher;
 import com.google.common.hash.Hashing;
 import com.google.common.hash.HashingOutputStream;
 import com.yahoo.component.Version;
-import com.yahoo.vespa.archive.ArchiveStreamReader;
-import com.yahoo.vespa.archive.ArchiveStreamReader.ArchiveFile;
-import com.yahoo.vespa.archive.ArchiveStreamReader.Options;
 import com.yahoo.config.application.FileSystemWrapper;
 import com.yahoo.config.application.FileSystemWrapper.FileWrapper;
 import com.yahoo.config.application.XmlPreProcessor;
@@ -23,10 +18,12 @@ import com.yahoo.config.provision.Tags;
 import com.yahoo.slime.Inspector;
 import com.yahoo.slime.Slime;
 import com.yahoo.slime.SlimeUtils;
+import com.yahoo.vespa.archive.ArchiveStreamReader;
+import com.yahoo.vespa.archive.ArchiveStreamReader.ArchiveFile;
+import com.yahoo.vespa.archive.ArchiveStreamReader.Options;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.deployment.ZipBuilder;
 import com.yahoo.yolean.Exceptions;
-
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
@@ -44,10 +41,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.Random;
 import java.util.Set;
-import java.util.SortedMap;
-import java.util.TreeMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.function.Function;
 import java.util.function.Predicate;
@@ -66,7 +60,8 @@ import static java.util.stream.Collectors.toMap;
  */
 public class ApplicationPackage {
 
-    static final String trustedCertificatesFile = "security/clients.pem";
+    static final String trustedCertificatesDir = "security/";
+    static final String trustedCertificatesFile = trustedCertificatesDir + "clients.pem";
     static final String buildMetaFile = "build-meta.json";
     static final String deploymentFile = "deployment.xml";
     static final String validationOverridesFile = "validation-overrides.xml";
@@ -90,7 +85,7 @@ public class ApplicationPackage {
      * it must not be further changed by the caller.
      */
     public ApplicationPackage(byte[] zippedContent) {
-        this(zippedContent, false);
+        this(zippedContent, false, false);
     }
 
     /**
@@ -99,9 +94,9 @@ public class ApplicationPackage {
      * it must not be further changed by the caller.
      * If 'requireFiles' is true, files needed by deployment orchestration must be present.
      */
-    public ApplicationPackage(byte[] zippedContent, boolean requireFiles) {
+    public ApplicationPackage(byte[] zippedContent, boolean requireFiles, boolean checkCertificateFile) {
         this.zippedContent = Objects.requireNonNull(zippedContent, "The application package content cannot be null");
-        this.files = new ZipArchiveCache(zippedContent, prePopulated);
+        this.files = new ZipArchiveCache(zippedContent, prePopulated, checkCertificateFile);
 
         Optional<DeploymentSpec> deploymentSpec = files.get(deploymentFile).map(bytes -> new String(bytes, UTF_8)).map(DeploymentSpec::fromXml);
         if (requireFiles && deploymentSpec.isEmpty())
@@ -253,10 +248,12 @@ public class ApplicationPackage {
         private final byte[] zip;
         private final Map<Path, Optional<byte[]>> cache;
 
-        public ZipArchiveCache(byte[] zip, Collection<String> prePopulated) {
+        public ZipArchiveCache(byte[] zip, Collection<String> prePopulated, boolean checkCertificateFile) {
             this.zip = zip;
             this.cache = new ConcurrentSkipListMap<>();
             this.cache.putAll(read(prePopulated));
+            if (checkCertificateFile)
+                verifyThatTrustedCertificateExists();
         }
 
         public Optional<byte[]> get(String path) {
@@ -274,17 +271,26 @@ public class ApplicationPackage {
         }
 
         private Map<Path, Optional<byte[]>> read(Collection<String> names) {
-            var entries = ZipEntries.from(zip,
-                                          names::contains,
-                                          maxSize,
-                                          true)
-                                    .asList().stream()
-                                    .collect(toMap(entry -> Paths.get(entry.name()).normalize(),
-                                                   ZipEntries.ZipEntryWithContent::content));
+            var entries = findZipFileEntries(names::contains);
             names.stream().map(Paths::get).forEach(path -> entries.putIfAbsent(path.normalize(), Optional.empty()));
             return entries;
         }
 
+
+        private void verifyThatTrustedCertificateExists() {
+            // Any name is valid for certificate files
+            var entries = findZipFileEntries((entry) -> entry.contains(trustedCertificatesDir) && entry.endsWith(".pem"));
+            if (entries.size() == 0)
+                throw new IllegalArgumentException("No client certificate found in " + trustedCertificatesDir + " in application package" +
+                                                           ", see https://cloud.vespa.ai/en/security/guide");
+        }
+
+        private Map<Path, Optional<byte[]>> findZipFileEntries(Predicate<String> names) {
+            return ZipEntries.from(zip, names, maxSize, true)
+                             .asList().stream()
+                             .collect(toMap(entry -> Paths.get(entry.name()).normalize(),
+                                            ZipEntries.ZipEntryWithContent::content));
+        }
     }
 
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
index af09b9dc761..9224c53136d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
@@ -52,6 +52,8 @@ import com.yahoo.slime.SlimeUtils;
 import com.yahoo.text.Text;
 import com.yahoo.vespa.athenz.api.OAuthCredentials;
 import com.yahoo.vespa.athenz.client.zms.ZmsClientException;
+import com.yahoo.vespa.flags.BooleanFlag;
+import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.Instance;
@@ -131,7 +133,6 @@ import com.yahoo.vespa.hosted.controller.tenant.TenantInfo;
 import com.yahoo.vespa.hosted.controller.versions.VersionStatus;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
 import com.yahoo.yolean.Exceptions;
-
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -166,6 +167,7 @@ import java.util.stream.Stream;
 
 import static com.yahoo.jdisc.Response.Status.BAD_REQUEST;
 import static com.yahoo.jdisc.Response.Status.CONFLICT;
+import static com.yahoo.vespa.flags.FetchVector.Dimension.APPLICATION_ID;
 import static com.yahoo.yolean.Exceptions.uncheck;
 import static java.util.Comparator.comparingInt;
 import static java.util.Map.Entry.comparingByKey;
@@ -187,6 +189,7 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler {
     private final Controller controller;
     private final AccessControlRequests accessControlRequests;
     private final TestConfigSerializer testConfigSerializer;
+    private final BooleanFlag failDeploymentOnMissingCertificateFile;
 
     @Inject
     public ApplicationApiHandler(ThreadedHttpRequestHandler.Context parentCtx,
@@ -196,6 +199,7 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler {
         this.controller = controller;
         this.accessControlRequests = accessControlRequests;
         this.testConfigSerializer = new TestConfigSerializer(controller.system());
+        this.failDeploymentOnMissingCertificateFile = Flags.FAIL_DEPLOYMENT_ON_MISSING_CERTIFICATE_FILE.bindTo(controller.flagSource());
     }
 
     @Override
@@ -3066,7 +3070,12 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler {
                 throw new IllegalArgumentException("Source URL must include scheme and host");
         });
 
-        ApplicationPackage applicationPackage = new ApplicationPackage(dataParts.get(EnvironmentResource.APPLICATION_ZIP), true);
+        ApplicationPackage applicationPackage =
+                new ApplicationPackage(dataParts.get(EnvironmentResource.APPLICATION_ZIP),
+                                       true,
+                                       failDeploymentOnMissingCertificateFile
+                                               .with(APPLICATION_ID, ApplicationId.from(tenant, application, "default").serializedForm())
+                                               .value());
 
         byte[] testPackage = dataParts.getOrDefault(EnvironmentResource.APPLICATION_TEST_ZIP, new byte[0]);
         Submission submission = new Submission(applicationPackage, testPackage, sourceUrl, sourceRevision, authorEmail, description, risk);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java
index 7f578d3017e..e915a204e4b 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageTest.java
@@ -5,7 +5,6 @@ import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.io.LazyInputStream;
 import org.junit.jupiter.api.Test;
-
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -97,7 +96,7 @@ public class ApplicationPackageTest {
                             "jdisc.xml", jdiscXml,
                             "content/content.xml", contentXml,
                             "content/nodes.xml", nodesXml),
-                     unzip(new ApplicationPackage(zip, false).metaDataZip()));
+                     unzip(new ApplicationPackage(zip).metaDataZip()));
     }
 
     @Test
@@ -105,7 +104,7 @@ public class ApplicationPackageTest {
         byte[] zip = filesZip(Map.of("services.xml", servicesXml.getBytes(UTF_8)));
 
         try {
-            new ApplicationPackage(zip, false).metaDataZip();
+            new ApplicationPackage(zip).metaDataZip();
             fail("Should fail on missing include file");
         }
         catch (RuntimeException e) {
@@ -152,6 +151,21 @@ public class ApplicationPackageTest {
         assertEquals(originalPackage.bundleHash(), similarDeploymentXml.bundleHash());
     }
 
+    @Test
+    void testCertificateFileExists() throws Exception {
+        getApplicationZip("with-certificate.zip", true);
+    }
+
+    @Test
+    void testCertificateFileMissing() throws Exception {
+        try {
+            getApplicationZip("original.zip", true);
+            fail("Should fail on missing certificate file file");
+        } catch (RuntimeException e) {
+            assertEquals("No client certificate found in security/ in application package, see https://cloud.vespa.ai/en/security/guide", e.getMessage());
+        }
+    }
+
     static Map<String, String> unzip(byte[] zip) {
         return ZipEntries.from(zip, __ -> true, 1 << 24, true)
                          .asList().stream()
@@ -160,7 +174,11 @@ public class ApplicationPackageTest {
     }
 
     private ApplicationPackage getApplicationZip(String path) throws IOException {
-        return new ApplicationPackage(Files.readAllBytes(Path.of("src/test/resources/application-packages/" + path)), true);
+        return getApplicationZip(path, false);
+    }
+
+    private ApplicationPackage getApplicationZip(String path, boolean checkCertificateFile) throws IOException {
+        return new ApplicationPackage(Files.readAllBytes(Path.of("src/test/resources/application-packages/" + path)), true, checkCertificateFile);
     }
 
     static byte[] zip(Map<String, String> content) {
diff --git a/controller-server/src/test/resources/application-packages/with-certificate.zip b/controller-server/src/test/resources/application-packages/with-certificate.zip
new file mode 100644
index 00000000000..1540b96c7ef
--- /dev/null
+++ b/controller-server/src/test/resources/application-packages/with-certificate.zip