Merge pull request #3933 from vespa-engine/bjorncs/remove-deprecated-authorization

Bjorncs/remove deprecated authorization

5 files changed, 51 insertions, 88 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/ErrorResponse.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/ErrorResponse.java
index a9643e21c00..4af833baa98 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/ErrorResponse.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/ErrorResponse.java
@@ -11,6 +11,7 @@ import static com.yahoo.jdisc.Response.Status.FORBIDDEN;
 import static com.yahoo.jdisc.Response.Status.INTERNAL_SERVER_ERROR;
 import static com.yahoo.jdisc.Response.Status.METHOD_NOT_ALLOWED;
 import static com.yahoo.jdisc.Response.Status.NOT_FOUND;
+import static com.yahoo.jdisc.Response.Status.UNAUTHORIZED;
 
 /**
  * A HTTP JSON response containing an error code and a message
@@ -24,7 +25,8 @@ public class ErrorResponse extends SlimeJsonResponse {
         BAD_REQUEST,
         FORBIDDEN,
         METHOD_NOT_ALLOWED,
-        INTERNAL_SERVER_ERROR
+        INTERNAL_SERVER_ERROR,
+        UNAUTHORIZED
     }
 
     public ErrorResponse(int statusCode, String errorType, String message) {
@@ -55,6 +57,10 @@ public class ErrorResponse extends SlimeJsonResponse {
         return new ErrorResponse(FORBIDDEN, errorCodes.FORBIDDEN.name(), message);
     }
 
+    public static ErrorResponse unauthorized(String message) {
+        return new ErrorResponse(UNAUTHORIZED, errorCodes.UNAUTHORIZED.name(), message);
+    }
+
     public static ErrorResponse methodNotAllowed(String message) {
         return new ErrorResponse(METHOD_NOT_ALLOWED, errorCodes.METHOD_NOT_ALLOWED.name(), message);
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
index a6d9f4c5d33..a259e221a1e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
@@ -67,7 +67,6 @@ import com.yahoo.vespa.hosted.controller.application.DeploymentMetrics;
 import com.yahoo.vespa.hosted.controller.application.JobStatus;
 import com.yahoo.vespa.hosted.controller.application.SourceRevision;
 import com.yahoo.vespa.hosted.controller.athenz.AthenzClientFactory;
-import com.yahoo.vespa.hosted.controller.athenz.AthenzPrincipal;
 import com.yahoo.vespa.hosted.controller.athenz.NToken;
 import com.yahoo.vespa.hosted.controller.athenz.ZmsException;
 import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse;
@@ -82,6 +81,7 @@ import com.yahoo.yolean.Exceptions;
 
 import javax.ws.rs.BadRequestException;
 import javax.ws.rs.ForbiddenException;
+import javax.ws.rs.NotAuthorizedException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URI;
@@ -140,6 +140,9 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
         catch (ForbiddenException e) {
             return ErrorResponse.forbidden(Exceptions.toMessageString(e));
         }
+        catch (NotAuthorizedException e) {
+            return ErrorResponse.unauthorized(Exceptions.toMessageString(e));
+        }
         catch (NotExistsException e) {
             return ErrorResponse.notFoundError(Exceptions.toMessageString(e));
         }
@@ -780,25 +783,8 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
         DeployAuthorizer deployAuthorizer = new DeployAuthorizer(controller.zoneRegistry(), athenzClientFactory);
         Tenant tenant = controller.tenants().tenant(new TenantId(tenantName)).orElseThrow(() -> new NotExistsException(new TenantId(tenantName)));
         Principal principal = authorizer.getPrincipal(request);
-        if (principal instanceof AthenzPrincipal) {
-            deployAuthorizer.throwIfUnauthorizedForDeploy(principal,
-                                                          Environment.from(environment),
-                                                          tenant,
-                                                          applicationId);
-        } else { // In case of host-based principal
-            // TODO What about other user type principals like Bouncer?
-            log.log(LogLevel.WARNING,
-                    "Using deprecated DeployAuthorizer.throwIfUnauthorizedForDeploy. Principal=" + principal);
-            UserId userId = new UserId(principal.getName());
-            deployAuthorizer.throwIfUnauthorizedForDeploy(
-                    Environment.from(environment),
-                    userId,
-                    tenant,
-                    applicationId,
-                    optional("screwdriverBuildJob", deployOptions).map(ScrewdriverId::new));
-        }
+        deployAuthorizer.throwIfUnauthorizedForDeploy(principal, Environment.from(environment), tenant, applicationId);
 
-        
         // TODO: get rid of the json object
         DeployOptions deployOptionsJsonClass = new DeployOptions(screwdriverBuildJobFromSlime(deployOptions.field("screwdriverBuildJob")),
                                                                  optional("vespaVersion", deployOptions).map(Version::new),
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/Authorizer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/Authorizer.java
index 93dc2541385..0c808e30c2a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/Authorizer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/Authorizer.java
@@ -1,7 +1,6 @@
 // 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.controller.restapi.application;
 
-import com.google.common.collect.ImmutableSet;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.vespa.hosted.controller.Controller;
@@ -12,19 +11,16 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.UserGroup;
 import com.yahoo.vespa.hosted.controller.api.identifiers.UserId;
 import com.yahoo.vespa.hosted.controller.api.integration.entity.EntityService;
 import com.yahoo.vespa.hosted.controller.athenz.AthenzClientFactory;
-import com.yahoo.vespa.hosted.controller.athenz.AthenzPrincipal;
 import com.yahoo.vespa.hosted.controller.athenz.AthenzUtils;
 import com.yahoo.vespa.hosted.controller.athenz.NToken;
 import com.yahoo.vespa.hosted.controller.common.ContextAttributes;
 import com.yahoo.vespa.hosted.controller.restapi.filter.NTokenRequestFilter;
-import com.yahoo.vespa.hosted.controller.restapi.filter.UnauthenticatedUserPrincipal;
 
 import javax.ws.rs.ForbiddenException;
 import javax.ws.rs.HttpMethod;
 import javax.ws.rs.core.SecurityContext;
 import java.security.Principal;
 import java.util.Optional;
-import java.util.Set;
 import java.util.logging.Logger;
 
 
@@ -41,11 +37,6 @@ public class Authorizer {
     // Must be kept in sync with bouncer filter configuration.
     private static final String VESPA_HOSTED_ADMIN_ROLE = "10707.A";
 
-    private static final Set<UserId> SCREWDRIVER_USERS = ImmutableSet.of(new UserId("screwdrv"), 
-                                                                         new UserId("screwdriver"), 
-                                                                         new UserId("sdrvtest"), 
-                                                                         new UserId("screwdriver-test"));
-
     private final Controller controller;
     private final AthenzClientFactory athenzClientFactory;
     private final EntityService entityService;
@@ -92,17 +83,8 @@ public class Authorizer {
     }
 
     public boolean isSuperUser(HttpRequest request) {
-        // TODO Check membership of admin role in Vespa's Athenz domain
-        return isMemberOfVespaBouncerGroup(request) || isScrewdriverPrincipal(getPrincipal(request));
-    }
-
-    public static boolean isScrewdriverPrincipal(Principal principal) {
-        if (principal instanceof UnauthenticatedUserPrincipal) // Host-based authentication
-            return SCREWDRIVER_USERS.contains(new UserId(principal.getName()));
-        else if (principal instanceof AthenzPrincipal)
-            return ((AthenzPrincipal)principal).getDomain().equals(AthenzUtils.SCREWDRIVER_DOMAIN);
-        else
-            return false;
+        // TODO Replace check with membership of a dedicated 'hosted Vespa super-user' role in Vespa's Athenz domain
+        return isMemberOfVespaBouncerGroup(request);
     }
 
     private static ForbiddenException loggedForbiddenException(String message, Object... args) {
@@ -149,6 +131,8 @@ public class Authorizer {
         return method.equals(HttpMethod.GET) || method.equals(HttpMethod.HEAD) || method.equals(HttpMethod.OPTIONS);
     }
 
+    @Deprecated
+    // TODO Remove this method. Stop using Bouncer for authorization and use Athenz instead
     private boolean isMemberOfVespaBouncerGroup(HttpRequest request) {
         Optional<SecurityContext> securityContext = securityContextOf(request);
         if ( ! securityContext.isPresent() ) throw Authorizer.loggedForbiddenException("User is not authenticated");
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/DeployAuthorizer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/DeployAuthorizer.java
index 7cf19629774..71126259417 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/DeployAuthorizer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/DeployAuthorizer.java
@@ -5,23 +5,19 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.vespa.hosted.controller.api.Tenant;
 import com.yahoo.vespa.hosted.controller.api.identifiers.AthenzDomain;
-import com.yahoo.vespa.hosted.controller.api.identifiers.ScrewdriverId;
-import com.yahoo.vespa.hosted.controller.api.identifiers.UserId;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
 import com.yahoo.vespa.hosted.controller.athenz.ApplicationAction;
 import com.yahoo.vespa.hosted.controller.athenz.AthenzClientFactory;
 import com.yahoo.vespa.hosted.controller.athenz.AthenzPrincipal;
 import com.yahoo.vespa.hosted.controller.athenz.AthenzUtils;
 import com.yahoo.vespa.hosted.controller.athenz.ZmsException;
-import com.yahoo.vespa.hosted.controller.restapi.filter.UnauthenticatedUserPrincipal;
 
 import javax.ws.rs.ForbiddenException;
+import javax.ws.rs.NotAuthorizedException;
 import java.security.Principal;
-import java.util.Optional;
 import java.util.logging.Logger;
 
 import static com.yahoo.vespa.hosted.controller.restapi.application.Authorizer.environmentRequiresAuthorization;
-import static com.yahoo.vespa.hosted.controller.restapi.application.Authorizer.isScrewdriverPrincipal;
 
 /**
  * @author bjorncs
@@ -43,36 +39,40 @@ public class DeployAuthorizer {
                                              Environment environment,
                                              Tenant tenant,
                                              ApplicationId applicationId) {
-        if (athenzCredentialsRequired(environment, tenant, applicationId, principal))
-            checkAthenzCredentials(principal, tenant, applicationId);
-    }
-
-    // TODO: inline when deployment via ssh is removed
-    private boolean athenzCredentialsRequired(Environment environment, Tenant tenant, ApplicationId applicationId, Principal principal) {
-        if (!environmentRequiresAuthorization(environment))  return false;
-
-        if (! isScrewdriverPrincipal(principal))
-            throw loggedForbiddenException(
-                    "Principal '%s' is not a screwdriver principal, and does not have deploy access to application '%s'",
-                    principal.getName(), applicationId.toShortString());
+        if (!environmentRequiresAuthorization(environment)) {
+            return;
+        }
 
-        return tenant.isAthensTenant();
-    }
+        if (principal == null) {
+            throw loggedUnauthorizedException("Principal not authenticated!");
+        }
 
+        if (!(principal instanceof AthenzPrincipal)) {
+            throw loggedUnauthorizedException(
+                    "Principal '%s' of type '%s' is not an Athenz principal, which is required for production deployments.",
+                    principal.getName(), principal.getClass().getSimpleName());
+        }
 
-    // TODO: inline when deployment via ssh is removed
-    private void checkAthenzCredentials(Principal principal, Tenant tenant, ApplicationId applicationId) {
-        AthenzDomain domain = tenant.getAthensDomain().get();
-        if (! (principal instanceof AthenzPrincipal))
-            throw loggedForbiddenException("Principal '%s' is not authenticated.", principal.getName());
+        AthenzPrincipal athenzPrincipal = (AthenzPrincipal) principal;
+        AthenzDomain principalDomain = athenzPrincipal.getDomain();
 
-        AthenzPrincipal athensPrincipal = (AthenzPrincipal)principal;
-        if ( ! hasDeployAccessToAthenzApplication(athensPrincipal, domain, applicationId))
+        if (!principalDomain.equals(AthenzUtils.SCREWDRIVER_DOMAIN)) {
             throw loggedForbiddenException(
-                    "Screwdriver principal '%1$s' does not have deploy access to '%2$s'. " +
-                    "Either the application has not been created at " + zoneRegistry.getDashboardUri() + " or " +
-                    "'%1$s' is not added to the application's deployer role in Athens domain '%3$s'.",
-                    athensPrincipal, applicationId, tenant.getAthensDomain().get());
+                    "Principal '%s' is not a Screwdriver principal. Excepted principal with Athenz domain '%s', got '%s'.",
+                    principal.getName(), AthenzUtils.SCREWDRIVER_DOMAIN.id(), principalDomain.id());
+        }
+
+        // NOTE: no fine-grained deploy authorization for non-Athenz tenants
+        if (tenant.isAthensTenant()) {
+            AthenzDomain tenantDomain = tenant.getAthensDomain().get();
+            if (!hasDeployAccessToAthenzApplication(athenzPrincipal, tenantDomain, applicationId)) {
+                throw loggedForbiddenException(
+                        "Screwdriver principal '%1$s' does not have deploy access to '%2$s'. " +
+                        "Either the application has not been created at " + zoneRegistry.getDashboardUri() + " or " +
+                        "'%1$s' is not added to the application's deployer role in Athenz domain '%3$s'.",
+                        athenzPrincipal.toYRN(), applicationId, tenantDomain.id());
+            }
+        }
     }
 
     private static ForbiddenException loggedForbiddenException(String message, Object... args) {
@@ -81,23 +81,10 @@ public class DeployAuthorizer {
         return new ForbiddenException(formattedMessage);
     }
 
-    /**
-     * @deprecated Only usable for ssh. Use the method that takes Principal instead of UserId and screwdriverId.
-     */
-    @Deprecated
-    public void throwIfUnauthorizedForDeploy(Environment environment,
-                                             UserId userId,
-                                             Tenant tenant,
-                                             ApplicationId applicationId,
-                                             Optional<ScrewdriverId> optionalScrewdriverId) {
-        Principal principal = new UnauthenticatedUserPrincipal(userId.id());
-
-        if (athenzCredentialsRequired(environment, tenant, applicationId, principal)) {
-            ScrewdriverId screwdriverId = optionalScrewdriverId.orElseThrow(
-                    () -> loggedForbiddenException("Screwdriver id must be provided when deploying from Screwdriver."));
-            principal = AthenzUtils.createPrincipal(screwdriverId);
-            checkAthenzCredentials(principal, tenant, applicationId);
-        }
+    private static NotAuthorizedException loggedUnauthorizedException(String message, Object... args) {
+        String formattedMessage = String.format(message, args);
+        log.info(formattedMessage);
+        return new NotAuthorizedException(formattedMessage);
     }
 
     private boolean hasDeployAccessToAthenzApplication(AthenzPrincipal principal, AthenzDomain domain, ApplicationId applicationId) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index b36a88ca1d4..eeeed08c2b8 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -577,7 +577,7 @@ public class ApplicationApiTest extends ControllerContainerTest {
                                       entity,
                                       Request.Method.POST,
                                       athenzUserDomain, "mytenant"),
-                              "{\"error-code\":\"FORBIDDEN\",\"message\":\"Principal 'mytenant' is not a screwdriver principal, and does not have deploy access to application 'tenant1.application1'\"}", 
+                              "{\"error-code\":\"FORBIDDEN\",\"message\":\"Principal 'mytenant' is not a Screwdriver principal. Excepted principal with Athenz domain 'cd.screwdriver.project', got 'domain1'.\"}",
                               403);
 
         // Deleting an application for an Athens domain the user is not admin for is disallowed