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

Support id tokens from metadata service #209

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

Conversation

jneem
Copy link
Contributor

@jneem jneem commented Oct 12, 2023

This adds support for getting an ID token (as opposed to an oauth2 access token) from Google's metadata service.

Edit: it's a breaking change because it added a field to a public struct. Maybe it's worth marking the option structs as non_exhaustive to make such changes less disruptive in the future?

@fiadliel
Copy link

(as a drive-by commentator who would in theory find this useful)

Functionality to get an ID token would be welcome, but I don't think this is the right approach. Fundamentally, that TokenInfo represents an OAuth2 access token, and an ID token is quite explicitly not an access token (even though Cloud Run/Functions/etc. use them to validate access).

What ID tokens and access tokens have in common, is that they care about similar things (e.g. is there a metadata server, vs running on local workstation, or using impersonated credentials, or workforce federation). But both the exact method of getting the token, and the use of them, varies after that. One might also want to get ID tokens that include the service account email, or licensing details (requires extra parameters to get this information).

I would prefer that, if implemented, this should be a separate method, e.g. async fn id_token(...) -> Result<IdToken, Error>

@jneem
Copy link
Contributor Author

jneem commented Oct 23, 2023

Fundamentally, that TokenInfo represents an OAuth2 access token

I agree that it would be nice to have a better separation between ID tokens and OAuth2 access tokens, but as far as this crate stands today, TokenInfo already (optionally) contains both of them.

I would prefer that, if implemented, this should be a separate method, e.g. async fn id_token(...) -> Result<IdToken, Error>

Where would you put this method? One of the difficulties I had here was that the actual token-fetching logic is a few layers down from the public API, and I wasn't really sure about the best way of "threading" things through.

@fiadliel
Copy link

I had a closer look at the support for ID tokens in the library - and it's unfortunate. The id_token() call provided is a convenience method for accessing the ID token from an OAuth2 client; but it has nothing to do with the authentication required by services in Google Cloud - and the interface provided doesn't match the concepts needed to provide that authentication (for example, scopes are not defined for ID tokens, but it's currently a required parameter to retrieve one).

So I see why your code is the way it is, as the code is already very fuzzy here, and fixing that would take a much larger rework. Renaming "scope" as "audience" allows you to leverage the existing misunderstandings :)

But the larger view is that getting an ID token isn't an authentication flow; it (maybe) uses an already successful authentication flow. If we look at the available methods of getting an ID token (https://cloud.google.com/docs/authentication/get-id-token), we have:

  • Request a token from the local metadata service
  • Call the IAM credentials service using existing valid Google credentials

In the second case in particular, we would want a pre-existing Authenticator to make that call.

So this points to the idea that getting an ID token is layered on top of existing code to get a Google access token, rather than being at the same level. That code could exist in this, or another library (in general, it has been bundled together in the same library, if one looks at examples from other languages).

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