eliminate validate-endpoint-certificates feature flag (#16385)

7 files changed, 28 insertions, 105 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 5eb7fb6e03d..d064bb17b2c 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -11,7 +11,6 @@ import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.ZoneId;
-import com.yahoo.container.jdisc.secretstore.SecretStore;
 import com.yahoo.vespa.athenz.api.AthenzDomain;
 import com.yahoo.vespa.athenz.api.AthenzIdentity;
 import com.yahoo.vespa.athenz.api.AthenzPrincipal;
@@ -78,13 +77,11 @@ import java.security.Principal;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.EnumSet;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -92,20 +89,15 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.function.Consumer;
-import java.util.function.Function;
-import java.util.function.Predicate;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.yahoo.vespa.hosted.controller.api.integration.configserver.Node.State.active;
 import static com.yahoo.vespa.hosted.controller.api.integration.configserver.Node.State.reserved;
 import static java.util.Comparator.naturalOrder;
-import static java.util.function.Function.identity;
-import static java.util.function.Predicate.not;
 import static java.util.stream.Collectors.joining;
 import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toMap;
-import static java.util.stream.Collectors.toUnmodifiableMap;
 
 /**
  * A singleton owned by the Controller which contains the methods and state for controlling applications.
@@ -134,7 +126,7 @@ public class ApplicationController {
     private final BillingController billingController;
 
     ApplicationController(Controller controller, CuratorDb curator, AccessControl accessControl, Clock clock,
-                          SecretStore secretStore, FlagSource flagSource, BillingController billingController) {
+                          FlagSource flagSource, BillingController billingController) {
 
         this.controller = controller;
         this.curator = curator;
@@ -148,8 +140,12 @@ public class ApplicationController {
 
         deploymentTrigger = new DeploymentTrigger(controller, clock);
         applicationPackageValidator = new ApplicationPackageValidator(controller);
-        endpointCertificateManager = new EndpointCertificateManager(controller.zoneRegistry(), curator, secretStore,
-                controller.serviceRegistry().endpointCertificateProvider(), clock, flagSource);
+        endpointCertificateManager = new EndpointCertificateManager(
+                controller.zoneRegistry(),
+                curator,
+                controller.serviceRegistry().endpointCertificateProvider(),
+                controller.serviceRegistry().endpointCertificateValidator(),
+                clock);
 
         // Update serialization format of all applications
         Once.after(Duration.ofMinutes(1), () -> {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
index ffe80866086..e12c1903426 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
@@ -110,7 +110,7 @@ public class Controller extends AbstractComponent {
         metrics = new ConfigServerMetrics(serviceRegistry.configServer());
         nameServiceForwarder = new NameServiceForwarder(curator);
         jobController = new JobController(this);
-        applicationController = new ApplicationController(this, curator, accessControl, clock, secretStore, flagSource, serviceRegistry.billingController());
+        applicationController = new ApplicationController(this, curator, accessControl, clock, flagSource, serviceRegistry.billingController());
         tenantController = new TenantController(this, curator, accessControl, flagSource);
         routingController = new RoutingController(this, Objects.requireNonNull(rotationsConfig, "RotationsConfig cannot be null"));
         auditLogger = new AuditLogger(curator, clock);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateException.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateException.java
deleted file mode 100644
index 321eb783dab..00000000000
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateException.java
+++ /dev/null
@@ -1,26 +0,0 @@
-// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.hosted.controller.certificate;
-
-public class EndpointCertificateException extends RuntimeException {
-
-    private final Type type;
-
-    public EndpointCertificateException(Type type, String message) {
-        super(message);
-        this.type = type;
-    }
-
-    public EndpointCertificateException(Type type, String message, Throwable cause) {
-        super(message, cause);
-        this.type = type;
-    }
-
-    public Type type() {
-        return type;
-    }
-
-    public enum Type {
-        CERT_NOT_AVAILABLE,
-        VERIFICATION_FAILURE
-    }
-}
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManager.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManager.java
index 87531240752..86ba25b7ad7 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManager.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManager.java
@@ -9,16 +9,10 @@ import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.zone.RoutingMethod;
 import com.yahoo.config.provision.zone.ZoneApi;
 import com.yahoo.config.provision.zone.ZoneId;
-import com.yahoo.container.jdisc.secretstore.SecretNotFoundException;
-import com.yahoo.container.jdisc.secretstore.SecretStore;
-import com.yahoo.security.SubjectAlternativeName;
-import com.yahoo.security.X509CertificateUtils;
-import com.yahoo.vespa.flags.BooleanFlag;
-import com.yahoo.vespa.flags.FlagSource;
-import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateProvider;
+import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateValidator;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
 import com.yahoo.vespa.hosted.controller.application.Endpoint;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
@@ -26,7 +20,6 @@ import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import org.jetbrains.annotations.NotNull;
 
 import java.nio.charset.Charset;
-import java.security.cert.X509Certificate;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
@@ -43,7 +36,7 @@ import java.util.stream.Collectors;
 /**
  * Looks up stored endpoint certificate metadata, provisions new certificates if none is found,
  * re-provisions if zone is not covered, and uses refreshed certificates if a newer version is available.
- *
+ * <p>
  * See also EndpointCertificateMaintainer, which handles refreshes, deletions and triggers deployments
  *
  * @author andreer
@@ -54,22 +47,20 @@ public class EndpointCertificateManager {
 
     private final ZoneRegistry zoneRegistry;
     private final CuratorDb curator;
-    private final SecretStore secretStore;
     private final EndpointCertificateProvider endpointCertificateProvider;
     private final Clock clock;
-    private final BooleanFlag validateEndpointCertificates;
+    private final EndpointCertificateValidator endpointCertificateValidator;
 
     public EndpointCertificateManager(ZoneRegistry zoneRegistry,
                                       CuratorDb curator,
-                                      SecretStore secretStore,
                                       EndpointCertificateProvider endpointCertificateProvider,
-                                      Clock clock, FlagSource flagSource) {
+                                      EndpointCertificateValidator endpointCertificateValidator,
+                                      Clock clock) {
         this.zoneRegistry = zoneRegistry;
         this.curator = curator;
-        this.secretStore = secretStore;
         this.endpointCertificateProvider = endpointCertificateProvider;
         this.clock = clock;
-        this.validateEndpointCertificates = Flags.VALIDATE_ENDPOINT_CERTIFICATES.bindTo(flagSource);
+        this.endpointCertificateValidator = endpointCertificateValidator;
     }
 
     public Optional<EndpointCertificateMetadata> getEndpointCertificateMetadata(Instance instance, ZoneId zone, Optional<DeploymentInstanceSpec> instanceSpec) {
@@ -101,11 +92,11 @@ public class EndpointCertificateManager {
             var reprovisionedCertificateMetadata = provisionEndpointCertificate(instance, currentCertificateMetadata, zone, instanceSpec);
             curator.writeEndpointCertificateMetadata(instance.id(), reprovisionedCertificateMetadata);
             // Verification is unlikely to succeed in this case, as certificate must be available first - controller will retry
-            validateEndpointCertificate(reprovisionedCertificateMetadata, instance, zone);
+            endpointCertificateValidator.validate(reprovisionedCertificateMetadata, instance.id().serializedForm(), zone, requiredSansForZone);
             return Optional.of(reprovisionedCertificateMetadata);
         }
 
-        validateEndpointCertificate(currentCertificateMetadata.get(), instance, zone);
+        endpointCertificateValidator.validate(currentCertificateMetadata.get(), instance.id().serializedForm(), zone, requiredSansForZone);
         return currentCertificateMetadata;
     }
 
@@ -145,53 +136,6 @@ public class EndpointCertificateManager {
         return endpointCertificateProvider.requestCaSignedCertificate(instance.id(), List.copyOf(requiredNames), currentMetadata);
     }
 
-    private void validateEndpointCertificate(EndpointCertificateMetadata endpointCertificateMetadata, Instance instance, ZoneId zone) {
-        if (validateEndpointCertificates.value())
-            try {
-                var pemEncodedEndpointCertificate = secretStore.getSecret(endpointCertificateMetadata.certName(), endpointCertificateMetadata.version());
-
-                if (pemEncodedEndpointCertificate == null)
-                    throw new EndpointCertificateException(EndpointCertificateException.Type.VERIFICATION_FAILURE, "Secret store returned null for certificate");
-
-                List<X509Certificate> x509CertificateList = X509CertificateUtils.certificateListFromPem(pemEncodedEndpointCertificate);
-
-                if (x509CertificateList.isEmpty())
-                    throw new EndpointCertificateException(EndpointCertificateException.Type.VERIFICATION_FAILURE, "Empty certificate list");
-                if (x509CertificateList.size() < 2)
-                    throw new EndpointCertificateException(EndpointCertificateException.Type.VERIFICATION_FAILURE, "Only a single certificate found in chain - intermediate certificates likely missing");
-
-                Instant now = clock.instant();
-                Instant firstExpiry = Instant.MAX;
-                for (X509Certificate x509Certificate : x509CertificateList) {
-                    Instant notBefore = x509Certificate.getNotBefore().toInstant();
-                    Instant notAfter = x509Certificate.getNotAfter().toInstant();
-                    if (now.isBefore(notBefore))
-                        throw new EndpointCertificateException(EndpointCertificateException.Type.VERIFICATION_FAILURE, "Certificate is not yet valid");
-                    if (now.isAfter(notAfter))
-                        throw new EndpointCertificateException(EndpointCertificateException.Type.VERIFICATION_FAILURE, "Certificate has expired");
-                    if (notAfter.isBefore(firstExpiry)) firstExpiry = notAfter;
-                }
-
-                X509Certificate endEntityCertificate = x509CertificateList.get(0);
-                Set<String> subjectAlternativeNames = X509CertificateUtils.getSubjectAlternativeNames(endEntityCertificate).stream()
-                        .filter(san -> san.getType().equals(SubjectAlternativeName.Type.DNS_NAME))
-                        .map(SubjectAlternativeName::getValue).collect(Collectors.toSet());
-
-                var dnsNamesOfZone = dnsNamesOf(instance.id(), zone);
-                if (!subjectAlternativeNames.containsAll(dnsNamesOfZone))
-                    throw new EndpointCertificateException(EndpointCertificateException.Type.VERIFICATION_FAILURE, "Certificate is missing required SANs for zone " + zone.value());
-
-            } catch (SecretNotFoundException s) {
-                // Normally because the cert is in the process of being provisioned - this will cause a retry in InternalStepRunner
-                throw new EndpointCertificateException(EndpointCertificateException.Type.CERT_NOT_AVAILABLE, "Certificate not found in secret store");
-            } catch (EndpointCertificateException e) {
-                log.log(Level.WARNING, "Certificate validation failure for " + instance.id().serializedForm(), e);
-                throw e;
-            } catch (Exception e) {
-                log.log(Level.WARNING, "Certificate validation failure for " + instance.id().serializedForm(), e);
-                throw new EndpointCertificateException(EndpointCertificateException.Type.VERIFICATION_FAILURE, "Certificate validation failure for app " + instance.id().serializedForm(), e);
-            }
-    }
 
     private List<String> dnsNamesOf(ApplicationId applicationId, ZoneId zone) {
         List<String> endpointDnsNames = new ArrayList<>();
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
index e789974ea13..77aefc90481 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
@@ -43,7 +43,7 @@ import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.application.Endpoint;
 import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId;
-import com.yahoo.vespa.hosted.controller.certificate.EndpointCertificateException;
+import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateException;
 import com.yahoo.vespa.hosted.controller.config.ControllerConfig;
 import com.yahoo.vespa.hosted.controller.maintenance.JobRunner;
 import com.yahoo.vespa.hosted.controller.routing.RoutingPolicyId;
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManagerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManagerTest.java
index 0d70fe48a77..be77f355e35 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManagerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificateManagerTest.java
@@ -16,6 +16,7 @@ import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMock;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata;
+import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateValidatorImpl;
 import com.yahoo.vespa.hosted.controller.integration.SecretStoreMock;
 import com.yahoo.vespa.hosted.controller.integration.ZoneRegistryMock;
 import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb;
@@ -47,8 +48,9 @@ public class EndpointCertificateManagerTest {
     private final EndpointCertificateMock endpointCertificateMock = new EndpointCertificateMock();
     private final InMemoryFlagSource inMemoryFlagSource = new InMemoryFlagSource();
     private static final Clock clock = Clock.fixed(Instant.EPOCH, java.time.ZoneId.systemDefault());
+    private final EndpointCertificateValidatorImpl endpointCertificateValidator = new EndpointCertificateValidatorImpl(secretStore, clock);
     private final EndpointCertificateManager endpointCertificateManager =
-            new EndpointCertificateManager(zoneRegistryMock, mockCuratorDb, secretStore, endpointCertificateMock, clock, inMemoryFlagSource);
+            new EndpointCertificateManager(zoneRegistryMock, mockCuratorDb, endpointCertificateMock, endpointCertificateValidator, clock);
 
     private static final List<String> expectedSans = List.of(
             "vt2ktgkqme5zlnp4tj4ttyor7fj3v7q5o.vespa.oath.cloud",
@@ -103,7 +105,6 @@ public class EndpointCertificateManagerTest {
     public void setUp() {
         zoneRegistryMock.exclusiveRoutingIn(zoneRegistryMock.zones().all().zones());
         testZone = zoneRegistryMock.zones().directlyRouted().in(Environment.prod).zones().stream().findFirst().orElseThrow().getId();
-        inMemoryFlagSource.withBooleanFlag(Flags.VALIDATE_ENDPOINT_CERTIFICATES.id(), true);
     }
 
     @Test
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
index fd0e7c20896..1b7970f6fcb 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
@@ -15,6 +15,8 @@ import com.yahoo.vespa.hosted.controller.api.integration.aws.ResourceTagger;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.BillingController;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.MockBillingController;
 import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMock;
+import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateValidator;
+import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateValidatorMock;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.MemoryNameService;
 import com.yahoo.vespa.hosted.controller.api.integration.entity.MemoryEntityService;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.MockContactRetriever;
@@ -44,6 +46,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
     private final MemoryGlobalRoutingService memoryGlobalRoutingService = new MemoryGlobalRoutingService();
     private final MockMailer mockMailer = new MockMailer();
     private final EndpointCertificateMock endpointCertificateMock = new EndpointCertificateMock();
+    private final EndpointCertificateValidatorMock endpointCertificateValidatorMock = new EndpointCertificateValidatorMock();
     private final MockMeteringClient mockMeteringClient = new MockMeteringClient();
     private final MockContactRetriever mockContactRetriever = new MockContactRetriever();
     private final MockIssueHandler mockIssueHandler = new MockIssueHandler();
@@ -103,6 +106,11 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
     }
 
     @Override
+    public EndpointCertificateValidator endpointCertificateValidator() {
+        return endpointCertificateValidatorMock;
+    }
+
+    @Override
     public MockMeteringClient meteringService() {
         return mockMeteringClient;
     }