Skip to content

Change HTTPBasicAuthenticator log message from warning to trace for 'No basic auth' error #5221

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

Closed
wants to merge 4 commits into from
Closed

Change HTTPBasicAuthenticator log message from warning to trace for 'No basic auth' error #5221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a10519
change log message from warning to trace for 'No basic auth' error
StewartWBrown1 Mar 14, 2025
4e54b7b
Merge branch 'opensearch-project:main' into silence-no-basic-auth-error
StewartWBrown Apr 24, 2025
7460337
add isChallenge flag to extractCredentials - print error as warn if c…
StewartWBrown1 Apr 25, 2025
ba9ad1d
fixes for code hygiene spotless scan
StewartWBrown1 Apr 25, 2025
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
2 changes: 1 addition & 1 deletion 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 @@ -279,7 +279,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
}
final AuthCredentials ac;
try {
ac = httpAuthenticator.extractCredentials(request, threadPool.getThreadContext());
ac = httpAuthenticator.extractCredentials(request, threadPool.getThreadContext(), authDomain.isChallenge());
} catch (Exception e1) {
if (isDebugEnabled) {
log.debug("'{}' extracting credentials from {} http authenticator", e1.toString(), httpAuthenticator.getType(), e1);
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/org/opensearch/security/auth/HTTPAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,15 @@ public interface HTTPAuthenticator {
*
* @param request The rest request
* @param context The current thread context
* @param isChallenge The challenge setting of authentication method currently being evaluated
* @return The authentication credentials (complete or incomplete) or null when no credentials are found in the request
* <p>
* When the credentials could be fully extracted from the request {@code .markComplete()} must be called on the {@link AuthCredentials} which are returned.
* If the authentication flow needs another roundtrip with the request originator do not mark it as complete.
* @throws OpenSearchSecurityException
*/
AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context) throws OpenSearchSecurityException;
AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context, final boolean isChallenge)
throws OpenSearchSecurityException;

/**
* If the {@code extractCredentials()} call was not successful or the authentication flow needs another roundtrip this method
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/auth/http/jwt/AbstractHTTPJwtAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public AbstractHTTPJwtAuthenticator(Settings settings, Path configPath) {

@Override
@SuppressWarnings("removal")
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context)
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context, final boolean isChallenge)
throws OpenSearchSecurityException {
final SecurityManager sm = System.getSecurityManager();

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/auth/http/jwt/HTTPJwtAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public HTTPJwtAuthenticator(final Settings settings, final Path configPath) {

@Override
@SuppressWarnings("removal")
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context)
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context, final boolean isChallenge)
throws OpenSearchSecurityException {
final SecurityManager sm = System.getSecurityManager();

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/auth/http/kerberos/HTTPSpnegoAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public Void run() {

@Override
@SuppressWarnings("removal")
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext) {
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext, final boolean isChallenge) {
final SecurityManager sm = System.getSecurityManager();

if (sm != null) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/auth/http/saml/HTTPSamlAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,15 @@
}

@Override
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext)
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext, final boolean isChallenge)
throws OpenSearchSecurityException {
Matcher matcher = PATTERN_PATH_PREFIX.matcher(request.path());
final String suffix = matcher.matches() ? matcher.group(2) : null;
if (API_AUTHTOKEN_SUFFIX.equals(suffix)) {
return null;
}

AuthCredentials authCredentials = this.httpJwtAuthenticator.extractCredentials(request, threadContext);
AuthCredentials authCredentials = this.httpJwtAuthenticator.extractCredentials(request, threadContext, isChallenge);

Check warning on line 169 in src/main/java/org/opensearch/security/auth/http/saml/HTTPSamlAuthenticator.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/auth/http/saml/HTTPSamlAuthenticator.java#L169 

Added line #L169 was not covered by tests

if (AUTHINFO_SUFFIX.equals(suffix)) {
this.initLogoutUrl(threadContext, authCredentials);
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/http/HTTPBasicAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public HTTPBasicAuthenticator(final Settings settings, final Path configPath) {
}

@Override
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext) {
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext, final boolean isChallenge) {

final boolean forceLogin = Boolean.getBoolean(request.params().get("force_login"));

Expand All @@ -62,7 +62,7 @@ public AuthCredentials extractCredentials(final SecurityRequest request, final T

final String authorizationHeader = request.header("Authorization");

return HTTPHelper.extractCredentials(authorizationHeader, log);
return HTTPHelper.extractCredentials(authorizationHeader, log, isChallenge);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/http/HTTPClientCertAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public HTTPClientCertAuthenticator(final Settings settings, final Path configPat
}

@Override
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext) {
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext threadContext, final boolean isChallenge) {

final String principal = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_PRINCIPAL);

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/http/HTTPProxyAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public HTTPProxyAuthenticator(Settings settings, final Path configPath) {
}

@Override
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context) {
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context, final boolean isChallenge) {

if (context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_XFF_DONE) != Boolean.TRUE) {
throw new OpenSearchSecurityException("xff not done");
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ private String[] extractBackendRolesFromClaims(Claims claims) {

@Override
@SuppressWarnings("removal")
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context)
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context, final boolean isChallenge)
throws OpenSearchSecurityException {
final SecurityManager sm = System.getSecurityManager();

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/http/proxy/HTTPExtendedProxyAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ public HTTPExtendedProxyAuthenticator(Settings settings, final Path configPath)
}

@Override
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context) {
AuthCredentials credentials = super.extractCredentials(request, context);
public AuthCredentials extractCredentials(final SecurityRequest request, final ThreadContext context, final boolean isChallenge) {
AuthCredentials credentials = super.extractCredentials(request, context, isChallenge);
if (credentials == null) {
return null;
}
Expand Down
10 changes: 8 additions & 2 deletions src/main/java/org/opensearch/security/support/HTTPHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,17 @@

public class HTTPHelper {

public static AuthCredentials extractCredentials(String authorizationHeader, Logger log) {
public static AuthCredentials extractCredentials(String authorizationHeader, Logger log, boolean isChallenge) {

boolean isTraceEnabled = log.isTraceEnabled();

if (authorizationHeader != null) {
if (!authorizationHeader.trim().toLowerCase().startsWith("basic ")) {
log.warn("No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic'");
if (isChallenge) {
Copy link
Member

@DarshitChanpura DarshitChanpura Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Apologies for joining the party late, i'm slightly confused on what exactly are we trying to solve here? Is it that we log at warn only when Basic Auth fails or is it that we will log at warn when no other domains to be evaluated against are present?
I'm okay with current implementation if the idea is to simply reduce warning messages for auth types other than Basic.

Copy link
Member

@cwperks cwperks Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The goal is to only log this in valid scenarios. The following criteria needs to be met:

  1. Request has no Authorization header
  2. There is a challenging basic auth authenticator
  3. Request is not for the SAML ACS endpoint which does not accept an Authorization header (its the endpoint that accepts the XML assertion) and is used in the SAML flow.

Currently, this is getting logged if a request has an Authorization header of a different kind (like a bearer token) and its annoying because its being logged excessively and its not accurate.

Copy link
Member

Choose a reason for hiding this comment

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

If the idea is to limit logging of this message only when basic auth is the challenging authenticator and creds fail, I'm okay with this flag.

log.warn("No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic'");
} else if (isTraceEnabled) {
log.trace("No 'Basic Authorization' header");

Check warning on line 50 in src/main/java/org/opensearch/security/support/HTTPHelper.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/support/HTTPHelper.java#L50 

Added line #L50 was not covered by tests
}
return null;
} else {

Expand Down
26 changes: 13 additions & 13 deletions src/test/java/org/opensearch/security/IntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,30 +108,30 @@ public void testSearchScroll() throws Exception {
public void testDnParsingCertAuth() throws Exception {
Settings settings = Settings.builder().put("username_attribute", "cn").put("roles_attribute", "l").build();
HTTPClientCertAuthenticator auth = new HTTPClientCertAuthenticator(settings, null);
assertThat(auth.extractCredentials(null, newThreadContext("cn=abc,cn=xxx,l=ert,st=zui,c=qwe")).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,st=zui,c=qwe")).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("CN=abc,L=ert,st=zui,c=qwe")).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("l=ert,cn=abc,st=zui,c=qwe")).getUsername(), is("abc"));
Assert.assertNull(auth.extractCredentials(null, newThreadContext("L=ert,CN=abc,c,st=zui,c=qwe")));
assertThat(auth.extractCredentials(null, newThreadContext("l=ert,st=zui,c=qwe,cn=abc")).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("L=ert,st=zui,c=qwe,CN=abc")).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("L=ert,st=zui,c=qwe")).getUsername(), is("L=ert,st=zui,c=qwe"));
assertThat(auth.extractCredentials(null, newThreadContext("cn=abc,cn=xxx,l=ert,st=zui,c=qwe"), false).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("cn=abc,cn=xxx,l=ert,st=zui,c=qwe"), true).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,st=zui,c=qwe"), false).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("CN=abc,L=ert,st=zui,c=qwe"), false).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("l=ert,cn=abc,st=zui,c=qwe"), false).getUsername(), is("abc"));
Assert.assertNull(auth.extractCredentials(null, newThreadContext("L=ert,CN=abc,c,st=zui,c=qwe"), false));
assertThat(auth.extractCredentials(null, newThreadContext("l=ert,st=zui,c=qwe,cn=abc"), false).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("L=ert,st=zui,c=qwe,CN=abc"), false).getUsername(), is("abc"));
assertThat(auth.extractCredentials(null, newThreadContext("L=ert,st=zui,c=qwe"), false).getUsername(), is("L=ert,st=zui,c=qwe"));
Assert.assertArrayEquals(
new String[] { "ert" },
auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,st=zui,c=qwe")).getBackendRoles().toArray(new String[0])
auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,st=zui,c=qwe"), false).getBackendRoles().toArray(new String[0])
);
Assert.assertArrayEquals(
new String[] { "bleh", "ert" },
new TreeSet<>(auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,L=bleh,st=zui,c=qwe")).getBackendRoles()).toArray(
new String[0]
)
new TreeSet<>(auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,L=bleh,st=zui,c=qwe"), false).getBackendRoles())
.toArray(new String[0])
);

settings = Settings.builder().build();
auth = new HTTPClientCertAuthenticator(settings, null);
assertThat(
"cn=abc,l=ert,st=zui,c=qwe",
is(auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,st=zui,c=qwe")).getUsername())
is(auth.extractCredentials(null, newThreadContext("cn=abc,l=ert,st=zui,c=qwe"), false).getUsername())
);
}

Expand Down
Loading
Loading