Make SignatureFilter skip requests with roles, and without signatures

2 files changed, 46 insertions, 37 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilter.java
index 41109d89bab..5cf29179d2a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilter.java
@@ -4,6 +4,8 @@ import ai.vespa.hosted.api.Method;
 import ai.vespa.hosted.api.RequestVerifier;
 import com.google.inject.Inject;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.ApplicationName;
+import com.yahoo.config.provision.TenantName;
 import com.yahoo.jdisc.Response;
 import com.yahoo.jdisc.http.filter.DiscFilterRequest;
 import com.yahoo.jdisc.http.filter.security.base.JsonSecurityRequestFilterBase;
@@ -21,9 +23,11 @@ import java.util.Set;
 import java.util.logging.Logger;
 
 /**
- * Accepts or rejects HTTP requests based on whether a signature header matches an implied public key.
+ * Assigns the {@link Role#buildService(TenantName, ApplicationName)} role to requests with a
+ * Authorization header signature matching the public key of the indicated application.
+ * Requests which already have a set of roles assigned to them are not modified.
  *
- * Requests that are approved are enriched with a {@link RoleDefinition#buildService} role.
+ * @author jonmv
  */
 public class SignatureFilter extends JsonSecurityRequestFilterBase {
 
@@ -38,31 +42,29 @@ public class SignatureFilter extends JsonSecurityRequestFilterBase {
 
     @Override
     protected Optional<ErrorResponse> filter(DiscFilterRequest request) {
-        try {
-            ApplicationId id = ApplicationId.fromSerializedForm(request.getHeader("X-Key-Id"));
-            boolean verified = controller.applications().get(id)
-                                         .flatMap(Application::pemDeployKey)
-                                         .map(key -> new RequestVerifier(key, controller.clock()))
-                                         .map(verifier -> verifier.verify(Method.valueOf(request.getMethod()),
-                                                                          request.getUri(),
-                                                                          request.getHeader("X-Timestamp"),
-                                                                          request.getHeader("X-Content-Hash"),
-                                                                          request.getHeader("X-Authorization")))
-                                         .orElse(false);
+        if (   request.getAttribute(SecurityContext.ATTRIBUTE_NAME) == null
+            && request.getHeader("X-Authorization") != null)
+            try {
+                ApplicationId id = ApplicationId.fromSerializedForm(request.getHeader("X-Key-Id"));
+                boolean verified = controller.applications().get(id)
+                                             .flatMap(Application::pemDeployKey)
+                                             .map(key -> new RequestVerifier(key, controller.clock()))
+                                             .map(verifier -> verifier.verify(Method.valueOf(request.getMethod()),
+                                                                              request.getUri(),
+                                                                              request.getHeader("X-Timestamp"),
+                                                                              request.getHeader("X-Content-Hash"),
+                                                                              request.getHeader("X-Authorization")))
+                                             .orElse(false);
 
-            if (verified) {
-                request.setAttribute(SecurityContext.ATTRIBUTE_NAME,
-                                     new SecurityContext(() -> "buildService@" + id.tenant() + "." + id.application(),
-                                                         Set.of(Role.buildService(id.tenant(), id.application()))));
-                return Optional.empty();
+                if (verified)
+                    request.setAttribute(SecurityContext.ATTRIBUTE_NAME,
+                                         new SecurityContext(() -> "buildService@" + id.tenant() + "." + id.application(),
+                                                             Set.of(Role.buildService(id.tenant(), id.application()))));
             }
-        }
-        catch (Exception e) {
-            logger.log(LogLevel.DEBUG, () -> "Exception verifying signed request: " + Exceptions.toMessageString(e));
-        }
-        return Optional.of(new ErrorResponse(Response.Status.UNAUTHORIZED, "Access denied"));
+            catch (Exception e) {
+                logger.log(LogLevel.DEBUG, () -> "Exception verifying signed request: " + Exceptions.toMessageString(e));
+            }
+        return Optional.empty();
     }
 
-
-
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilterTest.java
index 95ebbb7362b..bf44481c110 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/SignatureFilterTest.java
@@ -18,7 +18,7 @@ import java.net.http.HttpRequest;
 import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 public class SignatureFilterTest {
@@ -61,34 +61,41 @@ public class SignatureFilterTest {
 
     @Test
     public void testFilter() {
-        // Unsigned request is rejected.
+        // Unsigned request gets no role.
         HttpRequest.Builder request = HttpRequest.newBuilder(URI.create("https://host:123/path/./..//..%2F?query=empty&%3F=%26"));
         byte[] emptyBody = new byte[0];
         DiscFilterRequest unsigned = requestOf(request.method("GET", HttpRequest.BodyPublishers.ofByteArray(emptyBody)).build(), emptyBody);
-        assertFalse(filter.filter(unsigned).isEmpty());
+        filter.filter(unsigned);
+        assertNull(unsigned.getAttribute(SecurityContext.ATTRIBUTE_NAME));
 
-        // Signed request is rejected when no key is stored for the application.
+        // Signed request gets no role when no key is stored for the application.
         DiscFilterRequest signed = requestOf(signer.signed(request, Method.GET), emptyBody);
-        assertFalse(filter.filter(signed).isEmpty());
+        filter.filter(signed);
+        assertNull(signed.getAttribute(SecurityContext.ATTRIBUTE_NAME));
 
-        // Signed request is rejected when a non-matching key is stored for the application.
+        // Signed request gets no role when a non-matching key is stored for the application.
         applications.lockOrThrow(id, application -> applications.store(application.withPemDeployKey(otherPublicKey)));
-        assertFalse(filter.filter(signed).isEmpty());
+        filter.filter(signed);
+        assertNull(signed.getAttribute(SecurityContext.ATTRIBUTE_NAME));
 
-        // Signed request is accepted when a matching key is stored for the application.
+        // Signed request gets a build service role when a matching key is stored for the application.
         applications.lockOrThrow(id, application -> applications.store(application.withPemDeployKey(publicKey)));
         assertTrue(filter.filter(signed).isEmpty());
         SecurityContext securityContext = (SecurityContext) signed.getAttribute(SecurityContext.ATTRIBUTE_NAME);
         assertEquals("buildService@my-tenant.my-app", securityContext.principal().getName());
         assertEquals(Set.of(Role.buildService(id.tenant(), id.application())), securityContext.roles());
 
-        // Signed POST request is also accepted.
+        // Signed POST request also gets a build service role.
         byte[] hiBytes = new byte[]{0x48, 0x69};
         signed = requestOf(signer.signed(request, Method.POST), hiBytes);
-        assertTrue(filter.filter(signed).isEmpty());
+        filter.filter(signed);
+        securityContext = (SecurityContext) signed.getAttribute(SecurityContext.ATTRIBUTE_NAME);
+        assertEquals("buildService@my-tenant.my-app", securityContext.principal().getName());
+        assertEquals(Set.of(Role.buildService(id.tenant(), id.application())), securityContext.roles());
 
-        // Unsigned requests are still rejected.
-        assertFalse(filter.filter(unsigned).isEmpty());
+        // Unsigned requests still get no roles.
+        filter.filter(unsigned);
+        assertNull(unsigned.getAttribute(SecurityContext.ATTRIBUTE_NAME));
     }
 
     private static DiscFilterRequest requestOf(HttpRequest request, byte[] body) {