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

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

Merged
merged 21 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
4974f3a
copied changes for sign and id token requests.
zhumin8 Oct 9, 2024
336a109
add tests.
zhumin8 Oct 14, 2024
689cf68
imp credential changes to bring from sa-2-sa pr.
zhumin8 Oct 14, 2024
982e586
Merge branch 'main' into id-sign
zhumin8 Nov 1, 2024
546942b
rename IAM_SIGN_BLOB_ENDPOINT_FORMAT, and edit to comment.
zhumin8 Nov 1, 2024
424104b
Merge branch 'main' into id-sign
zhumin8 Nov 1, 2024
b5d43c9
fix exception in ComputeEngineCredentials, add test. Edit exception t…
zhumin8 Nov 4, 2024
4fc863a
fix: mistake in rewriting the assert.
zhumin8 Nov 4, 2024
b6e6d1a
fix: merging mistake.
zhumin8 Nov 4, 2024
d81aafe
move iam endpoint format strings from OAuth2Utils to IamUtils.
zhumin8 Nov 4, 2024
a8d466f
feedback: add message to exception.
zhumin8 Nov 4, 2024
083557d
feedback: add test for universedomain exception with mock.
zhumin8 Nov 4, 2024
9831d5d
Merge branch 'main' into id-sign
zhumin8 Dec 17, 2024
2fec328
address feedback: throw a SigningException to be consistent with Comp…
zhumin8 Dec 17, 2024
fb324dc
remove hardcoded iam sign endpoint used only for test setup.
zhumin8 Dec 17, 2024
3dbc8e9
Update oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredential…
zhumin8 Jan 13, 2025
bef8346
update test to throw SigningException at universeDomainException.
zhumin8 Jan 13, 2025
40e2f9e
revert accidental change in 2fec328.
zhumin8 Jan 14, 2025
e47e4f9
Update oauth2_http/java/com/google/auth/oauth2/IamUtils.java
zhumin8 Jan 14, 2025
88caa5e
Merge branch 'main' into id-sign
zhumin8 Jan 14, 2025
f3c9ae8
Merge branch 'main' into id-sign
zhumin8 Jan 21, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -599,11 +599,18 @@ public byte[] sign(byte[] toSign) {
try {
String account = getAccount();
return IamUtils.sign(
account, this, transportFactory.create(), toSign, Collections.<String, Object>emptyMap());
account,
this,
this.getUniverseDomain(),
transportFactory.create(),
toSign,
Collections.<String, Object>emptyMap());
} 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
}

Expand Down
30 changes: 22 additions & 8 deletions oauth2_http/java/com/google/auth/oauth2/IamUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,14 @@
* features like signing.
*/
class IamUtils {
private static final String SIGN_BLOB_URL_FORMAT =
"https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/%s:signBlob";
private static final String ID_TOKEN_URL_FORMAT =
"https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/%s:generateIdToken";

// iam credentials endpoints are to be formatted with universe domain and client email
static final String IAM_ID_TOKEN_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:generateIdToken";
static final String IAM_ACCESS_TOKEN_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:generateAccessToken";
static final String IAM_SIGN_BLOB_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:signBlob";
private static final String PARSE_ERROR_MESSAGE = "Error parsing error message response. ";
private static final String PARSE_ERROR_SIGNATURE = "Error parsing signature response. ";

Expand All @@ -88,6 +92,7 @@ class IamUtils {
static byte[] sign(
String serviceAccountEmail,
Credentials credentials,
String universeDomain,
HttpTransport transport,
byte[] toSign,
Map<String, ?> additionalFields) {
Expand All @@ -97,7 +102,12 @@ static byte[] sign(
String signature;
try {
signature =
getSignature(serviceAccountEmail, base64.encode(toSign), additionalFields, factory);
getSignature(
serviceAccountEmail,
universeDomain,
base64.encode(toSign),
additionalFields,
factory);
} catch (IOException ex) {
throw new ServiceAccountSigner.SigningException("Failed to sign the provided bytes", ex);
}
Expand All @@ -106,11 +116,13 @@ static byte[] sign(

private static String getSignature(
String serviceAccountEmail,
String universeDomain,
String bytes,
Map<String, ?> additionalFields,
HttpRequestFactory factory)
throws IOException {
String signBlobUrl = String.format(SIGN_BLOB_URL_FORMAT, serviceAccountEmail);
String signBlobUrl =
String.format(IAM_SIGN_BLOB_ENDPOINT_FORMAT, universeDomain, serviceAccountEmail);
GenericUrl genericUrl = new GenericUrl(signBlobUrl);

GenericData signRequest = new GenericData();
Expand Down Expand Up @@ -193,10 +205,12 @@ static IdToken getIdToken(
String targetAudience,
boolean includeEmail,
Map<String, ?> additionalFields,
CredentialTypeForMetrics credentialTypeForMetrics)
CredentialTypeForMetrics credentialTypeForMetrics,
String universeDomain)
throws IOException {

String idTokenUrl = String.format(ID_TOKEN_URL_FORMAT, serviceAccountEmail);
String idTokenUrl =
String.format(IAM_ID_TOKEN_ENDPOINT_FORMAT, universeDomain, serviceAccountEmail);
GenericUrl genericUrl = new GenericUrl(idTokenUrl);

GenericData idTokenRequest = new GenericData();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,12 +345,19 @@ public void setTransportFactory(HttpTransportFactory httpTransportFactory) {
*/
@Override
public byte[] sign(byte[] toSign) {
return IamUtils.sign(
getAccount(),
sourceCredentials,
transportFactory.create(),
toSign,
ImmutableMap.of("delegates", this.delegates));
try {
return IamUtils.sign(
getAccount(),
sourceCredentials,
getUniverseDomain(),
transportFactory.create(),
toSign,
ImmutableMap.of("delegates", this.delegates));
} catch (IOException e) {
// Throwing an IOException would be a breaking change, so wrap it here.
// This should not happen for this credential type.
throw new IllegalStateException(e);
Copy link
Contributor

@lqiu96 lqiu96 Nov 4, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

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 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let do this if this sounds fine:

  1. Let's have both methods throw a SigningException for an IOException (from retrieving the UD) to keep them consistent.
  2. 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.

Copy link
Contributor Author

@zhumin8 zhumin8 Dec 17, 2024

Choose a reason for hiding this comment

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

}
}

/**
Expand Down Expand Up @@ -525,7 +532,7 @@ public AccessToken refreshAccessToken() throws IOException {
this.iamEndpointOverride != null
? this.iamEndpointOverride
: String.format(
OAuth2Utils.IAM_ACCESS_TOKEN_ENDPOINT_FORMAT,
IamUtils.IAM_ACCESS_TOKEN_ENDPOINT_FORMAT,
getUniverseDomain(),
this.targetPrincipal);

Expand Down Expand Up @@ -593,7 +600,8 @@ public IdToken idTokenWithAudience(String targetAudience, List<IdTokenProvider.O
targetAudience,
includeEmail,
ImmutableMap.of("delegates", this.delegates),
getMetricsCredentialType());
getMetricsCredentialType(),
getUniverseDomain());
}

@Override
Expand Down
9 changes: 0 additions & 9 deletions oauth2_http/java/com/google/auth/oauth2/OAuth2Utils.java
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,6 @@ class OAuth2Utils {
static final String TOKEN_TYPE_TOKEN_EXCHANGE = "urn:ietf:params:oauth:token-type:token-exchange";
static final String GRANT_TYPE_JWT_BEARER = "urn:ietf:params:oauth:grant-type:jwt-bearer";

// generateIdToken endpoint is to be formatted with universe domain and client email
static final String IAM_ID_TOKEN_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:generateIdToken";

static final String IAM_ACCESS_TOKEN_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:generateAccessToken";
static final String SIGN_BLOB_ENDPOINT_FORMAT =
"https://iamcredentials.%s/v1/projects/-/serviceAccounts/%s:signBlob";

static final URI TOKEN_SERVER_URI = URI.create("https://oauth2.googleapis.com/token");

static final URI TOKEN_REVOKE_URI = URI.create("https://oauth2.googleapis.com/revoke");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -636,8 +636,7 @@ private IdToken getIdTokenIamEndpoint(String targetAudience) throws IOException
// `getUniverseDomain()` throws an IOException that would need to be caught
URI iamIdTokenUri =
URI.create(
String.format(
OAuth2Utils.IAM_ID_TOKEN_ENDPOINT_FORMAT, getUniverseDomain(), clientEmail));
String.format(IamUtils.IAM_ID_TOKEN_ENDPOINT_FORMAT, getUniverseDomain(), clientEmail));
HttpRequest request = buildIdTokenRequest(iamIdTokenUri, transportFactory, content);
// Use the Access Token from the SSJWT to request the ID Token from IAM Endpoint
request.setHeaders(new HttpHeaders().set(AuthHttpConstants.AUTHORIZATION, accessToken));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -541,7 +542,7 @@ public LowLevelHttpResponse execute() throws IOException {
}

@Test
public void sign_sameAs() throws IOException {
public void sign_sameAs() {
MockMetadataServerTransportFactory transportFactory = new MockMetadataServerTransportFactory();
String defaultAccountEmail = "mail@mail.com";
byte[] expectedSignature = {0xD, 0xE, 0xA, 0xD};
Expand All @@ -555,21 +556,36 @@ public void sign_sameAs() throws IOException {
}

@Test
public void sign_getAccountFails() throws IOException {
public void sign_getUniverseException() {
MockMetadataServerTransportFactory transportFactory = new MockMetadataServerTransportFactory();

String defaultAccountEmail = "mail@mail.com";
transportFactory.transport.setServiceAccountEmail(defaultAccountEmail);
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();

transportFactory.transport.setRequestStatusCode(501);
Assert.assertThrows(IOException.class, credentials::getUniverseDomain);

byte[] expectedSignature = {0xD, 0xE, 0xA, 0xD};
SigningException signingException =
Assert.assertThrows(SigningException.class, () -> credentials.sign(expectedSignature));
assertEquals("Failed to sign: Error obtaining universe domain", signingException.getMessage());
}

@Test
public void sign_getAccountFails() {
MockMetadataServerTransportFactory transportFactory = new MockMetadataServerTransportFactory();
byte[] expectedSignature = {0xD, 0xE, 0xA, 0xD};

transportFactory.transport.setSignature(expectedSignature);
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();

try {
credentials.sign(expectedSignature);
fail("Should not be able to use credential without exception.");
} catch (SigningException ex) {
assertNotNull(ex.getMessage());
assertNotNull(ex.getCause());
}
SigningException exception =
Assert.assertThrows(SigningException.class, () -> credentials.sign(expectedSignature));
assertNotNull(exception.getMessage());
assertNotNull(exception.getCause());
}

@Test
Expand Down Expand Up @@ -601,15 +617,13 @@ public LowLevelHttpResponse execute() throws IOException {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();

try {
byte[] bytes = {0xD, 0xE, 0xA, 0xD};
credentials.sign(bytes);
fail("Signing should have failed");
} catch (SigningException e) {
assertEquals("Failed to sign the provided bytes", e.getMessage());
assertNotNull(e.getCause());
assertTrue(e.getCause().getMessage().contains("403"));
}
byte[] bytes = {0xD, 0xE, 0xA, 0xD};

SigningException exception =
Assert.assertThrows(SigningException.class, () -> credentials.sign(bytes));
assertEquals("Failed to sign the provided bytes", exception.getMessage());
assertNotNull(exception.getCause());
assertTrue(exception.getCause().getMessage().contains("403"));
}

@Test
Expand Down Expand Up @@ -641,15 +655,13 @@ public LowLevelHttpResponse execute() throws IOException {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();

try {
byte[] bytes = {0xD, 0xE, 0xA, 0xD};
credentials.sign(bytes);
fail("Signing should have failed");
} catch (SigningException e) {
assertEquals("Failed to sign the provided bytes", e.getMessage());
assertNotNull(e.getCause());
assertTrue(e.getCause().getMessage().contains("500"));
}
byte[] bytes = {0xD, 0xE, 0xA, 0xD};

SigningException exception =
Assert.assertThrows(SigningException.class, () -> credentials.sign(bytes));
assertEquals("Failed to sign the provided bytes", exception.getMessage());
assertNotNull(exception.getCause());
assertTrue(exception.getCause().getMessage().contains("500"));
}

@Test
Expand All @@ -674,14 +686,11 @@ public LowLevelHttpResponse execute() throws IOException {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();

try {
credentials.refreshAccessToken();
fail("Should have failed");
} catch (IOException e) {
assertTrue(e.getCause().getMessage().contains("503"));
assertTrue(e instanceof GoogleAuthException);
assertTrue(((GoogleAuthException) e).isRetryable());
}
IOException exception =
Assert.assertThrows(IOException.class, () -> credentials.refreshAccessToken());
assertTrue(exception.getCause().getMessage().contains("503"));
assertTrue(exception instanceof GoogleAuthException);
assertTrue(((GoogleAuthException) exception).isRetryable());
}

@Test
Expand Down Expand Up @@ -714,12 +723,9 @@ public LowLevelHttpResponse execute() throws IOException {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();

try {
credentials.refreshAccessToken();
fail("Should have failed");
} catch (IOException e) {
assertFalse(e instanceof GoogleAuthException);
}
IOException exception =
Assert.assertThrows(IOException.class, () -> credentials.refreshAccessToken());
assertFalse(exception instanceof GoogleAuthException);
}
}

Expand Down Expand Up @@ -889,15 +895,13 @@ public LowLevelHttpResponse execute() throws IOException {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder().setHttpTransportFactory(transportFactory).build();

try {
byte[] bytes = {0xD, 0xE, 0xA, 0xD};
credentials.sign(bytes);
fail("Signing should have failed");
} catch (SigningException e) {
assertEquals("Failed to sign the provided bytes", e.getMessage());
assertNotNull(e.getCause());
assertTrue(e.getCause().getMessage().contains("Empty content"));
}
byte[] bytes = {0xD, 0xE, 0xA, 0xD};

SigningException exception =
Assert.assertThrows(SigningException.class, () -> credentials.sign(bytes));
assertEquals("Failed to sign the provided bytes", exception.getMessage());
assertNotNull(exception.getCause());
assertTrue(exception.getCause().getMessage().contains("Empty content"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import static org.junit.Assert.assertTrue;

import com.google.api.client.http.HttpStatusCodes;
import com.google.auth.Credentials;
import com.google.auth.ServiceAccountSigner;
import com.google.common.collect.ImmutableMap;
import java.io.IOException;
Expand All @@ -60,6 +61,7 @@ public void setup() throws IOException {
// token
credentials = Mockito.mock(ServiceAccountCredentials.class);
Mockito.when(credentials.getRequestMetadata(Mockito.any())).thenReturn(ImmutableMap.of());
Mockito.when(credentials.getUniverseDomain()).thenReturn("googleapis.com");
}

@Test
Expand All @@ -76,6 +78,7 @@ public void sign_success_noRetry() {
IamUtils.sign(
CLIENT_EMAIL,
credentials,
Credentials.GOOGLE_DEFAULT_UNIVERSE,
transportFactory.getTransport(),
expectedSignature,
ImmutableMap.of());
Expand Down Expand Up @@ -107,6 +110,7 @@ public void sign_retryTwoTimes_success() {
IamUtils.sign(
CLIENT_EMAIL,
credentials,
Credentials.GOOGLE_DEFAULT_UNIVERSE,
transportFactory.getTransport(),
expectedSignature,
ImmutableMap.of());
Expand Down Expand Up @@ -143,6 +147,7 @@ public void sign_retryThreeTimes_success() {
IamUtils.sign(
CLIENT_EMAIL,
credentials,
Credentials.GOOGLE_DEFAULT_UNIVERSE,
transportFactory.getTransport(),
expectedSignature,
ImmutableMap.of());
Expand Down Expand Up @@ -185,6 +190,7 @@ public void sign_retryThreeTimes_exception() {
IamUtils.sign(
CLIENT_EMAIL,
credentials,
Credentials.GOOGLE_DEFAULT_UNIVERSE,
transportFactory.getTransport(),
expectedSignature,
ImmutableMap.of()));
Expand Down Expand Up @@ -220,6 +226,7 @@ public void sign_4xxError_noRetry_exception() {
IamUtils.sign(
CLIENT_EMAIL,
credentials,
Credentials.GOOGLE_DEFAULT_UNIVERSE,
transportFactory.getTransport(),
expectedSignature,
ImmutableMap.of()));
Expand Down
Loading