From cb880364afd93f105e51a330bc52d277c596b0fc Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 Jan 2024 10:59:02 -0500 Subject: [PATCH 1/9] Fix unconsumed parameter exception when authentication with jwtUrlParameter Signed-off-by: Craig Perkins --- .../security/http/JwtAuthenticationTests.java | 19 ++++++++++++++++++- .../test/framework/JwtConfigBuilder.java | 9 +++++++++ .../framework/cluster/TestRestClient.java | 7 +++++++ .../auth/http/jwt/HTTPJwtAuthenticator.java | 4 ++-- .../security/filter/NettyRequest.java | 16 ++++++++++++++++ .../security/filter/OpenSearchRequest.java | 5 +++++ .../security/filter/SecurityRequest.java | 10 ++++++++++ .../filter/SecurityRequestChannel.java | 4 ++-- .../security/filter/SecurityRestFilter.java | 8 ++++++++ .../http/SecurityHttpServerTransport.java | 3 +++ .../Netty4HttpRequestHeaderVerifier.java | 3 +++ 11 files changed, 83 insertions(+), 5 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java index 659d7c178e..a046534ecb 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java @@ -108,7 +108,11 @@ public class JwtAuthenticationTests { "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) + .jwtUrlParameter("token") + .signingKey(PUBLIC_KEY) + .subjectKey(CLAIM_USERNAME) + .rolesKey(CLAIM_ROLES) ).backend("noop"); public static final String SONG_ID_1 = "song-id-01"; @@ -153,6 +157,19 @@ public void shouldAuthenticateWithJwtToken_positive() { } } + @Test + public void shouldAuthenticateWithJwtTokenInUrl_positive() { + Header jwtToken = tokenFactory.generateValidToken(USER_SUPERHERO); + String jwtTokenValue = jwtToken.getValue(); + try (TestRestClient client = cluster.getRestClient()) { + HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(USER_SUPERHERO)); + } + } + @Test public void shouldAuthenticateWithJwtToken_positiveWithAnotherUsername() { try (TestRestClient client = cluster.getRestClient(tokenFactory.generateValidToken(USERNAME_ROOT))) { 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/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java index 9bf22bf7f3..d0d68c74bc 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java @@ -130,10 +130,10 @@ private AuthCredentials extractCredentials0(final SecurityRequest request) { } if ((jwtToken == null || jwtToken.isEmpty()) && jwtUrlParameter != null) { - jwtToken = request.params().get(jwtUrlParameter); + jwtToken = request.param(jwtUrlParameter); } else { // just consume to avoid "contains unrecognized parameter" - request.params().get(jwtUrlParameter); + request.param(jwtUrlParameter); } if (jwtToken == null || jwtToken.length() == 0) { diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 7b65e4e0de..e0fb37142a 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -14,9 +14,11 @@ 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; @@ -35,6 +37,8 @@ public class NettyRequest implements SecurityRequest { protected final HttpRequest underlyingRequest; protected final Netty4HttpChannel underlyingChannel; + private final Set consumedParams = new HashSet<>(); + NettyRequest(final HttpRequest request, final Netty4HttpChannel channel) { this.underlyingRequest = request; this.underlyingChannel = channel; @@ -85,6 +89,18 @@ public Map params() { return params(underlyingRequest.uri()); } + @Override + public String param(String key) { + Map urlParams = params(); + consumedParams.add(key); + return urlParams != null ? params().get(key) : null; + } + + @Override + public Set getConsumedParams() { + return consumedParams; + } + private static Map params(String uri) { // Sourced from // https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/http/AbstractHttpServerTransport.java#L419-L422 diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index e86012f594..c64f7af881 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -72,6 +72,11 @@ public Map params() { return underlyingRequest.params(); } + @Override + public String param(String key) { + return underlyingRequest.param(key); + } + /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index 4c7ea27a87..ad46589e86 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequest.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequest.java @@ -12,9 +12,11 @@ package org.opensearch.security.filter; import java.net.InetSocketAddress; +import java.util.Collections; 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 +51,12 @@ default String header(final String headerName) { /** The parameters associated with this request */ Map params(); + + /** Gets the parameter for the given key */ + String param(String key); + + /** Gets the parameters that have been consumed */ + default Set getConsumedParams() { + return Collections.emptySet(); + }; } 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..a15813eec7 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -69,6 +69,7 @@ import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; +import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; 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; @@ -144,6 +145,13 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); + NettyAttribute.popFrom(request, CONSUMED_PARAMS).ifPresent(consumedParams -> { + for (String param : consumedParams) { + // Consume the parameter on the RestRequest + request.param(param); + } + }); + 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..0223632269 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> CONSUMED_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..978eb035e6 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 @@ -32,6 +32,7 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.util.ReferenceCountUtil; +import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; 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; @@ -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(CONSUMED_PARAMS).set(requestChannel.getConsumedParams()); + ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false); ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore); From 61aa3c2cc1ece420a6bdfa9ee392ba4804027e1a Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 Jan 2024 11:29:15 -0500 Subject: [PATCH 2/9] Create separate test suite Signed-off-by: Craig Perkins --- .../security/http/JwtAuthenticationTests.java | 19 +--- .../JwtAuthenticationWithUrlParamTests.java | 97 +++++++++++++++++++ 2 files changed, 98 insertions(+), 18 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java index a046534ecb..659d7c178e 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java @@ -108,11 +108,7 @@ public class JwtAuthenticationTests { "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER) - .jwtUrlParameter("token") - .signingKey(PUBLIC_KEY) - .subjectKey(CLAIM_USERNAME) - .rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtHeader(JWT_AUTH_HEADER).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) ).backend("noop"); public static final String SONG_ID_1 = "song-id-01"; @@ -157,19 +153,6 @@ public void shouldAuthenticateWithJwtToken_positive() { } } - @Test - public void shouldAuthenticateWithJwtTokenInUrl_positive() { - Header jwtToken = tokenFactory.generateValidToken(USER_SUPERHERO); - String jwtTokenValue = jwtToken.getValue(); - try (TestRestClient client = cluster.getRestClient()) { - HttpResponse response = client.getAuthInfo(Map.of("token", jwtTokenValue)); - - response.assertStatusCode(200); - String username = response.getTextFromJsonBody(POINTER_USERNAME); - assertThat(username, equalTo(USER_SUPERHERO)); - } - } - @Test public void shouldAuthenticateWithJwtToken_positiveWithAnotherUsername() { try (TestRestClient client = cluster.getRestClient(tokenFactory.generateValidToken(USERNAME_ROOT))) { 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..85dfec84aa --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -0,0 +1,97 @@ +/* + * 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.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 JWT_AUTH_HEADER = "jwt-auth"; + + private static final JwtAuthorizationHeaderFactory tokenFactory = new JwtAuthorizationHeaderFactory( + KEY_PAIR.getPrivate(), + CLAIM_USERNAME, + CLAIM_ROLES, + JWT_AUTH_HEADER + ); + + public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( + "jwt", + BASIC_AUTH_DOMAIN_ORDER - 1 + ).jwtHttpAuthenticator( + new JwtConfigBuilder().jwtUrlParameter("token").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", jwtTokenValue)); + + response.assertStatusCode(200); + String username = response.getTextFromJsonBody(POINTER_USERNAME); + assertThat(username, equalTo(ADMIN_USER.getName())); + } + } +} From 190df880c743502d2065ae70d7949d3571e6aa6e Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 Jan 2024 11:45:45 -0500 Subject: [PATCH 3/9] Add test that assert log entry for unauthed request Signed-off-by: Craig Perkins --- .../JwtAuthenticationWithUrlParamTests.java | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java index 85dfec84aa..6dfb3c3bbc 100644 --- a/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java +++ b/src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationWithUrlParamTests.java @@ -33,6 +33,7 @@ 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; @@ -52,20 +53,20 @@ public class JwtAuthenticationWithUrlParamTests { static final TestSecurityConfig.User ADMIN_USER = new TestSecurityConfig.User("admin").roles(ALL_ACCESS); - private static final String JWT_AUTH_HEADER = "jwt-auth"; + private static final String TOKEN_URL_PARAM = "token"; private static final JwtAuthorizationHeaderFactory tokenFactory = new JwtAuthorizationHeaderFactory( KEY_PAIR.getPrivate(), CLAIM_USERNAME, CLAIM_ROLES, - JWT_AUTH_HEADER + AUTHORIZATION ); public static final TestSecurityConfig.AuthcDomain JWT_AUTH_DOMAIN = new TestSecurityConfig.AuthcDomain( "jwt", BASIC_AUTH_DOMAIN_ORDER - 1 ).jwtHttpAuthenticator( - new JwtConfigBuilder().jwtUrlParameter("token").signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) + new JwtConfigBuilder().jwtUrlParameter(TOKEN_URL_PARAM).signingKey(PUBLIC_KEY).subjectKey(CLAIM_USERNAME).rolesKey(CLAIM_ROLES) ).backend("noop"); @ClassRule @@ -87,11 +88,21 @@ 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", jwtTokenValue)); + 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)); + } + } } From ab78f8e6e8637c9dee437e8c101b805793484b17 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 31 Jan 2024 14:17:57 -0500 Subject: [PATCH 4/9] getConsumedParams only on NettyRequest Signed-off-by: Craig Perkins --- .../java/org/opensearch/security/filter/NettyRequest.java | 1 - .../org/opensearch/security/filter/SecurityRequest.java | 7 ------- .../ssl/http/netty/Netty4HttpRequestHeaderVerifier.java | 5 ++++- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index e0fb37142a..5890521e2c 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -96,7 +96,6 @@ public String param(String key) { return urlParams != null ? params().get(key) : null; } - @Override public Set getConsumedParams() { return consumedParams; } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index ad46589e86..195c7d51a5 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRequest.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRequest.java @@ -12,11 +12,9 @@ package org.opensearch.security.filter; import java.net.InetSocketAddress; -import java.util.Collections; 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; @@ -54,9 +52,4 @@ default String header(final String headerName) { /** Gets the parameter for the given key */ String param(String key); - - /** Gets the parameters that have been consumed */ - default Set getConsumedParams() { - return Collections.emptySet(); - }; } 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 978eb035e6..b6ca4ca79c 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 @@ -14,6 +14,7 @@ import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.http.netty4.Netty4HttpServerTransport; +import org.opensearch.security.filter.NettyRequest; import org.opensearch.security.filter.SecurityRequestChannel; import org.opensearch.security.filter.SecurityRequestChannelUnsupported; import org.opensearch.security.filter.SecurityRequestFactory; @@ -85,7 +86,9 @@ 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(CONSUMED_PARAMS).set(requestChannel.getConsumedParams()); + if (requestChannel instanceof NettyRequest) { + ctx.channel().attr(CONSUMED_PARAMS).set(((NettyRequest) requestChannel).getConsumedParams()); + } ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false); ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore); From df8fe5936329ef7bfe9ec543545d3c00b27cd7d9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 1 Feb 2024 12:03:10 -0500 Subject: [PATCH 5/9] Update src/main/java/org/opensearch/security/filter/OpenSearchRequest.java Co-authored-by: Peter Nied Signed-off-by: Craig Perkins --- .../org/opensearch/security/filter/OpenSearchRequest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index c64f7af881..a54dfe744c 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -69,7 +69,12 @@ public String uri() { @Override public Map params() { - return underlyingRequest.params(); + return new Map<>(underlyingRequest.params()) { + @Override + public String get(String key) { + return underlyingRequest.param(key); + } + }; } @Override From 6a9330cfed878fdf48dadae01623384708cbe8e1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 1 Feb 2024 12:11:08 -0500 Subject: [PATCH 6/9] Use HashMap Signed-off-by: Craig Perkins --- .../org/opensearch/security/filter/OpenSearchRequest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index a54dfe744c..3827c3a8ab 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -12,6 +12,7 @@ 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; @@ -69,10 +70,10 @@ public String uri() { @Override public Map params() { - return new Map<>(underlyingRequest.params()) { + return new HashMap<>(underlyingRequest.params()) { @Override - public String get(String key) { - return underlyingRequest.param(key); + public String get(Object key) { + return underlyingRequest.param((String) key); } }; } From 5dc47baedaba8e94be28aa8162160a466d473ba4 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 14 Feb 2024 05:52:10 -0600 Subject: [PATCH 7/9] Alternative solution for tracking unconsumed parameters Signed-off-by: Peter Nied --- .../auth/http/jwt/HTTPJwtAuthenticator.java | 4 +- .../security/filter/NettyRequest.java | 46 ++++++++++++++----- .../security/filter/OpenSearchRequest.java | 8 +++- .../security/filter/SecurityRequest.java | 5 +- .../security/filter/SecurityRestFilter.java | 8 ++-- .../http/SecurityHttpServerTransport.java | 2 +- .../Netty4HttpRequestHeaderVerifier.java | 6 +-- 7 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java index d0d68c74bc..9bf22bf7f3 100644 --- a/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java @@ -130,10 +130,10 @@ private AuthCredentials extractCredentials0(final SecurityRequest request) { } if ((jwtToken == null || jwtToken.isEmpty()) && jwtUrlParameter != null) { - jwtToken = request.param(jwtUrlParameter); + jwtToken = request.params().get(jwtUrlParameter); } else { // just consume to avoid "contains unrecognized parameter" - request.param(jwtUrlParameter); + request.params().get(jwtUrlParameter); } if (jwtToken == null || jwtToken.length() == 0) { diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 5890521e2c..d5cd07dc7b 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -20,10 +20,19 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; +import java.util.stream.Collector; +import java.util.stream.Collectors; + import javax.net.ssl.SSLEngine; +import org.apache.commons.lang3.concurrent.ConcurrentException; +import org.apache.commons.lang3.concurrent.LazyInitializer; import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.rest.RestRequest.Method; + +import com.google.common.base.Supplier; +import com.google.common.base.Suppliers; + import org.opensearch.rest.RestUtils; import io.netty.handler.codec.http.HttpRequest; @@ -36,8 +45,7 @@ public class NettyRequest implements SecurityRequest { protected final HttpRequest underlyingRequest; protected final Netty4HttpChannel underlyingChannel; - - private final Set consumedParams = new HashSet<>(); + protected final Supplier parameters = Suppliers.memoize(() -> new CheckedAccessMap(params(uri()))); NettyRequest(final HttpRequest request, final Netty4HttpChannel channel) { this.underlyingRequest = request; @@ -86,18 +94,12 @@ public String uri() { @Override public Map params() { - return params(underlyingRequest.uri()); + return parameters.get(); } @Override - public String param(String key) { - Map urlParams = params(); - consumedParams.add(key); - return urlParams != null ? params().get(key) : null; - } - - public Set getConsumedParams() { - return consumedParams; + public Set getUnconsumedParams() { + return parameters.get().accessedKeys(); } private static Map params(String uri) { @@ -115,4 +117,26 @@ private static Map params(String uri) { return params; } + + /** Records access of any keys if explicetly 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 3827c3a8ab..244be6a568 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -16,6 +16,8 @@ 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; @@ -79,10 +81,12 @@ public String get(Object key) { } @Override - public String param(String key) { - return underlyingRequest.param(key); + public Set getUnconsumedParams() { + // params() Map consumes explict parameter access + return Set.of(); } + /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRequest.java b/src/main/java/org/opensearch/security/filter/SecurityRequest.java index 195c7d51a5..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; @@ -50,6 +51,6 @@ default String header(final String headerName) { /** The parameters associated with this request */ Map params(); - /** Gets the parameter for the given key */ - String param(String key); + /** 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/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index a15813eec7..0b721e42f9 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -69,7 +69,7 @@ import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; -import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; 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; @@ -145,10 +145,10 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c } }); - NettyAttribute.popFrom(request, CONSUMED_PARAMS).ifPresent(consumedParams -> { - for (String param : consumedParams) { + NettyAttribute.popFrom(request, UNCONSUMED_PARAMS).ifPresent(unconsumedParams -> { + for (String unconsumedParam : unconsumedParams) { // Consume the parameter on the RestRequest - request.param(param); + request.param(unconsumedParam); } }); diff --git a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java index 0223632269..eb75f898f4 100644 --- a/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java +++ b/src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java @@ -49,7 +49,7 @@ public class SecurityHttpServerTransport extends SecuritySSLNettyHttpServerTransport { public static final AttributeKey EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response"); - public static final AttributeKey> CONSUMED_PARAMS = AttributeKey.newInstance("opensearch-http-request-consumed-params"); + 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 b6ca4ca79c..da66c4c797 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 @@ -33,7 +33,7 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.util.ReferenceCountUtil; -import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; 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; @@ -86,9 +86,7 @@ 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); - if (requestChannel instanceof NettyRequest) { - ctx.channel().attr(CONSUMED_PARAMS).set(((NettyRequest) requestChannel).getConsumedParams()); - } + ctx.channel().attr(UNCONSUMED_PARAMS).set(requestChannel.getUnconsumedParams()); ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false); ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore); From b0b354ceb8b070e7d9c37bbef2535ae027b55c03 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 14 Feb 2024 05:53:54 -0600 Subject: [PATCH 8/9] SpotlessApply Signed-off-by: Peter Nied --- .../org/opensearch/security/filter/NettyRequest.java | 12 +++--------- .../security/filter/OpenSearchRequest.java | 2 -- .../security/filter/SecurityRestFilter.java | 2 +- .../http/netty/Netty4HttpRequestHeaderVerifier.java | 3 +-- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index d5cd07dc7b..143d94ae9a 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -20,19 +20,13 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; -import java.util.stream.Collector; -import java.util.stream.Collectors; - import javax.net.ssl.SSLEngine; -import org.apache.commons.lang3.concurrent.ConcurrentException; -import org.apache.commons.lang3.concurrent.LazyInitializer; -import org.opensearch.http.netty4.Netty4HttpChannel; -import org.opensearch.rest.RestRequest.Method; - 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; import io.netty.handler.codec.http.HttpRequest; @@ -130,7 +124,7 @@ public CheckedAccessMap(final Map map) { 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); + accessedKeys.add((String) key); } return super.get(key); } diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index 244be6a568..7cca8111c9 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -17,7 +17,6 @@ import java.util.Map; import java.util.Optional; import java.util.Set; - import javax.net.ssl.SSLEngine; import org.opensearch.rest.RestRequest; @@ -86,7 +85,6 @@ public Set getUnconsumedParams() { return Set.of(); } - /** Gets access to the underlying request object */ public RestRequest breakEncapsulationForRequest() { return underlyingRequest; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 0b721e42f9..b649b8d71f 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -69,10 +69,10 @@ import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX; import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX; -import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; 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 { 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 da66c4c797..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 @@ -14,7 +14,6 @@ import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.http.netty4.Netty4HttpServerTransport; -import org.opensearch.security.filter.NettyRequest; import org.opensearch.security.filter.SecurityRequestChannel; import org.opensearch.security.filter.SecurityRequestChannelUnsupported; import org.opensearch.security.filter.SecurityRequestFactory; @@ -33,11 +32,11 @@ import io.netty.handler.codec.http.HttpRequest; import io.netty.util.ReferenceCountUtil; -import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; 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.SHOULD_DECOMPRESS; +import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS; @Sharable public class Netty4HttpRequestHeaderVerifier extends SimpleChannelInboundHandler { From 21422b35ac92ff636f20f6a90c36cea690d3fa29 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 21 Feb 2024 23:37:31 +0000 Subject: [PATCH 9/9] Fix terrible spelling mistake Signed-off-by: Peter Nied --- src/main/java/org/opensearch/security/filter/NettyRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/filter/NettyRequest.java b/src/main/java/org/opensearch/security/filter/NettyRequest.java index 143d94ae9a..c827ddc779 100644 --- a/src/main/java/org/opensearch/security/filter/NettyRequest.java +++ b/src/main/java/org/opensearch/security/filter/NettyRequest.java @@ -112,7 +112,7 @@ private static Map params(String uri) { return params; } - /** Records access of any keys if explicetly requested from this map */ + /** Records access of any keys if explicitly requested from this map */ private static class CheckedAccessMap extends HashMap { private final Set accessedKeys = new HashSet<>();