Skip to content

Commit d87ab3f

Browse files
DarshitChanpuraDarshitChanpura
authored
Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails (opensearch-project#4152)
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
1 parent ba74d14 commit d87ab3f

File tree

3 files changed

+147
-15
lines changed

3 files changed

+147
-15
lines changed

‎src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java

+112
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*
5+
* The OpenSearch Contributors require contributions made to
6+
* this file be licensed under the Apache-2.0 license or a
7+
* compatible open source license.
8+
*
9+
*/
10+
package org.opensearch.security.http;
11+
12+
import java.util.Map;
13+
14+
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
15+
import org.junit.ClassRule;
16+
import org.junit.Test;
17+
import org.junit.runner.RunWith;
18+
19+
import org.opensearch.test.framework.TestSecurityConfig.AuthcDomain;
20+
import org.opensearch.test.framework.TestSecurityConfig.User;
21+
import org.opensearch.test.framework.cluster.ClusterManager;
22+
import org.opensearch.test.framework.cluster.LocalCluster;
23+
import org.opensearch.test.framework.cluster.TestRestClient;
24+
import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse;
25+
26+
import static org.apache.http.HttpStatus.SC_OK;
27+
import static org.apache.http.HttpStatus.SC_UNAUTHORIZED;
28+
import static org.hamcrest.MatcherAssert.assertThat;
29+
import static org.hamcrest.Matchers.is;
30+
import static org.hamcrest.Matchers.notNullValue;
31+
32+
@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
33+
@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
34+
public class BasicWithAnonymousAuthTests {
35+
static final User TEST_USER = new User("test_user").password("s3cret");
36+
37+
public static final String CUSTOM_ATTRIBUTE_NAME = "superhero";
38+
static final User SUPER_USER = new User("super-user").password("super-password").attr(CUSTOM_ATTRIBUTE_NAME, "true");
39+
public static final String NOT_EXISTING_USER = "not-existing-user";
40+
public static final String INVALID_PASSWORD = "secret-password";
41+
42+
public static final AuthcDomain AUTHC_DOMAIN = new AuthcDomain("basic", 0).httpAuthenticatorWithChallenge("basic").backend("internal");
43+
44+
@ClassRule
45+
public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE)
46+
.anonymousAuth(true)
47+
.authc(AUTHC_DOMAIN)
48+
.users(TEST_USER, SUPER_USER)
49+
.build();
50+
51+
/** No automatic login post anonymous auth request **/
52+
@Test
53+
public void testShouldRespondWith401WhenUserDoesNotExist() {
54+
try (TestRestClient client = cluster.getRestClient(NOT_EXISTING_USER, INVALID_PASSWORD)) {
55+
HttpResponse response = client.getAuthInfo();
56+
57+
assertThat(response, is(notNullValue()));
58+
response.assertStatusCode(SC_UNAUTHORIZED);
59+
}
60+
}
61+
62+
@Test
63+
public void testShouldRespondWith401WhenUserNameIsIncorrect() {
64+
try (TestRestClient client = cluster.getRestClient(NOT_EXISTING_USER, TEST_USER.getPassword())) {
65+
HttpResponse response = client.getAuthInfo();
66+
67+
assertThat(response, is(notNullValue()));
68+
response.assertStatusCode(SC_UNAUTHORIZED);
69+
}
70+
}
71+
72+
@Test
73+
public void testShouldRespondWith401WhenPasswordIsIncorrect() {
74+
try (TestRestClient client = cluster.getRestClient(TEST_USER.getName(), INVALID_PASSWORD)) {
75+
HttpResponse response = client.getAuthInfo();
76+
77+
assertThat(response, is(notNullValue()));
78+
response.assertStatusCode(SC_UNAUTHORIZED);
79+
}
80+
}
81+
82+
/** Test `?auth_type=""` param to authinfo request **/
83+
@Test
84+
public void testShouldAutomaticallyLoginAsAnonymousIfNoCredentialsArePassed() {
85+
try (TestRestClient client = cluster.getRestClient()) {
86+
87+
HttpResponse response = client.getAuthInfo();
88+
89+
assertThat(response, is(notNullValue()));
90+
response.assertStatusCode(SC_OK);
91+
92+
HttpResponse response2 = client.getAuthInfo(Map.of("auth_type", "anonymous"));
93+
94+
assertThat(response2, is(notNullValue()));
95+
response2.assertStatusCode(SC_OK);
96+
}
97+
}
98+
99+
@Test
100+
public void testShouldNotAutomaticallyLoginAsAnonymousIfRequestIsNonAnonymousLogin() {
101+
try (TestRestClient client = cluster.getRestClient()) {
102+
103+
HttpResponse response = client.getAuthInfo(Map.of("auth_type", "saml"));
104+
105+
assertThat(response, is(notNullValue()));
106+
response.assertStatusCode(SC_UNAUTHORIZED);
107+
108+
// should contain a redirect link
109+
assertThat(response.containHeader("WWW-Authenticate"), is(true));
110+
}
111+
}
112+
}

‎src/main/java/org/opensearch/security/auth/BackendRegistry.java

+32-15
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import java.util.Collections;
3333
import java.util.HashSet;
3434
import java.util.List;
35+
import java.util.Map;
3536
import java.util.Optional;
3637
import java.util.Set;
3738
import java.util.SortedSet;
@@ -44,6 +45,7 @@
4445
import com.google.common.cache.RemovalListener;
4546
import com.google.common.cache.RemovalNotification;
4647
import com.google.common.collect.Multimap;
48+
import org.apache.http.HttpHeaders;
4749
import org.apache.logging.log4j.LogManager;
4850
import org.apache.logging.log4j.Logger;
4951

@@ -190,7 +192,7 @@ public void onDynamicConfigModelChanged(DynamicConfigModel dcm) {
190192
* @param request
191193
* @return The authenticated user, null means another roundtrip
192194
* @throws OpenSearchSecurityException
193-
*/
195+
*/
194196
public boolean authenticate(final SecurityRequestChannel request) {
195197
final boolean isDebugEnabled = log.isDebugEnabled();
196198
final boolean isBlockedBasedOnAddress = request.getRemoteAddress()
@@ -286,7 +288,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
286288

287289
if (ac == null) {
288290
// no credentials found in request
289-
if (anonymousAuthEnabled) {
291+
if (anonymousAuthEnabled && isRequestForAnonymousLogin(request.params(), request.getHeaders())) {
290292
continue;
291293
}
292294

@@ -386,19 +388,6 @@ public boolean authenticate(final SecurityRequestChannel request) {
386388
log.debug("User still not authenticated after checking {} auth domains", restAuthDomains.size());
387389
}
388390

389-
if (authCredentials == null && anonymousAuthEnabled) {
390-
final String tenant = resolveTenantFrom(request);
391-
User anonymousUser = new User(User.ANONYMOUS.getName(), new HashSet<String>(User.ANONYMOUS.getRoles()), null);
392-
anonymousUser.setRequestedTenant(tenant);
393-
394-
threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser);
395-
auditLog.logSucceededLogin(anonymousUser.getName(), false, null, request);
396-
if (isDebugEnabled) {
397-
log.debug("Anonymous User is authenticated");
398-
}
399-
return true;
400-
}
401-
402391
Optional<SecurityResponse> challengeResponse = Optional.empty();
403392

404393
if (firstChallengingHttpAuthenticator != null) {
@@ -415,6 +404,19 @@ public boolean authenticate(final SecurityRequestChannel request) {
415404
}
416405
}
417406

407+
if (authCredentials == null && anonymousAuthEnabled && isRequestForAnonymousLogin(request.params(), request.getHeaders())) {
408+
final String tenant = resolveTenantFrom(request);
409+
User anonymousUser = new User(User.ANONYMOUS.getName(), new HashSet<String>(User.ANONYMOUS.getRoles()), null);
410+
anonymousUser.setRequestedTenant(tenant);
411+
412+
threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser);
413+
auditLog.logSucceededLogin(anonymousUser.getName(), false, null, request);
414+
if (isDebugEnabled) {
415+
log.debug("Anonymous User is authenticated");
416+
}
417+
return true;
418+
}
419+
418420
log.warn(
419421
"Authentication finally failed for {} from {}",
420422
authCredentials == null ? null : authCredentials.getUsername(),
@@ -432,6 +434,21 @@ public boolean authenticate(final SecurityRequestChannel request) {
432434
return authenticated;
433435
}
434436

437+
/**
438+
* Checks if incoming auth request is from an anonymous user
439+
* Defaults all requests to yes, to allow anonymous authentication to succeed
440+
* @param params the query parameters passed in this request
441+
* @return false only if an explicit `auth_type` param is supplied, and its value is not anonymous, OR
442+
* if request contains no authorization headers
443+
* otherwise returns true
444+
*/
445+
private boolean isRequestForAnonymousLogin(Map<String, String> params, Map<String, List<String>> headers) {
446+
if (params.containsKey("auth_type")) {
447+
return params.get("auth_type").equals("anonymous");
448+
}
449+
return !headers.containsKey(HttpHeaders.AUTHORIZATION);
450+
}
451+
435452
private String resolveTenantFrom(final SecurityRequest request) {
436453
return Optional.ofNullable(request.header("securitytenant")).orElse(request.header("security_tenant"));
437454
}

‎src/main/java/org/opensearch/security/rest/SecurityInfoAction.java

+3
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ public List<Route> routes() {
9191
@Override
9292
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
9393
final boolean verbose = request.paramAsBoolean("verbose", false);
94+
// need to consume `auth_type` param, without which a 500 is thrown on front-end
95+
final String authType = request.param("auth_type", "");
96+
9497
return new RestChannelConsumer() {
9598

9699
@Override

0 commit comments

Comments
 (0)