From f5cf6f814eeeaa7b77a5312ac8c6bf7cdc4fa4c3 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Thu, 15 Jun 2023 13:10:18 +0200 Subject: Misc improvements Remove duplicate unit tests. Improve symbol names. Use `Map` to simplify code and reduce cost. Introduce constant for the number of bytes in token check hash. Improve code comments. --- .../security/cloud/CloudDataPlaneFilter.java | 29 ++++++++++++++-------- .../security/cloud/CloudDataPlaneFilterTest.java | 17 +++---------- 2 files changed, 21 insertions(+), 25 deletions(-) (limited to 'jdisc-security-filters') diff --git a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilter.java b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilter.java index b4b51fbb8dc..b2a71d2e1b9 100644 --- a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilter.java +++ b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilter.java @@ -23,8 +23,10 @@ import java.security.Principal; import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.EnumSet; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.TreeSet; @@ -47,6 +49,7 @@ import static com.yahoo.jdisc.http.server.jetty.AccessLoggingRequestHandler.CONT public class CloudDataPlaneFilter extends JsonSecurityRequestFilterBase { private static final Logger log = Logger.getLogger(CloudDataPlaneFilter.class.getName()); + static final int CHECK_HASH_BYTES = 32; private final boolean legacyMode; private final List allowedClients; @@ -100,14 +103,15 @@ public class CloudDataPlaneFilter extends JsonSecurityRequestFilterBase { throw new IllegalArgumentException( "Client '%s' contains invalid X.509 certificate PEM: %s".formatted(c.id(), e.toString()), e); } - clients.add(new Client(c.id(), permissions, certs, List.of())); + clients.add(new Client(c.id(), permissions, certs, Map.of())); hasClientRequiringCertificate = true; } else { - var tokens = new ArrayList(); + var tokens = new HashMap(); for (var token : c.tokens()) { for (int version = 0; version < token.checkAccessHashes().size(); version++) { - tokens.add(TokenVersion.of( - token.id(), token.fingerprints().get(version), token.checkAccessHashes().get(version))); + var tokenVersion = TokenVersion.of( + token.id(), token.fingerprints().get(version), token.checkAccessHashes().get(version)); + tokens.put(tokenVersion.accessHash(), tokenVersion); } } // Add reverse proxy certificate as required certificate for client definition @@ -147,7 +151,7 @@ public class CloudDataPlaneFilter extends JsonSecurityRequestFilterBase { return Optional.of(new ErrorResponse(Response.Status.FORBIDDEN, "Forbidden")); } var clientCert = certs.get(0); - var requestTokenHash = requestToken(req).orElse(null); + var requestTokenHash = requestTokenHash(req).orElse(null); var clientIds = new TreeSet(); var permissions = new TreeSet(); var matchedTokens = new HashSet(); @@ -155,8 +159,8 @@ public class CloudDataPlaneFilter extends JsonSecurityRequestFilterBase { if (!c.permissions().contains(permission)) continue; if (!c.certificates().contains(clientCert)) continue; if (!c.tokens().isEmpty()) { - var matchedToken = c.tokens().stream() - .filter(t -> t.accessHash().equals(requestTokenHash)).findAny().orElse(null); + if (requestTokenHash == null) continue; + var matchedToken = c.tokens().get(requestTokenHash); if (matchedToken == null) continue; matchedTokens.add(matchedToken); } @@ -180,11 +184,11 @@ public class CloudDataPlaneFilter extends JsonSecurityRequestFilterBase { return Optional.empty(); } - private Optional requestToken(DiscFilterRequest req) { + private Optional requestTokenHash(DiscFilterRequest req) { return Optional.ofNullable(req.getHeader("Authorization")) .filter(h -> h.startsWith("Bearer ")) .map(t -> t.substring("Bearer ".length()).trim()) - .map(t -> TokenCheckHash.of(Token.of(tokenDomain, t), 32)); + .map(t -> TokenCheckHash.of(Token.of(tokenDomain, t), CHECK_HASH_BYTES)); } private static void addAccessLogEntry(DiscFilterRequest req, String key, String value) { @@ -225,7 +229,10 @@ public class CloudDataPlaneFilter extends JsonSecurityRequestFilterBase { } } - private record Client(String id, EnumSet permissions, List certificates, List tokens) { - Client { permissions = EnumSet.copyOf(permissions); certificates = List.copyOf(certificates); tokens = List.copyOf(tokens); } + private record Client(String id, EnumSet permissions, List certificates, + Map tokens) { + Client { + permissions = EnumSet.copyOf(permissions); certificates = List.copyOf(certificates); tokens = Map.copyOf(tokens); + } } } diff --git a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilterTest.java b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilterTest.java index 2dd577c18d6..e81ef45d3af 100644 --- a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilterTest.java +++ b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cloud/CloudDataPlaneFilterTest.java @@ -27,6 +27,7 @@ import java.util.Set; import static com.yahoo.jdisc.Response.Status.FORBIDDEN; import static com.yahoo.jdisc.Response.Status.UNAUTHORIZED; +import static com.yahoo.jdisc.http.filter.security.cloud.CloudDataPlaneFilter.CHECK_HASH_BYTES; import static com.yahoo.jdisc.http.filter.security.cloud.CloudDataPlaneFilter.Permission.READ; import static com.yahoo.jdisc.http.filter.security.cloud.CloudDataPlaneFilter.Permission.WRITE; import static com.yahoo.security.KeyAlgorithm.EC; @@ -52,9 +53,9 @@ class CloudDataPlaneFilterTest { private static final String TOKEN_CONTEXT = "my-token-context"; private static final String TOKEN_ID = "my-token-id"; private static final Token VALID_TOKEN = - TokenGenerator.generateToken(TokenDomain.of("fp-ctx", TOKEN_CONTEXT), "vespa_token_", 32); + TokenGenerator.generateToken(TokenDomain.of("fp-ctx", TOKEN_CONTEXT), "vespa_token_", CHECK_HASH_BYTES); private static final Token UNKNOWN_TOKEN = - TokenGenerator.generateToken(TokenDomain.of("fp-ctx", TOKEN_CONTEXT), "vespa_token_", 32); + TokenGenerator.generateToken(TokenDomain.of("fp-ctx", TOKEN_CONTEXT), "vespa_token_", CHECK_HASH_BYTES); @Test void accepts_any_trusted_client_certificate_in_legacy_mode() { @@ -189,18 +190,6 @@ class CloudDataPlaneFilterTest { assertEquals(FORBIDDEN, responseHandler.getResponse().getStatus()); } - @Test - void fails_for_reverse_proxy_without_configured_token() { - var req = FilterTestUtils.newRequestBuilder() - .withMethod(Method.GET) - .withClientCertificate(REVERSE_PROXY_CERT) - .build(); - var responseHandler = new MockResponseHandler(); - newFilterWithClientsConfig().filter(req, responseHandler); - assertNotNull(responseHandler.getResponse()); - assertEquals(FORBIDDEN, responseHandler.getResponse().getStatus()); - } - @Test void fails_for_missing_certificate_with_token() { var req = FilterTestUtils.newRequestBuilder() -- cgit v1.2.3