From d87ab3f7372ff799ca75fbf9741f5111e12d84dd Mon Sep 17 00:00:00 2001 From: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> Date: Tue, 9 Apr 2024 11:10:53 -0400 Subject: [PATCH] Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails (#4152) Signed-off-by: Darshit Chanpura --- .../http/BasicWithAnonymousAuthTests.java | 112 ++++++++++++++++++ .../security/auth/BackendRegistry.java | 47 +++++--- .../security/rest/SecurityInfoAction.java | 3 + 3 files changed, 147 insertions(+), 15 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java diff --git a/src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java b/src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java new file mode 100644 index 0000000000..842d5c4dd5 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java @@ -0,0 +1,112 @@ +/* +* 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.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.opensearch.test.framework.TestSecurityConfig.AuthcDomain; +import org.opensearch.test.framework.TestSecurityConfig.User; +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 static org.apache.http.HttpStatus.SC_OK; +import static org.apache.http.HttpStatus.SC_UNAUTHORIZED; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class BasicWithAnonymousAuthTests { + static final User TEST_USER = new User("test_user").password("s3cret"); + + public static final String CUSTOM_ATTRIBUTE_NAME = "superhero"; + static final User SUPER_USER = new User("super-user").password("super-password").attr(CUSTOM_ATTRIBUTE_NAME, "true"); + public static final String NOT_EXISTING_USER = "not-existing-user"; + public static final String INVALID_PASSWORD = "secret-password"; + + public static final AuthcDomain AUTHC_DOMAIN = new AuthcDomain("basic", 0).httpAuthenticatorWithChallenge("basic").backend("internal"); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(true) + .authc(AUTHC_DOMAIN) + .users(TEST_USER, SUPER_USER) + .build(); + + /** No automatic login post anonymous auth request **/ + @Test + public void testShouldRespondWith401WhenUserDoesNotExist() { + try (TestRestClient client = cluster.getRestClient(NOT_EXISTING_USER, INVALID_PASSWORD)) { + HttpResponse response = client.getAuthInfo(); + + assertThat(response, is(notNullValue())); + response.assertStatusCode(SC_UNAUTHORIZED); + } + } + + @Test + public void testShouldRespondWith401WhenUserNameIsIncorrect() { + try (TestRestClient client = cluster.getRestClient(NOT_EXISTING_USER, TEST_USER.getPassword())) { + HttpResponse response = client.getAuthInfo(); + + assertThat(response, is(notNullValue())); + response.assertStatusCode(SC_UNAUTHORIZED); + } + } + + @Test + public void testShouldRespondWith401WhenPasswordIsIncorrect() { + try (TestRestClient client = cluster.getRestClient(TEST_USER.getName(), INVALID_PASSWORD)) { + HttpResponse response = client.getAuthInfo(); + + assertThat(response, is(notNullValue())); + response.assertStatusCode(SC_UNAUTHORIZED); + } + } + + /** Test `?auth_type=""` param to authinfo request **/ + @Test + public void testShouldAutomaticallyLoginAsAnonymousIfNoCredentialsArePassed() { + try (TestRestClient client = cluster.getRestClient()) { + + HttpResponse response = client.getAuthInfo(); + + assertThat(response, is(notNullValue())); + response.assertStatusCode(SC_OK); + + HttpResponse response2 = client.getAuthInfo(Map.of("auth_type", "anonymous")); + + assertThat(response2, is(notNullValue())); + response2.assertStatusCode(SC_OK); + } + } + + @Test + public void testShouldNotAutomaticallyLoginAsAnonymousIfRequestIsNonAnonymousLogin() { + try (TestRestClient client = cluster.getRestClient()) { + + HttpResponse response = client.getAuthInfo(Map.of("auth_type", "saml")); + + assertThat(response, is(notNullValue())); + response.assertStatusCode(SC_UNAUTHORIZED); + + // should contain a redirect link + assertThat(response.containHeader("WWW-Authenticate"), is(true)); + } + } +} diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 3ab9a2afc9..97c060be35 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -32,6 +32,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.SortedSet; @@ -44,6 +45,7 @@ import com.google.common.cache.RemovalListener; import com.google.common.cache.RemovalNotification; import com.google.common.collect.Multimap; +import org.apache.http.HttpHeaders; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -190,7 +192,7 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { * @param request * @return The authenticated user, null means another roundtrip * @throws OpenSearchSecurityException - */ + */ public boolean authenticate(final SecurityRequestChannel request) { final boolean isDebugEnabled = log.isDebugEnabled(); final boolean isBlockedBasedOnAddress = request.getRemoteAddress() @@ -286,7 +288,7 @@ public boolean authenticate(final SecurityRequestChannel request) { if (ac == null) { // no credentials found in request - if (anonymousAuthEnabled) { + if (anonymousAuthEnabled && isRequestForAnonymousLogin(request.params(), request.getHeaders())) { continue; } @@ -386,19 +388,6 @@ public boolean authenticate(final SecurityRequestChannel request) { log.debug("User still not authenticated after checking {} auth domains", restAuthDomains.size()); } - if (authCredentials == null && anonymousAuthEnabled) { - final String tenant = resolveTenantFrom(request); - User anonymousUser = new User(User.ANONYMOUS.getName(), new HashSet(User.ANONYMOUS.getRoles()), null); - anonymousUser.setRequestedTenant(tenant); - - threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser); - auditLog.logSucceededLogin(anonymousUser.getName(), false, null, request); - if (isDebugEnabled) { - log.debug("Anonymous User is authenticated"); - } - return true; - } - Optional challengeResponse = Optional.empty(); if (firstChallengingHttpAuthenticator != null) { @@ -415,6 +404,19 @@ public boolean authenticate(final SecurityRequestChannel request) { } } + if (authCredentials == null && anonymousAuthEnabled && isRequestForAnonymousLogin(request.params(), request.getHeaders())) { + final String tenant = resolveTenantFrom(request); + User anonymousUser = new User(User.ANONYMOUS.getName(), new HashSet(User.ANONYMOUS.getRoles()), null); + anonymousUser.setRequestedTenant(tenant); + + threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser); + auditLog.logSucceededLogin(anonymousUser.getName(), false, null, request); + if (isDebugEnabled) { + log.debug("Anonymous User is authenticated"); + } + return true; + } + log.warn( "Authentication finally failed for {} from {}", authCredentials == null ? null : authCredentials.getUsername(), @@ -432,6 +434,21 @@ public boolean authenticate(final SecurityRequestChannel request) { return authenticated; } + /** + * Checks if incoming auth request is from an anonymous user + * Defaults all requests to yes, to allow anonymous authentication to succeed + * @param params the query parameters passed in this request + * @return false only if an explicit `auth_type` param is supplied, and its value is not anonymous, OR + * if request contains no authorization headers + * otherwise returns true + */ + private boolean isRequestForAnonymousLogin(Map params, Map> headers) { + if (params.containsKey("auth_type")) { + return params.get("auth_type").equals("anonymous"); + } + return !headers.containsKey(HttpHeaders.AUTHORIZATION); + } + private String resolveTenantFrom(final SecurityRequest request) { return Optional.ofNullable(request.header("securitytenant")).orElse(request.header("security_tenant")); } diff --git a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java index f6cf7f82ee..64075d5d0e 100644 --- a/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java +++ b/src/main/java/org/opensearch/security/rest/SecurityInfoAction.java @@ -91,6 +91,9 @@ public List routes() { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { final boolean verbose = request.paramAsBoolean("verbose", false); + // need to consume `auth_type` param, without which a 500 is thrown on front-end + final String authType = request.param("auth_type", ""); + return new RestChannelConsumer() { @Override