Skip to content
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.

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

Bug: authorization requests without acr_values and with enabled custom authn scripts fail #791

Closed
yuriyz opened this issue Feb 9, 2022 · 14 comments
Assignees
Labels
comp-jans-auth-server Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality priority-2 Issue or PR is important to majority of users, Currently workaround exists

Comments

@yuriyz
Copy link
Contributor

yuriyz commented Feb 9, 2022

Describe the bug

  1. If perform call without any custom authn script enabled and without acr set then it works.
  2. If enable basic (or any other script) and set acr it works
  3. If perform call with enabled basic but without acr set, it fails

Step 1

https://demoexample.gluu.org/jans-auth/restv1/authorize?response_type=code%20%20%20%20%20&redirect_uri=https://demoexample.gluu.org/admin&client_id=1901.b2486a6e-0a4e-4c34-b9d4-5283ff0286e8&scope=openid+profile+email+user_name&state=a1b86666-f8c9-47d1-a6c0-91ae3b39cd38&nonce=a62b5519-7776-4044-b480-ea732294ace1

Step 2

https://demoexample.gluu.org/jans-auth/restv1/authorize?response_type=code%20%20%20%20%20&redirect_uri=https://demoexample.gluu.org/admin&client_id=1901.b2486a6e-0a4e-4c34-b9d4-5283ff0286e8&scope=openid+profile+email+user_name&state=a1b86666-f8c9-47d1-a6c0-91ae3b39cd38&nonce=a62b5519-7776-4044-b480-ea732294ace1&acr_values=basic

Step 3

https://demoexample.gluu.org/jans-auth/restv1/authorize?response_type=code%20%20%20%20%20&redirect_uri=https://demoexample.gluu.org/admin&client_id=1901.b2486a6e-0a4e-4c34-b9d4-5283ff0286e8&scope=openid+profile+email+user_name&state=a1b86666-f8c9-47d1-a6c0-91ae3b39cd38&nonce=a62b5519-7776-4044-b480-ea732294ace1
@yuriyz yuriyz added kind-bug Issue or PR is a bug in existing functionality comp-jans-auth-server Component affected by issue or PR priority-2 Issue or PR is important to majority of users, Currently workaround exists labels Feb 9, 2022
@Milton-Ch
Copy link
Contributor

After a couple of tests in jans-ui.jans.io server I was able to validate that it was using simple_password_auth custom script, so basically when a custom script is enabled, we have to enable simple_password_auth custom script as well, in case this is requested via acr_values we have to have this enabled, or if client doesn't send acr_values param, then AS uses that by default.
I enabled A51E-76DD custom script in jans-ui.jans.io server, after this it worked as expected. I also validated that in case acr_values is sent, it works properly as well.

image

Suggestion is to have that authn script properly or define acr_values param accordingly.

@duttarnab duttarnab reopened this Mar 1, 2022
@duttarnab
Copy link
Contributor

duttarnab commented Mar 1, 2022

Hi @Milton-Ch
On Gluu CE (<5.0) everything is working without enabling simple_password_auth script. So the question is why do we have to enable a script by default on jans server?

Kindly let us know if there is any difference in the way the person_authentication scripts work in Gluu CE and jans servers.

@yurem
Copy link
Contributor

yurem commented Mar 8, 2022

There is one addition to this issue. In description we should mention that client configured to use simple_password_auth acr_values.

Current AS code adds to allowed acr_values simple_password_auth only if there is no custom scripts and LDAP authentications activated.

This added because simple_password_auth is authentication method with lower priority than any other custom script. Hence it's insecure to keep it enabled all the time. If admin really need to active user/pwd auth method we can add configuration property to enable simple_password_auth permanently. Or he can enable basic cusom script.

I think we need to improve acr_values selection algorithm to use next acr_values with higher priority if simple_password_auth is not allowed.

What do you think?

@nynymike
Copy link
Contributor

nynymike commented Mar 8, 2022

I agree that the AS admin should explitly allow simple_password_auth.

In oxAuth we had a way to configure a "Default acr" at the system level. So if a client does not specify acr_values, and the default acr for the client is not available, the AS should present the default acr.

If there is no available acr that is specified by the client (either in the request or client config), and no default acr is specified at the system level... what then? I think it would be prudent to present an available acr based on the highest 'level'.

@Milton-Ch
Copy link
Contributor

Ok, so just to confirm, in case client doesn't send acr param in authz url and default acr script doesn't exist, we should use the highest level script, right? not sure how backward compatible this is, but we could proceed with this if you agree.

@nynymike
Copy link
Contributor

nynymike commented Mar 8, 2022

In addition to the authz param, the client might have a default acr configured.

Right now, I'm not sure what the expected behavior would be if there is no default acr, and there is no acr specified in the client or request. Does it throw an error? Presenting the most secure authn workflow would surely be better then an error.

@Milton-Ch
Copy link
Contributor

Yes, in such scenario, currently we are throwing an exception with access denied error to the caller. It would be a kind of confusing concept this flow: if client sends acr_value=casa and then AS decided to use otp instead because casa script doesn't exist and otp is the highest level, that would be the behavior in case we go with highest level script option.

@yurem
Copy link
Contributor

yurem commented Mar 8, 2022

RP can use https://<server>/.well-known/openid-configuration to load auth_level_mapping. Based on this table it can make decision if trust or not resulted acr level.
Usually if we enable script like casa, otp it's available alll the time. The question is more about simple_password_auth.

But it's good discussion. In our script there are methods isValidAuthenticationMethod and getAlternativeAuthenticationMethod which do script internal self-checking and can redirect user to another acr if script can't handle user authentication due to 3rd system unavailability, for example.

@Milton-Ch
Copy link
Contributor

I see, not sure whether that option works for @mo-auto @duttarnab RP would take the decision whether to use casa, otp or any other script based on level which is available in discovery endpoint, that means that AS wouldn't take an automatic decision about which script should be used.

@nynymike
Copy link
Contributor

nynymike commented Mar 8, 2022

Here is the priority:

  1. acr_values param in Authz request
  2. default acr value in client config
  3. default acr value for AS
  4. acr with highest integer value for level

I don't understand why this approach would be either non-backwards compatible, or contrary to common sense.

@Milton-Ch
Copy link
Contributor

Ok, considering it, points 1, 2 and 3 are already implemented as expected, about point 4 is partially implemented, let me explain the two concepts here:

  1. If acr_values has simple_password_auth, then AS throws exception because that script doesn't exist
  2. If acr is not defined, then AS takes highest level script.

The special situation in the case reported here is that client doesn't send acr param, but since client has default acr set to simple_password_auth, AS tries to use it as default and then AS throws exception because that script doesn't exist.
If client default acr would be null and AS default authn method would be also null, then AS takes the highest level script, this is the way how it works currently, but it requires:

  • client default acr = null
  • AS default authn method = null

Basically, there is a difference when acr has a value (could be via param, client default or AS default) and when acr is null for all cases, in which case AS uses highest level script.

@nynymike
Copy link
Contributor

nynymike commented Mar 8, 2022

This is really a corner case. I think its ok to ignore the client param or default if the AS has no such script. But we could make a JSON property called acrHighestIfNull or something if the admin would prefer to throw an error.

@Milton-Ch
Copy link
Contributor

Ok, makes sense, I'll summarize it in the thread and will work on this change accordingly, thanks!

@Milton-Ch
Copy link
Contributor

In case script is not found (is not enabled or this doesn't exist), AS uses highest level script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-jans-auth-server Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality priority-2 Issue or PR is important to majority of users, Currently workaround exists
Projects
None yet
Development

No branches or pull requests

5 participants