Azure-Samples / ms-identity-python-webapp

A Python web application calling Microsoft graph that is secured using the Microsoft identity platform
MIT License
284 stars 135 forks source link

B2C token validation method #38

Closed rajesh-naik-by closed 3 years ago

rajesh-naik-by commented 4 years ago

I think better we have a token validation steps included in this webapp , so that it will be easy for an application team to refer the B2C token validation.

Basic validation needed for,

  1. Signature
  2. exp
  3. nbf
  4. iss
  5. aud
rayluo commented 4 years ago

Well, this sample app is powered by Microsoft's authentication library, MSAL Python, which already validates an ID token under the hood. That means, all the user data you get in this app, are already validated.

We just want to save everyone's time on implementing token validation, so that those time can be used to drink a beer. :-)

rajesh-naik-by commented 4 years ago

Thanks for the confirmation @rayluo :)

ghost commented 4 years ago

Looking at this code, MSAL only validates:

Looking at the AAD and B2C documentation this means:

is it okay to neglect these? Especially the last point? Is it generally recommended but not necessary here? If yes, why? Will this be done in MSAL (in the future)?

Edit: Okay, just now I fully understood the implications of your link:

If the ID Token is received via direct communication between the Client and the Token Endpoint (which it is in this flow), the TLS server validation MAY be used to validate the issuer in place of checking the token signature. [...]


Nevertheless: What to do with nbf? Compare it with local time? Compare it with iat. Validate it at all? That's currently missing from there.

rayluo commented 4 years ago

@frank-fischer-by Thanks for your code review! That was why I pasted that link for. :-)

With regard to nbf, as you can see from the comments in our code snippet, our implementation was driven by the OpenID Connect specs. nbf is not defined in such specs, so it was not included. That being said, we can add it at a later time.

ghost commented 4 years ago

Yeah, thanks a lot for the library link. Helped a lot with fostering understanding :).

With regards to nbf: There (IMHO) still is the question, what's generally the best practices:

  1. As you said yourself, nbf is non-standard OIDC. On the other hand, MSAL is also not OIDC-certified or could be called non-standard OIDC: The library is not compatible with any OIDC IdP, but specific to AAD/B2C. As such, integrating the nbf check centrally (optional, if the claim is present) might still be the right direction.
  2. The AAD/B2C documentation still advices to check nbf. If it is deliberately not checked centrally by MSAL, then the conclusion has to be, that it has to be checked in the applications. → We should reflect that in a repository which aims to provide an example application.
rayluo commented 4 years ago

Agree. We will implement this inside MSAL, so that our customers would not need to do that in their application. This application will automatically pick up the next release of MSAL. For now, I'll mark this issue as an enhancement here. You will also automatically receive updates when we make progress on this issue.

ghost commented 4 years ago

That sound great, thanks 👍.

Just to clarify: Is this a general decision/direction of MSAL or is this more or less "only" for https://github.com/AzureAD/microsoft-authentication-library-for-python (of course on different timelines across all libraries)?

One last question that circles back to the "example application": Where is the right place to define/discuss a generic function for B2C custom claim validation: Is this something to bring up with the MSAL team[s] or with the application[s]? Let's say a generic "pass in this Type[Callable] in order to validate B2C custom claims?

rayluo commented 4 years ago

Just to clarify: Is this a general decision/direction of MSAL (of course on different timelines across all libraries) or is this more or less "only" for MSAL Python?

My earlier status-quo snippet for code review and its subsequent response (i.e. "not currently validate nbf but we will") was "only" for MSAL Python. That being said, such a validation feature is very reasonable ask for any authentication library, if it has not already been implemented there. I could ask around for you, but perhaps a more systematic approach would be you to create such a specific question in each of the other MSAL library's repo, so that you will get a precise answer from each of their maintainers, and each Q&A would also become a long-term reference and/or an enhancement tracking item there.

One last question that circles back to the "example application":

Oh you are always welcome to ask as many questions as you want. :-) Just create a new issue for a different question, and do not mix different content in one Q&A.

Where is the right place to define/discuss a generic function for B2C custom claim validation: Is this something to bring up with the MSAL team[s] or with the application[s]?

Stackoverflow with proper tags is the place for generic or how-to questions for perhaps all developer-facing Microsoft product/service/sdks. For example, this one is for B2C.

But if your question is about feature request or bug report, you can create issue in the corresponding sample's repo or its underlying MSAL's github repo, to grab developer's attention.

Let's say a generic "pass in this Type[Callable] in order to validate B2C custom claims?

This sounds like a very specific api interface (which is a question welcomed in either this sample repo, or in MSAL Python's repo).

Right now, in this sample application powered by MSAL Python, you already have all the already-validated claims from an ID token (with nbf validation coming soon). You should be able to add your extra validation logic there. If you feel we should provide a built-in support for that, please describe your scenario, ideally in a different issue.

rayluo commented 3 years ago

Since MSAL Python 1.4.2, the enhancement of validating nbf has been shipped. (And since then, the 1.4.x series has another bugfix so far. Go pick the latest MSAL release please.)