Add RS256 support to OpenIDConnect #693
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
And use jwkest to decode signature, because it supports more than just HS256.
|
Hi @omab, What do you think of these PR's? Is this something you could accept? Please let me know if you have additional requirements or questions so I can address them! Thanks! |
| cls.oidc_config = self.get_json(self.OIDC_ENDPOINT + '/.well-known/openid-configuration') | ||
| cls.ACCESS_TOKEN_URL = self.oidc_config['token_endpoint'] | ||
| cls.AUTHORIZATION_URL = self.oidc_config['authorization_endpoint'] | ||
| cls.REVOKE_TOKEN_URL = self.oidc_config['revocation_endpoint'] |
There was a problem hiding this comment.
This line raised an exception for me because I don't have revocation_endpoint in my openid config.
The exception happens after cls.oidc_config has already been set. So the next time this won't get run and you won't get an exception, but you do end up with empty values for cls.USERINFO_URL and all other config defined below this line.
And delay caching of discovery document until all parameters are set to avoid using cache when document interpretation failed.
mvschaik
force-pushed
the
oidc-discovery
branch
from
8dee249 to
271710f
Compare
|
I overhauled the caching of OIDC configuration and keys, so now it is more robust. |
|
Closed in favor of #747 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the implementation of OpenIDConnect depended on statically provided keys. This worked when it could only do HMAC signature verification ("HS256"), but was not feasible for RSA based verification ("RS256"), since the keys should be fetched from the provider.
I added automatic configuration for OpenIDConnect to simplify creation of backends, and resolving of the keys. I had to switch to pyjwkest for signature verification, since pyjwt makes it really hard to do pass the keys.
PyJWT seems to be only used in Azure AD (which actually seems to be OpenIDConnect, so it might be simplified by just deriving from it and configuring the OIDC_ENDPOINT, I don't have an Azure account to test with though) and in ExactTarget. Maybe we can at some point remove the dependency in favor of PyJWKEST?
Together with #687 this PR fixes the issues with GoogleOpenIdConnectAuth, as described in #477 and #644.