Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skips authentication only for anonymous auth requests when anonymous-auth is enabled #4097

Closed
wants to merge 7 commits into from
Closed

Skips authentication only for anonymous auth requests when anonymous-auth is enabled #4097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d7af7eb
Adds a check to only skip authentication for anonymous requests when …
DarshitChanpura Mar 6, 2024
bbde01b
Adds a check for anonymous user creds
DarshitChanpura Mar 11, 2024
08ea955
Fixes spotless errors and removes log statements
DarshitChanpura Mar 11, 2024
0028c9f
Re-adds removed return statement
DarshitChanpura Mar 11, 2024
afacf33
Fixes citest and ciSecurityIntegrationTest
DarshitChanpura Mar 12, 2024
75d4a17
Fixes integrationtest CI
DarshitChanpura Mar 12, 2024
270f68c
Merge remote-tracking branch 'upstream/main' into anonymous-saml-bug-fix
DarshitChanpura Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/integrationTest/java/org/opensearch/security/http/AnonymousAuthenticationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-capping conversation on here. Anonymous will mean requests without credentials presented. If a request presents credentials and they are bogus credentials, then it should fail to authenticate the request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anonymous auth currently creates an anonymous user in the backend to be added to thread context.

The root problem of this issue is that the current approach creates an anonymous user if no auth credentials were found, which means that the flow would never reach SAML IdP server since this block will return true authentication is re-requested hence leading to Invalid SAML Configuration error since it was never able to reach SAML IdP server.

To solve this problem, we need to identify whether the request is coming as anonymous user or not. Following options were considered:

  1. Add a header { _auth_request_type_: anonymous } to incoming requests from anonymous. But this somehow got overwritten when a request was sent to read dashboards config. So this option was tabled.
  2. Add auth creds from front-end to let backend know that this request is coming as anonymous user. This is the current approach and it allows differentiating auth provider requests with anonymous auth requests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not just swap the order of processing by moving the block you linked to after any SAML auth attempts?

Copy link
Member Author

@DarshitChanpura DarshitChanpura Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already tried that. and that is the conversation Craig was referring to, to swap some code around to see if it works. My question is why are we opposed to using dummy creds coming from front-end, when we any way create an anonymous user on the backend? These creds are not requested from an user, instead they are generated on the fly when a user tries to log in as anonymous. In this approach we're not using the password part of the creds, the only thing the backends needs is the username part to match the anonymous user's name.

As I mentioned in the previous comment, both, anonymous auth + any IdP call, expect credentials to be empty in order to follow correct path, and the current implementation skips check over auth domains if anonymous auth is enabled. Once the for loop completes it then enters this else block to assume anonymous user identity.

To avoid adding auth creds, I moved the if block where we assume anonymous user identity at the end post re-requestAuthentication call, but still resulted in 500 for SAML with Invalid SAML config error.

Copy link
Member Author

@DarshitChanpura DarshitChanpura Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, anonymous auth is considered a sub-part of basic auth (username + password) in the design, so I followed that same pattern in addressing this.

This would only cause some issues for API callers without passing creds. They would now have to pass in a basic auth header with anonymous user name and a random string as password. No experience has changed on front-end.

One possible solution, which is much larger than this bug fix is to re-write the authenticate method to ensure that anonymous auth is always checked at the very end outside loop and other authenticate check, but I'm not sure how would Log in as anonymous react in the case where multiple auth domains are enabled. How would we identify the type of login request?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the API calls - I have no concerns from front end, since we are hooking up all the buttons anyway and can make the change appropriately, but not sure if it is considered a breaking change to require a username after this change

);
TestRestClient.HttpResponse response = client.getAuthInfo(anonyAuthHeader);

response.assertStatusCode(200);

Expand Down
29 changes: 16 additions & 13 deletions src/main/java/org/opensearch/security/auth/BackendRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,6 @@

if (ac == null) {
// no credentials found in request
if (anonymousAuthEnabled) {
continue;
}

if (authDomain.isChallenge()) {
final Optional<SecurityResponse> restResponse = httpAuthenticator.reRequestAuthentication(request, null);
if (restResponse.isPresent()) {
Expand All @@ -309,6 +305,10 @@
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
Expand Down Expand Up @@ -386,17 +386,20 @@
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<String>(User.ANONYMOUS.getRoles()), null);
anonymousUser.setRequestedTenant(tenant);
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<String>(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");
threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser);
auditLog.logSucceededLogin(anonymousUser.getName(), false, null, request);
if (isDebugEnabled) {
log.debug("Anonymous User is authenticated");

Check warning on line 399 in src/main/java/org/opensearch/security/auth/BackendRegistry.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/auth/BackendRegistry.java#L399 

Added line #L399 was not covered by tests
}
return true;
}
return true;
}

Optional<SecurityResponse> challengeResponse = Optional.empty();
Expand Down
9 changes: 6 additions & 3 deletions src/test/java/org/opensearch/security/HttpIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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());

Expand Down
5 changes: 4 additions & 1 deletion src/test/java/org/opensearch/security/InitializationIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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<TransportAddress> iterator = clusterInfo.httpAdresses.iterator(); iterator.hasNext();) {
TransportAddress TransportAddress = iterator.next();
HttpResponse res = rh.executeRequest(
Expand All @@ -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"));
Expand Down
5 changes: 4 additions & 1 deletion src/test/java/org/opensearch/security/SecurityRolesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {

Expand All @@ -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());
Expand Down
13 changes: 9 additions & 4 deletions src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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";
Expand All @@ -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";
Expand All @@ -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(
Expand Down
Loading