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

Loginpass + Azure - claim validation error for claim "iss" #65

Open
blackdwarf opened this issue Jul 1, 2020 · 6 comments
Open

Loginpass + Azure - claim validation error for claim "iss" #65

blackdwarf opened this issue Jul 1, 2020 · 6 comments

Comments

@blackdwarf
Copy link

What I did?

Installed loginpass using pipenv. Used the example to pull in Google and Azure provider. Tried to log in using Azure provider.

The authorization is below, it is incredibly simple. The rest is boilerplate loginpass code.

def handle_authorization(remote, token, user_info):
    printer.pprint(user_info)
    return user_info

What should happen?

The flow should complete and I should get the user_info dumped to the screen.

What actually happened?

An error authlib.jose.errors.InvalidClaimError: invalid_claim: Invalid claim "iss" is thrown and login does not proceed.

What I think is the root cause

I think that loginpass is actually using OpenID Connect to get the metadata from the server for Azure, which returns the default iss claim as a template with the {tenant} placeholder that needs to be replaced with a GUID, in my case a static one (as per https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens). Looking at loginpass code, specifically azure.py at line 30, this should work, but I'm still getting the error.

@lepture
Copy link
Member

lepture commented Jul 16, 2020

Can you leave the discovery endpoint URL? In our code: https://github.com/authlib/loginpass/blob/master/loginpass/azure.py#L34

issuer = issuer.replace('{tenantid}', tenant)

It is replacing {tenantid} instead of {tenant}, I think this is the problem.

@blackdwarf
Copy link
Author

blackdwarf commented Jul 18, 2020

@lepture I think I figured out what is wrong, and managed to unblock myself, however, there could be a more sinister issue in there.

The docs @ https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-protocols-oidc give all the info needed. Basically, what value is used for the tenant in the loginpass code:

issuer = issuer.replace('{tenantid}', tenant)

is dependent on what tenant was used for the discovery URL. If you put in common, loginpass will try to validate the iss claim in the id_token returned as https://login.microsoftonline.com/common/v2.0, however, that is not what will be returned for personal Microsoft accounts. What will be returned is https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0. This will fail validation.

However, if you use the consumers OpenID Connect URL (https://login.microsoftonline.com/consumers/v2.0/.well-known/openid-configuration), you can see that the issuer is set to https://login.microsoftonline.com/9188040d-6c67-4c5b-b112-36a304b66dad/v2.0 which is correct. If you're wondering why this works (after all consumers != [guid]), it is because the response URL in issuer doesn't even contain {tenantid} string, nothing gets replaced. Thus, to unblock myself, I've simply put AZURE_SERVER_METADATA_URL in my config, set it to the consumers tenant discovery endpoint, and that worked.

But, the issue still persists, since for common, which is configured by default (and I imagine a lot of people will use), you can still get two different issuer responses in the main metadata response when you hit the endpoint, and you can still run into this invalid claim situation. I can't think what a proper fix would be, unfortunately, since the validation code in Authlib is pretty generic and the "fixer" in loginpass, at the time of replacing {tenantid} doesn't really know what will be the returning issuer.

@vmsp
Copy link

vmsp commented Aug 29, 2020

It seems like this is a known issue that's not going to be corrected. See MicrosoftDocs/azure-docs#38427.

As an alternative, I made a provider that assumes no OpenID Connect support. E.g.

class Microsoft:
    NAME = 'microsoft'
    OAUTH_CONFIG = {
        'api_base_url':
            'https://graph.microsoft.com/',
        'authorize_url':
            'https://login.microsoftonline.com/common/oauth2/v2.0/authorize',
        'access_token_url':
            'https://login.microsoftonline.com/common/oauth2/v2.0/token',
        'jwks_uri':
            'https://login.microsoftonline.com/common/discovery/v2.0/keys',
        'userinfo_endpoint':
            'https://graph.microsoft.com/oidc/userinfo',
        'client_kwargs': {
            'scope': 'openid email profile'
        },
    }

Microsoft recommends not hard-coding the UserInfo endpoint, though.

lepture added a commit that referenced this issue Oct 31, 2020
@lepture
Copy link
Member

lepture commented Oct 31, 2020

@blackdwarf I've improved create_azure_backend function, I think this will solve all the problem. You can always pass a compliance_fix function to fix the returned value of server metadata.

@mavaras
Copy link

mavaras commented Nov 6, 2020

@blackdwarf I've improved create_azure_backend function, I think this will solve all the problem. You can always pass a compliance_fix function to fix the returned value of server metadata.

This isn't released yet, right?

Edit: Nope

I also think it should be a good idea to add some testing to Azure, there isn't nothing related in tests/. Or maybe providing a use example.

@blackdwarf
Copy link
Author

@lepture thanks for the fix!

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

No branches or pull requests

4 participants