Merge pull request #5187 from vespa-engine/mpolden/orchestrator-authorization

Authorize orchestrator APIs

3 files changed, 128 insertions, 34 deletions

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java
index 993c87f63b5..2d97bbdc494 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/filter/SecurityRequestFilterChain.java
@@ -69,7 +69,7 @@ public final class SecurityRequestFilterChain extends AbstractResource implement
         }
     }
 
-    /** Returns an unmodifiable viuew of the filters in this */
+    /** Returns an unmodifiable view of the filters in this */
     public List<SecurityRequestFilter> getFilters() {
         return Collections.unmodifiableList(filters);
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java
index 8ea26d053e3..55a1a1e620d 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/Authorizer.java
@@ -9,15 +9,20 @@ import org.apache.http.client.utils.URLEncodedUtils;
 
 import java.net.URI;
 import java.nio.charset.StandardCharsets;
-import java.nio.file.Paths;
 import java.security.Principal;
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 import java.util.function.BiPredicate;
 import java.util.stream.Collectors;
 
 /**
- * An authorizer for the node-repository REST API. This contains the authorization rules for all API paths in this
- * module.
+ * Authorizer for the node-repository and orchestrator REST APIs. This contains the authorization rules for all API
+ * paths.
+ *
+ * Ideally, the authorization rules for orchestrator APIs should live in the orchestrator module. However, the node
+ * repository is required to make decisions in some cases, which is not accessible in the orchestrator module.
  *
  * @author mpolden
  */
@@ -40,12 +45,7 @@ public class Authorizer implements BiPredicate<Principal, URI> {
         }
 
         // Nodes can only access its own resources
-        if (isNodeResource(uri) && canAccess(hostnameFrom(uri), principal)) {
-            return true;
-        }
-
-        // For resources that support filtering, nodes can only apply filter to themselves and their children
-        if (supportsFiltering(uri) && canAccess(hostnamesFrom(uri), principal)) {
+        if (canAccess(hostnamesFrom(uri), principal)) {
             return true;
         }
 
@@ -54,10 +54,17 @@ public class Authorizer implements BiPredicate<Principal, URI> {
 
     /** Returns whether principal can access node identified by hostname */
     private boolean canAccess(String hostname, Principal principal) {
+        // Ignore potential path traversal. Node repository happily passes arguments unsanitized all the way down to
+        // curator...
+        if (hostname.chars().allMatch(c -> c == '.')) {
+            return false;
+        }
+
         // Node can always access itself
         if (principal.getName().equals(hostname)) {
             return true;
         }
+
         // Parent node can access its children
         return nodeRepository.getNode(hostname)
                              .flatMap(Node::parentHostname)
@@ -78,13 +85,8 @@ public class Authorizer implements BiPredicate<Principal, URI> {
         return "vespa.vespa.hosting";
     }
 
-    /** Returns the last element (basename) of given path */
-    private static String hostnameFrom(URI uri) {
-        return Paths.get(uri.getPath()).getFileName().toString();
-    }
-
     /** Returns hostnames contained in query parameters of given URI */
-    private static List<String> hostnamesFrom(URI uri) {
+    private static List<String> hostnamesFromQuery(URI uri) {
         return URLEncodedUtils.parse(uri, StandardCharsets.UTF_8.name())
                               .stream()
                               .filter(pair -> "hostname".equals(pair.getName()) ||
@@ -94,17 +96,29 @@ public class Authorizer implements BiPredicate<Principal, URI> {
                               .collect(Collectors.toList());
     }
 
-    /** Returns whether given URI is a node-specific resource, e.g. /nodes/v2/node/node1.fqdn */
-    private static boolean isNodeResource(URI uri) {
-        return isChildOf("/nodes/v2/acl/", uri.getPath()) ||
-               isChildOf("/nodes/v2/node/", uri.getPath()) ||
-               isChildOf("/nodes/v2/state/", uri.getPath());
-    }
-
-    /** Returns whether given path supports filtering through query parameters */
-    private static boolean supportsFiltering(URI uri) {
-        return isChildOf("/nodes/v2/command/", uri.getPath()) ||
-               "/nodes/v2/node/".equals(uri.getPath());
+    /** Returns hostnames from a URI if any, e.g. /nodes/v2/node/node1.fqdn */
+    private static List<String> hostnamesFrom(URI uri) {
+        if (isChildOf("/nodes/v2/acl/", uri.getPath()) ||
+            isChildOf("/nodes/v2/node/", uri.getPath()) ||
+            isChildOf("/nodes/v2/state/", uri.getPath())) {
+            return Collections.singletonList(lastChildOf(uri.getPath()));
+        }
+        if (isChildOf("/orchestrator/v1/hosts/", uri.getPath())) {
+            return firstChildOf("/orchestrator/v1/hosts/", uri.getPath())
+                    .map(Collections::singletonList)
+                    .orElseGet(Collections::emptyList);
+        }
+        if (isChildOf("/orchestrator/v1/suspensions/hosts/", uri.getPath())) {
+            List<String> hostnames = new ArrayList<>();
+            hostnames.add(lastChildOf(uri.getPath()));
+            hostnames.addAll(hostnamesFromQuery(uri));
+            return hostnames;
+        }
+        if (isChildOf("/nodes/v2/command/", uri.getPath()) ||
+            "/nodes/v2/node/".equals(uri.getPath())) {
+            return hostnamesFromQuery(uri);
+        }
+        return Collections.emptyList();
     }
 
     /** Returns whether child is a sub-path of parent */
@@ -112,4 +126,29 @@ public class Authorizer implements BiPredicate<Principal, URI> {
         return child.startsWith(parent) && child.length() > parent.length();
     }
 
+    /** Returns the first component of path relative to root */
+    private static Optional<String> firstChildOf(String root, String path) {
+        if (!isChildOf(root, path)) {
+            return Optional.empty();
+        }
+        path = path.substring(root.length(), path.length());
+        int firstSeparator = path.indexOf('/');
+        if (firstSeparator == -1) {
+            return Optional.of(path);
+        }
+        return Optional.of(path.substring(0, firstSeparator));
+    }
+
+    /** Returns the last component of the given path */
+    private static String lastChildOf(String path) {
+        if (path.endsWith("/")) {
+            path = path.substring(0, path.length() - 1);
+        }
+        int lastSeparator = path.lastIndexOf("/");
+        if (lastSeparator == - 1) {
+            return path;
+        }
+        return path.substring(lastSeparator + 1, path.length());
+    }
+
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java
index 7f7a26cbcb2..097c8d92165 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/AuthorizerTest.java
@@ -1,14 +1,24 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.restapi.v2;
 
+import com.yahoo.config.provision.Flavor;
+import com.yahoo.config.provision.NodeFlavors;
+import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.vespa.curator.mock.MockCurator;
+import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.testutils.MockNodeFlavors;
 import com.yahoo.vespa.hosted.provision.testutils.MockNodeRepository;
 import org.junit.Before;
 import org.junit.Test;
 
 import java.net.URI;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -23,12 +33,30 @@ public class AuthorizerTest {
 
     @Before
     public void before() {
-        nodeRepository = new MockNodeRepository(new MockCurator(), new MockNodeFlavors());
+        NodeFlavors flavors = new MockNodeFlavors();
+        nodeRepository = new MockNodeRepository(new MockCurator(), flavors);
         authorizer = new Authorizer(SystemName.main, nodeRepository);
+        { // Populate with nodes used in this test. Note that only nodes requiring node repository lookup are added here
+            Set<String> ipAddresses = new HashSet<>(Arrays.asList("127.0.0.1", "::1"));
+            Flavor flavor = flavors.getFlavorOrThrow("default");
+            List<Node> nodes = new ArrayList<>();
+            nodes.add(nodeRepository.createNode("host1", "host1", ipAddresses,
+                                                Optional.empty(), flavor, NodeType.host));
+            nodes.add(nodeRepository.createNode("child1-1", "child1-1", ipAddresses,
+                                                Optional.of("host1"), flavor, NodeType.tenant));
+            nodes.add(nodeRepository.createNode("child1-2", "child1-2", ipAddresses,
+                                                Optional.of("host1"), flavor, NodeType.tenant));
+
+            nodes.add(nodeRepository.createNode("host2", "host2", ipAddresses,
+                                                Optional.empty(), flavor, NodeType.host));
+            nodes.add(nodeRepository.createNode("child2-1", "child2-1", ipAddresses,
+                                                Optional.of("host1.tld"), flavor, NodeType.tenant));
+            nodeRepository.addNodes(nodes);
+        }
     }
 
     @Test
-    public void authorization() {
+    public void nodes_authorization() {
         // Empty principal
         assertFalse(authorized("", ""));
         assertFalse(authorized("", "/"));
@@ -41,6 +69,9 @@ public class AuthorizerTest {
         assertFalse(authorized("node1", "/nodes/v2/node/node2"));
         assertFalse(authorized("node1", "/nodes/v2/state/dirty/"));
         assertFalse(authorized("node1", "/nodes/v2/state/dirty/node2"));
+        // Path traversal fails gracefully
+        assertFalse(authorized("node1", "/nodes/v2/node/."));
+        assertFalse(authorized("node1", "/nodes/v2/node/.."));
         assertFalse(authorized("node1", "/nodes/v2/acl/node2"));
         assertFalse(authorized("node1", "/nodes/v2/node/?parentHost=node2"));
         // Node resource always takes precedence over filter
@@ -55,11 +86,11 @@ public class AuthorizerTest {
         assertTrue(authorized("node1", "/nodes/v2/node/?parentHost=node1"));
 
         // Host node can access itself and its children
-        assertFalse(authorized("dockerhost1.yahoo.com", "/nodes/v2/node/host5.yahoo.com"));
-        assertFalse(authorized("dockerhost1.yahoo.com", "/nodes/v2/command/reboot?hostname=host5.yahoo.com"));
-        assertTrue(authorized("dockerhost1.yahoo.com", "/nodes/v2/node/dockerhost1.yahoo.com"));
-        assertTrue(authorized("dockerhost1.yahoo.com", "/nodes/v2/node/host4.yahoo.com"));
-        assertTrue(authorized("dockerhost1.yahoo.com", "/nodes/v2/command/reboot?hostname=host4.yahoo.com"));
+        assertFalse(authorized("host1", "/nodes/v2/node/child2-1"));
+        assertFalse(authorized("host1", "/nodes/v2/command/reboot?hostname=child2-1"));
+        assertTrue(authorized("host1", "/nodes/v2/node/host1"));
+        assertTrue(authorized("host1", "/nodes/v2/node/child1-1"));
+        assertTrue(authorized("host1", "/nodes/v2/command/reboot?hostname=child1-1"));
 
         // Trusted services can access everything in their own system
         assertFalse(authorized("vespa.vespa.cd.hosting", "/")); // Wrong system
@@ -69,6 +100,30 @@ public class AuthorizerTest {
         assertTrue(authorized("vespa.vespa.hosting", "/nodes/v2/node/node1"));
     }
 
+    @Test
+    public void orchestrator_authorization() {
+        // Node can only access its own resources
+        assertFalse(authorized("node1", "/orchestrator/v1/hosts"));
+        assertFalse(authorized("node1", "/orchestrator/v1/hosts/"));
+        assertFalse(authorized("node1", "/orchestrator/v1/hosts/node2"));
+        assertFalse(authorized("node1", "/orchestrator/v1/hosts/node2/suspended"));
+
+        // Node can suspend itself
+        assertTrue(authorized("node1", "/orchestrator/v1/hosts/node1"));
+        assertTrue(authorized("node1", "/orchestrator/v1/hosts/node1/suspended"));
+
+        // Host node can suspend itself and its children
+        assertFalse(authorized("host1", "/orchestrator/v1/hosts/child2-1/suspended"));
+        assertFalse(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child2-1"));
+        // All given hostnames must be children
+        assertFalse(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child1-1&hostname=child2-1"));
+        assertTrue(authorized("host1", "/orchestrator/v1/hosts/host1/suspended"));
+        assertTrue(authorized("host1", "/orchestrator/v1/hosts/child1-1/suspended"));
+        assertTrue(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child1-1"));
+        // Multiple children
+        assertTrue(authorized("host1", "/orchestrator/v1/suspensions/hosts/host1?hostname=child1-1&hostname=child1-2"));
+    }
+
     private boolean authorized(String principal, String path) {
         return authorizer.test(() -> principal, uri(path));
     }