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

[StewartWBrown]

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.

[shikharj05]

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?

[RyanL1997]

+1 to @shikharj05

[terryquigleysas]

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:

1. Pass the isChallenge flag from the authDomain to the extractCredentials calls. Only log.warn when isChallenge is true
2. 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

[StewartWBrown]

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!

[codecov]

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 11 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.

[StewartWBrown]

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.