Skip to content

Conversation

@gnossen
Copy link
Contributor

@gnossen gnossen commented Jul 6, 2020

No description provided.

@gnossen gnossen requested a review from apolcyn July 6, 2020 19:35
@gnossen gnossen changed the title L69: Allow Call Credentials to be Specified in grpc_google_default_credentials_create L72: Allow Call Credentials to be Specified in grpc_google_default_credentials_create Jul 6, 2020
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

token that must assert the same identity as the channel-level ALTS credential.

This oauth2 token attached by `grpc_google_default_credentials_create` is based
on the default VM service account, but the auth libraries for wrapped languages
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that the wrapped languages have a mechanism for changing the service account used in the channel-level ALTS credential? How does that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm told. I don't have the context myself. @apolcyn should be able to provide it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrapped languages don't have a mechanism for changing the service account used in the ALTS credentials, but they don't really need to (as long as they're OK with authenticating as the default service account).

ALTS credentials by default authenticate the VM's default service account. So long as the call credentials also authenticate the VM's default service account, then we're ok. And note that any call credentials which ultimately fetches the token from http://metadata.google.internal/computeMetadata/v1/project/service-accounts/default/token will authenticate the VM's default service account, creating a guarantee that we don't have a call-creds/ALTS identity mismatch.

Whether or not the wrapped language layers and above do the right thing, i.e. being sure that per-RPC tokens authenticate the VM's default service account, when potentially using ALTS, will be up to them though.

a mismatch in identity occurs that may cause an unexpected persistent failure of
RPCs at runtime.

While the default service account is the correct option is most cases, there
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to change wording here. This is not so much about allowing higher layers to authenticate alternative identities, rather than it is allowing client libraries to continue to use their own mechanisms to get per-RPC tokens, when potentially using ALTS. I.e. this API described here allows client libraries to continue uses the auth metadata plugin APIs to get per-RPC metadata, when using ALTS.

```

Supplying `nullptr` for `call_credentials` will result in the current behavior
of the function -- oauth2 call credentials based on the default service account
Copy link
Contributor

Choose a reason for hiding this comment

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

we should change wording here -- the current behavior of how google default credentials chooses call credentials is actually based on the "Application Default Credentials" spec (see https://cloud.google.com/docs/authentication/production?_ga=2.68587985.1354052904.1594166352-2074181900.1593114348#finding_credentials_automatically). This by default authenticates the VM's default service account but isn't necessarily guaranteed to, e.g. if the GOOGLE_APPLICATION_CREDENTIALS environment variable is set and points a JSON config file for another identity.

@gnossen
Copy link
Contributor Author

gnossen commented Jul 8, 2020

@apolcyn Thank you for the clarifications! Very helpful. I've tried to incorporate them into the proposal. PTALA. Leaving it to you to resolve the comments if you feel they've been addressed properly.

Copy link
Contributor

@apolcyn apolcyn left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments

## Background

The Google default credentials created by the
`grpc_google_default_credentials_create` function in Core enable connection to
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/enable/enables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"The Google default credentials ... enables"? "Enable" agrees with the plural subject, unless we've atomized "credentials" to be singular.

The Google default credentials created by the
`grpc_google_default_credentials_create` function in Core enable connection to
Google services via a combination of ALTS and SSL credentials, along with a special oauth2
token that must assert the same identity as the channel-level ALTS credential.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: related to below comment, for accuracy lets replace "must assert" with "by default asserts"

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe describe how the token that google default credentials uses is decided per the Application Default Credentials spec

Copy link
Contributor

Choose a reason for hiding this comment

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

these comments are still relevant, to improve accuracy of the wording here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed the s/must/by default/. Sorry about that. I added a numbered list describing how ADC works in detail.

The Google default credentials created by the
`grpc_google_default_credentials_create` function in Core enable connection to
Google services via a combination of ALTS and SSL credentials, along with a special oauth2
token that must assert the same identity as the channel-level ALTS credential.
Copy link
Contributor

Choose a reason for hiding this comment

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

lets note here that the identity we're discussing here is specifically the one authenticated by the token available on the metadata server's http://metadata.google.internal/computeMetadata/v1/project/service-accounts/default/token endpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, to be clear I meant to say that we should clarify that the ALTS channel-level identity is that authenticated by the token served from this metadata server endpoint.

Google default credentials picks it's per-RPC metadata token via the ADC spec, though (which by default is, but in general isn't necessarily going to result in a per-RPC token from that same endpoint).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. The current wording is ambiguous. Updated to clarify that it's the ALTS identity that's taken from that HTTP endpoint.

@gnossen
Copy link
Contributor Author

gnossen commented Jul 9, 2020

@apolcyn Thanks for the suggestions! PTALA.

@gnossen gnossen merged commit 2442f75 into grpc:master Jul 20, 2020
@gnossen gnossen deleted the default_credentials_extension branch July 20, 2020 18:45
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.

3 participants