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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 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
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 @@ -95,9 +95,6 @@ public class ComputeEngineCredentials extends GoogleCredentials

static final String DEFAULT_METADATA_SERVER_URL = "http://metadata.google.internal";

static final String SIGN_BLOB_URL_FORMAT =
"https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/%s:signBlob";

// Note: the explicit `timeout` and `tries` below is a workaround. The underlying
// issue is that resolving an unknown host on some networks will take
// 20-30 seconds; making this timeout short fixes the issue, but
Expand Down Expand Up @@ -599,11 +596,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
zhumin8 marked this conversation as resolved.
Show resolved Hide resolved
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 ex) {
// Throwing an IOException would be a breaking change, so wrap it here.
// This should not happen for this credential type.
throw new IllegalStateException("Failed to sign: Error obtaining universe domain", ex);
zhumin8 marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
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 Expand Up @@ -775,8 +783,9 @@ public ImpersonatedCredentials build() {
return new ImpersonatedCredentials(this);
} catch (IOException e) {
// throwing exception would be breaking change. catching instead.
zhumin8 marked this conversation as resolved.
Show resolved Hide resolved
// this should never happen.
throw new IllegalStateException(e);
// this should never happen because ImpersonatedCredential can only be SA or User
zhumin8 marked this conversation as resolved.
Show resolved Hide resolved
// Credentials.
throw new SigningException("Signing failed", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the exception type changed 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.

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

}
}
}
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
Loading
Loading