diff options
author | Bjørn Christian Seime <bjorn.christian@seime.no> | 2018-04-25 13:48:10 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-04-25 13:48:10 +0200 |
commit | e2d4b112ef1308ea1e03c22e91e8dae561071f81 (patch) | |
tree | a0c61bd39868b2f32eed7b48c411058372b56bfd | |
parent | e050d9611ae75ef0d887d1a34593b08a6c85d4ab (diff) | |
parent | 1ff6812d0b8c012129439307eb486fda763fc8d4 (diff) |
Merge pull request #5694 from vespa-engine/bjorncs/json-security-filter-base
Bjorncs/json security filter base
10 files changed, 192 insertions, 52 deletions
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java index e3df55a9c85..5166f53c6d2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java @@ -58,7 +58,7 @@ public class AthenzPrincipalFilter extends CorsRequestFilterBase { } @Override - public Optional<ErrorResponse> filter(DiscFilterRequest request) { + public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) { try { Optional<AthenzPrincipal> certificatePrincipal = getClientCertificate(request) .map(AthenzIdentities::from) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java index 909051dcefc..910cf05b156 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java @@ -48,7 +48,7 @@ public class UserAuthWithAthenzPrincipalFilter extends AthenzPrincipalFilter { } @Override - public Optional<ErrorResponse> filter(DiscFilterRequest request) { + public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) { if (request.getMethod().equals("OPTIONS")) return Optional.empty(); // Skip authentication on OPTIONS - required for Javascript CORS try { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java index 41b4091f836..0b1b88c4389 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java @@ -78,7 +78,7 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase { // NOTE: Be aware of the ordering of the path pattern matching. Semantics may change if the patterns are evaluated // in different order. @Override - public Optional<ErrorResponse> filter(DiscFilterRequest request) { + public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) { Method method = getMethod(request); if (isWhiteListedMethod(method)) return Optional.empty(); 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 22d487628e7..c7a3cf76085 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 @@ -746,7 +746,7 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", POST) .userIdentity(unauthorizedUser) .nToken(N_TOKEN), - "{\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", + "{\n \"code\" : 403,\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", 403); // (Create it with the right tenant id) @@ -761,13 +761,13 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-west-1/instance/default/deploy", POST) .data(entity) .userIdentity(USER_ID), - "{\n \"message\" : \"'user.myuser' is not a Screwdriver identity. Only Screwdriver is allowed to deploy to this environment.\"\n}", + "{\n \"code\" : 403,\n \"message\" : \"'user.myuser' is not a Screwdriver identity. Only Screwdriver is allowed to deploy to this environment.\"\n}", 403); // Deleting an application for an Athens domain the user is not admin for is disallowed tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", DELETE) .userIdentity(unauthorizedUser), - "{\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", + "{\n \"code\" : 403,\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", 403); // (Deleting it with the right tenant id) @@ -781,7 +781,7 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request("/application/v4/tenant/tenant1", PUT) .data("{\"athensDomain\":\"domain1\", \"property\":\"property1\"}") .userIdentity(unauthorizedUser), - "{\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", + "{\n \"code\" : 403,\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", 403); // Change Athens domain @@ -796,7 +796,7 @@ public class ApplicationApiTest extends ControllerContainerTest { // Deleting a tenant for an Athens domain the user is not admin for is disallowed tester.assertResponse(request("/application/v4/tenant/tenant1", DELETE) .userIdentity(unauthorizedUser), - "{\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", + "{\n \"code\" : 403,\n \"message\" : \"Tenant admin or Vespa operator role required\"\n}", 403); } diff --git a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBase.java b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBase.java new file mode 100644 index 00000000000..e2440bc4c5f --- /dev/null +++ b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBase.java @@ -0,0 +1,86 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.jdisc.http.filter.security.base; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; +import com.yahoo.jdisc.Response; +import com.yahoo.jdisc.handler.FastContentWriter; +import com.yahoo.jdisc.handler.ResponseDispatch; +import com.yahoo.jdisc.handler.ResponseHandler; +import com.yahoo.jdisc.http.filter.DiscFilterRequest; +import com.yahoo.jdisc.http.filter.SecurityRequestFilter; + +import java.io.UncheckedIOException; +import java.util.Optional; + +/** + * A base class for {@link SecurityRequestFilter} implementations that renders an error response as JSON. + * + * @author bjorncs + */ +public abstract class JsonSecurityRequestFilterBase implements SecurityRequestFilter { + + private static final ObjectMapper mapper = new ObjectMapper(); + + @Override + public final void filter(DiscFilterRequest request, ResponseHandler handler) { + filter(request) + .ifPresent(errorResponse -> writeResponse(errorResponse, handler)); + } + + protected abstract Optional<ErrorResponse> filter(DiscFilterRequest request); + + private void writeResponse(ErrorResponse error, ResponseHandler responseHandler) { + ObjectNode errorMessage = mapper.createObjectNode(); + errorMessage.put("code", error.errorCode); + errorMessage.put("message", error.message); + error.response.headers().put("Content-Type", "application/json"); // Note: Overwrites header if already exists + try (FastContentWriter writer = ResponseDispatch.newInstance(error.response).connectFastWriter(responseHandler)) { + writer.write(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(errorMessage)); + } catch (JsonProcessingException e) { + throw new UncheckedIOException(e); + } + } + + /** + * An error response that contains a {@link Response}, error code and message. + * The error code and message will be rendered as JSON fields with name 'code' and 'message' respectively. + */ + protected static class ErrorResponse { + private final Response response; + private final int errorCode; + private final String message; + + public ErrorResponse(Response response, int errorCode, String message) { + this.response = response; + this.errorCode = errorCode; + this.message = message; + } + + public ErrorResponse(Response response, String message) { + this(response, response.getStatus(), message); + } + + public ErrorResponse(int httpStatusCode, int errorCode, String message) { + this(new Response(httpStatusCode), errorCode, message); + } + + public ErrorResponse(int httpStatusCode, String message) { + this(new Response(httpStatusCode), message); + } + + public Response getResponse() { + return response; + } + + public int getErrorCode() { + return errorCode; + } + + public String getMessage() { + return message; + } + + } +} diff --git a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/package-info.java b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/package-info.java new file mode 100644 index 00000000000..38f7b126443 --- /dev/null +++ b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/package-info.java @@ -0,0 +1,8 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +/** + * @author bjorncs + */ +@ExportPackage +package com.yahoo.jdisc.http.filter.security.base; + +import com.yahoo.osgi.annotation.ExportPackage;
\ No newline at end of file diff --git a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java index 7bdbd7eddf4..eafe17153ad 100644 --- a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java +++ b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java @@ -1,15 +1,9 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jdisc.http.filter.security.cors; -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ObjectNode; import com.yahoo.jdisc.Response; -import com.yahoo.jdisc.handler.FastContentWriter; -import com.yahoo.jdisc.handler.ResponseDispatch; -import com.yahoo.jdisc.handler.ResponseHandler; import com.yahoo.jdisc.http.filter.DiscFilterRequest; -import com.yahoo.jdisc.http.filter.SecurityRequestFilter; +import com.yahoo.jdisc.http.filter.security.base.JsonSecurityRequestFilterBase; import java.util.HashSet; import java.util.Optional; @@ -23,9 +17,7 @@ import static com.yahoo.jdisc.http.filter.security.cors.CorsLogic.createCorsResp * * @author bjorncs */ -public abstract class CorsRequestFilterBase implements SecurityRequestFilter { - - private static final ObjectMapper mapper = new ObjectMapper(); +public abstract class CorsRequestFilterBase extends JsonSecurityRequestFilterBase { private final Set<String> allowedUrls; @@ -38,44 +30,17 @@ public abstract class CorsRequestFilterBase implements SecurityRequestFilter { } @Override - public final void filter(DiscFilterRequest request, ResponseHandler handler) { - filter(request) - .ifPresent(errorResponse -> sendErrorResponse(request, errorResponse, handler)); + public final Optional<ErrorResponse> filter(DiscFilterRequest request) { + Optional<ErrorResponse> errorResponse = filterRequest(request); + errorResponse.ifPresent(response -> addCorsHeaders(request, response.getResponse())); + return errorResponse; } - protected abstract Optional<ErrorResponse> filter(DiscFilterRequest request); - - private void sendErrorResponse(DiscFilterRequest request, - ErrorResponse errorResponse, - ResponseHandler responseHandler) { - Response response = new Response(errorResponse.statusCode); - addHeaders(request, response); - writeResponse(errorResponse, responseHandler, response); - } + protected abstract Optional<ErrorResponse> filterRequest(DiscFilterRequest request); - private void addHeaders(DiscFilterRequest request, Response response) { + private void addCorsHeaders(DiscFilterRequest request, Response response) { createCorsResponseHeaders(request.getHeader("Origin"), allowedUrls) .forEach(response.headers()::add); - response.headers().add("Content-Type", "application/json"); } - private void writeResponse(ErrorResponse errorResponse, ResponseHandler responseHandler, Response response) { - ObjectNode errorMessage = mapper.createObjectNode(); - errorMessage.put("message", errorResponse.message); - try (FastContentWriter writer = ResponseDispatch.newInstance(response).connectFastWriter(responseHandler)) { - writer.write(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(errorMessage)); - } catch (JsonProcessingException e) { - throw new RuntimeException(e); - } - } - - protected static class ErrorResponse { - final int statusCode; - final String message; - - public ErrorResponse(int statusCode, String message) { - this.statusCode = statusCode; - this.message = message; - } - } } diff --git a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBaseTest.java b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBaseTest.java new file mode 100644 index 00000000000..9b0a875a73d --- /dev/null +++ b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBaseTest.java @@ -0,0 +1,57 @@ +package com.yahoo.jdisc.http.filter.security.base; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.yahoo.container.jdisc.RequestHandlerTestDriver; +import com.yahoo.jdisc.Response; +import com.yahoo.jdisc.http.filter.DiscFilterRequest; +import org.junit.Test; + +import java.io.IOException; +import java.util.Optional; + +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; + +/** + * @author bjorncs + */ +public class JsonSecurityRequestFilterBaseTest { + + private final ObjectMapper mapper = new ObjectMapper(); + + @Test + public void filter_renders_errors_as_json() throws IOException { + int statusCode = 403; + String message = "Forbidden"; + DiscFilterRequest request = mock(DiscFilterRequest.class); + SimpleSecurityRequestFilter filter = + new SimpleSecurityRequestFilter(new JsonSecurityRequestFilterBase.ErrorResponse(statusCode, message)); + RequestHandlerTestDriver.MockResponseHandler responseHandler = new RequestHandlerTestDriver.MockResponseHandler(); + filter.filter(request, responseHandler); + + Response response = responseHandler.getResponse(); + assertThat(response, notNullValue()); + assertThat(response.getStatus(), equalTo(statusCode)); + + JsonNode jsonNode = mapper.readTree(responseHandler.readAll()); + assertThat(jsonNode.get("message").asText(), equalTo(message)); + assertThat(jsonNode.get("code").asInt(), equalTo(statusCode)); + } + + private static class SimpleSecurityRequestFilter extends JsonSecurityRequestFilterBase { + private final ErrorResponse errorResponse; + + SimpleSecurityRequestFilter(ErrorResponse errorResponse) { + this.errorResponse = errorResponse; + } + + @Override + protected Optional<ErrorResponse> filter(DiscFilterRequest request) { + return Optional.ofNullable(this.errorResponse); + } + } + +}
\ No newline at end of file diff --git a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java index 29d28499a28..2cb25bc93cb 100644 --- a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java +++ b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java @@ -52,7 +52,7 @@ public class CorsRequestFilterBaseTest { } @Override - protected Optional<ErrorResponse> filter(DiscFilterRequest request) { + protected Optional<ErrorResponse> filterRequest(DiscFilterRequest request) { return Optional.ofNullable(this.errorResponse); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java index cc95945c495..62d161a20df 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision.restapi.v2.filter; import java.security.Principal; import java.security.cert.X509Certificate; import java.util.List; +import java.util.Objects; import java.util.Optional; /** @@ -66,4 +67,27 @@ public class NodePrincipal implements Principal { public enum Type { ATHENZ, LEGACY } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + NodePrincipal principal = (NodePrincipal) o; + return Objects.equals(identityName, principal.identityName) && + Objects.equals(hostname, principal.hostname) && + type == principal.type; + } + + @Override + public int hashCode() { + return Objects.hash(identityName, hostname, type); + } + + @Override + public String toString() { + return "NodePrincipal{" + + "identityName='" + identityName + '\'' + + ", hostname='" + hostname + '\'' + + ", type=" + type + + '}'; + } } |