feat: ImpersonatedCredentials to support universe domain for idtoken and signblob #1566

zhumin8 · 2024-11-01T21:02:03Z

follow up to #1528.

idtoken and sign flow are tested E2E according to TPC test guide for sa-to-sa impersonation.

…ests to use assertThrows.

lqiu96 · 2024-11-04T20:33:05Z

oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java

+      // Throwing an IOException would be a breaking change, so wrap it here.
+      // This should not happen for this credential type.
+      throw new IllegalStateException(e);


Can we throw a SigningException here for consistency just like ComputeEngineCredentials?

If not, can the IllegalStateException have an error message that is the same as the one in ComputeEngineCredentials for consistency?

Also, I think adding a new runtime exception might be a behavior breaking change. I think we can justify this addition given that signing needs to support Universe Domains. Perhaps we can add a small sentence in the description about this?

Here is my thought on this, I opted for IlligalStateException similar to ExternalAccountCredentials here

Currently, unlike ComputeEngineCredentials where IOException can actually be thrown (ref), ImpersonatedCredentials should not throw exception on getUniverseDomain() calls. (this method throws because we do not want breaking changes when introducing the override) Because neither of the allowed source credential types (sa, u, external credentials) throw exception.

We are wrapping this with try-catch block to avoid breaking change. Throwing SigningException if it can happen for say, future allowed source credentials, it seems to imply a behavior change that should change the method signature. But for now, since we do not expect any of the allowed source credential to be throwing, it is acceptable to wrap and not add exception to method signature. Which IllegalStateException seems more appropriate.

Added message in a8d466f

ImpersonatedCredentials should not throw exception on getUniverseDomain() calls

IIUC (I may be completely off on this as I don't know Impersonation at all), the sourceCredential in the ImpersonatedCredential could be of any type, right? Unless there is a limitation that the underlying sourceCredential for an ImpersonatedCredential can't be of ComputeEngineCredential. I am assuming that if a user tries to impersonate a ComputeEngineCredential, the call getUniverseDomain() may end up throwing an IOException.

Throwing SigningException if it can happen for say, future allowed source credentials, it seems to imply a behavior change that should change the method signature

I think SigningException is a RuntimeException which shouldn't require adding it as part of the method signature.

an ImpersonatedCredential can't be of ComputeEngineCredential

This is my understanding. sourceCredential can be user or sa (ref), or a couple of external account types (ref). So ComputeEngineCredential cannot be sourceCredential (and I don't know if impersonate a GCE cred have a use case?).

I think SigningException shouldn't require adding it as part of the method signature.

Right, no signature change required, but I was concerned about behavior change.

Ah, I was looking at this method which seems to allow any type of credential to be passed in.

I only see this comment that tries to prevent other Credential types:

* @param sourceCredentials the source credential used to acquire the impersonated credentials. It * should be either a user account credential or a service account credential.

Seems a bit odd that certain static methods are checking for Credential types and others aren't. Maybe there is a reason for this... If not, probably something we can backfill and fix in a different PR.

I'm assuming in some downstream use case, some functionality will fail when using ImpersonatedCredentials with ComputeEngineCredentials as the source. We probably don't have any users that have this setup (Impersonating a Compute Credentials), I just don't know enough about Impersonation to be sure about that. Would you know if this is the case?

Right, no signature change required, but I was concerned about behavior change.

I am just thinking about keeping these consistent. As of now, I think they anything that signs should be either SigningException or IllegalStateException due to getUniverseDomain() call (even if ImpersonatedCredentials may not end up ever throwing it).

I lean to agree with you on keeping consistent.

On the " using ImpersonatedCredentials with ComputeEngineCredentials as the source", I also find it a bit odd that this method you are quoting only specifies allowed source credential types in javadoc but not enforced. @TimurSadykov By any chance you know any context about it? Was this a miss or intentional? Also, in general, is it fair to assume ComputeEngineCredentials should not be source credential for ImpersonatedCredentials?

Ok, let do this if this sounds fine:

Let's have both methods throw a SigningException for an IOException (from retrieving the UD) to keep them consistent.

Let's create an issue to add validation to ensure ImpersonatedCredential can only be SA or User Credentials and add add it to the backlog. Shouldn't block this PR any more.

done in 3dbc8e9

filed add validation to ensure ImpersonatedCredential can only be SA or User Credentials #1602

oauth2_http/javatests/com/google/auth/oauth2/ImpersonatedCredentialsTest.java

sonarqubecloud · 2024-11-04T21:49:35Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

zhumin8 · 2024-11-07T21:22:59Z

@TimurSadykov can you please also take a look?

…ute.

zhumin8 · 2024-12-17T20:01:20Z

@sai-sunder-s If you can take a look.
@aeitzman @lsirac These changes makes changes ImpersonatedCredentials, can you take a look to confirm if this change works fine with the BYOID flows?

sai-sunder-s · 2024-12-19T20:29:28Z

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

    } catch (SigningException ex) {
      throw ex;
    } catch (RuntimeException ex) {
      throw new SigningException("Signing failed", ex);
+    } catch (IOException ex) {
+      throw new SigningException("Failed to sign: Error obtaining universe domain", ex);


What is the guarantee that IO exception can happen only due to universe domain check?

maybe get universe domain separately so that we can be confident in the error messaging?

The IAMUtils.sign() method catches IOExceptions inside the method and re-throws it as ServiceAccountSigner.SigningException. I believe the only place that can throw IOException is from the getUniverseDomain() call, which should happen before we enter the sign() method.

oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java

…s.java Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>

oauth2_http/java/com/google/auth/oauth2/IamUtils.java

oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java

aeitzman · 2025-01-13T21:19:00Z

oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java

-        throw new IllegalStateException(e);
+        // this should never happen because ImpersonatedCredential can only be SA or User
+        // Credentials.
+        throw new SigningException("Signing failed", e);


Why is the exception type changed here?

Thanks for capturing this! This is actually a mistake on my side made in 2fec328 trying to address this feedback

The intended change is done in 3dbc8e9. I am reverting this accidental change in 40e2f9e

Co-authored-by: aeitzman <12433791+aeitzman@users.noreply.github.com>

sonarqubecloud · 2025-01-14T22:00:57Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zhumin8 added 5 commits October 9, 2024 16:52

copied changes for sign and id token requests.

4974f3a

add tests.

336a109

imp credential changes to bring from sa-2-sa pr.

689cf68

Merge branch 'main' into id-sign

982e586

rename IAM_SIGN_BLOB_ENDPOINT_FORMAT, and edit to comment.

546942b

product-auto-label bot added the size: m Pull request size is medium. label Nov 1, 2024

zhumin8 added 2 commits November 1, 2024 17:02

Merge branch 'main' into id-sign

424104b

fix exception in ComputeEngineCredentials, add test. Edit exception t…

b5d43c9

…ests to use assertThrows.

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Nov 4, 2024

zhumin8 added 2 commits November 4, 2024 11:43

fix: mistake in rewriting the assert.

4fc863a

fix: merging mistake.

b6e6d1a

product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Nov 4, 2024

move iam endpoint format strings from OAuth2Utils to IamUtils.

d81aafe

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Nov 4, 2024

zhumin8 requested a review from lqiu96 November 4, 2024 19:03

zhumin8 marked this pull request as ready for review November 4, 2024 19:03

zhumin8 requested review from a team as code owners November 4, 2024 19:03

lqiu96 reviewed Nov 4, 2024

View reviewed changes

oauth2_http/javatests/com/google/auth/oauth2/ImpersonatedCredentialsTest.java Show resolved Hide resolved

zhumin8 added 2 commits November 4, 2024 15:39

feedback: add message to exception.

a8d466f

feedback: add test for universedomain exception with mock.

083557d

zhumin8 requested a review from TimurSadykov November 7, 2024 21:22

zhumin8 removed the request for review from TimurSadykov December 17, 2024 19:36

zhumin8 requested a review from sai-sunder-s December 17, 2024 19:36

zhumin8 added 3 commits December 17, 2024 14:38

Merge branch 'main' into id-sign

9831d5d

address feedback: throw a SigningException to be consistent with Comp…

2fec328

…ute.

remove hardcoded iam sign endpoint used only for test setup.

fb324dc

zhumin8 mentioned this pull request Dec 17, 2024

add validation to ensure ImpersonatedCredential can only be SA or User Credentials #1602

Open

sai-sunder-s approved these changes Dec 19, 2024

View reviewed changes

lqiu96 reviewed Jan 8, 2025

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java Outdated Show resolved Hide resolved

lqiu96 approved these changes Jan 8, 2025

View reviewed changes

zhumin8 and others added 2 commits January 13, 2025 10:22

Update oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredential…

3dbc8e9

…s.java Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>

update test to throw SigningException at universeDomainException.

bef8346

aeitzman approved these changes Jan 13, 2025

View reviewed changes

zhumin8 and others added 3 commits January 14, 2025 16:44

revert accidental change in 2fec328.

40e2f9e

Update oauth2_http/java/com/google/auth/oauth2/IamUtils.java

e47e4f9

Co-authored-by: aeitzman <12433791+aeitzman@users.noreply.github.com>

Merge branch 'main' into id-sign

88caa5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ImpersonatedCredentials to support universe domain for idtoken and signblob #1566

feat: ImpersonatedCredentials to support universe domain for idtoken and signblob #1566

zhumin8 commented Nov 1, 2024 •

edited

Loading

lqiu96 Nov 4, 2024 •

edited

Loading

zhumin8 Nov 4, 2024

lqiu96 Nov 4, 2024

zhumin8 Nov 7, 2024

lqiu96 Nov 11, 2024

zhumin8 Nov 11, 2024

lqiu96 Dec 12, 2024

zhumin8 Dec 17, 2024 •

edited

Loading

sonarqubecloud bot commented Nov 4, 2024

zhumin8 commented Nov 7, 2024

zhumin8 commented Dec 17, 2024

sai-sunder-s Dec 19, 2024

lqiu96 Jan 8, 2025

aeitzman Jan 13, 2025

zhumin8 Jan 14, 2025

sonarqubecloud bot commented Jan 14, 2025

feat: ImpersonatedCredentials to support universe domain for idtoken and signblob #1566

Are you sure you want to change the base?

feat: ImpersonatedCredentials to support universe domain for idtoken and signblob #1566

Conversation

zhumin8 commented Nov 1, 2024 • edited Loading

lqiu96 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhumin8 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 4, 2024

Quality Gate passed

zhumin8 commented Nov 7, 2024

zhumin8 commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 14, 2025

Quality Gate passed

zhumin8 commented Nov 1, 2024 •

edited

Loading

lqiu96 Nov 4, 2024 •

edited

Loading

zhumin8 Dec 17, 2024 •

edited

Loading