Merge pull request #14238 from vespa-engine/bjorncs/access-control-with-response-filter-chain

Ignore response filter chain bindings when constructing access contro…

4 files changed, 75 insertions, 13 deletions

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 efde2d43350..f04edeb67f4 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
@@ -144,8 +144,7 @@ public class AccessControl {
         for (FilterBinding binding : http.getBindings()) {
             if (binding.chainId().toId().equals(chainId)) {
                 for (FilterBinding otherBinding : http.getBindings()) {
-                    if (!binding.chainId().equals(otherBinding.chainId())
-                            && effectivelyDuplicateOf(binding.binding(), otherBinding.binding())) {
+                    if (effectivelyDuplicateOf(binding, otherBinding)) {
                         duplicateBindings.add(binding);
                     }
                 }
@@ -154,14 +153,17 @@ public class AccessControl {
         duplicateBindings.forEach(http.getBindings()::remove);
     }
 
-    private static boolean effectivelyDuplicateOf(BindingPattern accessControlBinding, BindingPattern other) {
-        return accessControlBinding.equals(other)
-                || (accessControlBinding.path().equals(other.path()) && other.matchesAnyPort());
+    private static boolean effectivelyDuplicateOf(FilterBinding accessControlBinding, FilterBinding other) {
+        if (accessControlBinding.chainId().equals(other.chainId())) return false; // Same filter chain
+        if (other.type() == FilterBinding.Type.RESPONSE) return false;
+        return accessControlBinding.binding().equals(other.binding())
+                || (accessControlBinding.binding().path().equals(other.binding().path()) && other.binding().matchesAnyPort());
     }
 
 
     private static FilterBinding createAccessControlBinding(String path) {
         return FilterBinding.create(
+                FilterBinding.Type.REQUEST,
                 new ComponentSpecification(ACCESS_CONTROL_CHAIN_ID.stringValue()),
                 SystemBindingPattern.fromHttpPortAndPath(Integer.toString(HOSTED_CONTAINER_PORT), path));
     }
@@ -170,6 +172,7 @@ public class AccessControl {
         BindingPattern rewrittenBinding = SystemBindingPattern.fromHttpPortAndPath(
                 Integer.toString(HOSTED_CONTAINER_PORT), excludedBinding.path()); // only keep path from excluded binding
         return FilterBinding.create(
+                FilterBinding.Type.REQUEST,
                 new ComponentSpecification(ACCESS_CONTROL_EXCLUDED_CHAIN_ID.stringValue()),
                 rewrittenBinding);
     }
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..2921cdc9f11 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
@@ -11,16 +11,20 @@ import java.util.Objects;
  */
 public class FilterBinding {
 
+    public enum Type {REQUEST, RESPONSE}
+
+    private final Type type;
     private final ComponentSpecification chainId;
     private final BindingPattern binding;
 
-    private FilterBinding(ComponentSpecification chainId, BindingPattern binding) {
+    private FilterBinding(Type type, ComponentSpecification chainId, BindingPattern binding) {
+        this.type = type;
         this.chainId = chainId;
         this.binding = binding;
     }
 
-    public static FilterBinding create(ComponentSpecification chainId, BindingPattern binding) {
-        return new FilterBinding(chainId, binding);
+    public static FilterBinding create(Type type, ComponentSpecification chainId, BindingPattern binding) {
+        return new FilterBinding(type, chainId, binding);
     }
 
     public ComponentSpecification chainId() {
@@ -31,17 +35,20 @@ public class FilterBinding {
         return binding;
     }
 
+    public Type type() { return type; }
+
     @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) &&
+        return type == that.type &&
+                Objects.equals(chainId, that.chainId) &&
                 Objects.equals(binding, that.binding);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(chainId, binding);
+        return Objects.hash(type, chainId, 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..5b360b478fa 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
@@ -31,6 +31,10 @@ import java.util.logging.Level;
  */
 public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http> {
 
+    static final String REQUEST_CHAIN_TAG_NAME = "request-chain";
+    static final String RESPONSE_CHAIN_TAG_NAME = "response-chain";
+    static final List<String> VALID_FILTER_CHAIN_TAG_NAMES = List.of(REQUEST_CHAIN_TAG_NAME, RESPONSE_CHAIN_TAG_NAME);
+
     @Override
     protected Http doBuild(DeployState deployState, AbstractConfigProducer ancestor, Element spec) {
         FilterChains filterChains;
@@ -116,18 +120,26 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
 
         for (Element child: XML.getChildren(filteringSpec)) {
             String tagName = child.getTagName();
-            if ((tagName.equals("request-chain") || tagName.equals("response-chain"))) {
+            if (VALID_FILTER_CHAIN_TAG_NAMES.contains(tagName)) {
                 ComponentSpecification chainId = XmlHelper.getIdRef(child);
 
                 for (Element bindingSpec: XML.getChildren(child, "binding")) {
                     String binding = XML.getValue(bindingSpec);
-                    result.add(FilterBinding.create(chainId, UserBindingPattern.fromPattern(binding)));
+                    result.add(FilterBinding.create(toFilterBindingType(tagName), chainId, UserBindingPattern.fromPattern(binding)));
                 }
             }
         }
         return result;
     }
 
+    private static FilterBinding.Type toFilterBindingType(String chainTag) {
+        switch (chainTag) {
+            case REQUEST_CHAIN_TAG_NAME: return FilterBinding.Type.REQUEST;
+            case RESPONSE_CHAIN_TAG_NAME: return FilterBinding.Type.RESPONSE;
+            default: throw new IllegalArgumentException("Unknown filter chain tag: " + chainTag);
+        }
+    }
+
     static int readPort(ModelElement spec, boolean isHosted, DeployLogger logger) {
         Integer port = spec.integerAttribute("port");
         if (port == null)
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java
index f5d0c2d1825..92b54a4679d 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java
@@ -192,7 +192,7 @@ public class AccessControlTest extends ContainerModelBuilderTestBase {
     }
 
     @Test
-    public void access_control_chains_does_not_contain_duplicate_bindings_to_user_filter_chain() {
+    public void access_control_chains_does_not_contain_duplicate_bindings_to_user_request_filter_chain() {
         Http http = createModelAndGetHttp(
                 "  <http>",
                 "    <handler id='custom.Handler'>",
@@ -227,6 +227,46 @@ public class AccessControlTest extends ContainerModelBuilderTestBase {
         assertThat(actualCustomChainBindings, containsInAnyOrder("http://*/custom-handler/*", "http://*/"));
     }
 
+    @Test
+    public void access_control_excludes_are_not_affected_by_user_response_filter_chain() {
+        Http http = createModelAndGetHttp(
+                "  <http>",
+                "    <handler id='custom.Handler'>",
+                "      <binding>http://*/custom-handler/*</binding>",
+                "    </handler>",
+                "    <filtering>",
+                "      <access-control>",
+                "        <exclude>",
+                "          <binding>http://*/custom-handler/*</binding>",
+                "        </exclude>",
+                "      </access-control>",
+                "      <response-chain id='my-custom-response-chain'>",
+                "        <filter id='my-custom-response-filter' />",
+                "        <binding>http://*/custom-handler/*</binding>",
+                "      </response-chain>",
+                "    </filtering>",
+                "  </http>");
+
+        Set<String> actualExcludeBindings = getFilterBindings(http, AccessControl.ACCESS_CONTROL_EXCLUDED_CHAIN_ID);
+        assertThat(actualExcludeBindings, containsInAnyOrder(
+                "http://*:4443/ApplicationStatus",
+                "http://*:4443/status.html",
+                "http://*:4443/state/v1",
+                "http://*:4443/state/v1/*",
+                "http://*:4443/prometheus/v1",
+                "http://*:4443/prometheus/v1/*",
+                "http://*:4443/metrics/v2",
+                "http://*:4443/metrics/v2/*",
+                "http://*:4443/",
+                "http://*:4443/custom-handler/*"));
+
+        Set<String> actualAccessControlBindings = getFilterBindings(http, AccessControl.ACCESS_CONTROL_CHAIN_ID);
+        assertThat(actualAccessControlBindings, containsInAnyOrder("http://*:4443/*"));
+
+        Set<String> actualCustomChainBindings = getFilterBindings(http, ComponentId.fromString("my-custom-response-chain"));
+        assertThat(actualCustomChainBindings, containsInAnyOrder("http://*/custom-handler/*"));
+    }
+
     private Http createModelAndGetHttp(String... httpElement) {
         List<String> servicesXml = new ArrayList<>();
         servicesXml.add("<container version='1.0'>");