feat: Implement Client-Side CAB token generation. by huangjiahua · Pull Request #1598 · googleapis/google-auth-library-java

huangjiahua · 2024-12-12T20:27:00Z

Implement Client-Side CAB token generation.

Temporarily added the staging repository for CEL with protobuf 3.25
Add a new dependency tink for encryption.
Will wait feat: Implement refreshCredentialsIfRequired for intermediate token r… #1583 to be merged first and resolve conflicts

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)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

nbayati · 2025-01-02T23:23:22Z

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

+    String intermediaryToken, sessionKey;
+    Date intermediaryTokenExpirationTime;
+
+    synchronized (this) {


Why do we need a synchronized block here?

Since the value is written in a synchronized block:

google-auth-library-java/cab-token-generator/java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

Lines 298 to 307 in 0d96dcf

synchronized (refreshLock) {

try {

this.intermediateCredentials = Futures.getDone(finishedTask);

} finally {

if (this.refreshTask != null && this.refreshTask.task == finishedTask) {

this.refreshTask = null;

}

}

}

}

.

I'll update my code to use the refreshLock and the IntermediateCredentials wrapper.

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

Change-Id: I2c217656584cf5805297f02340cbbabca471f609

lqiu96 · 2025-01-13T23:24:38Z

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

+    try {
+      AeadConfig.register();
+    } catch (GeneralSecurityException e) {
+      throw new IllegalStateException("Error occurred when registering Tink");


Can you throw with the exception as well IllegalStateException(String, Exception)?

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

…ption during Tink initialization Change-Id: I12af5b84eae4dcec5865adfdad1f9396d54c0200

nbayati

LGTM

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

lqiu96 · 2025-01-15T22:58:10Z

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

+      Aead aead =
+          keysetHandle.getPrimitive(RegistryConfiguration.get(), Aead.class);
+
+      return aead.encrypt(restriction, /*associatedData=*/new byte[0]);


nit: Can you note the significance of byte[0]?

Do you mean documenting why we need a byte[0] here?

Yeah, mostly because byte[0] seems random and not related to anything. Wondering why it's needed/ what it's doing.

I already added the comment.

lqiu96 · 2025-01-15T22:58:41Z

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

+
+      return aead.encrypt(restriction, /*associatedData=*/new byte[0]);
+    } catch (GeneralSecurityException exception) {
+      throw new InternalError("Failed to parse keyset: " + exception.getMessage());


qq, what's the benefit of re-throwing this as InternalError instead of keeping it as GeneralSecurityException?

Changed it to throw GeneralSecurityException

On second thought, do you know what the error message is from the GeneralSecurityException when it fails? If it's too vague (i'm assuming it may be because it says General), perhaps we can re-throw with the error message you wrote above. If we do re-throw, I'm not too certain about InternalError. I think a different exception that would highlight the error type would be best.

If the error message is relatively specific (i.e. not something like something wrong occurred) then I think I'm fine with it.

The message is "Parse keyset failed". I think it's specific.

Change-Id: If8c94c786ee39201029d9c27856fd2eafb61e51c

lqiu96 · 2025-01-15T23:16:44Z

.../com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java

+    byte[] rawKey = Base64.getDecoder().decode(
+        transportFactory.transport.getAccessBoundarySessionKey());
+
+    KeysetHandle keysetHandle = TinkProtoKeysetFormat.parseKeyset(
+        rawKey, InsecureSecretKeyAccess.get());
+
+    Aead aead =
+        keysetHandle.getPrimitive(RegistryConfiguration.get(), Aead.class);
+    byte[] rawRestrictions =
+        aead.decrypt(Base64.getUrlDecoder().decode(parts[1]), new byte[0]);
+    ClientSideAccessBoundary clientSideAccessBoundary =
+        ClientSideAccessBoundary.parseFrom(rawRestrictions);
+    assertEquals(clientSideAccessBoundary.getAccessBoundaryRulesCount(), 1);
+    ClientSideAccessBoundaryRule rule =
+        clientSideAccessBoundary.getAccessBoundaryRules(0);
+    assertEquals(rule.getAvailableResource(),
+                 "//storage.googleapis.com/projects/_/buckets/example-bucket");
+    assertEquals(rule.getAvailablePermissions(0),
+                 "inRole:roles/storage.objectViewer");
+    Expr expr = rule.getCompiledAvailabilityCondition();
+    assertEquals(expr.getCallExpr()
+                     .getTarget()
+                     .getSelectExpr()
+                     .getOperand()
+                     .getIdentExpr()
+                     .getName(),
+                 "resource");
+    assertEquals(expr.getCallExpr().getFunction(), "startsWith");
+    assertEquals(expr.getCallExpr().getArgs(0).getConstExpr().getStringValue(),
+                 "projects/_/buckets/example-bucket/objects/customer-a");


If possible, can we break testing two main logic components (serializeCredentialAccessBoundary and encryptRestrictions) into smaller unit tests? I think it may be easier to read, but let me know if you have any concerns with this.

i.e. Perhaps something like

serializeCredentialAccessBoundary_withAvailabilityRestrictions_success

serializeCredentialAccessBoundary_withoutAvailabilityRestrictions_success

serializeCredentialAccessBoundary_failure

Then also for encryptRestrictions

encryptRestrictions_success

encryptRestrictions_failure

I would prefer not exposing private methods but testing the public generateToken as a whole, but I see currently we don't have a test to cover encryption failures, so I'll add one with invalid keys from STS. I'll also rename the test cases.

You can make the methods package-private. It will still be accessible via the tests and hidden from the user. We can also annotate them with @VisibleForTesting

I considered using @VisibleForTesting here, but I'm hesitant to introduce it as I believe it can sometimes be a shortcut for testing things we should ideally test through public APIs. We can fully test the behavior we need using the generateToken method, even though the assertions are a bit longer. I don't think the added verbosity warrants using @VisibleForTesting in this case. Let me know if you see other issues or have a different perspective.

I'm not sure what you mean by shortcut for testing things and why they must be tested via the public APIs. Most of the core logic is inside the internal methods and they can be tested separately for the behavior that we expect. If it's easier to test from the public method and the individual methods require lots of workarounds and mocks, I understand and I think that would be fine (just document the rationale).

My concerns with these test stem around readability and maintenance. The success unit tests are 50+ lines long and is not easy to figure out what is going on. For example, generateToken() ends up encrypting all these values and the only way to confirm is to decrypt the values of the token. From what I see, it looks like the general flow/ logic is duplicated twice (one in library to encrypt and one in the unit test to decrypt). If we something like byte[0] in Aead changes, then we have to change it in multiple places.

Inside generateToken(), it also can spit out all these different success and failure scenarios (availabilitycondition exists or doesn't, Cel Exception, Aead Exception, etc). For example, it's not easy to trace what exactly causes a CelException if I have to go read through 2-3 nested methods worth of source (generateToken() -> serializeCredentialAccessBoundary() -> compileCel()) while keeping track of the state.

Perhaps we can keep the success tests from the public method and test the error cases from the individual methods? i.e. compileCel_throwsCelException(), etc

I understand your concerns. I'd like to address them by introducing helper functions in the test code to improve readability and adding comments to clarify the purpose of each test.

I'm still hesitant to add tests for private methods such as compileCel or encryptRestrictions because I believe it goes against the principle that:

If you change your software's internal implementation, your tests should not break as long as the change is not observable by users. Therefore, per the black-box testing principle, most of the time you should test your code through its public interfaces.

Testing through the public API allows us to validate the system's behavior as a user would experience it. For instance, we can ensure that an invalid CEL expression results in a CelValidationException for the user. Even if we tested private method failures directly, those tests wouldn't necessarily cover the public API, and we couldn't be certain the same errors would be exposed to the user.

If you think failure test cases for cel compilation and encryption are necessary. I can add tests for those methods but I would still keep the failure cases for the public method.

Refactored the tests as I mentioned above.

I'll leave this up to you since I believe test unit tests do a thorough enough test and I'm confident about the changes. I don't want to make this a blocker for this PR.

I don't think these unit tests don't seem to follow the ideas above besides just calling from the public method. If you're using the internal logic/ code (the black box) to validate the output, I'm not sure why it needs to be a done via black box testing to begin with. To me, it's opening the black box and making the validation reliant what's in the black box, which seems to ruin the whole idea of having the black box. For example If Cel now requires you to use byte[100] instead of byte[0] to encrypt and this also returns a different, valid access token, these unit tests would probably fail since it's decrypting with byte[0].

Logic within refreshCredentialsIfRequired() also is only run, but that isn't tested within this black-box testing principle. Instead we have dedicated unit tests for refreshCredentialsIfRequired. Had this been done via the blackbox testing principle, I would think we'd see a whole matrix of new test cases pop up:

generateToken_blocking_availabilityConditionsExist_success()

generateToken_blocking_availabilityConditionsExist_celExceptionError()
etc

Perhaps I don't understand how these tests follow black-box testing and I'll leave it as that.

Change-Id: Ib41cb81c779534fc6efd74d66bf4728efd743906

Change-Id: I9cfc589ade8a91040fc9c447740493fd49e392af

Change-Id: Icfd0bc24c1694f220bcbffc6cde41462c59119c4

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

lqiu96 · 2025-01-16T23:00:19Z

.../com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java

+    assertThrows(IllegalArgumentException.class,
+                 () -> { factory.generateToken(accessBoundary); });


for this one, can you document where IllegalArgumentException may stem from. This is a runtimeexception and isn't marked as thrown by generateToken() so it may be confusing for future devs to figure out where it's coming from.

This comes from the base64 decoding of the session key. Since the session key is from the server, I changed it to catch it and re-throw it as an IllegalStateExeption.

huangjiahua · 2025-01-16T23:56:49Z

~~@lqiu96 Looks like CLA check fails because it can't associate your email to Google~~ I see, it's the auto commit does not pick up the work email. Let me clean that.

Change-Id: I5fa0c25fe020e9612735e4ac5df2b85a2a5aab11

Change-Id: I46572488dcd28de450a6b1b2f732bee5baa86910

Change-Id: Icef9ef5f7c3567224ec507303543b78e61f43ec1

…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 <jh@jiahuah.com> Co-authored-by: aeitzman <12433791+aeitzman@users.noreply.github.com>

product-auto-label bot added the size: l Pull request size is large. label Dec 12, 2024

huangjiahua changed the title ~~Generate cab~~ feat: Implement Client-Side CAB token generation. Dec 12, 2024

huangjiahua force-pushed the generate-cab branch from 14715a8 to c9af3ff Compare December 12, 2024 20:32

huangjiahua marked this pull request as ready for review December 12, 2024 22:10

huangjiahua requested a review from a team December 12, 2024 22:10

huangjiahua requested a review from a team as a code owner December 12, 2024 22:10

nbayati reviewed Jan 2, 2025

View reviewed changes

huangjiahua marked this pull request as draft January 8, 2025 23:04

huangjiahua force-pushed the generate-cab branch 5 times, most recently from 2bd4316 to 855a2cc Compare January 9, 2025 00:57

huangjiahua marked this pull request as ready for review January 9, 2025 00:59

lsirac requested review from aeitzman and lqiu96 January 13, 2025 19:14

aeitzman reviewed Jan 13, 2025

View reviewed changes

feat: Implement Client-Side CAB token generation.

d39a16c

Change-Id: I2c217656584cf5805297f02340cbbabca471f609

huangjiahua force-pushed the generate-cab branch from 855a2cc to d39a16c Compare January 13, 2025 21:46

huangjiahua requested a review from aeitzman January 13, 2025 21:47

aeitzman approved these changes Jan 13, 2025

View reviewed changes

lqiu96 reviewed Jan 13, 2025

View reviewed changes

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java Show resolved Hide resolved

lqiu96 reviewed Jan 13, 2025

View reviewed changes

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java Show resolved Hide resolved

Use IllegalStateException(String, Throwable) to capture upstream exce…

752a9a9

…ption during Tink initialization Change-Id: I12af5b84eae4dcec5865adfdad1f9396d54c0200

nbayati approved these changes Jan 15, 2025

View reviewed changes

huangjiahua requested a review from lqiu96 January 15, 2025 19:14

lqiu96 reviewed Jan 15, 2025

View reviewed changes

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java Outdated Show resolved Hide resolved

lqiu96 reviewed Jan 15, 2025

View reviewed changes

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java Show resolved Hide resolved

lqiu96 reviewed Jan 15, 2025

View reviewed changes

Rethrow exceptions from tink and CEL

b31bb8c

Change-Id: If8c94c786ee39201029d9c27856fd2eafb61e51c

lqiu96 reviewed Jan 15, 2025

View reviewed changes

Add tests for invalid keys from upstream, and rename test cases.

2050f4b

Change-Id: Ib41cb81c779534fc6efd74d66bf4728efd743906

huangjiahua requested a review from lqiu96 January 15, 2025 23:49

huangjiahua added 2 commits January 16, 2025 19:26

Add additional throws comment for generatToken method.

a43dd7c

Change-Id: I9cfc589ade8a91040fc9c447740493fd49e392af

Refactor tests for better readability.

e548e4d

Change-Id: Icfd0bc24c1694f220bcbffc6cde41462c59119c4

lqiu96 reviewed Jan 16, 2025

View reviewed changes

...java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java Outdated Show resolved Hide resolved

lqiu96 reviewed Jan 16, 2025

View reviewed changes

lqiu96 approved these changes Jan 16, 2025

View reviewed changes

huangjiahua added 3 commits January 17, 2025 00:00

Catch and rethrow the exception of session key not being base64 encoded.

730d97e

Change-Id: I5fa0c25fe020e9612735e4ac5df2b85a2a5aab11

Format the code using mvn com.coveo:fmt-maven-plugin:format.

b1b4d42

Change-Id: I46572488dcd28de450a6b1b2f732bee5baa86910

Fix a typo in the javadoc comment.

08d2d97

Change-Id: Icef9ef5f7c3567224ec507303543b78e61f43ec1

huangjiahua force-pushed the generate-cab branch from d19a735 to 08d2d97 Compare January 17, 2025 00:01

nbayati merged commit 5573169 into googleapis:client-side-cab Jan 17, 2025
11 of 12 checks passed

	synchronized (refreshLock) {
	try {
	this.intermediateCredentials = Futures.getDone(finishedTask);
	} finally {
	if (this.refreshTask != null && this.refreshTask.task == finishedTask) {
	this.refreshTask = null;
	}
	}
	}
	}

		assertThrows(IllegalArgumentException.class,
		() -> { factory.generateToken(accessBoundary); });

Conversation

huangjiahua commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nbayati left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huangjiahua commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huangjiahua commented Dec 12, 2024 •

edited

Loading

lqiu96 Jan 16, 2025 •

edited

Loading

lqiu96 Jan 16, 2025 •

edited

Loading

huangjiahua commented Jan 16, 2025 •

edited

Loading