-
Notifications
You must be signed in to change notification settings - Fork 524
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
feat: add impersonated credential support #1838
feat: add impersonated credential support #1838
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass - these are mostly nits for formatting etc... I'd need another pass to really check the impersonation-specific logic.
Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/ImpersonatedCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/ImpersonatedCredentialTests.cs
Outdated
Show resolved
Hide resolved
I've suggested that @amanda-tarafa holds off on reviewing this until you've had a chance to work through this first round of review comments, btw. I'm not perturbed by there being a lot of comments - that's normally the way for new contributors to any code base, so please don't consider it any sort of personal judgement :) |
@amanda-tarafa I moved #1840 out of backlog in this PR. Please reopen it and assign to me since I don't have write permission to do so. Thanks! |
@jskeet Thank you for all the comments and suggestions! They are super helpful! :) PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments... I haven't validated all the changes, but Amanda will have another look anyway before we're done.
Src/Support/Google.Apis.Auth.Tests/OAuth2/ImpersonatedCredentialTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits left, but I think we're nearly there. Amanda may well find things I missed though :)
Src/Support/Google.Apis.Auth.Tests/OAuth2/ImpersonatedCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/ImpersonatedCredentialTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some specific comments but I'd also like to explore the possibility of adding an IBlogSigner interface, that can be implemented by ImpersonatedCredential and ServiceAccountCredential and GoogleCredential. GoogleCredential checks first for the underlying credential being an IBlobSigner, else it fails (similiar to OIdc). In that way:
a) The user doesn't have to cast the underlying credential to ImpersonatedCredential which is always a bad user experience.
b) We make more discoverable the capability of ServiceAccountCredential for signing.
c) We don't have to make ImpersonatedCredential public.
Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs
Outdated
Show resolved
Hide resolved
Src/Support/Google.Apis.Auth.Tests/OAuth2/GoogleCredentialTests.cs
Outdated
Show resolved
Hide resolved
@amanda-tarafa PTAL, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most comments are just nits. Thanks!
var body = NewtonsoftJsonSerializer.Instance.Serialize(bodyJson); | ||
var content = new StringContent(body, Encoding.UTF8, "application/json"); | ||
|
||
var response = await HttpClient.PostAsync(TokenServerUrl, content, taskCancellationToken).ConfigureAwait(false); | ||
var newToken = await TokenResponse.FromHttpResponseAsync(response, Clock, Logger).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See TokenRequestExtensions. I think you can add similar extension methods there for ImpersonationAccessTokenRequest, and reuse ParameterUtils as well so you don't need to do any of this here.
Same for the other requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is necessary. This basically means moving these methods to TokenRequestExtensions class. ParameterUtils won't be used because it url encodes the body instead of using json format.
Audience = oidcTokenOptions.TargetAudience, | ||
IncludeEmail = true | ||
}; | ||
var body = NewtonsoftJsonSerializer.Instance.Serialize(bodyJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above for adding extension methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace Google.Apis.Auth.OAuth2 | ||
{ | ||
/// <summary> | ||
/// ImpersonatedCredentials allowing credentials issued to a user or service account to impersonate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is internal now, so the description is slightly less important. I'm leaving a suggestions with what I think could acceptable, @jskeet what do you think?
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
Co-authored-by: Amanda Tarafa Mas <14878252+amanda-tarafa@users.noreply.github.com>
I'll merge now. We can do a release once I'm done with #1851. I'll try to have that done this week. |
@amanda-tarafa Thank you! |
Features * googleapis#1832 Helper for loading Client Secrets directly from file. * googleapis#1838 and googleapis#1874 Support for credential impersonation. * googleapis#1851 Make it easier to configure a credential's HTTP client, in particular it's now easier to set a proxy to be used for token fetching. Fixes * googleapis#1874 ServiceAccountCredential should copy scopes from initializer. Dependencies * googleapis#1842 System.Net.Http to 4.3.4 * googleapis#1873 Add target for .NET 4.6.1
Features * googleapis#1832 Helper for loading Client Secrets directly from file. * googleapis#1838 and googleapis#1874 Support for credential impersonation. * googleapis#1851 Make it easier to configure a credential's HTTP client, in particular it's now easier to set a proxy to be used for token fetching. Fixes * googleapis#1874 ServiceAccountCredential should copy scopes from initializer. Dependencies * googleapis#1842 System.Net.Http to 4.3.4 * googleapis#1873 Add target for .NET 4.6.1
Features * #1832 Helper for loading Client Secrets directly from file. * #1838 and #1874 Support for credential impersonation. * #1851 Make it easier to configure a credential's HTTP client, in particular it's now easier to set a proxy to be used for token fetching. Fixes * #1874 ServiceAccountCredential should copy scopes from initializer. Dependencies * #1842 System.Net.Http to 4.3.4 * #1873 Add target for .NET 4.6.1
This PR adds impersonated credential support. ImpersonatedCredentials allowing credentials issued to a user or service account to impersonate another service account.The cloud reference is: https://cloud.google.com/iam/docs/creating-short-lived-service-account-credentials#sa-credentials-oauth. (Googlers see go/dotnet-auth-impersonated-creds for the design doc.)
The following shows how to create an impersonated credential.
Then you can use it to get access token and id token the same way as you do for other types of GoogleCredential.
To sign bytes.