Skip to content

Conversation

@amanda-tarafa
Copy link
Contributor

@amanda-tarafa amanda-tarafa requested a review from jskeet August 31, 2022 14:04
@amanda-tarafa amanda-tarafa requested a review from a team as a code owner August 31, 2022 14:04
@jpassing
Copy link
Contributor

jpassing commented Sep 1, 2022

My understanding is that Workforce Identity Federation shouldn't allow automatic service account impersonation. I haven't seen anything in the code to check for that.

But it's also possible that my understanding is wrong as the spec is pretty vague.

@amanda-tarafa
Copy link
Contributor Author

My understanding is that Workforce Identity Federation shouldn't allow automatic service account impersonation. I haven't seen anything in the code to check for that.

But it's also possible that my understanding is wrong as the spec is pretty vague.

There's no check for that. I also had that question and also couldn't find anything about it. If my understanding of Java's implementation is right, they are not checking either. So, the implementation for Workforce is the same as for Workload in that respect:

  • If impersonation is configured on the credential JSON we do it implicitly and user code cannot use the resulting credential to impersontate further.
  • If impersonation is not configured the user code can use the resulting credential if they want, else, the STS returned access token will be used.

/// </summary>
/// <remarks>
/// If this external account credential represents a Workforce Pool
/// enabled identity and this values is not specified, then an API key needs to be
Copy link
Collaborator

Choose a reason for hiding this comment

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

values => value here and below

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Looks fine so far - one more question I'll follow up on by email.

GoogleOptions = googleOptions;
AuthenticationHeader = authenticationHeader;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: remove one of these blank lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// Storage from a web application without a user's involvement.
/// <para>
/// <code>ServiceAccountCredential</code> inherits from this class in order to support Service Account. More
/// <see cref="ServiceAccountCredential"/> inherits from this class in order to support Service Account. More
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: "in order to support Service Account" => "in order to support Service Accounts"? (While we're changing the comment anyway...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// https://cloud.google.com/compute/docs/authentication.
/// </para>
/// <para>
/// <see cref="ExternalAccountCredential"/> inherits from this class to support both Workload Indentity Federation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentity => Identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// and Workforce Identity Federation. You can read more about these topics in
/// https://cloud.google.com/iam/docs/workload-identity-federation and
/// https://cloud.google.com/iam/docs/workforce-identity-federation respectively.
/// Note that in the case of Workforce Identity Federation, the external account does not represent a service acount
Copy link
Collaborator

Choose a reason for hiding this comment

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

acount => account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// https://cloud.google.com/iam/docs/workforce-identity-federation respectively.
/// Note that in the case of Workforce Identity Federation, the external account does not represent a service acount
/// but a user account, so, the fact that <see cref="ExternalAccountCredential"/> inherits from <see cref="ServiceCredential"/>
/// might be construed as missleading. In reality <see cref="ServiceCredential"/> it's not tied to a service account
Copy link
Collaborator

Choose a reason for hiding this comment

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

missleading => misleading

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's => is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// Note that in the case of Workforce Identity Federation, the external account does not represent a service acount
/// but a user account, so, the fact that <see cref="ExternalAccountCredential"/> inherits from <see cref="ServiceCredential"/>
/// might be construed as missleading. In reality <see cref="ServiceCredential"/> it's not tied to a service account
/// implementation wise, only name wise, for instance, a better name for this class might have been NoUserFlowCredential, and
Copy link
Collaborator

Choose a reason for hiding this comment

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

"implementation wise, only name wise" => "in terms of implementation; only in terms of name". (And then a new sentence.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lsirac
Copy link

lsirac commented Sep 1, 2022

@jpassing @amanda-tarafa

We support Workforce Identity Federation + service account impersonation in the other auth libraries, we just don't document it / tell people to use it

@amanda-tarafa
Copy link
Contributor Author

@jpassing @amanda-tarafa

We support Workforce Identity Federation + service account impersonation in the other auth libraries, we just don't document it / tell people to use it

Thanks Leo, then this implementation is correct!

Copy link
Contributor Author

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

@jskeet I've addressed all your comments, and also replied internally.
Second commit contains review changes. Thanks.

GoogleOptions = googleOptions;
AuthenticationHeader = authenticationHeader;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// Storage from a web application without a user's involvement.
/// <para>
/// <code>ServiceAccountCredential</code> inherits from this class in order to support Service Account. More
/// <see cref="ServiceAccountCredential"/> inherits from this class in order to support Service Account. More
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// https://cloud.google.com/compute/docs/authentication.
/// </para>
/// <para>
/// <see cref="ExternalAccountCredential"/> inherits from this class to support both Workload Indentity Federation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// and Workforce Identity Federation. You can read more about these topics in
/// https://cloud.google.com/iam/docs/workload-identity-federation and
/// https://cloud.google.com/iam/docs/workforce-identity-federation respectively.
/// Note that in the case of Workforce Identity Federation, the external account does not represent a service acount
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// https://cloud.google.com/iam/docs/workforce-identity-federation respectively.
/// Note that in the case of Workforce Identity Federation, the external account does not represent a service acount
/// but a user account, so, the fact that <see cref="ExternalAccountCredential"/> inherits from <see cref="ServiceCredential"/>
/// might be construed as missleading. In reality <see cref="ServiceCredential"/> it's not tied to a service account
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

/// Note that in the case of Workforce Identity Federation, the external account does not represent a service acount
/// but a user account, so, the fact that <see cref="ExternalAccountCredential"/> inherits from <see cref="ServiceCredential"/>
/// might be construed as missleading. In reality <see cref="ServiceCredential"/> it's not tied to a service account
/// implementation wise, only name wise, for instance, a better name for this class might have been NoUserFlowCredential, and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

public string ServiceAccountImpersonationUrl { get; }

/// <summary>
/// The GCP project ID or project number to be used for Workforce Pools
Copy link

Choose a reason for hiding this comment

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

At the time I wrote the doc the project ID was not yet supported. Not sure if that changed, just FYI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Public docs still only mention project number, I'll change the text to only mention project number.

@amanda-tarafa amanda-tarafa merged commit 92d5708 into googleapis:main Sep 2, 2022
@amanda-tarafa amanda-tarafa deleted the workforce-pools branch September 2, 2022 09:20
amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Oct 18, 2022
Features
- googleapis#2228 Supports Quota Project environment variable for ADC.
- googleapis#2219 Removes obsolete OOB code.
   BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- googleapis#2216 Log errors when trying to reach Compute's metadata server.
- googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation.
- googleapis#2103 Compute credentials support explicit scopes.
- googleapis#2096 Improves deserialization error handling.
amanda-tarafa added a commit that referenced this pull request Oct 18, 2022
Features
- #2228 Supports Quota Project environment variable for ADC.
- #2219 Removes obsolete OOB code.
   BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- #2216 Log errors when trying to reach Compute's metadata server.
- #2177, #2197, #2200, #2206, #2215, #2235 Support for Workload Identity Federation and Worforce Identity Federation.
- #2103 Compute credentials support explicit scopes.
- #2096 Improves deserialization error handling.
amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Nov 30, 2022
And support version: 1.58.0-beta02 => 1.58.0

Bug fixes:

- googleapis#2273 Don't attempt to use an empty/null media type in batch responses
- googleapis#2251 Which fixes a typo on then name of one of the AWS Credential environment variables.
- googleapis#2264 Which adds a missing line break in AWS Credential canonical request.

Features
- googleapis#2269 Support per-request max retry configuration.
- googleapis#2228 Supports Quota Project environment variable for ADC.
- googleapis#2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- googleapis#2216 Log errors when trying to reach Compute's metadata server.
- googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation.
- googleapis#2103 Compute credentials support explicit scopes.
- googleapis#2096 Improves deserialization error handling.
amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Nov 30, 2022
And support version: 1.58.0-beta02 => 1.58.0

Dependencies:

- Client libraries no longer target netstandard1.0 or net40.

Bug fixes:

- googleapis#2273 Don't attempt to use an empty/null media type in batch responses
- googleapis#2251 Which fixes a typo on then name of one of the AWS Credential environment variables.
- googleapis#2264 Which adds a missing line break in AWS Credential canonical request.

Features
- googleapis#2269 Support per-request max retry configuration.
- googleapis#2228 Supports Quota Project environment variable for ADC.
- googleapis#2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- googleapis#2216 Log errors when trying to reach Compute's metadata server.
- googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation.
- googleapis#2103 Compute credentials support explicit scopes.
- googleapis#2096 Improves deserialization error handling.
amanda-tarafa added a commit that referenced this pull request Nov 30, 2022
And support version: 1.58.0-beta02 => 1.58.0

Dependencies:

- Client libraries no longer target netstandard1.0 or net40.

Bug fixes:

- #2273 Don't attempt to use an empty/null media type in batch responses
- #2251 Which fixes a typo on then name of one of the AWS Credential environment variables.
- #2264 Which adds a missing line break in AWS Credential canonical request.

Features
- #2269 Support per-request max retry configuration.
- #2228 Supports Quota Project environment variable for ADC.
- #2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- #2216 Log errors when trying to reach Compute's metadata server.
- #2177, #2197, #2200, #2206, #2215, #2235 Support for Workload Identity Federation and Worforce Identity Federation.
- #2103 Compute credentials support explicit scopes.
- #2096 Improves deserialization error handling.
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.

4 participants