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

Validating an id_token should avoid making network requests when possible #218

Closed
ajaska opened this issue Nov 15, 2017 · 9 comments
Closed
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@ajaska
Copy link
Contributor

ajaska commented Nov 15, 2017

i.e. google.oauth2.id_token.verify_oauth2_token should cache public keys.

Otherwise, there's not much benefit to using this over the tokeninfo endpoint, despite numerous claims in official documentation to the contrary.

https://developers.google.com/identity/sign-in/web/backend-auth#calling-the-tokeninfo-endpoint

Calling [the tokeninfo] endpoint involves an additional network request that does most of the validation for you, but introduces some latency and the potential for network errors.

https://developers.google.com/identity/protocols/OpenIDConnect#validatinganidtoken

[Using the tokeninfo endpoint] involves an HTTP round trip, introducing latency and the potential for network breakage. The tokeninfo endpoint is useful for debugging but for production purposes, we recommend that you retrieve Google’s public keys from the keys endpoint and perform the validation locally.
Since Google changes its public keys only infrequently (on the order of once per day), you can cache them [...] Fortunately, there are well-debugged libraries available in a wide variety of languages to accomplish this.

Since this is the flagship Google auth library for python, it would be excellent to have this implemented.

Also, it would be great to have more documentation around the usage of verify_oauth2_token in the project -- several documentation pages link to https://google-auth.readthedocs.io for id_token validation, but there's no visible information about it. Copying the example code in https://developers.google.com/identity/sign-in/web/backend-auth#using-a-google-api-client-library would be a great start!

p.s. I know it's way easier to ask for features than to do them -- would you be open to accepting PRs for each of these? I'd be willing to contribute the code if it'd be considered.

@dhermes
Copy link
Contributor

dhermes commented Nov 15, 2017

For reference, oauth2client uses a cache. (oauth2client is the predecessor of this library, but oauth2client has been retired.)

@theacodes
Copy link
Contributor

p.s. I know it's way easier to ask for features than to do them -- would you be open to accepting PRs for each of these? I'd be willing to contribute the code if it'd be considered.

Yes. Caching the certificates was always intended, we just didn't feel the need to do it before 1.0.

@theacodes
Copy link
Contributor

Fwiw, because we use requests users can use things like requests-cache or httpcache and it'll "just work".

@ajaska
Copy link
Contributor Author

ajaska commented Nov 15, 2017

Good point! Looks like cache invalidation wouldn't be handled in the case of requests-cache though :(

@ajaska
Copy link
Contributor Author

ajaska commented Nov 20, 2017

Hmm, Python HTTP library caching support looks to be all over the place. Since urllib3 and http.client have no existing caching support/libraries and writing that support seems a bit out of scope for this library, it looks like it wouldn't be a great idea to add support for caching at the google.auth.transport.Request interface level.

For requests, it looks like cachecontrol is the go-to for proper cache header handling.

Since it seems to me that it's only really practical to implement it for requests, I think there's three options:

  1. make a separate transport.Request implementer (e.g. auth.transport.requests.CachingRequest) that wraps auth.transport.requests.Request, and uses cachecontrol to wrap the session created in the auth.transport.requests.Request.__init__ function. (I don't really like this)
  2. add an optional kwarg param cache (bool) to auth.transport.requests.Request.__init__ which wraps the session with cachecontrol
  3. add documentation to show how to make a requests sessions wrapped with cachecontrol

I think 3 is definitely the simplest, but 2 doesn't seem that bad considering the APIs for transport initialization already differ. I guess 3 is easy enough to start with :)

@theacodes
Copy link
Contributor

Yeah, it'd be great to start with (3), which gives us a clear implementation strategy for (2). I would definitely want cachecontrol to be an optional dependency.

Do you want to take on sending a PR for (3)? I'm happy to review/help.

@ajaska
Copy link
Contributor Author

ajaska commented Nov 27, 2017

Yeah 😄 already took a stab at it, was cleaning it up but got sidetracked by Thanksgiving.

@theacodes
Copy link
Contributor

Awesome.

@theacodes
Copy link
Contributor

Closed by #224.

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

4 participants