Changing logging type to give warning for basic auth with no creds

[abhivka7]

Signed-off-by: Abhi Kalra [email protected]

Description

[Describe what this change achieves]

• Category
  Bug Fix

• Why these changes are required?
  Changing logging type to give warning for basic auth with no creds.

• What is the old behavior before changes and new behavior after changes?
  When using basic auth without credentials, users just get a message telling "Unauthorized" which is not very descriptive. As an admin, I don't have any visibility into why my users are not able to login without enabling this log which is currently at trace level. Hence, changing log level to warn.

Issues Resolved

#2346

Is this a backport? If so, please add backport PR # and/or commits #
We want to backport it to latest OS versions.

Signed-off-by: Abhi Kalra [email protected]

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.

[codecov-commenter]

Codecov Report

Merging #2347 (54c8e92) into main (aad9379) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2347      +/-   ##
============================================
- Coverage     61.15%   61.07%   -0.09%     
+ Complexity     3273     3269       -4     
============================================
  Files           259      260       +1     
  Lines         18337    18369      +32     
  Branches       3248     3251       +3     
============================================
+ Hits          11214    11218       +4     
- Misses         5538     5563      +25     
- Partials       1585     1588       +3     

Impacted Files	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	62.74% <100.00%> (ø)
...t/keybyoidc/AuthenticatorUnavailableException.java	0.00% <0.00%> (-20.00%)	⬇️
.../auth/http/jwt/keybyoidc/SelfRefreshingKeySet.java	59.85% <0.00%> (-8.46%)	⬇️
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java	55.81% <0.00%> (-3.49%)	⬇️
...ecurity/configuration/ConfigurationRepository.java	72.13% <0.00%> (-2.19%)	⬇️
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	57.46% <0.00%> (-1.50%)	⬇️
...arch/security/ssl/OpenSearchSecuritySSLPlugin.java	78.82% <0.00%> (-1.29%)	⬇️
...ensearch/security/compliance/ComplianceConfig.java	82.63% <0.00%> (-0.70%)	⬇️
...ensearch/security/ssl/DefaultSecurityKeyStore.java	67.28% <0.00%> (-0.55%)	⬇️
...a/org/opensearch/security/tools/SecurityAdmin.java	36.60% <0.00%> (-0.24%)	⬇️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[stephen-crawford]

In light of the previous PR's coversation (#2334), I think it may be necessary to first implement a precaution against DoS attacks or justification for why this change cannot wait until such a precaution is in place.

[abhivka7]

implement a precaution against DoS attacks

Security plugin does not cover any implementation of precaution against DoS attack. That has to be done from infrastructure change. if needed we can create a backlog to see what can be done from our end.
Anyways, having a log to tell if there is an attack is still needed and therefore the PR should be merged.

[davidlago]

I'm struggling to see how this is any different than logging any other messages for unauthenticated requests. If you get millions of them, yes, your logs are filling up but also you have other resources under strain as well that will probably be an issue and/or be detected and acted upon (and you might find those logs useful after all?)

Based on @peternied 's comment here, perhaps the compromise is to let this type of error log level be configurable, default it to current level and let people override. Right now it seems like OP is already resorting to setting it to trace to be able to see these, which is an even worse scenario if we're thinking of DoS, so adding this as a configurable level will consider all users' needs while preserving our current default behavior.

wdyt?

[varun-lodaya]

I think we should decouple the problem of logs filling up disk from adding more logs, since there are tons of places where we log lack of FGAC permissions at request level or SAML JWT failures for every request. Eg -

• security/src/main/java/com/amazon/dlic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java

  Line 173 in b836040

  log.warn("Failed to get subject from JWT claims, check if subject_key '{}' is correct.", subjectKey);

• security/src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java

  Line 73 in f431ec2

  log.warn("Tenant {} is not allowed for user {}", requestedTenant, user.getName());

Moreover, as @davidlago mentioned, if a user is sending millions of request, they are probably already stressing other system resources.

[stephen-crawford]

@RyanL1997 @cwperks @DarshitChanpura could one of you please merge this. Thank you!