search

CZ-NIC / pyoidc

A complete OpenID Connect implementation in Python
Other
709 stars 258 forks source link

srv_discovery_url: "https://login.microsoftonline.com/common/" does not work #520

Open agilezebra opened 6 years ago

agilezebra commented 6 years ago

We are attempting to use pyoidc with Microsoft as an OIDC provider, using various tenant ids as described in https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-v2-protocols-oidc . Has pyoidc been testing with Microsoft? It appears that the implementation as it stands isn't compatible with Microsoft's "interpretation" of the standard. In particular it looks as though MS mix and match whether they use the string {tenant}, an alias such as common or a raw UUID such as 2e4146d4-a7f8-45b2-9dd1-bbd74093bee5, as well as mixing the domain of sts.windows.net and login.microsoftonline.com for their issuer strings in various responses. The effect of this is that the pyoidc library is unable to find the correct keyjar when the keys are returned.

Here is the config we are using:

"Microsoft:Common": {
    "allow" : {
        "issuer_mismatch" : true
    },
    "behaviour" : {
        "response_type" : "code",
        "scope" : [ 
            "openid", 
            "profile", 
            "email"
        ]
    },
    "client_registration" : {
        "client_id" : "***",
        "client_secret" : "***",
        "redirect_uris" : [ 
            "{base}/code_flow"
        ]
    },
    "srv_discovery_url" : "https://login.microsoftonline.com/common/"
}

We get this output:

2018-04-16 11:41:36,227:urllib3.connectionpool:connectionpool.py:824:DEBUG: Starting new HTTPS connection (1): login.microsoftonline.com
2018-04-16 11:41:36,537:urllib3.connectionpool:connectionpool.py:396:DEBUG: https://login.microsoftonline.com:443 "POST /common/oauth2/token HTTP/1.1" 200 2685
2018-04-16 11:41:36,542:oic.oauth2.message:message.py:498:ERROR: Issuer "https://sts.windows.net/2e4146d4-a7f8-45b2-9dd1-bbd74093bee5/" not in keyjar
2018-04-16 11:41:36,543:oic.oauth2.message:message.py:498:ERROR: Issuer "0452b54b-705b-4eb9-a250-7295618bd34e" not in keyjar
2018-04-16 11:41:36,543:jwkest.jws:jws.py:384:DEBUG: Picking key by key type=RSA
2018-04-16 11:41:36,544:jwkest.jws:jws.py:397:DEBUG: Picking key based on alg=RS256, kid=FSimuFrFNoC0sJXGmv13nNZceDc and use=
2018-04-16 11:41:36,545:root:keyio.py:163:DEBUG: KeyBundle fetch keys from: https://login.microsoftonline.com/common/discovery/keys
2018-04-16 11:41:36,547:urllib3.connectionpool:connectionpool.py:824:DEBUG: Starting new HTTPS connection (1): login.microsoftonline.com
2018-04-16 11:41:36,675:urllib3.connectionpool:connectionpool.py:396:DEBUG: https://login.microsoftonline.com:443 "GET /common/discovery/keys HTTP/1.1" 200 4573
2018-04-16 11:41:36,678:oic.utils.keyio:keyio.py:217:WARNING: Wrong Content_type
2018-04-16 11:41:36,680:oic.oauth2.message:message.py:498:ERROR: Issuer "https://sts.windows.net/2e4146d4-a7f8-45b2-9dd1-bbd74093bee5/" not in keyjar
2018-04-16 11:41:36,680:oic.oauth2.message:message.py:498:ERROR: Issuer "0452b54b-705b-4eb9-a250-7295618bd34e" not in keyjar
2018-04-16 11:41:36,681:jwkest.jws:jws.py:384:DEBUG: Picking key by key type=RSA
2018-04-16 11:41:36,681:jwkest.jws:jws.py:397:DEBUG: Picking key based on alg=RS256, kid=FSimuFrFNoC0sJXGmv13nNZceDc and use=
2018-04-16 11:41:36,682:cherrypy.error.139789454951760:_cplogging.py:223:ERROR: [16/Apr/2018:11:41:36] HTTP 
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/oic/oauth2/message.py", line 699, in from_jwt
    _jw.verify_compact(txt, key)
  File "/usr/local/lib/python3.5/dist-packages/jwkest/jws.py", line 517, in verify_compact
    return self.verify_compact_verbose(jws, keys, allow_none, sigalg)['msg']
  File "/usr/local/lib/python3.5/dist-packages/jwkest/jws.py", line 565, in verify_compact_verbose
    "No key with kid: %s" % (self["kid"]))
jwkest.jws.NoSuitableSigningKeys: No key with kid: FSimuFrFNoC0sJXGmv13nNZceDc

We are however able to make pyoidc work if we do some horrendous monkey-patching using the module shown below, which we simply import before using pyoidc. This is obviously not ideal and far from future-proof. 

# patch.py

import re
from oic.oauth2.message import Message
from oic.oic import Client

class EquivalentIssuer:
    #https://sts.windows.net/2e4146d4-a7f8-45b2-9dd1-bbd74093bee5/
    #https://sts.windows.net/{tenantid}/
    patterns = [
        (re.compile(pattern), replace)
        for pattern, replace
        in [
            (r'https://sts.windows.net/[a-z0-9-]{36}\/', 'https://sts.windows.net/{tenantid}/'),
            (r'https://login.microsoftonline.com/[a-z0-9-]{36}\/', 'https://sts.windows.net/{tenantid}/'),
            (r'https://login.microsoftonline.com/\{tenantid\}\/', 'https://sts.windows.net/{tenantid}/'),
        ]
    ]

    @classmethod
    def rewrite(cls, issuer):
        for pattern, replace in cls.patterns:
            match = pattern.match(issuer)
            if match:
                return replace
        return issuer

def patch_handle_provider_config(function):
    def patched(self, pcr, issuer, *arguments, **keywords):
        if 'issuer' in pcr:
            pcr['issuer'] = EquivalentIssuer.rewrite(pcr['issuer'])
        issuer = EquivalentIssuer.rewrite(issuer)
        return function(self, pcr, issuer, *arguments, **keywords)
    return patched
Client.handle_provider_config = patch_handle_provider_config(Client.handle_provider_config)

def patch_add_key(function):
    def patched(self, keyjar, issuer, key, *arguments, **keywords):
        issuer = EquivalentIssuer.rewrite(issuer)
        return function(self, keyjar, issuer, key, *arguments, **keywords)
    return patched
Message._add_key = patch_add_key(Message._add_key)  # pylint: disable=protected-access

def patch_getitem(function):
    def patched(self, key):
        result = function(self, key)
        if key == 'iss':
            result = EquivalentIssuer.rewrite(result)
        return result
    return patched
Message.__getitem__ = patch_getitem(Message.__getitem__)
schlenk commented 6 years ago

Microsoft does weird things. AD FS does it differently than Azure AD for example. And Azure AD v1 API is different than v2 API. And all of them are mostly OAuth2 APIs that were hacked to do some OIDC too. ADFS is the best of the bunch...

The multi-tenant logic in Azure AD is broken in various ways. It works halfway ok for a single tenant, but Microsoft added a few extra quirks here and there to do multi-tenancy that simply don't fit with the OIDC model.

I'm not aware of a readymade solution to talk with Azure AD via pyoidc, but maybe @rohe knows something about it.

I am actually using pyoidc to work with Azure AD (v2), ADFS 2016, Keycloak and a few others, so it works in general for my small usecase, but needs some handholding like the one you made there. My local one did similar things in different ways.

I found i needed the following, if memory serves me right:

And so on.

agilezebra commented 6 years ago

Of course Microsoft are doing the wrong thing (as usual) but sadly they are largest provider of SSO in the corporate world. Given that I have to support Azure AD in order to service my clients, that means the Python OIDC library I use has to handle all their quirks.

@rohe What can be done to get the above logic into pyodic natively to homogenise Microsoft’s various representations of the same tenant id so that pyodic works with Azure?

rohe commented 6 years ago

22 aug. 2018 kl. 11:28 skrev agilezebra notifications@github.com:

Of course Microsoft are doing the wrong thing (as usual) but sadly they are largest provider of SSO in the corporate world. Given that I have to support Azure AD in order to service my clients, that means the Python OIDC library I use has to handle all their quirks.

@rohe What can be done to get the above logic into pyodic natively to homogenise Microsoft’s various representations of the same tenant id so that pyodic works with Azure?

Well, there is no short answer to that.

These last 6 month I’ve been working on a new OIDC RP library implementation. Sponsored by Google but now owned by the OpenID Foundation (OIDF). (There are 2 more libraries comming out from that project one in Java and one in JavaScript.)

Using it I only needed configuration to make it work aganist Azure AD in the single tenant case. That was using an UUID as identifier for the tenant.

As Microsoft is a founding member of the OIDF one would hope that they would be interested in getting OIDF's libraries to work with their services. But I’ve been in this game for a while so I wouldn’t bet on it :-(

But basically, I or someone else with knowledge of the library and access to instances of the service we want pyoidc/JWTConnect-Python to work with needs to have time allocated.

Michael listed a number of things that had to have special handling. He wasn’t sure it was exhaustive so the first thing we have to find out is what, if anything, he missed.

I might time to work on this but I’d love to have someone more knowledgeable on the MS side to help me out.

-- Roland "Education is the path from cocky ignorance to miserable uncertainty.” - Mark Twain

agilezebra commented 5 years ago

I see that this issue is closed but I presume that this library still isn’t truly compatible with Microsoft as an OpenID provider. Given that, I’m not really sure I understand the rational for closing the bug. I note comments from @rohe above about how he is able to make it work against Azure in a single tenant case. This unfortunately is not much help unless you are building an internal application for a given company. If you are providing a product to the general public (and by public, in my case, I mean some universe of typical business users) these users will not come from a single tenant id; they will come from all different companies and therefore different tenant ids. They will have no idea what a tenant id is and they just want to be able to sign in with their Microsoft account.

We continue to use this library as it is pretty much the only game in town for Python 3. However, it only works with Microsoft (who are, for better or worse, the latest provider of corporate accounts) if we monkey patch it as above. That’s a very precarious position to be in, given that any refactoring to the pyoidc library will break our software unless we pin the version.

Is there any way we can get the logic above to handle the equivalence of the 3 representations of tenant ids into the library proper? I’d prefer to not to contribute a fix myself as I really don’t understand the library well enough to ensure I don't break anything for others.

tpazderka commented 5 years ago

The rationale for closing the bug was simple - it was labelled as usage question and I was doing a cleanup of issues based on that.

Looking at the patch you have included earlier, this doesn't look like a very good idea to have in the library for following reasons:

1) It is very Microsoft specific, it wouldn't (and shouldn't) be used for anything else 1) It is bypassing security verifications in OpenID Connect protocol 1) It will be prone to be broken if something in the Microsoft implementation changes

So unless @schlenk and @rohe have different opinion I am for not implementing this.

agilezebra commented 5 years ago

Firstly, I wasn’t suggesting that the patch be used as as-is; rather, that it demonstrates that treating Microsoft’s 3 different representations of their tenant id as equivalent makes it work as needed.

It is very Microsoft specific, it wouldn't (and shouldn't) be used for anything else

It needn't be used for anything else except making pyoidc work with Microsoft. Whilst I appreciate the desire to keep the library free of cruft that is there only to deal with Microsoft's weirdness, if the pyoidc library doesn’t work fully with Microsoft’s implementation of OICD, it’s utility is severely limited. It’s no use saying “they are doing the wrong thing, they should change”; they won’t (we all know that) and we need a Python OIDC library that works with all the popular identity providers and this one currently doesn’t work with the provider that is most popular, in the most common usage scenario (i.e. not limited to a single tenant).

It is bypassing security verifications in OpenID Connect protocol

I can’t speak to that, but flags to disable strict checking of this or that are common and that's all it needs for those that need to use multi-tenancy. Indeed, there is already an issuer_mismatch flag present in the configuration.

It will be prone to be broken if something in the Microsoft implementation changes

Perhaps not, but it doesn’t work with Microsoft properly now so there isn’t much to lose.

tpazderka commented 5 years ago

I know that you weren't suggesting the patch as it is, but the different workarounds present in the patch show that the change might be quite complicated and it can possibly break other things. It will probably be quite complicated to test that properly.

I will wait for other opinion before we decide what to do with this issue.

agilezebra commented 5 years ago

For what it’s worth, and said with the best of intentions, we switched from pyoidc to authlib. It works perfectly out-of-the-box with all the main identity providers, like Azure. Working with real-world providers is more important to our business (and I suspect most applications) than sticking rigidly to the spec.

kshpytsya commented 4 years ago

Would it be possible to define some compact interface that would allow implementing Microsoft (or any other non-conformant implementation) specific fixes in some external package?