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

Add oauth metadata discovery as per RFC8414 #81

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

djw8605
Copy link
Contributor

@djw8605 djw8605 commented Jul 31, 2018

Fixes #75

@djw8605 djw8605 requested a review from bbockelm July 31, 2018 20:53
Copy link
Contributor

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few inline comments. The biggest issue is how to build up the OAuth2 discovery path for multitenant hosts.

src/scitokens/utils/config.py Show resolved Hide resolved
response = request.urlopen(request.Request(meta_uri, headers=headers))
data = json.loads(response.read().decode('utf-8'))
# https://tools.ietf.org/html/rfc8414
# We first try the RFC's metadata location, then try the openid defined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, RFC8414 is more complex. To quote:

The client would make the following request when the
issuer identifier is "https://example.com/issuer1" and the well-known
URI suffix is "oauth-authorization-server" to obtain the metadata,
since the issuer identifier contains a path component:

GET /.well-known/oauth-authorization-server/issuer1 HTTP/1.1
Host: example.com

Using path components enables supporting multiple issuers per host.
This is required in some multi-tenant hosting configurations. This
use of ".well-known" is for supporting multiple issuers per host;
unlike its use in RFC 5785 [RFC5785], it does not provide general
information about the host.

So for the RFC8414 case, you have to prepend .well-known/oauth-authorization-server to the path while in OIDC you add it as a suffix.

Can you also make the ordering of the discovery (OIDC or OAuth2) something specified in the configuration file instead of embedded in the code?

well_known_uri = [".well-known/oauth-authorization-server", ".well-known/openid-configuration"]
last_exception = MissingIssuerException("Unable to retrieve public key from issuer")
jwks_uri = None
for uri in well_known_uri:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code block is getting long. Since there is less commonality between OAuth2 and OIDC than thought, maybe make these into two separate subroutines?

@@ -100,6 +100,23 @@ def test_ec_deserialization(self):
scitoken = scitokens.SciToken.deserialize(serialized_token, insecure=True)


def test_failures(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also have a test for the RFC8414 style of discovery?

@djw8605
Copy link
Contributor Author

djw8605 commented Nov 7, 2018

Addressed all the comments above.

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

Successfully merging this pull request may close these issues.

2 participants