Skips authentication only for anonymous auth requests when anonymous-auth is enabled

[DarshitChanpura]

Description

This PR fixes a bug where SAML and Anonymous auth cannot co-exist. At present, enabling SAML along with anonymous auth causes SAML Login to fail before it hits the IdP because both SAML and anonymous auth are wired to not have credentials on the first login attempt, causing the authn check against the auth domains to be skipped (see here). This, eventually, results in setting the default user as anonymous (see here) since anonymous auth is enabled.

• Category: Bug fix
• Why these changes are required?
  • To allow SAML auth to work when anonymous auth is enabled

Security-dashboards companion PR: opensearch-project/security-dashboards-plugin#1817

Issues Resolved

• Related [BUG] Having Anonymous and SAML as sign in options does not work. SAML Login fails when Anonymous is enabled security-dashboards-plugin#1731

Testing

• Automated + Manual

Check List

• New functionality includes testing
  - [ ] New functionality has been documented
• 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.

[codecov]

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 65.91%. Comparing base (d526c9f) to head (270f68c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4097   +/-   ##
=======================================
  Coverage   65.90%   65.91%           
=======================================
  Files         298      298           
  Lines       21352    21355    +3     
  Branches     3475     3477    +2     
=======================================
+ Hits        14073    14077    +4     
+ Misses       5540     5537    -3     
- Partials     1739     1741    +2     

Files	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	62.58% <76.92%> (+0.04%)	⬆️

... and 3 files with indirect coverage changes

[cwperks]

Re-capping conversation on here. Anonymous will mean requests without credentials presented. If a request presents credentials and they are bogus credentials, then it should fail to authenticate the request.

[DarshitChanpura]

anonymous auth currently creates an anonymous user in the backend to be added to thread context.

The root problem of this issue is that the current approach creates an anonymous user if no auth credentials were found, which means that the flow would never reach SAML IdP server since this block will return true authentication is re-requested hence leading to Invalid SAML Configuration error since it was never able to reach SAML IdP server.

To solve this problem, we need to identify whether the request is coming as anonymous user or not. Following options were considered:

1. Add a header { _auth_request_type_: anonymous } to incoming requests from anonymous. But this somehow got overwritten when a request was sent to read dashboards config. So this option was tabled.
2. Add auth creds from front-end to let backend know that this request is coming as anonymous user. This is the current approach and it allows differentiating auth provider requests with anonymous auth requests.

[stephen-crawford]

Can we not just swap the order of processing by moving the block you linked to after any SAML auth attempts?

[DarshitChanpura]

already tried that. and that is the conversation Craig was referring to, to swap some code around to see if it works. My question is why are we opposed to using dummy creds coming from front-end, when we any way create an anonymous user on the backend? These creds are not requested from an user, instead they are generated on the fly when a user tries to log in as anonymous. In this approach we're not using the password part of the creds, the only thing the backends needs is the username part to match the anonymous user's name.

As I mentioned in the previous comment, both, anonymous auth + any IdP call, expect credentials to be empty in order to follow correct path, and the current implementation skips check over auth domains if anonymous auth is enabled. Once the for loop completes it then enters this else block to assume anonymous user identity.

To avoid adding auth creds, I moved the if block where we assume anonymous user identity at the end post re-requestAuthentication call, but still resulted in 500 for SAML with Invalid SAML config error.

[DarshitChanpura]

Also, anonymous auth is considered a sub-part of basic auth (username + password) in the design, so I followed that same pattern in addressing this.

This would only cause some issues for API callers without passing creds. They would now have to pass in a basic auth header with anonymous user name and a random string as password. No experience has changed on front-end.

One possible solution, which is much larger than this bug fix is to re-write the authenticate method to ensure that anonymous auth is always checked at the very end outside loop and other authenticate check, but I'm not sure how would Log in as anonymous react in the case where multiple auth domains are enabled. How would we identify the type of login request?

[derek-ho]

+1 on the API calls - I have no concerns from front end, since we are hooking up all the buttons anyway and can make the change appropriately, but not sure if it is considered a breaking change to require a username after this change

[peternied]

Overloading http basic to support anonymous authentication goes against standard practices, this should not be done.

Have you considered a new header/query parameter that indicates the request should be treated as anonymous, such as X-Access-Mode: anonymous or ?access_as_anonymous=true as a way for the client to indicate it should not get a 401 or redirected to an IDP for login.

[DarshitChanpura]

Overloading http basic to support anonymous authentication goes against standard practices, this should not be done.

Have you considered a new header/query parameter that indicates the request should be treated as anonymous, such as X-Access-Mode: anonymous or ?access_as_anonymous=true as a way for the client to indicate it should not get a 401 or redirected to an IDP for login.

I tried with something similar: { _auth_request_type: "anonymous" }. Check this comment thread out. Appending params didn't work as request is read-only. So after creating a structuredDeepClone of request object, params modification throws compile error: params is of type Unknown.

The problem at hand is that API users will have a change in behavior i.e. they will now have to pass extra header to specify the auth type, which we do not want and hence we will have to table this PR.

In my approach I used auth credentials, i'm only concerned with username here. Also in this approach the anonymous user will not be created in the backend, specifying auth credentials is just a way for front-end to let backend know that this is an anonymous request similar to your suggestion of using a Access-Mode: anonymous.

[cwperks]

@DarshitChanpura take a look at these PRs I made against my local fork that corrects the SAML flow when anonymous is enabled:

• Security - Add FailOnAnonymousAction for SAML auth to get redirect cwperks/security#23
• security-dashboards-plugin - Switch from using authinfo to failonanomyous endpoint in case anonymous is enabled to get SAML details cwperks/security-dashboards-plugin#3

SAML actually expects the authinfo endpoint to return an error like this:

// wwwAuthenticateDirective:
//   '
//     X-Security-IdP realm="Open Distro Security"
//     location="https://<your-auth-domain.com>/api/saml2/v1/sso?SAMLRequest=<some-encoded-string>"
//     requestId="<request_id>"

when anonymous is enabled, that won't happen since the request succeeds. The PR on my fork adds an action called failonanonymous that explicitly fails on anonymous requests. It also makes it so the endpoint /_plugins/_security/api/authtoken does not support anonymous. Anonymous doesn't make sense on that endpoint. That endpoint does not require credentials, but it does require a body containing a SAMLResponse which is verifiable against the configured SAML backend and it will extract auth info from the SAML Response and issue a jwt.

[peternied]

@DarshitChanpura It looks like the issue came from Dashboards around multi-authentication feature, the dashboard's client can be modified so the client's behavior if the 'Anonymous' authentication box is selected in the multi-auth workflow, no?

[DarshitChanpura]

This work will be continued with a different approach in: #4152

Closing this PR out.