From ccea7441545c807b7c4eb531706b1d919ea11988 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 21 Feb 2024 20:48:31 -0500 Subject: [PATCH] Fix unconsumed parameter exception when authenticating with jwtUrlParameter (#3975) Signed-off-by: Craig Perkins Signed-off-by: Peter Nied Signed-off-by: Peter Nied Co-authored-by: Peter Nied Co-authored-by: Peter Nied --- .../JwtAuthenticationWithUrlParamTests.java | 108 ++++++++++++++++++ .../test/framework/JwtConfigBuilder.java | 9 ++ .../framework/cluster/TestRestClient.java | 7 ++ .../security/filter/NettyRequest.java | 35 +++++- .../security/filter/OpenSearchRequest.java | 15 ++- .../security/filter/SecurityRequest.java | 4 + .../filter/SecurityRequestChannel.java | 4 +- .../security/filter/SecurityRestFilter.java | 8 ++ .../http/SecurityHttpServerTransport.java | 3 + .../Netty4HttpRequestHeaderVerifier.java | 3 + 10 files changed, 192 insertions(+), 4 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java new file mode 100644 index 0000000000..6dfb3c3bbc --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -0,0 +1,108 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + */ +package org.opensearch.security.http; + +import java.security.KeyPair; +import java.util.Base64; +import java.util.List; +import java.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.hc.core5.http.Header; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.opensearch.test.framework.JwtConfigBuilder; +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; +import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; +import org.opensearch.test.framework.log.LogsRule; + +import io.jsonwebtoken.SignatureAlgorithm; +import io.jsonwebtoken.security.Keys; + +import static java.nio.charset.StandardCharsets.US_ASCII; +import static org.apache.http.HttpHeaders.AUTHORIZATION; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.BASIC_AUTH_DOMAIN_ORDER; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class JwtAuthenticationWithUrlParamTests { + + public static final String CLAIM_USERNAME = "preferred-username"; + public static final String CLAIM_ROLES = "backend-user-roles"; + public static final String POINTER_USERNAME = "/user_name"; + + private static final KeyPair KEY_PAIR = Keys.keyPairFor(SignatureAlgorithm.RS256); + private static final String PUBLIC_KEY = new String(Base64.getEncoder().encode(KEY_PAIR.getPublic().getEncoded()), US_ASCII); + + static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); + + private static final String TOKEN_URL_PARAM = "token"; + + private static final JwtAuthorizationHeaderFactory tokenFactory = new JwtAuthorizationHeaderFactory( + KEY_PAIR.getPrivate(), + CLAIM_USERNAME, + CLAIM_ROLES, + AUTHORIZATION + ); + + public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( + "jwt", + BASIC_AUTH_DOMAIN_ORDER - 1 + ).jwtHttpAuthenticator( + new JwtConfigBuilder().jwtUrlParameter(TOKEN_URL_PARAM).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) + ).backend("noop"); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .nodeSettings( + Map.of("plugins.security.restapi.roles_enabled", List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName())) + ) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .authc(JWT_AUTH_DOMAIN) + .users(ADMIN_USER) + .build(); + + @Rule + public LogsRule logsRule = new LogsRule("com.amazon.dlic.auth.http.jwt.HTTPJwtAuthenticator"); + + @Test + public void shouldAuthenticateWithJwtTokenInUrl_positive() { + Header jwtToken = tokenFactory.generateValidToken(ADMIN_USER.getName()); + String jwtTokenValue = jwtToken.getValue(); + try (TestRestClient client = cluster.getRestClient()) { + HttpResponse response = client.getAuthInfo(Map.of(TOKEN_URL_PARAM, jwtTokenValue)); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(ADMIN_USER.getName())); + } + } + + @Test + public void testUnauthenticatedRequest() { + try (TestRestClient client = cluster.getRestClient()) { + HttpResponse response = client.getAuthInfo(); + + response.assertStatusCode(401); + logsRule.assertThatContainExactly(String.format("No JWT token found in '%s' url parameter header", TOKEN_URL_PARAM)); + } + } +} diff --git a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java index 48dfa128e0..88297bacd2 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java +++ b/src/integrationTest/java/org/opensearch/test/framework/JwtConfigBuilder.java @@ -18,6 +18,7 @@ public class JwtConfigBuilder { private String jwtHeader; + private String jwtUrlParameter; private String signingKey; private String subjectKey; private String rolesKey; @@ -27,6 +28,11 @@ public JwtConfigBuilder jwtHeader(String jwtHeader) { return this; } + public JwtConfigBuilder jwtUrlParameter(String jwtUrlParameter) { + this.jwtUrlParameter = jwtUrlParameter; + return this; + } + public JwtConfigBuilder signingKey(String signingKey) { this.signingKey = signingKey; return this; @@ -51,6 +57,9 @@ public Map build() { if (isNoneBlank(jwtHeader)) { builder.put("jwt_header", jwtHeader); } + if (isNoneBlank(jwtUrlParameter)) { + builder.put("jwt_url_parameter", jwtUrlParameter); + } if (isNoneBlank(subjectKey)) { builder.put("subject_key", subjectKey); } diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java index c2e01bb338..b7da92b270 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java @@ -36,6 +36,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; @@ -113,6 +114,12 @@ public HttpResponse getAuthInfo(Header... headers) { return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo?pretty"), headers); } + public HttpResponse getAuthInfo(Map urlParams, Header... headers) { + String urlParamsString = "?" + + urlParams.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining("&")); + return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo" + urlParamsString), headers); + } + public void confirmCorrectCredentials(String expectedUserName) { HttpResponse response = getAuthInfo(); assertThat(response, notNullValue()); diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 7b65e4e0de..c827ddc779 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -14,12 +14,17 @@ import java.net.InetSocketAddress; import java.util.Collections; 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.TreeMap; import javax.net.ssl.SSLEngine; +import com.google.common.base.Supplier; +import com.google.common.base.Suppliers; + import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.rest.RestRequest.Method; import org.opensearch.rest.RestUtils; @@ -34,6 +39,7 @@ public class NettyRequest implements SecurityRequest { protected final HttpRequest underlyingRequest; protected final Netty4HttpChannel underlyingChannel; + protected final Supplier parameters = Suppliers.memoize(() -> new CheckedAccessMap(params(uri()))); NettyRequest(final HttpRequest request, final Netty4HttpChannel channel) { this.underlyingRequest = request; @@ -82,7 +88,12 @@ public String uri() { @Override public Map params() { - return params(underlyingRequest.uri()); + return parameters.get(); + } + + @Override + public Set getUnconsumedParams() { + return parameters.get().accessedKeys(); } private static Map params(String uri) { @@ -100,4 +111,26 @@ private static Map params(String uri) { return params; } + + /** Records access of any keys if explicitly requested from this map */ + private static class CheckedAccessMap extends HashMap { + private final Set accessedKeys = new HashSet<>(); + + public CheckedAccessMap(final Map map) { + super(map); + } + + @Override + public String get(final Object key) { + // Never noticed this about java's map interface the getter is not generic + if (key instanceof String) { + accessedKeys.add((String) key); + } + return super.get(key); + } + + public Set accessedKeys() { + return accessedKeys; + } + } } diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index e86012f594..7cca8111c9 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -12,9 +12,11 @@ package org.opensearch.security.filter; import java.net.InetSocketAddress; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import javax.net.ssl.SSLEngine; import org.opensearch.rest.RestRequest; @@ -69,7 +71,18 @@ public String uri() { @Override public Map params() { - return underlyingRequest.params(); + return new HashMap<>(underlyingRequest.params()) { + @Override + public String get(Object key) { + return underlyingRequest.param((String) key); + } + }; + } + + @Override + public Set getUnconsumedParams() { + // params() Map consumes explict parameter access + return Set.of(); } /** Gets access to the underlying request object */ diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index 4c7ea27a87..d3741585ac 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequest.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequest.java @@ -15,6 +15,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Stream; import javax.net.ssl.SSLEngine; @@ -49,4 +50,7 @@ default String header(final String headerName) { /** The parameters associated with this request */ Map params(); + + /** The list of parameters that have been accessed but not recorded as being consumed */ + Set getUnconsumedParams(); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java b/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java index 66744d01dd..241c9009f4 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequestChannel.java @@ -14,13 +14,13 @@ import java.util.Optional; /** - * When a request is recieved by the security plugin this governs getting information about the request and complete with with a response + * When a request is received by the security plugin this governs getting information about the request and complete with a response */ public interface SecurityRequestChannel extends SecurityRequest { /** Associate a response with this channel */ void queueForSending(final SecurityResponse response); - /** Acess the queued response */ + /** Access the queued response */ Optional getQueuedResponse(); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index d52c5109fc..b649b8d71f 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -72,6 +72,7 @@ import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE; import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; public class SecurityRestFilter { @@ -144,6 +145,13 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); + NettyAttribute.popFrom(request, UNCONSUMED_PARAMS).ifPresent(unconsumedParams -> { + for (String unconsumedParam : unconsumedParams) { + // Consume the parameter on the RestRequest + request.param(unconsumedParam); + } + }); + final SecurityRequestChannel requestChannel = SecurityRequestFactory.from(request, channel); // Authenticate request diff --git a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java index c5fbbfbbc6..eb75f898f4 100644 --- a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java +++ b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java @@ -26,6 +26,8 @@ package org.opensearch.security.http; +import java.util.Set; + import org.opensearch.common.network.NetworkService; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; @@ -47,6 +49,7 @@ public class SecurityHttpServerTransport extends SecuritySSLNettyHttpServerTransport { public static final AttributeKey EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); + public static final AttributeKey> UNCONSUMED_PARAMS = AttributeKey.newInstance("opensearch-http-request-consumed-params"); public static final AttributeKey CONTEXT_TO_RESTORE = AttributeKey.newInstance( "opensearch-http-request-thread-context" ); diff --git a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java index 9adca0f377..e6dec0c213 100644 --- a/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java +++ b/src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java @@ -36,6 +36,7 @@ import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE; import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED; import static org.opensearch.security.http.SecurityHttpServerTransport.SHOULD_DECOMPRESS; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; @Sharable public class Netty4HttpRequestHeaderVerifier extends SimpleChannelInboundHandler { @@ -84,6 +85,8 @@ public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) thro // If request channel is completed and a response is sent, then there was a failure during authentication restFilter.checkAndAuthenticateRequest(requestChannel); + ctx.channel().attr(UNCONSUMED_PARAMS).set(requestChannel.getUnconsumedParams()); + ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false); ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore);