feat: Add the generated ClientSideAccessBoundaryProto class for the Client-Side CAB feature #1571

huangjiahua · 2024-11-04T18:54:44Z

See: go/client-side-cab-client-library-java

This PR introduces the ClientSideAccessBoundaryProto Protobuf Java class, necessary for serializing the Client-Side Access Boundary (CAB) token. Following the Auth SDK team's recommendation, the Protobuf definition itself isn't included in the repository, as it's anticipated to remain stable.

To support this generated class and the Client-Side CAB functionality, two new dependencies, protobuf and cel, have been added to the cab-token-generator package.

To address Maven build issues, I've excluded findbugs and guava from the cel dependency.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

huangjiahua · 2024-11-07T21:34:46Z

Will move the generated code to the new package #1562 creates

huangjiahua · 2024-11-14T22:31:04Z

Moved to the new package. Ready for review

lqiu96 · 2024-11-14T22:43:17Z

...or/java/com/google/auth/credentialaccessboundary/protobuf/ClientSideAccessBoundaryProto.java

+// Generated by the protocol buffer compiler.  DO NOT EDIT!
+// source: client_side_access_boundary.proto


How we plan on on keeping this up to date? Where does this client_side_access_boundary.proto file live and how does it get updated?

I got the suggestion from @aeitzman to manually generate this class first and not include the source proto file for now. We do have a task to figure out a way to automate the protobuf workflow later in the project.

The proto lives in google3 right now, and we do not expect it to get updated very often. For context, the credential access boundary definition used by the existing online cab feature is also based off a Google3 protobuf file, and has not changed since launch in 2021.

We do have a task to figure out a way to automate the protobuf workflow later in the project.

Sure, I think that's fine for now to unblock development.

Regarding this generated file, was this generated with an older Protoc version? I think the later versions have a comment that describes the protoc version (i.e. // Protobuf Java Version: 3.25.5). If so, can we generate this with Protoc 25.5?

The proto lives in google3 right now, and we do not expect it to get updated very often

Agree that this is fine for now to unblock the development. But I want to emphasize that introducing a new dependency protobuf-java in auth and a protoc gen code without a .proto file will cause frictions to the overall protobuf maintenance strategy in Java Client Libraries. Even if the content do not update at all, we may need to regenerate it with a newer version of protoc due to compatibility issues.
For example, if we want to upgrade protobuf-java to a version that is not compatible with 3.25.5 gen code anymore, we would need to manually regenerate this file from somewhere.

That being said, we have a lot of experience in automating proto generation process, please let us know when you start the task, it would be great if it can be integrated into our existing client library generation process.

sonarqubecloud · 2024-11-14T22:45:34Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
12.7% Duplication on New Code (required ≤ 3%)
B Reliability Rating on New Code (required ≥ A)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

lqiu96 · 2024-11-14T22:47:29Z

pom.xml

@@ -77,6 +77,8 @@
    <project.findbugs.version>3.0.2</project.findbugs.version>
    <deploy.autorelease>false</deploy.autorelease>
    <project.error-prone.version>2.35.1</project.error-prone.version>
+    <project.protobuf.version>4.28.3</project.protobuf.version>


The Java SDK protos are generated with Protoc 25.5. Libraries-Bom will set the Protobuf-Java runtime version to 4.28.3. I think this should be 3.25.5 since this is part of the BOM.

Actually downgrading it to 3.25.5 causes RequireUpperBoundDeps issue since CEL depends on 4.28.2:

[ERROR] Rule 2: org.apache.maven.enforcer.rules.dependency.RequireUpperBoundDeps failed with message: [ERROR] Failed while enforcing RequireUpperBoundDeps. The error(s) are [ [ERROR] Require upper bound dependencies error for com.google.protobuf:protobuf-java:3.25.5 paths to dependency are: [ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT [ERROR] +-com.google.protobuf:protobuf-java:3.25.5 [ERROR] and [ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT [ERROR] +-dev.cel:cel:0.8.0 [ERROR] +-com.google.protobuf:protobuf-java:3.25.5 (managed) <-- com.google.protobuf:protobuf-java:4.28.2 [ERROR] and [ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT [ERROR] +-dev.cel:cel:0.8.0 [ERROR] +-com.google.protobuf:protobuf-java-util:4.28.2 [ERROR] +-com.google.protobuf:protobuf-java:3.25.5 (managed) <-- com.google.protobuf:protobuf-java:4.28.2 [ERROR] ]

lqiu96 · 2024-11-14T22:49:34Z

To address Maven build issues, I've excluded findbugs and guava from the cel dependency.

Would you be able to share the build issues? I'm curious what the issues are cause off the top of my mind, I don't think we need to exclude them

huangjiahua · 2024-11-14T22:59:38Z

To address Maven build issues, I've excluded findbugs and guava from the cel dependency.

Would you be able to share the build issues? I'm curious what the issues are cause off the top of my mind, I don't think we need to exclude them

Sure, let me reproduce the errors.

If we don't exclude findbugs, we get:

[ERROR] Duplicate classes found:
[ERROR] 
[ERROR]   Found in:
[ERROR]     com.google.code.findbugs:jsr305:jar:3.0.2:compile
[ERROR]     com.google.code.findbugs:annotations:jar:3.0.1:compile
[ERROR]   Duplicate classes:
[ERROR]     javax/annotation/RegEx$Checker.class
[ERROR]     javax/annotation/MatchesPattern$Checker.class
[ERROR]     javax/annotation/meta/When.class
[ERROR]     javax/annotation/Nonnull$Checker.class
[ERROR]     javax/annotation/Nonnegative$Checker.class

If we don't exclude guava, we get:

[ERROR] Rule 2: org.apache.maven.enforcer.rules.dependency.RequireUpperBoundDeps failed with message:
[ERROR] Failed while enforcing RequireUpperBoundDeps. The error(s) are [
[ERROR] Require upper bound dependencies error for com.google.guava:guava:33.3.1-android paths to dependency are:
[ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT
[ERROR]   +-com.google.guava:guava:33.3.1-android
[ERROR] and
[ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT
[ERROR]   +-com.google.auth:google-auth-library-oauth2-http:1.29.1-SNAPSHOT
[ERROR]     +-com.google.guava:guava:33.3.1-android (managed) <-- com.google.guava:guava:33.3.1-android
[ERROR] and
[ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT
[ERROR]   +-com.google.http-client:google-http-client:1.45.0
[ERROR]     +-com.google.guava:guava:33.3.1-android (managed) <-- com.google.guava:guava:30.1.1-android
[ERROR] and
[ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT
[ERROR]   +-dev.cel:cel:0.8.0
[ERROR]     +-com.google.guava:guava:33.3.1-android (managed) <-- com.google.guava:guava:33.3.1-jre
[ERROR] and
[ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT
[ERROR]   +-com.google.http-client:google-http-client:1.45.0
[ERROR]     +-io.opencensus:opencensus-contrib-http-util:0.31.1
[ERROR]       +-com.google.guava:guava:33.3.1-android (managed) <-- com.google.guava:guava:29.0-android
[ERROR] and
[ERROR] +-com.google.auth:cab-token-generator:1.29.1-SNAPSHOT
[ERROR]   +-dev.cel:cel:0.8.0
[ERROR]     +-com.google.protobuf:protobuf-java-util:4.28.2
[ERROR]       +-com.google.guava:guava:33.3.1-android (managed) <-- com.google.guava:guava:32.0.1-jre
[ERROR] ]

lqiu96 · 2024-11-15T05:07:17Z

Took a look at the logs above. I'm not entirely sure why CEL is bringing in findbugs-annotations instead of findbug-jsr305. Either way, I think that should be fine to ignore since we don't depend on it in the auth library at all.

For Guava, I think the issue is due to the guava flavor. It looks to be treating guava-android and guava-jre as two different versions and can't tell that they're actually the same version. I think we are also fine to exclude this.

I'll do a bit more digging to confirm this. If the above is confirmed, a few small comments to explain the exclusion would be great

huangjiahua · 2024-11-19T00:53:30Z

I'll do a bit more digging to confirm this. If the above is confirmed, a few small comments to explain the exclusion would be great

I added a comment here. I also added the protobuf to the exclusion list because CEL is using 4.28.2 which will trigger RequireUpperBoundDeps error.

lqiu96 · 2024-11-22T19:01:20Z

pom.xml

@@ -77,6 +77,8 @@
    <project.findbugs.version>3.0.2</project.findbugs.version>
    <deploy.autorelease>false</deploy.autorelease>
    <project.error-prone.version>2.35.1</project.error-prone.version>
+    <project.protobuf.version>3.25.5</project.protobuf.version>


Thanks, I will need to double check this because I know CEL brings in Protobuf-Java 4.x.

I just need to confirm compatibility. I'll coordinate with CEL and confirm.

blakeli0 · 2024-11-22T19:54:58Z

necessary for serializing the Client-Side Access Boundary (CAB) token

Are we making a grpc call or rest call? If it is not a grpc call, I don't think we have to introduce the proto gen code into auth. Especially at the cost of maintaining a new protobuf-java dependency and gen code. We can create our own POJO object to serialize the JSON response. We may have to update the POJO when the proto changes, but since it does not change often, I think the risk is very low.

huangjiahua · 2024-11-25T20:28:43Z

Are we making a grpc call or rest call?

The proto here is not for grpc call. This proto is for serializing the CAB restriction for the CAB token generation, so protobuf is still required.

blakeli0 · 2024-12-02T21:35:41Z

Are we making a grpc call or rest call?

The proto here is not for grpc call. This proto is for serializing the CAB restriction for the CAB token generation, so protobuf is still required.

If that's the case, I don't think protobuf is required. We can use Gson for serialization as well.

huangjiahua · 2024-12-02T22:54:44Z

If that's the case, I don't think protobuf is required. We can use Gson for serialization as well.

@blakeli0 Does Gson support serializing to protobuf wire format? I couldn't find any documentation on this use case.

blakeli0 · 2024-12-02T22:57:53Z

If that's the case, I don't think protobuf is required. We can use Gson for serialization as well.

@blakeli0 Does Gson support serializing to protobuf wire format? I couldn't find any documentation on this use case.

I might be missing some context. If it is not used for making gRPC calls, why do we have to serialize it to protobuf wire format?

huangjiahua · 2024-12-02T23:04:16Z

If that's the case, I don't think protobuf is required. We can use Gson for serialization as well.

@blakeli0 Does Gson support serializing to protobuf wire format? I couldn't find any documentation on this use case.

I might be missing some context. If it is not used for making gRPC calls, why do we have to serialize it to protobuf wire format?

Yeah, I should've included the context: The client-side CAB token is designed to include an encrypted protobuf wired format. One of the reasons the token need to be a protobuf wired format is because it needs to carry a compiled CEL expression in AST format. The server will need to verify the token using the same protobuf. More on go/client-side-cab-design-doc.

blakeli0 · 2024-12-02T23:27:32Z

If that's the case, I don't think protobuf is required. We can use Gson for serialization as well.

@blakeli0 Does Gson support serializing to protobuf wire format? I couldn't find any documentation on this use case.

I might be missing some context. If it is not used for making gRPC calls, why do we have to serialize it to protobuf wire format?

Yeah, I should've included the context: The client-side CAB token is designed to include an encrypted protobuf wired format. One of the reasons the token need to be a protobuf wired format is because it needs to carry a compiled CEL expression in AST format. The server will need to verify the token using the same protobuf. More on go/client-side-cab-design-doc.

I see, that makes sense. It might be too late to change the design, why does it have to be protobuf wired format? Can JSON format suffice? I think all other types of tokens are sent as Strings directly through rest calls.

blakeli0

Discussed offline that this PR can be merged for now to unblock development. Before this feature goes GA/merges to main, we need to make sure we have a proper strategy of maintaining the protobuf gencode and protobuf runtime dependency in this repo.

…t-Side CAB feature. Change-Id: Ic7ef3cbd80b2ad778d61b9ccabf780561d3cc709

…ality (#1629) * feat: Implement ClientSideCredentialAccessBoundaryFactory (#1562) * feat: Implement ClientSideCredentialAccessBoundaryFactory.refreshCredentials() Set up the ClientSideCredentialAccessBoundaryFactory class and module. Implement the function to fetch and refresh intermediary tokens from STS. * feat: Add the generated ClientSideAccessBoundaryProto class for Client-Side CAB feature. (#1571) Change-Id: Ic7ef3cbd80b2ad778d61b9ccabf780561d3cc709 * feat: Implement refreshCredentialsIfRequired for intermediate token r… (#1583) * feat: Implement refreshCredentialsIfRequired for intermediate token refresh Implement `refreshCredentialsIfRequired`, called by `generateToken()`, to handle token refresh. It uses `refreshMargin` and `minimumTokenLifetime` to decide on synchronous or asynchronous refresh * Add unit tests for the builder and refreshCredentials() * Improve concurrency handling during credential refresh. Introduced a refresh task to manage concurrent refresh requests, preventing redundant attempts and potential race conditions. This aligns the refresh mechanism with the pattern used in OAuth2Credentials and ensures more robust credential management. * Update existing unit tests for compatibility and readability. * Add unit tests for refreshCredentialsIfRequired. * Fix a merge issue. * Temporary add sonatype-snapshots repository and cel version to fix the build error. * Remove duplicated code. * Fix lint issue. * Fix: Propagate credential refresh exceptions in blocking refresh. * Change cel version * Change cel version * Add jsr305 dependency * Fix Javadoc error * Minor code readability enhancements. * Revert "Fix Javadoc error" This reverts commit 2157fdb. * Address comments (add javadoc and use assertThrows in tests) * Run format script * feat: Implement Client-Side CAB token generation. (#1598) * feat: Implement Client-Side CAB token generation. Change-Id: I2c217656584cf5805297f02340cbbabca471f609 * Use IllegalStateException(String, Throwable) to capture upstream exception during Tink initialization Change-Id: I12af5b84eae4dcec5865adfdad1f9396d54c0200 * Rethrow exceptions from tink and CEL Change-Id: If8c94c786ee39201029d9c27856fd2eafb61e51c * Add tests for invalid keys from upstream, and rename test cases. Change-Id: Ib41cb81c779534fc6efd74d66bf4728efd743906 * Add additional throws comment for generatToken method. Change-Id: I9cfc589ade8a91040fc9c447740493fd49e392af * Refactor tests for better readability. Change-Id: Icfd0bc24c1694f220bcbffc6cde41462c59119c4 * Catch and rethrow the exception of session key not being base64 encoded. Change-Id: I5fa0c25fe020e9612735e4ac5df2b85a2a5aab11 * Format the code using mvn com.coveo:fmt-maven-plugin:format. Change-Id: I46572488dcd28de450a6b1b2f732bee5baa86910 * Fix a typo in the javadoc comment. Change-Id: Icef9ef5f7c3567224ec507303543b78e61f43ec1 * chore: Update version tag in cab-token-generator pom.xml This commit updates the version tag in the pom.xml file. * feat: Add integration test for the client side cab * Remove volatile keyword and use refreshLock when reading intermediateCredentials. * Define new default values for refreshMargin and minimumTokenLifetime. * Update version in pom.xml * Run formatter to resolve lint errors * add missing dependency * Swap the assertEquals parameters so the expected value is first. * Docs: Added javadocs Improvements: Cleaned up code, resolved readability enhancements --------- Co-authored-by: Jiahua Huang <[email protected]> Co-authored-by: aeitzman <[email protected]>

huangjiahua requested review from a team as code owners November 4, 2024 18:54

product-auto-label bot added the size: xl Pull request size is extra large. label Nov 4, 2024

nbayati mentioned this pull request Nov 13, 2024

feat: Implement ClientSideCredentialAccessBoundaryFactory #1562

Merged

huangjiahua force-pushed the main branch from 0084e22 to d3e680c Compare November 14, 2024 21:40

huangjiahua marked this pull request as draft November 14, 2024 21:41

huangjiahua force-pushed the main branch 3 times, most recently from a8a5a97 to cefb612 Compare November 14, 2024 22:28

huangjiahua marked this pull request as ready for review November 14, 2024 22:31

lsirac requested review from lqiu96 and aeitzman November 14, 2024 22:33

lqiu96 reviewed Nov 14, 2024

View reviewed changes

huangjiahua force-pushed the main branch from cefb612 to e46e1df Compare November 14, 2024 22:49

huangjiahua force-pushed the main branch 2 times, most recently from d1b7682 to 3ce542a Compare November 19, 2024 00:51

lqiu96 reviewed Nov 22, 2024

View reviewed changes

huangjiahua requested review from lqiu96 and blakeli0 November 26, 2024 18:28

blakeli0 approved these changes Dec 5, 2024

View reviewed changes

feat: Add the generated ClientSideAccessBoundaryProto class for Clien…

6342e3c

…t-Side CAB feature. Change-Id: Ic7ef3cbd80b2ad778d61b9ccabf780561d3cc709

huangjiahua force-pushed the main branch from 3ce542a to 6342e3c Compare December 5, 2024 23:56

lqiu96 merged commit 50d9027 into googleapis:client-side-cab Dec 6, 2024
10 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add the generated ClientSideAccessBoundaryProto class for the Client-Side CAB feature #1571

feat: Add the generated ClientSideAccessBoundaryProto class for the Client-Side CAB feature #1571

huangjiahua commented Nov 4, 2024 •

edited

Loading

huangjiahua commented Nov 7, 2024

huangjiahua commented Nov 14, 2024

lqiu96 Nov 14, 2024

huangjiahua Nov 14, 2024

aeitzman Nov 14, 2024

lqiu96 Nov 15, 2024

huangjiahua Nov 15, 2024

blakeli0 Nov 22, 2024 •

edited

Loading

sonarqubecloud bot commented Nov 14, 2024

lqiu96 Nov 14, 2024

huangjiahua Nov 14, 2024

huangjiahua Nov 14, 2024

lqiu96 commented Nov 14, 2024

huangjiahua commented Nov 14, 2024 •

edited

Loading

lqiu96 commented Nov 15, 2024

huangjiahua commented Nov 19, 2024

lqiu96 Nov 22, 2024

blakeli0 commented Nov 22, 2024 •

edited

Loading

huangjiahua commented Nov 25, 2024

blakeli0 commented Dec 2, 2024

huangjiahua commented Dec 2, 2024

blakeli0 commented Dec 2, 2024

huangjiahua commented Dec 2, 2024

blakeli0 commented Dec 2, 2024

blakeli0 left a comment

		// Generated by the protocol buffer compiler. DO NOT EDIT!
		// source: client_side_access_boundary.proto

feat: Add the generated ClientSideAccessBoundaryProto class for the Client-Side CAB feature #1571

feat: Add the generated ClientSideAccessBoundaryProto class for the Client-Side CAB feature #1571

Conversation

huangjiahua commented Nov 4, 2024 • edited Loading

huangjiahua commented Nov 7, 2024

huangjiahua commented Nov 14, 2024

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

blakeli0 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 14, 2024

Quality Gate failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 commented Nov 14, 2024

huangjiahua commented Nov 14, 2024 • edited Loading

lqiu96 commented Nov 15, 2024

huangjiahua commented Nov 19, 2024

Choose a reason for hiding this comment

blakeli0 commented Nov 22, 2024 • edited Loading

huangjiahua commented Nov 25, 2024

blakeli0 commented Dec 2, 2024

huangjiahua commented Dec 2, 2024

blakeli0 commented Dec 2, 2024

huangjiahua commented Dec 2, 2024

blakeli0 commented Dec 2, 2024

blakeli0 left a comment

Choose a reason for hiding this comment

huangjiahua commented Nov 4, 2024 •

edited

Loading

blakeli0 Nov 22, 2024 •

edited

Loading

huangjiahua commented Nov 14, 2024 •

edited

Loading

blakeli0 commented Nov 22, 2024 •

edited

Loading