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

Support max_age in initiate_auth_code_flow() for OIDC #381

Closed
kevindixon opened this issue Jul 19, 2021 · 12 comments · Fixed by #389
Closed

Support max_age in initiate_auth_code_flow() for OIDC #381

kevindixon opened this issue Jul 19, 2021 · 12 comments · Fixed by #389

Comments

@kevindixon
Copy link

Describe the bug
We have a need to be able to force re-authentication for certain operations in our product.

Whilst ConfidentialClientApplication.initiate_auth_code_flow supports prompt=login this is an imperfect solution because our side there is no way to confirm that re-authentication took place.

This right way to do this is to pass max_age=0 (ref) and use the auth_time claim value to confirm re-authentication took place.

From what I can see ConfidentialClientApplication.initiate_auth_code_flow doesn't support max_age

To Reproduce
N/A

Expected behavior
max_age is supported, and functions as defined in the OIDC spec

What you see instead
Exception thrown if max_age passed.

The MSAL Python version you are using
msal==1.12.0

Additional context
Add any other context about the problem here.

@rayluo
Copy link
Collaborator

rayluo commented Jul 20, 2021

Labeling this as enhancement, for now. We will need to check with our service-side team to see whether max_age is supported by our backend. max_age is not currently documented in our doc.

@rayluo
Copy link
Collaborator

rayluo commented Aug 3, 2021

@kevindixon , thanks for this suggestion! There is a PR available now. And you can test it by installing the cutting-edge MSAL by:

pip install git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@max_age

Let us know whether it works for you.

@kevindixon
Copy link
Author

@rayluo thanks for the speedy response. About to go on vacation, but will endeavour to take a look on my return

@kevindixon
Copy link
Author

@rayluo doesn't seem to work as I expect.
When I use max_age=0, re-authentication happens as expected, but passing the code to acquire_token_by_auth_code_flow results in an exception in msal/oauth2cli/oidc.py in decode_id_token, line 76:

9. The current time MUST be before the time represented by the exp Claim. The id_token was: {
...
  "iat": 1629829463,
  "nbf": 1629829463,
  "exp": 1629829763,
  "auth_time": 1629829715,
...

By the time I've called acquire_token_by_auth_code_flow exp is after the current time.
So, looks like exp has been set to an extraordinarily short time.

Or am I missing something...?

@rayluo
Copy link
Collaborator

rayluo commented Aug 24, 2021

In practice, there wouldn't make sense to use max_age=0. A rule of thumb is to consider "we support max_age down to five minutes".

By the time I've called acquire_token_by_auth_code_flow exp is after the current time.

When did you call acquire_token_by_auth_code_flow? Based on the snippet you posted above, the exp, even when issued by an edge case max_age=0 is still 5 minutes after iat, which looks good to me. You may also print the current time in your investigation and let us know what you find.

@kevindixon
Copy link
Author

kevindixon commented Aug 25, 2021

max_age=0 DOES make sense in practice - it is essentially an assertion to FORCE re-authentication no matter what.
In some secure environments always forcing re-authentication for certain privileged actions is standard practice - for instance, systems conforming to 21 CFR part 11.
See https://auth0.com/docs/login/max-age-reauthentication for some discussion on this issue.

This is exactly what I am trying to do - user is already logged in, but attempting a privileged action and I want to be able to FORCE re-authentication (and confirm authentication in the issued token).

Here is the series of operations I have re-run with timings:

  1. At 1629881476 called initiate_auth_code_flow (with no max_age) - this is initial log-in
  2. At 1629881501 called acquire_token_by_auth_code_flow and received back an ID token with an auth_time of 1629881496, exp of 1629885101 (1 hour time) and iat of 1629881201.
  3. At 1629881543 called initiate_auth_code_flow passing max_age of 0) - i.e. privileged operation, force re-auth
  4. At 1629881578 called acquire_token_by_auth_code_flow - this is where the exception is raised. The ID token has auth_time of 1629881570, exp of 1629881578 ("now"), and iat of 1629881278.

So.. the call to acquire_token_by_auth_code_flow appears to produce an ID token with an exp only 8 seconds after auth_time which seems wrong.

Note in both cases that iat appears to always be 5 minutes before the call to acquire_token_by_auth_code_flow which doesn't seem to make sense.

@rayluo rayluo added this to In progress in MSAL Python Board Aug 26, 2021
@rayluo
Copy link
Collaborator

rayluo commented Aug 26, 2021

Hi Kevin, thanks for the feedback. That was a good read. You are right, max_age=0 makes sense.

And I can also reproduce the issue you found. I made some adjustment to the PR, and it would work now. Besides, that PR will now automatically check the auth_time against max_age, and raise exception when the check fails, so that your app does not need to check auth_time. Please test it again.

However, my test revealed some other corner cases. I'll discuss with our service-side team before I can get back to you.

@rayluo
Copy link
Collaborator

rayluo commented Sep 3, 2021

Hi @kevindixon , did you get some time to test out the updated PR? In your test environment, you would need to do pip uninstall msal and then pip install git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@max_age. Let us know how it goes. Thanks!

@ineesalmeida
Copy link

ineesalmeida commented Sep 13, 2021

Hi @rayluo,
I work with @kevindixon and just tried the PR version. The max_age parameter seems to work on a first look, but it doesn't seem to be optional, which breaks existing code.

@rayluo
Copy link
Collaborator

rayluo commented Sep 13, 2021

Hi Inês, thanks for the good catch! We have updated that fix. Please re-do pip uninstall msal and then pip install git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@max_age. It should work this time. Let us know how it goes. Thanks!

@ineesalmeida
Copy link

Hi Ray, seems to work nicely now, thank you!
What's the timeframe for this to be released?

@rayluo
Copy link
Collaborator

rayluo commented Sep 15, 2021

What's the timeframe for this to be released?

Thanks for your confirmation! You are all set. The rest of the tasks are all on our side.

  1. We will merge the fix into our code base, so that it will be automatically included into our next release.
  2. The current issue here will also be (automatically) marked as "Closed", since there will be no more actionable work for this issue.
  3. The actual release will happen at a later time. We do not have a specific date yet, but historically our release cadence is once a month.
  4. When we cut a new release, we will manually update the included issues. At that time, you as existing participants in this issue may receive a notification from github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants