Merge pull request #27632 from vespa-engine/hakonhall/exclude-private-ip-addresses-in-other-cloud-accounts-in-acls

Exclude private IP addresses in other cloud accounts in ACLs

6 files changed, 73 insertions, 43 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
index 864566f119e..a80f07acba2 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
@@ -616,8 +616,8 @@ public final class Node implements Nodelike {
     }
 
     /** Returns the ACL for the node (trusted nodes, networks and ports) */
-    public NodeAcl acl(NodeList allNodes, LoadBalancers loadBalancers, Zone zone) {
-        return NodeAcl.from(this, allNodes, loadBalancers, zone);
+    public NodeAcl acl(NodeList allNodes, LoadBalancers loadBalancers, Zone zone, boolean simplerAcl) {
+        return NodeAcl.from(this, allNodes, loadBalancers, zone, simplerAcl);
     }
 
     @Override
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index f3d69fdf103..2d4e7142622 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -215,11 +215,11 @@ public class NodeRepository extends AbstractComponent {
      * @param host node for which to generate ACLs
      * @return the list of node ACLs
      */
-    public List<NodeAcl> getChildAcls(Node host) {
+    public List<NodeAcl> getChildAcls(Node host, boolean simplerAcl) {
         if ( ! host.type().isHost()) throw new IllegalArgumentException("Only hosts have children");
         NodeList allNodes = nodes().list();
         return allNodes.childrenOf(host)
-                       .mapToList(childNode -> childNode.acl(allNodes, loadBalancers, zone));
+                       .mapToList(childNode -> childNode.acl(allNodes, loadBalancers, zone, simplerAcl));
     }
 
     /** Removes this application: all nodes are set dirty. */
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
index 1ff6d2b300d..c34b357b758 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
@@ -119,7 +119,7 @@ public record IP() {
                 for (var other : sortedNodes) {
                     if (node.equals(other)) continue;
                     if (canAssignIpOf(other, node)) continue;
-                    Predicate<String> sharedIpSpace = other.cloudAccount().equals(node.cloudAccount()) ? __ -> true : IP::isPublic;
+                    Predicate<String> sharedIpSpace = ip -> inSharedIpSpace(ip, other.cloudAccount(), node.cloudAccount());
 
                     var addresses = new HashSet<>(node.ipConfig().primary());
                     var otherAddresses = new HashSet<>(other.ipConfig().primary());
@@ -473,4 +473,9 @@ public record IP() {
         return ! address.isLoopbackAddress() && ! address.isLinkLocalAddress() && ! address.isSiteLocalAddress();
     }
 
+    /** Returns true if the IP address is in the IP space of both sourceCloudAccount and targetCloudAccount. */
+    public static boolean inSharedIpSpace(String ip, CloudAccount sourceCloudAccount, CloudAccount targetCloudAccount) {
+        return sourceCloudAccount.equals(targetCloudAccount) || isPublic(ip);
+    }
+
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java
index 843ba240ce9..14c4a63a500 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/NodeAcl.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.provision.node;
 
 import com.google.common.collect.ImmutableSet;
+import com.yahoo.config.provision.CloudAccount;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.hosted.provision.Node;
@@ -11,12 +12,16 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancerInstance;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancers;
 
 import java.util.Comparator;
+import java.util.EnumSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.function.Consumer;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
 
 /**
@@ -41,7 +46,7 @@ public record NodeAcl(Node node,
         this.trustedUdpPorts = ImmutableSet.copyOf(Objects.requireNonNull(trustedUdpPorts, "trustedUdpPorts must be non-null"));
     }
 
-    public static NodeAcl from(Node node, NodeList allNodes, LoadBalancers loadBalancers, Zone zone) {
+    public static NodeAcl from(Node node, NodeList allNodes, LoadBalancers loadBalancers, Zone zone, boolean simplerAcl) {
         Set<TrustedNode> trustedNodes = new TreeSet<>(Comparator.comparing(TrustedNode::hostname));
         Set<Integer> trustedPorts = new LinkedHashSet<>();
         Set<Integer> trustedUdpPorts = new LinkedHashSet<>();
@@ -53,12 +58,13 @@ public record NodeAcl(Node node,
         //   SSH opened (which is safe for 2 reasons: SSH daemon is not run inside containers, and NPT networks
         //   will (should) not forward port 22 traffic to container).
         // - parent host (for health checks and metrics)
-        // - nodes in same application
+        // - nodes in same application (Slobrok for tenant nodes, file distribution and ZK for config servers, etc),
+        //   and parents if necessary due to NAT.
         // - load balancers allocated to application
         trustedPorts.add(22);
-        allNodes.parentOf(node).map(TrustedNode::of).ifPresent(trustedNodes::add);
+        allNodes.parentOf(node).map(parent -> TrustedNode.of(parent, node.cloudAccount(), simplerAcl)).ifPresent(trustedNodes::add);
         node.allocation().ifPresent(allocation -> {
-            trustedNodes.addAll(TrustedNode.of(allNodes.owner(allocation.owner())));
+            trustedNodes.addAll(trustedNodesForChildrenMatching(node, allNodes, n -> n.allocation().map(Allocation::owner).equals(Optional.of(allocation.owner())), Set.of(), simplerAcl));
             loadBalancers.list(allocation.owner()).asList()
                          .stream()
                          .map(LoadBalancer::instance)
@@ -72,19 +78,8 @@ public record NodeAcl(Node node,
                 // Tenant nodes in other states than ready, trust:
                 // - config servers
                 // - proxy nodes
-                // - parents of the nodes in the same application: If some nodes are on a different IP version
-                //   or only a subset of them are dual-stacked, the communication between the nodes may be NAT-ed
-                //   via parent's IP address
-                trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.config)));
-                trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.proxy)));
-                node.allocation().ifPresent(allocation -> trustedNodes.addAll(TrustedNode.of(allNodes.parentsOf(allNodes.owner(allocation.owner())))));
-                if (node.state() == Node.State.ready) {
-                    // Tenant nodes in state ready, trust:
-                    // - All tenant nodes in zone. When a ready node is allocated to an application there's a brief
-                    //   window where current ACLs have not yet been applied on the node. To avoid service disruption
-                    //   during this window, ready tenant nodes trust all other tenant nodes
-                    trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.tenant)));
-                }
+                trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.config), node.cloudAccount(), simplerAcl));
+                trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.proxy), node.cloudAccount(), simplerAcl));
             }
             case config -> {
                 // Config servers trust:
@@ -92,9 +87,7 @@ public record NodeAcl(Node node,
                 // - port 19070 (RPC) from all proxy nodes (and their hosts, in case traffic is NAT-ed via parent)
                 // - port 4443 from the world
                 // - udp port 51820 from the world
-                trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.host, NodeType.tenant,
-                                                                     NodeType.proxyhost, NodeType.proxy),
-                                                   RPC_PORTS));
+                trustedNodes.addAll(trustedNodesForChildrenMatching(node, allNodes, n -> EnumSet.of(NodeType.tenant, NodeType.proxy).contains(n.type()), RPC_PORTS, simplerAcl));
                 trustedPorts.add(4443);
                 if (zone.system().isPublic() && zone.cloud().allowEnclave()) {
                     trustedUdpPorts.add(WIREGUARD_PORT);
@@ -104,7 +97,7 @@ public record NodeAcl(Node node,
                 // Proxy nodes trust:
                 // - config servers
                 // - all connections from the world on 443 (production traffic) and 4443 (health checks)
-                trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.config)));
+                trustedNodes.addAll(TrustedNode.of(allNodes.nodeType(NodeType.config), node.cloudAccount(), simplerAcl));
                 trustedPorts.add(443);
                 trustedPorts.add(4443);
             }
@@ -121,26 +114,54 @@ public record NodeAcl(Node node,
         return new NodeAcl(node, trustedNodes, trustedNetworks, trustedPorts, trustedUdpPorts);
     }
 
+    /** Returns the set of children matching the selector, and their parent host if traffic from child may be NATed */
+    private static Set<TrustedNode> trustedNodesForChildrenMatching(Node node, NodeList allNodes, Predicate<Node> childNodeSelector,
+                                                                    Set<Integer> ports, boolean simplerAcl) {
+        if (node.type().isHost())
+            throw new IllegalArgumentException("Host nodes cannot have NAT parents");
+
+        boolean hasIp4 = node.ipConfig().primary().stream().anyMatch(IP::isV4);
+        boolean hasIp6 = node.ipConfig().primary().stream().anyMatch(IP::isV6);
+        return allNodes.stream()
+                       .filter(n -> !n.type().isHost())
+                       .filter(childNodeSelector)
+                       .mapMulti((Node otherNode, Consumer<TrustedNode> consumer) -> {
+                           consumer.accept(TrustedNode.of(otherNode, ports, node.cloudAccount(), simplerAcl));
+
+                           // And parent host if traffic from otherNode may be NATed
+                           if (hasIp4 && otherNode.ipConfig().primary().stream().noneMatch(IP::isV4) ||
+                               hasIp6 && otherNode.ipConfig().primary().stream().noneMatch(IP::isV6)) {
+                               consumer.accept(TrustedNode.of(allNodes.parentOf(otherNode).orElseThrow(), ports, node.cloudAccount(), simplerAcl));
+                           }
+                       })
+                       .collect(Collectors.toSet());
+    }
+
     public record TrustedNode(String hostname, NodeType type, Set<String> ipAddresses, Set<Integer> ports) {
 
-        /** Trust given ports from node */
-        public static TrustedNode of(Node node, Set<Integer> ports) {
-            return new TrustedNode(node.hostname(), node.type(), node.ipConfig().primary(), ports);
+        /** Trust given ports from node, and primary IP addresses shared with given cloud account */
+        public static TrustedNode of(Node node, Set<Integer> ports, CloudAccount sourceCloudAccount, boolean simplerAcl) {
+            Set<String> ipAddresses = node.ipConfig()
+                                          .primary()
+                                          .stream()
+                                          .filter(ip -> !simplerAcl || IP.inSharedIpSpace(ip, sourceCloudAccount, node.cloudAccount()))
+                                          .collect(Collectors.toSet());
+            return new TrustedNode(node.hostname(), node.type(), ipAddresses, ports);
         }
 
-        /** Trust all ports from given node */
-        public static TrustedNode of(Node node) {
-            return of(node, Set.of());
+        /** The node in the given sourceCloudAccount should trust all ports from given node */
+        public static TrustedNode of(Node node, CloudAccount sourceCloudAccount, boolean simplerAcl) {
+            return of(node, Set.of(), sourceCloudAccount, simplerAcl);
         }
 
-        public static List<TrustedNode> of(Iterable<Node> nodes, Set<Integer> ports) {
+        public static List<TrustedNode> of(Iterable<Node> nodes, Set<Integer> ports, CloudAccount sourceCloudAccount, boolean simplerAcl) {
             return StreamSupport.stream(nodes.spliterator(), false)
-                                .map(node -> TrustedNode.of(node, ports))
+                                .map(node -> TrustedNode.of(node, ports, sourceCloudAccount, simplerAcl))
                                 .toList();
         }
 
-        public static List<TrustedNode> of(Iterable<Node> nodes) {
-            return of(nodes, Set.of());
+        public static List<TrustedNode> of(Iterable<Node> nodes, CloudAccount sourceCloudAccount, boolean simplerAcl) {
+            return of(nodes, Set.of(), sourceCloudAccount, simplerAcl);
         }
 
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodeAclResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodeAclResponse.java
index 6fe14715355..784f8f82d14 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodeAclResponse.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodeAclResponse.java
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision.restapi;
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.restapi.SlimeJsonResponse;
 import com.yahoo.slime.Cursor;
+import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.NodeAcl;
@@ -33,8 +34,9 @@ public class NodeAclResponse extends SlimeJsonResponse {
         Node node = nodeRepository.nodes().node(hostname)
                 .orElseThrow(() -> new NotFoundException("No node with hostname '" + hostname + "'"));
 
-        List<NodeAcl> acls = aclsForChildren ? nodeRepository.getChildAcls(node) :
-                                               List.of(node.acl(nodeRepository.nodes().list(), nodeRepository.loadBalancers(), nodeRepository.zone()));
+        boolean simplerAcl = Flags.SIMPLER_ACL.bindTo(nodeRepository.flagSource()).value();
+        List<NodeAcl> acls = aclsForChildren ? nodeRepository.getChildAcls(node, simplerAcl) :
+                                               List.of(node.acl(nodeRepository.nodes().list(), nodeRepository.loadBalancers(), nodeRepository.zone(), simplerAcl));
 
         Cursor trustedNodesArray = object.setArray("trustedNodes");
         acls.forEach(nodeAcl -> toSlime(nodeAcl, trustedNodesArray));
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
index 26478d2b566..e3f67721eb5 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
@@ -130,7 +130,7 @@ public class MockNodeRepository extends NodeRepository {
                 .status(Status.initial()
                         .withVespaVersion(new Version("1.2.3"))
                         .withContainerImage(DockerImage.fromString("docker-registry.domain.tld:8080/dist/vespa:1.2.3")))
-                .cloudAccount(defaultCloudAccount)
+                .cloudAccount(tenantAccount)
                 .build();
         nodes.add(node5);
 
@@ -175,9 +175,11 @@ public class MockNodeRepository extends NodeRepository {
 
         // Config servers
         nodes.add(Node.create("cfg1", ipConfig(201), "cfg1.yahoo.com", flavors.getFlavorOrThrow("default"), NodeType.config)
-                          .wireguardPubKey(WireguardKey.from("lololololololololololololololololololololoo=")).build());
+                      .cloudAccount(defaultCloudAccount)
+                      .wireguardPubKey(WireguardKey.from("lololololololololololololololololololololoo=")).build());
         nodes.add(Node.create("cfg2", ipConfig(202), "cfg2.yahoo.com", flavors.getFlavorOrThrow("default"), NodeType.config)
-                          .build());
+                      .cloudAccount(defaultCloudAccount)
+                      .build());
 
         // Ready all nodes, except 7 and 55
         nodes = new ArrayList<>(nodes().addNodes(nodes, Agent.system));
@@ -245,8 +247,8 @@ public class MockNodeRepository extends NodeRepository {
         activate(provisioner.prepare(app3, cluster3, Capacity.from(new ClusterResources(2, 1, new NodeResources(1, 4, 100, 1)), false, true), null), app3, provisioner);
 
         List<Node> largeNodes = new ArrayList<>();
-        largeNodes.add(Node.create("node13", ipConfig(13), "host13.yahoo.com", resources(10, 48, 500, 1, fast, local), NodeType.tenant).build());
-        largeNodes.add(Node.create("node14", ipConfig(14), "host14.yahoo.com", resources(10, 48, 500, 1, fast, local), NodeType.tenant).build());
+        largeNodes.add(Node.create("node13", ipConfig(13), "host13.yahoo.com", resources(10, 48, 500, 1, fast, local), NodeType.tenant).cloudAccount(defaultCloudAccount).build());
+        largeNodes.add(Node.create("node14", ipConfig(14), "host14.yahoo.com", resources(10, 48, 500, 1, fast, local), NodeType.tenant).cloudAccount(defaultCloudAccount).build());
         nodes().addNodes(largeNodes, Agent.system);
         largeNodes.forEach(node -> nodes().setReady(new NodeMutex(node, () -> {}), Agent.system, getClass().getSimpleName()));
         ApplicationId app4 = ApplicationId.from(TenantName.from("tenant4"), ApplicationName.from("application4"), InstanceName.from("instance4"));