From d7af7eb99d0d79b478891ca3e15259e0f0d5262d Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 5 Mar 2024 22:24:56 -0500 Subject: [PATCH 1/6] Adds a check to only skip authentication for anonymous requests when anonymous-auth is enabled Signed-off-by: Darshit Chanpura --- .../security/auth/BackendRegistry.java | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 3ab9a2afc9..8e03c9bea4 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -286,7 +286,9 @@ public boolean authenticate(final SecurityRequestChannel request) { if (ac == null) { // no credentials found in request - if (anonymousAuthEnabled) { + if (anonymousAuthEnabled && checkIfRequestIsForAnonymousLogin(request.header("_auth_request_type_"))) { + log.info(httpAuthenticator.getClass().getName()); + log.info("Skipped {} because anonymous auth is enabled", authDomain.getBackend().getClass()); continue; } @@ -386,7 +388,12 @@ public boolean authenticate(final SecurityRequestChannel request) { log.debug("User still not authenticated after checking {} auth domains", restAuthDomains.size()); } - if (authCredentials == null && anonymousAuthEnabled) { + log.info(request.uri()); + log.info(request.getHeaders()); + if (authCredentials == null + && anonymousAuthEnabled + && checkIfRequestIsForAnonymousLogin(request.header("_auth_request_type_"))) { + // TODO why do we automatically assume anonymous user ?? final String tenant = resolveTenantFrom(request); User anonymousUser = new User(User.ANONYMOUS.getName(), new HashSet(User.ANONYMOUS.getRoles()), null); anonymousUser.setRequestedTenant(tenant); @@ -396,6 +403,7 @@ public boolean authenticate(final SecurityRequestChannel request) { if (isDebugEnabled) { log.debug("Anonymous User is authenticated"); } + log.info("Anonymous User is authenticated"); return true; } @@ -432,6 +440,10 @@ public boolean authenticate(final SecurityRequestChannel request) { return authenticated; } + private boolean checkIfRequestIsForAnonymousLogin(String authLoginType) { + return authLoginType != null && authLoginType.equalsIgnoreCase("anonymous"); + } + private String resolveTenantFrom(final SecurityRequest request) { return Optional.ofNullable(request.header("securitytenant")).orElse(request.header("security_tenant")); } From bbde01b4a38374df394124b421a2bd6743dc8c75 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 11 Mar 2024 16:42:40 -0400 Subject: [PATCH 2/6] Adds a check for anonymous user creds Signed-off-by: Darshit Chanpura --- .../security/auth/BackendRegistry.java | 47 +++++++++---------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 8e03c9bea4..5160cb84b3 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -28,6 +28,7 @@ import java.net.InetAddress; import java.net.InetSocketAddress; +import java.util.Base64; import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -38,12 +39,14 @@ import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; +import com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator; import com.google.common.base.Strings; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.cache.RemovalListener; import com.google.common.cache.RemovalNotification; import com.google.common.collect.Multimap; +import io.netty.handler.codec.base64.Base64Decoder; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -286,12 +289,6 @@ public boolean authenticate(final SecurityRequestChannel request) { if (ac == null) { // no credentials found in request - if (anonymousAuthEnabled && checkIfRequestIsForAnonymousLogin(request.header("_auth_request_type_"))) { - log.info(httpAuthenticator.getClass().getName()); - log.info("Skipped {} because anonymous auth is enabled", authDomain.getBackend().getClass()); - continue; - } - if (authDomain.isChallenge()) { final Optional restResponse = httpAuthenticator.reRequestAuthentication(request, null); if (restResponse.isPresent()) { @@ -301,7 +298,8 @@ public boolean authenticate(final SecurityRequestChannel request) { } notifyIpAuthFailureListeners(request, authCredentials); request.queueForSending(restResponse.get()); - return false; + + } } else { // no reRequest possible @@ -311,6 +309,10 @@ public boolean authenticate(final SecurityRequestChannel request) { continue; } } else { + // credentials found for anonymous user. Skip looping over rest auth domains as this is a login request for anonymous user + if (anonymousAuthEnabled && ac.getUsername().equals(User.ANONYMOUS.getName())) { + break; + } org.apache.logging.log4j.ThreadContext.put("user", ac.getUsername()); if (!ac.isComplete()) { // credentials found in request but we need another client challenge @@ -390,21 +392,20 @@ public boolean authenticate(final SecurityRequestChannel request) { log.info(request.uri()); log.info(request.getHeaders()); - if (authCredentials == null - && anonymousAuthEnabled - && checkIfRequestIsForAnonymousLogin(request.header("_auth_request_type_"))) { - // TODO why do we automatically assume anonymous user ?? - 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"); + if (anonymousAuthEnabled) { + assert authCredentials != null; + if (authCredentials.getUsername().equals(User.ANONYMOUS.getName())) { + 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.info("Anonymous User is authenticated"); - return true; } Optional challengeResponse = Optional.empty(); @@ -440,10 +441,6 @@ && checkIfRequestIsForAnonymousLogin(request.header("_auth_request_type_"))) { return authenticated; } - private boolean checkIfRequestIsForAnonymousLogin(String authLoginType) { - return authLoginType != null && authLoginType.equalsIgnoreCase("anonymous"); - } - private String resolveTenantFrom(final SecurityRequest request) { return Optional.ofNullable(request.header("securitytenant")).orElse(request.header("security_tenant")); } From 08ea955f200dec9c7094b504523038921d298861 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 11 Mar 2024 17:02:56 -0400 Subject: [PATCH 3/6] Fixes spotless errors and removes log statements Signed-off-by: Darshit Chanpura --- .../java/org/opensearch/security/auth/BackendRegistry.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 5160cb84b3..4c13ae002e 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -28,7 +28,6 @@ import java.net.InetAddress; import java.net.InetSocketAddress; -import java.util.Base64; import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -39,14 +38,12 @@ import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; -import com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator; import com.google.common.base.Strings; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.cache.RemovalListener; import com.google.common.cache.RemovalNotification; import com.google.common.collect.Multimap; -import io.netty.handler.codec.base64.Base64Decoder; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -299,7 +296,6 @@ public boolean authenticate(final SecurityRequestChannel request) { notifyIpAuthFailureListeners(request, authCredentials); request.queueForSending(restResponse.get()); - } } else { // no reRequest possible @@ -390,8 +386,6 @@ public boolean authenticate(final SecurityRequestChannel request) { log.debug("User still not authenticated after checking {} auth domains", restAuthDomains.size()); } - log.info(request.uri()); - log.info(request.getHeaders()); if (anonymousAuthEnabled) { assert authCredentials != null; if (authCredentials.getUsername().equals(User.ANONYMOUS.getName())) { From 0028c9f237ae97f9bd2dd8feb68e0a10b397d29c Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 11 Mar 2024 17:06:18 -0400 Subject: [PATCH 4/6] Re-adds removed return statement Signed-off-by: Darshit Chanpura --- src/main/java/org/opensearch/security/auth/BackendRegistry.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java index 4c13ae002e..0f16ef18fb 100644 --- a/src/main/java/org/opensearch/security/auth/BackendRegistry.java +++ b/src/main/java/org/opensearch/security/auth/BackendRegistry.java @@ -295,7 +295,7 @@ public boolean authenticate(final SecurityRequestChannel request) { } notifyIpAuthFailureListeners(request, authCredentials); request.queueForSending(restResponse.get()); - + return false; } } else { // no reRequest possible From afacf3315caec2677c19188c8d6147304ec56cdc Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 11 Mar 2024 22:56:55 -0400 Subject: [PATCH 5/6] Fixes citest and ciSecurityIntegrationTest Signed-off-by: Darshit Chanpura --- .../opensearch/security/HttpIntegrationTests.java | 9 ++++++--- .../security/InitializationIntegrationTests.java | 5 ++++- .../org/opensearch/security/SecurityRolesTests.java | 5 ++++- .../multitenancy/test/MultitenancyTests.java | 13 +++++++++---- 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/test/java/org/opensearch/security/HttpIntegrationTests.java b/src/test/java/org/opensearch/security/HttpIntegrationTests.java index 3a437ea80a..1f1c45b320 100644 --- a/src/test/java/org/opensearch/security/HttpIntegrationTests.java +++ b/src/test/java/org/opensearch/security/HttpIntegrationTests.java @@ -31,6 +31,7 @@ import java.nio.file.Files; import com.fasterxml.jackson.databind.JsonNode; +import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.NoHttpResponseException; import org.apache.hc.core5.http.message.BasicHeader; import org.apache.http.HttpStatus; @@ -56,6 +57,7 @@ import org.opensearch.security.test.helper.file.FileHelper; import org.opensearch.security.test.helper.rest.RestHelper; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; +import org.opensearch.security.user.User; import static org.opensearch.security.DefaultObjectMapper.readTree; @@ -465,16 +467,17 @@ public void testHTTPAnon() throws Exception { setup(Settings.EMPTY, new DynamicSecurityConfig().setConfig("config_anon.yml"), Settings.EMPTY, true); RestHelper rh = nonSslRestHelper(); + Header anonyAuthHeader = encodeBasicHeader(User.ANONYMOUS.getName(), randomAsciiAlphanumOfLength(8)); - Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("").getStatusCode()); + Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", anonyAuthHeader).getStatusCode()); Assert.assertEquals(HttpStatus.SC_UNAUTHORIZED, rh.executeGetRequest("", encodeBasicHeader("worf", "wrong")).getStatusCode()); Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("nagilum", "nagilum")).getStatusCode()); - HttpResponse resc = rh.executeGetRequest("_opendistro/_security/authinfo"); + HttpResponse resc = rh.executeGetRequest("_opendistro/_security/authinfo", anonyAuthHeader); Assert.assertTrue(resc.getBody().contains("opendistro_security_anonymous")); Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode()); - resc = rh.executeGetRequest("_opendistro/_security/authinfo?pretty=true"); + resc = rh.executeGetRequest("_opendistro/_security/authinfo?pretty=true", anonyAuthHeader); Assert.assertTrue(resc.getBody().contains("\"remote_address\" : \"")); // check pretty print Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode()); diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index 7545822620..7b4e2c87f0 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -60,6 +60,7 @@ import org.opensearch.security.test.helper.file.FileHelper; import org.opensearch.security.test.helper.rest.RestHelper; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; +import org.opensearch.security.user.User; public class InitializationIntegrationTests extends SingleClusterTest { @@ -255,6 +256,7 @@ public void testConfigHotReload() throws Exception { Assert.assertEquals(clusterInfo.numNodes, cur.getNodes().size()); } + Header anonyAuthHeader = encodeBasicHeader(User.ANONYMOUS.getName(), randomAsciiAlphanumOfLength(8)); for (Iterator iterator = clusterInfo.httpAdresses.iterator(); iterator.hasNext();) { TransportAddress TransportAddress = iterator.next(); HttpResponse res = rh.executeRequest( @@ -265,7 +267,8 @@ public void testConfigHotReload() throws Exception { + TransportAddress.getPort() + "/" + "_opendistro/_security/authinfo?pretty=true" - ) + ), + anonyAuthHeader ); log.debug(res.getBody()); Assert.assertTrue(res.getBody().contains("role_host1")); diff --git a/src/test/java/org/opensearch/security/SecurityRolesTests.java b/src/test/java/org/opensearch/security/SecurityRolesTests.java index 0b4dd0b95b..b7e614e7fe 100644 --- a/src/test/java/org/opensearch/security/SecurityRolesTests.java +++ b/src/test/java/org/opensearch/security/SecurityRolesTests.java @@ -26,6 +26,7 @@ package org.opensearch.security; +import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.message.BasicHeader; import org.apache.http.HttpStatus; import org.junit.Assert; @@ -37,6 +38,7 @@ import org.opensearch.security.test.SingleClusterTest; import org.opensearch.security.test.helper.rest.RestHelper; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; +import org.opensearch.security.user.User; public class SecurityRolesTests extends SingleClusterTest { @@ -51,8 +53,9 @@ public void testSecurityRolesAnon() throws Exception { ); RestHelper rh = nonSslRestHelper(); + Header anonyAuthHeader = encodeBasicHeader(User.ANONYMOUS.getName(), randomAsciiAlphanumOfLength(8)); - HttpResponse resc = rh.executeGetRequest("_opendistro/_security/authinfo?pretty"); + HttpResponse resc = rh.executeGetRequest("_opendistro/_security/authinfo?pretty", anonyAuthHeader); Assert.assertTrue(resc.getBody().contains("anonymous")); Assert.assertFalse(resc.getBody().contains("xyz_sr")); Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode()); diff --git a/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java b/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java index 0a785d7b80..47b6fca567 100644 --- a/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java +++ b/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java @@ -35,6 +35,7 @@ import org.opensearch.security.test.SingleClusterTest; import org.opensearch.security.test.helper.rest.RestHelper; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; +import org.opensearch.security.user.User; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; @@ -613,13 +614,15 @@ public void testMultitenancyAnonymousUser() throws Exception { new BasicHeader("securitytenant", anonymousTenant) ); + Header anonyAuthHeader = encodeBasicHeader(User.ANONYMOUS.getName(), randomAsciiAlphanumOfLength(8)); + /* The anonymous user has access to its tenant */ - res = rh.executeGetRequest(url, new BasicHeader("securitytenant", anonymousTenant)); + res = rh.executeGetRequest(url, new BasicHeader("securitytenant", anonymousTenant), anonyAuthHeader); Assert.assertEquals(HttpStatus.SC_OK, res.getStatusCode()); Assert.assertEquals(anonymousTenant, res.findValueInJson("_source.tenant")); /* No access to other tenants */ - res = rh.executeGetRequest(url, new BasicHeader("securitytenant", "human_resources")); + res = rh.executeGetRequest(url, new BasicHeader("securitytenant", "human_resources"), anonyAuthHeader); Assert.assertEquals(HttpStatus.SC_FORBIDDEN, res.getStatusCode()); } @@ -660,6 +663,7 @@ public void testMultitenancyUserReadWriteActions() throws Exception { @Test public void testMultitenancyAnonymousUserReadOnlyActions() throws Exception { setup(Settings.EMPTY, new DynamicSecurityConfig().setConfig("config_anonymous.yml"), Settings.EMPTY); + Header anonyAuthHeader = encodeBasicHeader(User.ANONYMOUS.getName(), randomAsciiAlphanumOfLength(8)); /* Create the tenant for the anonymous user to run the tests */ final String tenant = "anonymous_tenant"; @@ -671,12 +675,13 @@ public void testMultitenancyAnonymousUserReadOnlyActions() throws Exception { tenantExpectation.updateIndexStatusCode = HttpStatus.SC_FORBIDDEN; tenantExpectation.deleteIndexStatuCode = HttpStatus.SC_FORBIDDEN; - verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, /* Anonymous user*/ null); + verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, /* Anonymous user*/ anonyAuthHeader); } @Test public void testMultitenancyAnonymousUserWriteActionAllowed() throws Exception { setup(Settings.EMPTY, new DynamicSecurityConfig().setConfig("config_anonymous.yml"), Settings.EMPTY); + Header anonyAuthHeader = encodeBasicHeader(User.ANONYMOUS.getName(), randomAsciiAlphanumOfLength(8)); /* Create the tenant for the anonymous user to run the tests */ final String tenant = "opendistro_security_anonymous"; @@ -688,7 +693,7 @@ public void testMultitenancyAnonymousUserWriteActionAllowed() throws Exception { tenantExpectation.updateIndexStatusCode = HttpStatus.SC_OK; tenantExpectation.deleteIndexStatuCode = HttpStatus.SC_BAD_REQUEST; // tenant index cannot be deleted because its an alias - verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, /* Anonymous user*/ null); + verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, /* Anonymous user*/ anonyAuthHeader); } private static void verifyTenantActions( From 75d4a1784c1f8725f3cffd87dcd9148ad21b491d Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 11 Mar 2024 23:04:37 -0400 Subject: [PATCH 6/6] Fixes integrationtest CI Signed-off-by: Darshit Chanpura --- .../security/http/AnonymousAuthenticationTest.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/http/AnonymousAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/AnonymousAuthenticationTest.java index b1c13aeedc..bc874305de 100644 --- a/src/integrationTest/java/org/opensearch/security/http/AnonymousAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/AnonymousAuthenticationTest.java @@ -9,14 +9,19 @@ */ package org.opensearch.security.http; +import java.nio.charset.StandardCharsets; +import java.util.Base64; import java.util.List; import com.carrotsearch.randomizedtesting.RandomizedRunner; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.message.BasicHeader; import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; +import org.opensearch.security.user.User; import org.opensearch.test.framework.RolesMapping; import org.opensearch.test.framework.TestSecurityConfig; import org.opensearch.test.framework.cluster.ClusterManager; @@ -29,6 +34,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; +import static com.carrotsearch.randomizedtesting.RandomizedTest.randomAsciiAlphanumOfLength; @RunWith(RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -80,7 +86,13 @@ public class AnonymousAuthenticationTest { public void shouldAuthenticate_positive_anonymousUser() { try (TestRestClient client = cluster.getRestClient()) { - TestRestClient.HttpResponse response = client.getAuthInfo(); + Header anonyAuthHeader = new BasicHeader( + "Authorization", + "Basic " + + Base64.getEncoder() + .encodeToString((User.ANONYMOUS.getName() + ":" + randomAsciiAlphanumOfLength(8)).getBytes(StandardCharsets.UTF_8)) + ); + TestRestClient.HttpResponse response = client.getAuthInfo(anonyAuthHeader); response.assertStatusCode(200);