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

StewartWBrown · 2025-03-27T16:56:29Z

Description

When basic auth and SAML authentications are both enabled, the logs are filled with messages stating:
"level": "WARN", "component": "o.o.s.h.HTTPBasicAuthenticator", "message": "No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic'"

This was thought to be addressed by #3364, however the logging message remained in another area which this PR has tuned to trace level instead.

Issues Resolved

[BUG] Annoying warning message "No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic'" still not fixed #4054
[BUG] Lots of waring message No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic' in log #3273

Testing

Bulk Integration testing workflow with this change seems to look fine!

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Stewart Brown <stewart.brown@sas.com>

shikharj05 · 2025-03-27T18:07:10Z

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

-            if (!authorizationHeader.trim().toLowerCase().startsWith("basic ")) {
-                log.warn("No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic'");
+            if (!authorizationHeader.trim().toLowerCase().startsWith("basic ") && isTraceEnabled) {
+                log.trace("No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic'");


Thanks for the PR. While I understand noise created when multiple auth types are used, moving this line to trace logging might be an issue for clusters with only basic auth enabled. I don't think there's another log line available.

For #4054, would it be possible to change the order , i.e. mark SAML as order 1 and basic auth as order 2? Do you still see same amount of logs?

+1 to @shikharj05

I also agree that changing to trace isn't the answer but the behavior should change for valid scenarios requiring multiple authenticators using fallthrough.

The fix required could be one of the following:

Pass the isChallenge flag from the authDomain to the extractCredentials calls. Only log.warn when isChallenge is true

Alternatively, and probably a more complete fix, would be be to pass the firstChallengingHttpAuthenticator to the extractCredentials calls. Only log.warn when the firstChallengingHttpAuthenticator is of type HTTPBasicAuthenticator

The starting point for these changes would be the httpAuthenticator.extractCredentials call at https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/auth/BackendRegistry.java#L282

Thanks all! Initially raised this PR as i felt that it was odd that basic auth was the only authentication type logging a warn message on checking that the header was in the expected format- leaving this message-spam only occurring when basic auth was earlier in the order.

But i understand this is could be a helpful message, so banishing it away to Trace or Debug may not be useful (especially for a more common auth type like Basic). I agree with Terry that passing isChallenge may be the way forward in this case. Especially after reading the docs, which at the moment seems inconsistent with this logging message: https://opensearch.org/docs/latest/security/authentication-backends/basic-authc/#the-challenge-setting

Will look into this a little more later this week with a bit more of a complete fix!

@shikharj05 This log line is available elsewhere in the BackendRegistry here: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/auth/BackendRegistry.java#L352-L354

tbh, if the log level is a concern I would suggest to remove this line and make the one in BackendRegistry warn level to solve this issue.

Something like this: cwperks#57

Something like this: cwperks#57

@cwperks Yes, removing the problematic log line, as you have here, should also resolve it.

codecov · 2025-03-30T20:11:50Z

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.69%. Comparing base (cac58bc) to head (ba9ad1d).
Report is 58 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/org/opensearch/security/support/HTTPHelper.java	40.00%	2 Missing and 1 partial ⚠️
...security/auth/http/saml/HTTPSamlAuthenticator.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5221      +/-   ##
==========================================
- Coverage   72.04%   71.69%   -0.36%     
==========================================
  Files         336      384      +48     
  Lines       22648    23862    +1214     
  Branches     3560     3635      +75     
==========================================
+ Hits        16317    17107     +790     
- Misses       4556     4931     +375     
- Partials     1775     1824      +49

Files with missing lines	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	`77.77% <100.00%> (ø)`
...rg/opensearch/security/auth/HTTPAuthenticator.java	`100.00% <ø> (ø)`
...ty/auth/http/jwt/AbstractHTTPJwtAuthenticator.java	`60.43% <ø> (ø)`
...h/security/auth/http/jwt/HTTPJwtAuthenticator.java	`80.32% <ø> (ø)`
...ty/auth/http/kerberos/HTTPSpnegoAuthenticator.java	`0.00% <ø> (ø)`
...ensearch/security/http/HTTPBasicAuthenticator.java	`81.81% <100.00%> (ø)`
...rch/security/http/HTTPClientCertAuthenticator.java	`88.57% <ø> (ø)`
...ensearch/security/http/HTTPProxyAuthenticator.java	`82.14% <ø> (ø)`
...nsearch/security/http/OnBehalfOfAuthenticator.java	`85.84% <ø> (ø)`
...ity/http/proxy/HTTPExtendedProxyAuthenticator.java	`84.00% <100.00%> (ø)`
... and 2 more

... and 57 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…hallenge is true Signed-off-by: Stewart Brown <stewart.brown@sas.com>

Signed-off-by: Stewart Brown <stewart.brown@sas.com>

StewartWBrown · 2025-04-25T13:17:44Z

Have added in the isChallenge flag to extractCredentials as suggested! Log the error message as a warn if 'challenge' is enabled, and at trace level if 'trace' is enabled. Otherwise do not print the error which will hopefully reduce the log spam being seen when security configs are set up in this specific ordering.

Sorry for delay, other things popped up over past few weeks! Still need to test this works as expected, and will have time to do so at the start of next week.

cwperks · 2025-05-07T15:20:39Z

@StewartWBrown Sorry for the delay on review. I was out at OpenSearchCon EU last week and back in the office this week.

cwperks · 2025-05-07T19:19:19Z

@StewartWBrown I think the fix for this can be even simpler. wdyt about only logging the No 'Basic Authorization' header, send 401 and 'WWW-Authenticate Basic' message if the Authorization header is missing?

It doesn't make sense to log that out when the Authorization header is present, but of a different type of token that is not a basic auth header.

StewartWBrown · 2025-05-07T22:20:13Z

@cwperks No worries at all - hope OpenSearchCon EU went well! Looking forward to catch up on some sessions if they become available online :)

Although this would stop the log message if using JWT authentication for example (where the header is present, but with a different type of token). Wouldn't this issue still persist if using SAML which I believe doesn't have an Authorization header? Not too familiar with some of the other Auth methods OpenSearch can use so not sure if there's others that fall in the same boat too.

nibix · 2025-05-08T03:16:03Z

I'd just give my five cents here, but then I'll immediately disappear again, because I have two days off ;-)

It can be helpful to just consult the guidelines for using log4j log levels at https://commons.apache.org/proper/commons-logging/guide.html#Message_Priorities.2FLevels

WARN: Use of deprecated APIs, poor use of API, 'almost' errors, other runtime situations that are undesirable or unexpected, but not necessarily "wrong". Expect these to be immediately visible on a status console.

IMHO, these criteria do not match the absence of an "Authorization" header. This is not an undesirable or unexpected condition, it is part of a normal protocol flow.

Let's check the guidelines for INFO:

INFO: Interesting runtime events (startup/shutdown). Expect these to be immediately visible on a console, so be conservative and keep to a minimum.

Also for this, I'd say that the criteria is not met. It will be impossible to keep them at a minimum if these are caused by normal protocol flows.

Thus, my opinion would be to just generally lower the log level to DEBUG.

StewartWBrown · 2025-05-08T05:46:35Z

For further support of changing the message logging level, I can see similar checks to this one for different Auth types happening at either Debug or Trace:

HTTPProxyAuthenticator, OnBehalfOf, HTTPClientCertAuthenticator, JWT Authentication.

From @nibix suggestion above, I think these should all be consistently Debug instead of Trace too which I can change as part of this PR.

That being said, I can see the value in raising a warning for Basic Auth as it's likely to be more commonly used. Making the message more visible could help with quicker diagnosis- which is why I felt the isChallenge tag approach was an appropriate middleground for this scenario. But I recognise the perspective that having an extra parameter passed into extractCredentials could be a bit overkill. Unless there's additional benefit of passing challenge context for other authentication methods' extractCredentials too which we could utilize?

cwperks · 2025-05-08T13:35:05Z

Wouldn't this issue still persist if using SAML which I believe doesn't have an Authorization header?

I would need to check, but it would still certainly reduce the log volume immensely since SAML login does not occur frequently. FYI The SAML login flow occurs once per OSD session. After successful login, OpenSearch will issue a JWT and then use that for subsequent requests against the backend cluster.

I wonder if there's another value to look for to determine if a request is in the initial SAML flow to skip this message.

terryquigleysas · 2025-06-02T16:07:31Z

@cwperks Thank you for revisiting this. We found that the change submitted here was the least intrusive that would actually fix the bug correctly.

The basic auth check should only log that it is returning a 401 when set to challenge so we passed that flag through. Note we also have a finalizing basic authenticator that is set to challenge and will log and behave appropriately when authentication details are missing.

The first basic auth check cannot challenge as we have others that can follow, e.g. JWT , that use the header. For other auth types it seems better to check for the challenge flag too rather than relying on specific text included in the header - this is also to ensure it is future-proof.

cwperks · 2025-06-02T20:22:06Z

@terryquigleysas I think there's an opportunity to make the logic much simpler and achieve the same outcome without having to keep track of whether the authenticator is a challenging authenticator.

If these 2 conditions are met:

Request lacks Authorization HTTP Header
The route is not /_opendistro/_security/saml/acs <- This is the ACS SAML endpoint where the request body contains the XML SAML assertion that we change to a JWT for the remainder of the session and will not have the Authorization http header.

Then we should log out that warning, otherwise skip it. The current logic is definitely wrong and should not log a warning when an Authorization header is present..of any kind.

terryquigleysas · 2025-06-03T08:02:40Z

@terryquigleysas I think there's an opportunity to make the logic much simpler and achieve the same outcome without having to keep track of whether the authenticator is a challenging authenticator.

If these 2 conditions are met:

Request lacks Authorization HTTP Header

The route is not /_opendistro/_security/saml/acs <- This is the ACS SAML endpoint where the request body contains the XML SAML assertion that we change to a JWT for the remainder of the session and will not have the Authorization http header.

Then we should log out that warning, otherwise skip it. The current logic is definitely wrong and should not log a warning when an Authorization header is present..of any kind.

Thanks @cwperks , we will take a look again with this suggestion.

DarshitChanpura · 2025-06-03T19:36:22Z

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

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


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.

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

Request has no Authorization header

There is a challenging basic auth authenticator

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.

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.

terryquigleysas · 2025-06-11T09:17:31Z

@StewartWBrown This can be closed off now #5377 has been merged.
Thanks for the PR.

change log message from warning to trace for 'No basic auth' error

3a10519

Signed-off-by: Stewart Brown <stewart.brown@sas.com>

StewartWBrown requested review from DarshitChanpura, RyanL1997, cwperks, derek-ho, nibix, peternied, reta and willyborankin as code owners March 27, 2025 16:56

shikharj05 reviewed Mar 27, 2025

View reviewed changes

StewartWBrown and others added 3 commits April 24, 2025 15:38

Merge branch 'opensearch-project:main' into silence-no-basic-auth-error

4e54b7b

add isChallenge flag to extractCredentials - print error as warn if c…

7460337

…hallenge is true Signed-off-by: Stewart Brown <stewart.brown@sas.com>

fixes for code hygiene spotless scan

ba9ad1d

Signed-off-by: Stewart Brown <stewart.brown@sas.com>

DarshitChanpura reviewed Jun 3, 2025

View reviewed changes

This was referenced Jun 6, 2025

Simplify logic for when log challenge for basic authenticator cwperks/security#57

Open

Only log Invalid Authorization header when HTTP Basic auth challenge is called #5377

Merged

StewartWBrown closed this Jun 11, 2025

Uh oh!

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

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

Uh oh!

Conversation

StewartWBrown commented Mar 27, 2025

Description

Issues Resolved

Testing

Check List

Uh oh!

shikharj05 Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanL1997 Mar 30, 2025

Choose a reason for hiding this comment

Uh oh!

terryquigleysas Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

StewartWBrown Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

cwperks Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

cwperks Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

terryquigleysas Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

StewartWBrown commented Apr 25, 2025

Uh oh!

cwperks commented May 7, 2025

Uh oh!

cwperks commented May 7, 2025

Uh oh!

StewartWBrown commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nibix commented May 8, 2025

Uh oh!

StewartWBrown commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwperks commented May 8, 2025

Uh oh!

terryquigleysas commented Jun 2, 2025

Uh oh!

cwperks commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryquigleysas commented Jun 3, 2025

Uh oh!

DarshitChanpura Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

terryquigleysas commented Jun 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

shikharj05 Mar 27, 2025 •

edited

Loading

codecov bot commented Mar 30, 2025 •

edited

Loading

StewartWBrown commented May 7, 2025 •

edited

Loading

StewartWBrown commented May 8, 2025 •

edited

Loading

cwperks commented Jun 2, 2025 •

edited

Loading

DarshitChanpura Jun 3, 2025 •

edited

Loading

cwperks Jun 3, 2025 •

edited

Loading