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

rmehta19 · 2024-10-26T02:06:20Z

No description provided.

rmehta19 · 2024-10-26T02:08:41Z

westarle · 2024-10-28T12:18:59Z

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

@@ -696,6 +725,14 @@ public Collection<String> getDefaultScopes() {
      return defaultScopes;
    }

+    public String getTransport() {


Please provide javadoc on these, pointing to publicly documented Metadata Service parameters.

Discussed offline: We will update public MDS docs when the feature is non-experimental. Since the usage of the S2A + MTLS + bound tokens flow is guarded by an environment variable (EXPERIMENTAL_GOOGLE_API_USE_S2A). Until the public docs are updated, we can link to AIP here and #1400 (AIP already linked in this merged PR).

rmehta19 · 2024-11-07T20:01:48Z

@lqiu96 , @zhumin8 : Would be able to take a look at this change and leave any feedback? Thanks!

sonarqubecloud · 2024-11-19T19:25:38Z

Quality Gate passed

Issues
0 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

lqiu96 · 2024-12-03T17:10:05Z

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

+    if (!transport.isEmpty()) {
+      tokenUrl.set("transport", transport);
+    }
+    if (!bindingEnforcement.isEmpty()) {
+      tokenUrl.set("binding-enforcement", bindingEnforcement);
+    }


I think this should be null and empty check. Setter can set a null value unless we want to add a check there

Thanks for pointing that out, done in 9d46bbd

lqiu96 · 2024-12-03T17:10:21Z

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

+    private String transport = "";
+    private String bindingEnforcement = "";


nit:

Suggested change

private String transport = "";

private String bindingEnforcement = "";

private String transport;

private String bindingEnforcement;

Done in 3cb6e98

lqiu96 · 2024-12-03T17:11:04Z

Update to use conventional commits:
feat: Support transport and binding-enforcement MDS parameters

rmehta19 · 2024-12-03T23:38:18Z

Thanks for the review @lqiu96 !

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

blakeli0 · 2024-12-13T18:51:37Z

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

@@ -200,6 +218,16 @@ String createTokenUrlWithScopes() {
    if (!scopes.isEmpty()) {
      tokenUrl.set("scopes", Joiner.on(',').join(scopes));
    }
+    if (transport == Transport.MTLS) {
+      tokenUrl.set("transport", "mtls");


Fields can be added to the Enum class, so that each Enum can have its own label, then you can just do tokenUrl.set("transport", transport.getLabel()). See this example.

Thanks for pointing this out! Done in latest commit.

blakeli0 · 2024-12-13T18:57:06Z

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

@@ -109,6 +109,16 @@ public class ComputeEngineCredentials extends GoogleCredentials
  static final int MAX_COMPUTE_PING_TRIES = 3;
  static final int COMPUTE_PING_CONNECTION_TIMEOUT_MS = 500;

+  public enum Transport {


I know the it is called transport in the token url, but since Transport in GAPIC has different meaning (the value of it is either gRPC or httpJson), can we name this differently? Maybe AuthTransport? Javadocs for what ALTS and MTLS mean would also be appreciated.

Renamed to AuthTransport in latest commit and added some javadocs. Thanks!

lqiu96 · 2024-12-19T21:50:07Z

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

+    // Binding enforcement will always happen, irrespective of the IAM policy.
+    ON("on"),
+    // Binding enforcement will depend on IAM policy.
+    IAMPOLICY("iam-policy");


Suggested change

IAMPOLICY("iam-policy");

IAM_POLICY("iam-policy");

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

Done in f36a19c

lqiu96 · 2024-12-19T21:51:35Z

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

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


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

Done in 8a2d967

lqiu96 · 2024-12-19T21:52:12Z

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

+    public AuthTransport getTransport() {
+      return transport;
+    }
+
+    public BindingEnforcement getBindingEnforcement() {
+      return bindingEnforcement;
+    }
+


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

Done in 8a2d967

lqiu96 · 2024-12-19T21:53:43Z

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

+  @Test
+  public void buildTokenUrl_nullTransport() {


nit: one additional test where both transport and bindingenforcements are null

Done in ce74e45

lqiu96 · 2024-12-19T21:56:33Z

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

@@ -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 {


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

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.

rmehta19 requested review from a team as code owners October 26, 2024 02:06

product-auto-label bot added the size: s Pull request size is small. label Oct 26, 2024

Support transport and binding-enforcement MDS parameters.

91dba11

rmehta19 force-pushed the mds-cred-token-req-param branch from 04db8a3 to 91dba11 Compare October 26, 2024 02:07

westarle reviewed Oct 28, 2024

View reviewed changes

add some tests.

ba11e8a

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Oct 28, 2024

Merge branch 'main' into mds-cred-token-req-param

47ec152

lqiu96 reviewed Dec 3, 2024

View reviewed changes

rmehta19 changed the title ~~Support transport and binding-enforcement MDS parameters.~~ feat: Support transport and binding-enforcement MDS parameters. Dec 3, 2024

rmehta19 added 2 commits December 3, 2024 15:33

null or empty.

9d46bbd

no default value in builder.

3cb6e98

Merge branch 'main' into mds-cred-token-req-param

cb70556

lqiu96 reviewed Dec 4, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java Show resolved Hide resolved

rmehta19 added 2 commits December 13, 2024 09:30

Merge branch 'main' into mds-cred-token-req-param

4d0440b

use enums to hold possible Transport + BindingEnforcement types

c96df22

blakeli0 reviewed Dec 13, 2024

View reviewed changes

rmehta19 added 2 commits December 13, 2024 14:54

Merge branch 'main' into mds-cred-token-req-param

a625889

Enum labels + Transport -> AuthTransport.

3d46963

lqiu96 reviewed Dec 19, 2024

View reviewed changes

rmehta19 added 4 commits December 20, 2024 18:45

separate constant with underscore.

f36a19c

add javadocs + getTransport -> getAuthTransport.

8a2d967

add test both params null.

ce74e45

AuthTransport -> GoogleAuthTransport.

8ce3a4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rmehta19 commented Oct 26, 2024

rmehta19 commented Oct 26, 2024

westarle Oct 28, 2024

rmehta19 Oct 30, 2024

rmehta19 commented Nov 7, 2024

sonarqubecloud bot commented Nov 19, 2024

lqiu96 Dec 3, 2024

rmehta19 Dec 3, 2024

lqiu96 Dec 3, 2024

rmehta19 Dec 3, 2024

lqiu96 commented Dec 3, 2024

rmehta19 commented Dec 3, 2024

blakeli0 Dec 13, 2024 •

edited

Loading

rmehta19 Dec 13, 2024

blakeli0 Dec 13, 2024

rmehta19 Dec 13, 2024

lqiu96 Dec 19, 2024

rmehta19 Dec 21, 2024

lqiu96 Dec 19, 2024

rmehta19 Dec 21, 2024

lqiu96 Dec 19, 2024

rmehta19 Dec 21, 2024

lqiu96 Dec 19, 2024

rmehta19 Dec 21, 2024

lqiu96 Dec 19, 2024 •

edited

Loading

rmehta19 Dec 21, 2024

		private String transport = "";
		private String bindingEnforcement = "";

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

Are you sure you want to change the base?

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

Conversation

rmehta19 commented Oct 26, 2024

rmehta19 commented Oct 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehta19 commented Nov 7, 2024

sonarqubecloud bot commented Nov 19, 2024

Quality Gate passed

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

lqiu96 commented Dec 3, 2024

rmehta19 commented Dec 3, 2024

blakeli0 Dec 13, 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

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

lqiu96 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 Dec 13, 2024 •

edited

Loading

lqiu96 Dec 19, 2024 •

edited

Loading