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

feat: Support transport and binding-enforcement MDS parameters. #1558

Merged
merged 19 commits into from
Jan 9, 2025
Merged

feat: Support transport and binding-enforcement MDS parameters. #1558

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
91dba11
Support transport and binding-enforcement MDS parameters.
rmehta19 Oct 26, 2024
ba11e8a
add some tests.
rmehta19 Oct 28, 2024
47ec152
Merge branch 'main' into mds-cred-token-req-param
rmehta19 Nov 19, 2024
9d46bbd
null or empty.
rmehta19 Dec 3, 2024
3cb6e98
no default value in builder.
rmehta19 Dec 3, 2024
cb70556
Merge branch 'main' into mds-cred-token-req-param
rmehta19 Dec 3, 2024
4d0440b
Merge branch 'main' into mds-cred-token-req-param
rmehta19 Dec 13, 2024
c96df22
use enums to hold possible Transport + BindingEnforcement types
rmehta19 Dec 13, 2024
a625889
Merge branch 'main' into mds-cred-token-req-param
rmehta19 Dec 13, 2024
3d46963
Enum labels + Transport -> AuthTransport.
rmehta19 Dec 13, 2024
f36a19c
separate constant with underscore.
rmehta19 Dec 21, 2024
8a2d967
add javadocs + getTransport -> getAuthTransport.
rmehta19 Dec 21, 2024
ce74e45
add test both params null.
rmehta19 Dec 21, 2024
8ce3a4d
AuthTransport -> GoogleAuthTransport.
rmehta19 Dec 21, 2024
838380f
Merge branch 'main' into mds-cred-token-req-param
rmehta19 Jan 7, 2025
e921991
Merge branch 'main' into mds-cred-token-req-param
rmehta19 Jan 9, 2025
20964e2
add some javadoc.
rmehta19 Jan 9, 2025
94618e0
move javadoc to GoogleAuthTransport javadoc.
rmehta19 Jan 9, 2025
9261fe8
clean up comments.
rmehta19 Jan 9, 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
73 changes: 72 additions & 1 deletion oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,40 @@ public class ComputeEngineCredentials extends GoogleCredentials
static final int MAX_COMPUTE_PING_TRIES = 3;
static final int COMPUTE_PING_CONNECTION_TIMEOUT_MS = 500;

public enum AuthTransport {
Copy link
Contributor

@lqiu96 lqiu96 Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think AuthTransport is ok for this. I would prefer if this could be more specific if possible.

Is there a term for alts and mtls type of transport that we can use? Does TlsTransport make sense? I don't know if TLS has a transport concept or if that would be confusing.

GoogleMtlsTransport? Though I'm not sure mtls/atls are google specific terms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR: WDYT of GoogleAuthTransport?

I think AuthTransport is ok for this. I would prefer if this could be more specific if possible.

We generally call this "transport type": describes how the client is authenticating to Google APIS either via GFE (using mtls) or via DirectPath (using alts). However Blake mentioned above that transport has a different meaning in Client libraries (grpc vs http), so it would be good to clarify to something more specific, which is how we landed on AuthTransport initially.

Is there a term for alts and mtls type of transport that we can use? Does TlsTransport make sense? I don't know if TLS has a transport concept or if that would be confusing.

TLS is the security protocol being used, it's used in both mtls and alts. mtls is mutually authenticated TLS, alts is (in short) "Google-optimized" mtls (see public docs).

In the enum name, I think we want to focus on how clients are authenticating to Google APIS, not details of the protocol being used (mtls vs alts). How about GoogleAuthTransport? I think this captures that the enum is listing different ways to ways clients can authenticate to Google APIs.

GoogleMtlsTransport? Though I'm not sure mtls/atls are google specific terms.
I think this name would be ok. One thing is that mtls via GFE strictly adheres to the TLS standard (i.e. not Google specific). Whereas alts is a Google specific protocol.

I've done this in 8ce3a4d, let me know if the name sounds ok.

// Authenticating to Google APIs via DirectPath
ALTS("alts"),
// Authenticating to Google APIs via GFE
MTLS("mtls");

private final String label;

private AuthTransport(String label) {
this.label = label;
}

public String getLabel() {
return label;
}
}

public enum BindingEnforcement {
// Binding enforcement will always happen, irrespective of the IAM policy.
ON("on"),
// Binding enforcement will depend on IAM policy.
IAMPOLICY("iam-policy");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IAMPOLICY("iam-policy");
IAM_POLICY("iam-policy");

I believe the convention is underscore to separate the words for constants

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 in f36a19c


private final String label;

private BindingEnforcement(String label) {
this.label = label;
}

public String getLabel() {
return label;
}
}

private static final String METADATA_FLAVOR = "Metadata-Flavor";
private static final String GOOGLE = "Google";
private static final String WINDOWS = "windows";
Expand All @@ -122,6 +156,9 @@ public class ComputeEngineCredentials extends GoogleCredentials

private final Collection<String> scopes;

private final AuthTransport transport;
private final BindingEnforcement bindingEnforcement;

private transient HttpTransportFactory transportFactory;
private transient String serviceAccountEmail;

Expand Down Expand Up @@ -152,6 +189,8 @@ private ComputeEngineCredentials(ComputeEngineCredentials.Builder builder) {
scopeList.removeAll(Arrays.asList("", null));
this.scopes = ImmutableSet.<String>copyOf(scopeList);
}
this.transport = builder.getTransport();
this.bindingEnforcement = builder.getBindingEnforcement();
}

@Override
Expand Down Expand Up @@ -191,7 +230,10 @@ public final Collection<String> getScopes() {
}

/**
* If scopes is specified, add "?scopes=comma-separated-list-of-scopes" to the token url.
* If scopes is specified, add "?scopes=comma-separated-list-of-scopes" to the token url. If
* transport is specified, add "?transport=xyz" to the token url; xyz is one of "alts" or "mtls".
* If bindingEnforcement is specified, add "?binding-enforcement=xyz" to the token url; xyz is one
* of "iam-policy" or "on".
*
* @return token url with the given scopes
*/
Expand All @@ -200,6 +242,12 @@ String createTokenUrlWithScopes() {
if (!scopes.isEmpty()) {
tokenUrl.set("scopes", Joiner.on(',').join(scopes));
}
if (transport != null) {
tokenUrl.set("transport", transport.getLabel());
}
if (bindingEnforcement != null) {
tokenUrl.set("binding-enforcement", bindingEnforcement.getLabel());
}
return tokenUrl.toString();
}

Expand Down Expand Up @@ -643,6 +691,9 @@ public static class Builder extends GoogleCredentials.Builder {
private Collection<String> scopes;
private Collection<String> defaultScopes;

private AuthTransport transport;
private BindingEnforcement bindingEnforcement;

protected Builder() {
setRefreshMargin(COMPUTE_REFRESH_MARGIN);
setExpirationMargin(COMPUTE_EXPIRATION_MARGIN);
Expand Down Expand Up @@ -684,6 +735,18 @@ public Builder setQuotaProjectId(String quotaProjectId) {
return this;
}

@CanIgnoreReturnValue
public Builder setTransport(AuthTransport transport) {
this.transport = transport;
return this;
}

@CanIgnoreReturnValue
public Builder setBindingEnforcement(BindingEnforcement bindingEnforcement) {
this.bindingEnforcement = bindingEnforcement;
return this;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you some small/ simple javadocs for these public setters?

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 in 8a2d967


public HttpTransportFactory getHttpTransportFactory() {
return transportFactory;
}
Expand All @@ -696,6 +759,14 @@ public Collection<String> getDefaultScopes() {
return defaultScopes;
}

public AuthTransport getTransport() {
return transport;
}

public BindingEnforcement getBindingEnforcement() {
return bindingEnforcement;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

same here. also if it's returning AuthTransport, can it be getAuthTransport()?

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 in 8a2d967

@Override
public ComputeEngineCredentials build() {
return new ComputeEngineCredentials(this);
Expand Down
92 changes: 92 additions & 0 deletions oauth2_http/javatests/com/google/auth/oauth2/ComputeEngineCredentialsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,98 @@ public void buildTokenUrlWithScopes_defaultScopes() {
assertEquals("bar", scopes.toArray()[1]);
}

@Test
public void buildTokenUrl_nullTransport() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setTransport(null)
.setBindingEnforcement(ComputeEngineCredentials.BindingEnforcement.ON)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?binding-enforcement=on", softBoundTokenUrl);
}

@Test
public void buildTokenUrl_nullBindingEnforcement() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setTransport(ComputeEngineCredentials.AuthTransport.MTLS)
.setBindingEnforcement(null)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?transport=mtls", softBoundTokenUrl);
}

@Test
public void buildTokenUrlSoftMtlsBound_mtls_transport() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setTransport(ComputeEngineCredentials.AuthTransport.MTLS)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?transport=mtls", softBoundTokenUrl);
}

@Test
public void buildTokenUrlSoftMtlsBound_iam_enforcement() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setBindingEnforcement(ComputeEngineCredentials.BindingEnforcement.IAMPOLICY)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?binding-enforcement=iam-policy", softBoundTokenUrl);
}

@Test
public void buildTokenUrlSoftMtlsBound_mtls_transport_iam_enforcement() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setTransport(ComputeEngineCredentials.AuthTransport.MTLS)
.setBindingEnforcement(ComputeEngineCredentials.BindingEnforcement.IAMPOLICY)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?transport=mtls&binding-enforcement=iam-policy", softBoundTokenUrl);
}

@Test
public void buildTokenUrlHardMtlsBound_always_enforced() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setBindingEnforcement(ComputeEngineCredentials.BindingEnforcement.ON)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?binding-enforcement=on", softBoundTokenUrl);
}

@Test
public void buildTokenUrlHardMtlsBound_mtls_transport_always_enforced() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setTransport(ComputeEngineCredentials.AuthTransport.MTLS)
.setBindingEnforcement(ComputeEngineCredentials.BindingEnforcement.ON)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?transport=mtls&binding-enforcement=on", softBoundTokenUrl);
}

@Test
public void buildTokenUrlHardDirectPathBound_alts_transport() {
ComputeEngineCredentials credentials =
ComputeEngineCredentials.newBuilder()
.setTransport(ComputeEngineCredentials.AuthTransport.ALTS)
.build();
String softBoundTokenUrl = credentials.createTokenUrlWithScopes();

assertEquals(TOKEN_URL + "?transport=alts", softBoundTokenUrl);
}

@Test
public void buildScoped_scopesPresent() throws IOException {
ComputeEngineCredentials credentials =
Expand Down
Loading