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

Enhancement Request - decoulple token decode from token validation #331

Closed
dudil opened this issue Mar 21, 2021 · 5 comments
Closed

Enhancement Request - decoulple token decode from token validation #331

dudil opened this issue Mar 21, 2021 · 5 comments

Comments

@dudil
Copy link

dudil commented Mar 21, 2021

Hi (Again :) ) Guys,

Is it ok by you guys to decouple the decode and validation done on the Client.decode_id_token?
(Don't mind to push that change as PR myself if approved).

Main reason is that as discussed on issue #147 I use the id_token as JWT barrier between the FE and API BE.
This is very much useful and provide code and straight forward support for authorization and authentication flow :)

However, when the JWS token (the original id_token) hit "exp" claim (which is normal scenario) I would like to check first if indeed the "sub"/"oid" (subject/user_id for that matter) are the same as returned from cache accounts.
(Since I'm working on AAD B2C - there is no username here).

I hope that's make sense and would be happy to provide the PR as mentioned.

Cheers,
Dudi

@rayluo
Copy link
Collaborator

rayluo commented Mar 23, 2021

Welcome back, Dudi. :-)

Personally, I understand #147 is a popular and common need. It is just that the feature is traditionally outside of the scope of MSAL library, so, it did not land at the top of our todo list.

You do not have to come up with a production-level PR, but I'm curious to see if you can jot down some pseudo code to show what you mean by "decouple the decode and validation done on the Client.decode_id_token", so that I can better understand what kind of API you have in your mind.

@dudil
Copy link
Author

dudil commented Mar 29, 2021

Hi @rayluo
In general if there were two different APIs such as:

def decode_token_to_claims(id_token: str) -> Optional[Dict[str, str]]:
""" decode_id_token is a better name here but will break the current API """
    return json.loads(oidc.decode_part(id_token.split(".")[1]))

def validate_token_claims(decoded: Dict[str, str], client_id=None, issuer=None, nonce=None, now=None) -> None:
    err = None  # https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
    _now = now or time.time()
    skew = 120  # 2 minutes
    if _now + skew < decoded.get("nbf", _now - 1):  # nbf is optional per JWT specs
        # This is not an ID token validation, but a JWT validation
        # https://tools.ietf.org/html/rfc7519#section-4.1.5
        err = "0. The ID token is not yet valid."
    if issuer and issuer != decoded["iss"]:
        # https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationResponse
        err = ('2. The Issuer Identifier for the OpenID Provider, "%s", '
            "(which is typically obtained during Discovery), "
            "MUST exactly match the value of the iss (issuer) Claim.") % issuer
    if client_id:
        valid_aud = client_id in decoded["aud"] if isinstance(
            decoded["aud"], list) else client_id == decoded["aud"]
        if not valid_aud:
            err = (
                "3. The aud (audience) claim must contain this client's client_id "
                '"%s", case-sensitively. Was your client_id in wrong casing?'
                # Some IdP accepts wrong casing request but issues right casing IDT
                ) % client_id
    # Per specs:
    # 6. If the ID Token is received via direct communication between
    # the Client and the Token Endpoint (which it is during _obtain_token()),
    # the TLS server validation MAY be used to validate the issuer
    # in place of checking the token signature.
    if _now > decoded["exp"]:
        err = "9. The current time MUST be before the time represented by the exp Claim."
    if nonce and nonce != decoded.get("nonce"):
        err = ("11. Nonce must be the same value "
            "as the one that was sent in the Authentication Request.")
    if err:
        raise RuntimeError("%s The id_token was: %s" % (
            err, json.dumps(decoded, indent=2)))

Which will still allow keeping the original API with the implementation such as

def decode_id_token(id_token, client_id=None, issuer=None, nonce=None, now=None) -> Optional[Dict[str, str]]:
    decoded: Optional[Dict[str, str]] = decode_token_to_claims(id_token=id_token)
    if decoded:
        validate_token_claims(decoded, client_idm issuer, nonce, now)
    return decoded

It will allow the more advanced user to decode the token into claims and perform whatever more advanced validation they are asking for (AKA #147 if I get it right)

As for me, I'm using the id_token as Authorization barrier (which is it purpose in general) and would like to avoid exceptions where the token expired and need to be refreshed to cases where it is not even decoded or might be belong to a different app (these are severe cases and must be handled in a different way than the very common case of token expired)

By the way - reason I'm digging in so deep is since I'm writing a fastapi plugin for your library.
I think it is ready to go public as early alpha soon (today / tomorrow) and hope you will like it :)

@rayluo
Copy link
Collaborator

rayluo commented Mar 29, 2021

Replied 14 minutes after your post, because I can't wait to see your work! :-)

I'll give your API proposal some more thoughts, too.

@dudil
Copy link
Author

dudil commented Mar 29, 2021

Here it is: https://github.com/dudil/fastapi_msal
I've just published it so please consider it as early alpha :)

@bgavrilMS
Copy link
Member

You are not required to validate the id token. Just parse it. You have obtained the id token directly from AAD, the HTTPS connection you have with AAD is proof enough that the id token is valid. In fact, AAD v1 endpoint issued unsigned id tokens.

AAD access token validation libraries are however not supported outside .NET

@bgavrilMS bgavrilMS closed this as not planned Won't fix, can't repro, duplicate, stale Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants