Revert "Bjorncs/improve athenz access control"

6 files changed, 75 insertions, 96 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/SystemBindingPattern.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/SystemBindingPattern.java
index 3ae531539ef..559651b3b4f 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/SystemBindingPattern.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/SystemBindingPattern.java
@@ -13,7 +13,6 @@ public class SystemBindingPattern extends BindingPattern {
 
     public static SystemBindingPattern fromHttpPath(String path) { return new SystemBindingPattern("http", "*", null, path);}
     public static SystemBindingPattern fromPattern(String binding) { return new SystemBindingPattern(binding);}
-    public static SystemBindingPattern fromHttpPortAndPath(String port, String path) { return new SystemBindingPattern("http", "*", port, path); }
 
     @Override
     public String toString() {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java
index e96951dc83a..87c6d41c80d 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java
@@ -8,15 +8,17 @@ import com.yahoo.vespa.model.container.ContainerCluster;
 import com.yahoo.vespa.model.container.component.BindingPattern;
 import com.yahoo.vespa.model.container.component.FileStatusHandlerComponent;
 import com.yahoo.vespa.model.container.component.Handler;
+import com.yahoo.vespa.model.container.component.Servlet;
 import com.yahoo.vespa.model.container.component.SystemBindingPattern;
-import com.yahoo.vespa.model.container.component.chain.Chain;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Helper class for http access control.
@@ -24,15 +26,11 @@ import java.util.Set;
  * @author gjoranv
  * @author bjorncs
  */
-public class AccessControl {
+public final class AccessControl {
 
     public static final ComponentId ACCESS_CONTROL_CHAIN_ID = ComponentId.fromString("access-control-chain");
-    public static final ComponentId ACCESS_CONTROL_EXCLUDED_CHAIN_ID = ComponentId.fromString("access-control-excluded-chain");
 
-    private static final int HOSTED_CONTAINER_PORT = 4443;
-
-    // Handlers that are excluded from access control
-    private static final List<String> EXCLUDED_HANDLERS = List.of(
+    public static final List<String> UNPROTECTED_HANDLERS = List.of(
             FileStatusHandlerComponent.CLASS,
             ContainerCluster.APPLICATION_STATUS_HANDLER_CLASS,
             ContainerCluster.BINDINGS_OVERVIEW_HANDLER_CLASS,
@@ -42,12 +40,13 @@ public class AccessControl {
             ApplicationContainerCluster.PROMETHEUS_V1_HANDLER_CLASS
     );
 
-    public static class Builder {
-        private final String domain;
+    public static final class Builder {
+        private String domain;
         private boolean readEnabled = false;
         private boolean writeEnabled = true;
         private final Set<BindingPattern> excludeBindings = new LinkedHashSet<>();
         private Collection<Handler<?>> handlers = Collections.emptyList();
+        private Collection<Servlet> servlets = Collections.emptyList();
 
         public Builder(String domain) {
             this.domain = domain;
@@ -58,8 +57,8 @@ public class AccessControl {
             return this;
         }
 
-        public Builder writeEnabled(boolean writeEnabled) {
-            this.writeEnabled = writeEnabled;
+        public Builder writeEnabled(boolean writeEnalbed) {
+            this.writeEnabled = writeEnalbed;
             return this;
         }
 
@@ -70,11 +69,13 @@ public class AccessControl {
 
         public Builder setHandlers(ApplicationContainerCluster cluster) {
             this.handlers = cluster.getHandlers();
+            this.servlets = cluster.getAllServlets();
             return this;
         }
 
         public AccessControl build() {
-            return new AccessControl(domain, writeEnabled, readEnabled, excludeBindings, handlers);
+            return new AccessControl(domain, writeEnabled, readEnabled,
+                                     excludeBindings, servlets, handlers);
         }
     }
 
@@ -83,83 +84,69 @@ public class AccessControl {
     public final boolean writeEnabled;
     private final Set<BindingPattern> excludedBindings;
     private final Collection<Handler<?>> handlers;
+    private final Collection<Servlet> servlets;
 
     private AccessControl(String domain,
                           boolean writeEnabled,
                           boolean readEnabled,
                           Set<BindingPattern> excludedBindings,
+                          Collection<Servlet> servlets,
                           Collection<Handler<?>> handlers) {
         this.domain = domain;
         this.readEnabled = readEnabled;
         this.writeEnabled = writeEnabled;
         this.excludedBindings = Collections.unmodifiableSet(excludedBindings);
         this.handlers = handlers;
+        this.servlets = servlets;
     }
 
-    public void configureHttpFilterChains(Http http) {
-        http.setAccessControl(this);
-        addAccessControlFilterChain(http);
-        addAccessControlExcludedChain(http);
-        removeDuplicateBindingsFromAccessControlChain(http);
+    public List<FilterBinding> getBindings() {
+        return Stream.concat(getHandlerBindings(), getServletBindings())
+                .collect(Collectors.toCollection(ArrayList::new));
     }
 
     public static boolean hasHandlerThatNeedsProtection(ApplicationContainerCluster cluster) {
-        return cluster.getHandlers().stream()
-                .anyMatch(handler -> ! isExcludedHandler(handler) && hasNonMbusBinding(handler));
+        return cluster.getHandlers().stream().anyMatch(AccessControl::handlerNeedsProtection);
     }
 
-    private void addAccessControlFilterChain(Http http) {
-        http.getFilterChains().add(createChain(ACCESS_CONTROL_CHAIN_ID));
-        http.getBindings().addAll(List.of(createAccessControlBinding("/"), createAccessControlBinding("/*")));
+    private Stream<FilterBinding> getHandlerBindings() {
+        return handlers.stream()
+                        .filter(this::shouldHandlerBeProtected)
+                        .flatMap(handler -> handler.getServerBindings().stream())
+                        .map(binding -> accessControlBinding(binding));
     }
 
-    private void addAccessControlExcludedChain(Http http) {
-        http.getFilterChains().add(createChain(ACCESS_CONTROL_EXCLUDED_CHAIN_ID));
-        for (BindingPattern excludedBinding : excludedBindings) {
-            http.getBindings().add(createAccessControlExcludedBinding(excludedBinding));
-        }
-        for (Handler<?> handler : handlers) {
-            if (isExcludedHandler(handler)) {
-                for (BindingPattern binding : handler.getServerBindings()) {
-                    http.getBindings().add(createAccessControlExcludedBinding(binding));
-                }
-            }
-        }
+    private Stream<FilterBinding> getServletBindings() {
+        return servlets.stream()
+                .filter(this::shouldServletBeProtected)
+                .flatMap(AccessControl::servletBindings)
+                .map(binding -> accessControlBinding(binding));
     }
 
-    // Remove bindings from access control chain that have binding pattern as a different filter chain
-    private void removeDuplicateBindingsFromAccessControlChain(Http http) {
-        Set<FilterBinding> duplicateBindings = new HashSet<>();
-        for (FilterBinding binding : http.getBindings()) {
-            if (binding.chainId().toId().equals(ACCESS_CONTROL_CHAIN_ID)) {
-                for (FilterBinding otherBinding : http.getBindings()) {
-                    if (!binding.chainId().equals(otherBinding.chainId())
-                            && binding.binding().equals(otherBinding.binding())) {
-                        duplicateBindings.add(binding);
-                    }
-                }
-            }
-        }
-        duplicateBindings.forEach(http.getBindings()::remove);
+    private boolean shouldHandlerBeProtected(Handler<?> handler) {
+        return ! isBuiltinGetOnly(handler)
+                && handler.getServerBindings().stream().noneMatch(excludedBindings::contains);
     }
 
-    private static FilterBinding createAccessControlBinding(String path) {
-        return FilterBinding.create(
-                new ComponentSpecification(ACCESS_CONTROL_CHAIN_ID.stringValue()),
-                SystemBindingPattern.fromHttpPortAndPath(Integer.toString(HOSTED_CONTAINER_PORT), path));
+    private static boolean isBuiltinGetOnly(Handler<?> handler) {
+        return UNPROTECTED_HANDLERS.contains(handler.getClassId().getName());
     }
 
-    private static FilterBinding createAccessControlExcludedBinding(BindingPattern excludedBinding) {
-        BindingPattern rewrittenBinding = SystemBindingPattern.fromHttpPortAndPath(
-                Integer.toString(HOSTED_CONTAINER_PORT), excludedBinding.path()); // only keep path from excluded binding
-        return FilterBinding.create(
-                new ComponentSpecification(ACCESS_CONTROL_EXCLUDED_CHAIN_ID.stringValue()),
-                rewrittenBinding);
+    private boolean shouldServletBeProtected(Servlet servlet) {
+        return servletBindings(servlet).noneMatch(excludedBindings::contains);
     }
 
-    private static Chain<Filter> createChain(ComponentId id) { return new Chain<>(FilterChains.emptyChainSpec(id)); }
+    private static FilterBinding accessControlBinding(BindingPattern binding) {
+        return FilterBinding.create(new ComponentSpecification(ACCESS_CONTROL_CHAIN_ID.stringValue()), binding);
+    }
+
+    private static Stream<BindingPattern> servletBindings(Servlet servlet) {
+        return Stream.of(SystemBindingPattern.fromHttpPath("/" + servlet.bindingPath));
+    }
 
-    private static boolean isExcludedHandler(Handler<?> handler) { return EXCLUDED_HANDLERS.contains(handler.getClassId().getName()); }
+    private static boolean handlerNeedsProtection(Handler<?> handler) {
+        return ! isBuiltinGetOnly(handler) && hasNonMbusBinding(handler);
+    }
 
     private static boolean hasNonMbusBinding(Handler<?> handler) {
         return handler.getServerBindings().stream().anyMatch(binding -> ! binding.scheme().equals("mbus"));
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/FilterBinding.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/FilterBinding.java
index 1ca54769683..8ae06b7cebd 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/FilterBinding.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/FilterBinding.java
@@ -4,44 +4,29 @@ package com.yahoo.vespa.model.container.http;
 import com.yahoo.component.ComponentSpecification;
 import com.yahoo.vespa.model.container.component.BindingPattern;
 
-import java.util.Objects;
-
 /**
  * @author bjorncs
  */
 public class FilterBinding {
 
-    private final ComponentSpecification chainId;
+    private final ComponentSpecification filterId;
     private final BindingPattern binding;
 
-    private FilterBinding(ComponentSpecification chainId, BindingPattern binding) {
-        this.chainId = chainId;
+    private FilterBinding(ComponentSpecification filterId, BindingPattern binding) {
+        this.filterId = filterId;
         this.binding = binding;
     }
 
-    public static FilterBinding create(ComponentSpecification chainId, BindingPattern binding) {
-        return new FilterBinding(chainId, binding);
+    public static FilterBinding create(ComponentSpecification filterId, BindingPattern binding) {
+        return new FilterBinding(filterId, binding);
     }
 
-    public ComponentSpecification chainId() {
-        return chainId;
+    public ComponentSpecification filterId() {
+        return filterId;
     }
 
     public BindingPattern binding() {
         return binding;
     }
 
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
-        FilterBinding that = (FilterBinding) o;
-        return Objects.equals(chainId, that.chainId) &&
-                Objects.equals(binding, that.binding);
-    }
-
-    @Override
-    public int hashCode() {
-        return Objects.hash(chainId, binding);
-    }
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
index f58f5faa382..3155669527a 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/Http.java
@@ -76,7 +76,7 @@ public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> impl
     public void getConfig(ServerConfig.Builder builder) {
         for (FilterBinding binding : bindings) {
             builder.filter(new ServerConfig.Filter.Builder()
-                    .id(binding.chainId().stringValue())
+                    .id(binding.filterId().stringValue())
                     .binding(binding.binding().patternString()));
         }
     }
@@ -92,8 +92,8 @@ public class Http extends AbstractConfigProducer<AbstractConfigProducer<?>> impl
         ComponentRegistry<Chain<Filter>> chains = filterChains.allChains();
 
         for (FilterBinding binding: bindings) {
-            if (filters.getComponent(binding.chainId()) == null && chains.getComponent(binding.chainId()) == null)
-                throw new RuntimeException("Can't find filter " + binding.chainId() + " for binding " + binding.binding());
+            if (filters.getComponent(binding.filterId()) == null && chains.getComponent(binding.filterId()) == null)
+                throw new RuntimeException("Can't find filter " + binding.filterId() + " for binding " + binding.binding());
         }
     }
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
index c86d8b206d5..9d5fead7dfb 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
@@ -14,6 +14,7 @@ import com.yahoo.vespa.model.builder.xml.dom.VespaDomBuilder;
 import com.yahoo.vespa.model.container.ApplicationContainerCluster;
 import com.yahoo.vespa.model.container.Container;
 import com.yahoo.vespa.model.container.component.UserBindingPattern;
+import com.yahoo.vespa.model.container.component.chain.Chain;
 import com.yahoo.vespa.model.container.http.AccessControl;
 import com.yahoo.vespa.model.container.http.FilterBinding;
 import com.yahoo.vespa.model.container.http.FilterChains;
@@ -25,6 +26,8 @@ import java.util.List;
 import java.util.Optional;
 import java.util.logging.Level;
 
+import static com.yahoo.vespa.model.container.http.AccessControl.ACCESS_CONTROL_CHAIN_ID;
+
 /**
  * @author Tony Vaagenes
  * @author gjoranv
@@ -45,6 +48,8 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
             Element accessControlElem = XML.getChild(filteringElem, "access-control");
             if (accessControlElem != null) {
                 accessControl = buildAccessControl(deployState, ancestor, accessControlElem);
+                bindings.addAll(accessControl.getBindings());
+                filterChains.add(new Chain<>(FilterChains.emptyChainSpec(ACCESS_CONTROL_CHAIN_ID)));
             }
         } else {
             filterChains = new FilterChainsBuilder().newChainsInstance(ancestor);
@@ -52,10 +57,8 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
 
         Http http = new Http(filterChains);
         http.getBindings().addAll(bindings);
+        http.setAccessControl(accessControl);
         http.setHttpServer(new JettyHttpServerBuilder().build(deployState, ancestor, spec));
-        if (accessControl != null) {
-            accessControl.configureHttpFilterChains(http);
-        }
         return http;
     }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index 51583588201..d11e0f9a891 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -61,6 +61,7 @@ import com.yahoo.vespa.model.container.component.FileStatusHandlerComponent;
 import com.yahoo.vespa.model.container.component.Handler;
 import com.yahoo.vespa.model.container.component.SystemBindingPattern;
 import com.yahoo.vespa.model.container.component.UserBindingPattern;
+import com.yahoo.vespa.model.container.component.chain.Chain;
 import com.yahoo.vespa.model.container.component.chain.ProcessingHandler;
 import com.yahoo.vespa.model.container.docproc.ContainerDocproc;
 import com.yahoo.vespa.model.container.docproc.DocprocChains;
@@ -93,6 +94,7 @@ import java.util.function.Consumer;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
+import static com.yahoo.vespa.model.container.http.AccessControl.ACCESS_CONTROL_CHAIN_ID;
 import static java.util.logging.Level.WARNING;
 
 /**
@@ -369,12 +371,15 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         if (http.getAccessControl().isPresent()) return; // access control added explicitly
         AthenzDomain tenantDomain = deployState.getProperties().athenzDomain().orElse(null);
         if (tenantDomain == null) return; // tenant domain not present, cannot add access control. this should eventually be a failure.
-        new AccessControl.Builder(tenantDomain.value())
-                .setHandlers(cluster)
-                .readEnabled(false)
-                .writeEnabled(false)
-                .build()
-                .configureHttpFilterChains(http);
+        AccessControl accessControl =
+                new AccessControl.Builder(tenantDomain.value())
+                        .setHandlers(cluster)
+                        .readEnabled(false)
+                        .writeEnabled(false)
+                        .build();
+        http.getFilterChains().add(new Chain<>(FilterChains.emptyChainSpec(ACCESS_CONTROL_CHAIN_ID)));
+        http.setAccessControl(accessControl);
+        http.getBindings().addAll(accessControl.getBindings());
     }
 
     private Http buildHttp(DeployState deployState, ApplicationContainerCluster cluster, Element httpElement) {