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.

Sign up for GitHub

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

Jump to bottom

Add support for explicit java.security.Provider to ServiceAccountCred… #411

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add support for explicit java.security.Provider to ServiceAccountCred… #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 49 additions & 24 deletions oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java
View file
Open in desktop
Expand Up @@ -46,13 +46,15 @@
import com.google.api.client.json.JsonObjectParser;
import com.google.api.client.json.webtoken.JsonWebSignature;
import com.google.api.client.json.webtoken.JsonWebToken;
import com.google.api.client.util.Base64;
import com.google.api.client.util.ExponentialBackOff;
import com.google.api.client.util.GenericData;
import com.google.api.client.util.Joiner;
import com.google.api.client.util.PemReader;
import com.google.api.client.util.PemReader.Section;
import com.google.api.client.util.Preconditions;
import com.google.api.client.util.SecurityUtils;
import com.google.api.client.util.StringUtils;
import com.google.auth.ServiceAccountSigner;
import com.google.auth.http.HttpTransportFactory;
import com.google.common.annotations.Beta;
Expand All @@ -66,11 +68,11 @@
import java.io.StringReader;
import java.net.URI;
import java.net.URISyntaxException;
import java.security.GeneralSecurityException;
import java.security.InvalidKeyException;
import java.security.KeyFactory;
import java.security.NoSuchAlgorithmException;
import java.security.PrivateKey;
import java.security.Provider;
import java.security.Signature;
import java.security.SignatureException;
import java.security.spec.InvalidKeySpecException;
Expand Down Expand Up @@ -103,6 +105,7 @@ public class ServiceAccountCredentials extends GoogleCredentials
private final URI tokenServerUri;
private final Collection<String> scopes;
private final String quotaProjectId;
private final Provider signingProvider;

private transient HttpTransportFactory transportFactory;

Expand All @@ -122,6 +125,8 @@ public class ServiceAccountCredentials extends GoogleCredentials
* authority to the service account.
* @param projectId the project used for billing
* @param quotaProjectId The project used for quota and billing purposes. May be null.
* @param signingProvider The JCA provider to use during request signing. May be null, in which case the default
* provider will be used.
*/
ServiceAccountCredentials(
String clientId,
Expand All @@ -133,7 +138,8 @@ public class ServiceAccountCredentials extends GoogleCredentials
URI tokenServerUri,
String serviceAccountUser,
String projectId,
String quotaProjectId) {
String quotaProjectId,
Provider signingProvider) {
this.clientId = clientId;
this.clientEmail = Preconditions.checkNotNull(clientEmail);
this.privateKey = Preconditions.checkNotNull(privateKey);
Expand All @@ -143,6 +149,7 @@ public class ServiceAccountCredentials extends GoogleCredentials
firstNonNull(
transportFactory,
getFromServiceLoader(HttpTransportFactory.class, OAuth2Utils.HTTP_TRANSPORT_FACTORY));
this.signingProvider = signingProvider;
this.transportFactoryClassName = this.transportFactory.getClass().getName();
this.tokenServerUri = (tokenServerUri == null) ? OAuth2Utils.TOKEN_SERVER_URI : tokenServerUri;
this.serviceAccountUser = serviceAccountUser;
Expand Down Expand Up @@ -324,7 +331,8 @@ static ServiceAccountCredentials fromPkcs8(
tokenServerUri,
serviceAccountUser,
projectId,
quotaProject);
quotaProject,
null);
}

/** Helper to convert from a PKCS#8 String to an RSA private key */
Expand Down Expand Up @@ -512,7 +520,8 @@ public GoogleCredentials createScoped(Collection<String> newScopes) {
tokenServerUri,
serviceAccountUser,
projectId,
quotaProjectId);
quotaProjectId,
null);
}

@Override
Expand All @@ -527,7 +536,8 @@ public GoogleCredentials createDelegated(String user) {
tokenServerUri,
user,
projectId,
quotaProjectId);
quotaProjectId,
null);
}

public final String getClientId() {
Expand Down Expand Up @@ -570,7 +580,9 @@ public String getAccount() {
@Override
public byte[] sign(byte[] toSign) {
try {
Signature signer = Signature.getInstance(OAuth2Utils.SIGNATURE_ALGORITHM);
Signature signer = signingProvider == null
? Signature.getInstance(OAuth2Utils.SIGNATURE_ALGORITHM)
: Signature.getInstance(OAuth2Utils.SIGNATURE_ALGORITHM, signingProvider);
signer.initSign(getPrivateKey());
signer.update(toSign);
return signer.sign();
Expand Down Expand Up @@ -647,6 +659,19 @@ public boolean equals(Object obj) {
&& Objects.equals(this.quotaProjectId, other.quotaProjectId);
}

private String signJsonWebSignature(JsonFactory jsonFactory, JsonWebSignature.Header header, JsonWebToken.Payload payload) throws IOException {
String signedContentString = Base64.encodeBase64URLSafeString(jsonFactory.toByteArray(header)) + "." + Base64.encodeBase64URLSafeString(jsonFactory.toByteArray(payload));
byte[] signedContentBytes = StringUtils.getBytesUtf8(signedContentString);
try {
byte[] signature = this.sign(signedContentBytes);
return signedContentString + "." + Base64.encodeBase64URLSafeString(signature);

} catch (SigningException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to convert this to an IOException instead of declaring this method to throw SigningException?

Copy link
Author

Choose a reason for hiding this comment

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

No, not really, I just kept the pattern that was there before.

SigningException is unchecked. Whilst the api for refreshAccessToken declares that it throws IOException. (I'm not sure I agree that SigningException should be unchecked, but that is outside the scope of this change).

In any case I argue that a failing signature should be handled by the calling code, and as such it should be changed to an IOException before exiting refreshAccessToken.

As to whether I should keep the conversion inside signJsonWebSignature or move it to createAssertion (and duplicate it in createAssertionForIdToken) I'll follow ruling.

Copy link
Author

Choose a reason for hiding this comment

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

@elharo I'll await your answer on this before I reupload. (I'm I doing this correctly with the --amends by the way?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly do not have enough git-fu to tell. Whatever you're doing seems to work fine on my end. Just commit and upload as you need to. We'll squash the commits when we merge.

throw new IOException(
"Error signing service account access token request with private key.", e);
}
}

String createAssertion(JsonFactory jsonFactory, long currentTime, String audience)
throws IOException {
JsonWebSignature.Header header = new JsonWebSignature.Header();
Expand All @@ -667,14 +692,8 @@ String createAssertion(JsonFactory jsonFactory, long currentTime, String audienc
payload.setAudience(audience);
}

String assertion;
try {
assertion = JsonWebSignature.signUsingRsaSha256(privateKey, jsonFactory, header, payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

Critical: I'm not sure the new code is using this algorithm. I might need to patch this into an IDE and check.

Copy link
Author

Choose a reason for hiding this comment

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

The signing algorithm is dictated by OAuth2Utils.SIGNATURE_ALGORITHM. Which is "SHA256withRSA"

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Since this is security critical code I'm going to need go over this carefully and make sure of that. It might take a little time to review, or perhaps someone else can get to this before I do.

} catch (GeneralSecurityException e) {
throw new IOException(
"Error signing service account access token request with private key.", e);
}
return assertion;
String jsonWebSignature = signJsonWebSignature(jsonFactory, header, payload);
return jsonWebSignature;
}

@VisibleForTesting
Expand All @@ -698,16 +717,10 @@ String createAssertionForIdToken(
payload.setAudience(audience);
}

try {
payload.set("target_audience", targetAudience);
payload.set("target_audience", targetAudience);

String assertion =
JsonWebSignature.signUsingRsaSha256(privateKey, jsonFactory, header, payload);
return assertion;
} catch (GeneralSecurityException e) {
throw new IOException(
"Error signing service account access token request with private key.", e);
}
String assertion = signJsonWebSignature(jsonFactory, header, payload);
return assertion;
}

@SuppressWarnings("unused")
Expand Down Expand Up @@ -742,6 +755,7 @@ public static class Builder extends GoogleCredentials.Builder {
private Collection<String> scopes;
private HttpTransportFactory transportFactory;
private String quotaProjectId;
private Provider signatureProvider;

protected Builder() {}

Expand All @@ -756,6 +770,7 @@ protected Builder(ServiceAccountCredentials credentials) {
this.serviceAccountUser = credentials.serviceAccountUser;
this.projectId = credentials.projectId;
this.quotaProjectId = credentials.quotaProjectId;
this.signatureProvider = credentials.signingProvider;
}

public Builder setClientId(String clientId) {
Expand Down Expand Up @@ -808,6 +823,11 @@ public Builder setQuotaProjectId(String quotaProjectId) {
return this;
}

public Builder setSignatureProvider(Provider signatureProvider) {
this.signatureProvider = signatureProvider;
return this;
}

public String getClientId() {
return clientId;
}
Expand Down Expand Up @@ -848,6 +868,10 @@ public String getQuotaProjectId() {
return quotaProjectId;
}

public Provider getSignatureProvider() {
return signatureProvider;
}

public ServiceAccountCredentials build() {
return new ServiceAccountCredentials(
clientId,
Expand All @@ -859,7 +883,8 @@ public ServiceAccountCredentials build() {
tokenServerUri,
serviceAccountUser,
projectId,
quotaProjectId);
quotaProjectId,
signatureProvider);
}
}
}
40 changes: 40 additions & 0 deletions oauth2_http/javatests/com/google/auth/oauth2/ServiceAccountCredentialsTest.java
View file
Open in desktop
Expand Up @@ -49,6 +49,7 @@
import com.google.api.client.testing.http.MockLowLevelHttpResponse;
import com.google.api.client.util.Clock;
import com.google.api.client.util.Joiner;
import com.google.api.client.util.SecurityUtils;
import com.google.auth.TestUtils;
import com.google.auth.http.HttpTransportFactory;
import com.google.auth.oauth2.GoogleCredentialsTest.MockHttpTransportFactory;
Expand All @@ -61,6 +62,7 @@
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.PrivateKey;
import java.security.Provider;
import java.security.Signature;
import java.security.SignatureException;
import java.util.Arrays;
Expand Down Expand Up @@ -109,6 +111,15 @@ public class ServiceAccountCredentialsTest extends BaseSerializationTest {
+ "aXNzIjoiaHR0cHM6Ly9hY2NvdW50cy5nb29nbGUuY29tIiwic3ViIjoiMTAyMTAxNTUwODM0MjAwNzA4NTY4In0"
+ ".redacted";
private static final String QUOTA_PROJECT = "sample-quota-project-id";
private static Provider defaultRsaSignatureProvider;

static {
try {
defaultRsaSignatureProvider = SecurityUtils.getRsaKeyFactory().getProvider();
} catch (NoSuchAlgorithmException e) {
throw new RuntimeException("RSA keys not supported on this JVM", e);
}
}

@Test
public void createdScoped_clones() throws IOException {
Expand Down Expand Up @@ -200,6 +211,35 @@ public void createAssertion_correct() throws IOException {
assertEquals(Joiner.on(' ').join(scopes), payload.get("scope"));
}

@Test
public void createAssertionWithExplicitProvider_correct() throws IOException {
PrivateKey privateKey = ServiceAccountCredentials.privateKeyFromPkcs8(PRIVATE_KEY_PKCS8);
List<String> scopes = Arrays.asList("scope1", "scope2");
ServiceAccountCredentials credentials =
ServiceAccountCredentials.newBuilder()
.setClientId(CLIENT_ID)
.setClientEmail(CLIENT_EMAIL)
.setPrivateKey(privateKey)
.setPrivateKeyId(PRIVATE_KEY_ID)
.setScopes(scopes)
.setServiceAccountUser(USER)
.setProjectId(PROJECT_ID)
.setSignatureProvider(defaultRsaSignatureProvider)
.build();

long currentTimeMillis = FixedClock.SYSTEM.currentTimeMillis();
String assertion = credentials.createAssertion(OAuth2Utils.JSON_FACTORY, currentTimeMillis, null);

JsonWebSignature signature = JsonWebSignature.parse(OAuth2Utils.JSON_FACTORY, assertion);
JsonWebToken.Payload payload = signature.getPayload();
assertEquals(CLIENT_EMAIL, payload.getIssuer());
assertEquals(OAuth2Utils.TOKEN_SERVER_URI.toString(), payload.getAudience());
assertEquals(currentTimeMillis / 1000, (long) payload.getIssuedAtTimeSeconds());
stian-svedenborg-gul-katt marked this conversation as resolved.
Show resolved Hide resolved
assertEquals(currentTimeMillis / 1000 + 3600, (long) payload.getExpirationTimeSeconds());
assertEquals(USER, payload.getSubject());
assertEquals(Joiner.on(' ').join(scopes), payload.get("scope"));
}

@Test
public void createAssertionForIdToken_correct() throws IOException {

Expand Down