feat: Implement ClientSideCredentialAccessBoundaryFactory #1562

nbayati · 2024-10-30T22:39:07Z

See: go/client-side-cab-client-library-java
This PR creates the ClientSideCredentialAccessBoundaryFactory class, and implements the function to fetch the intermediary token and session key from the STS endpoint. It completes the item Fetching and parsing intermediate tokens in the execution plan. go/offline-cab-sdk-execution-plan

conventional-commit-lint-gcf · 2024-10-30T22:39:12Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

Set up the ClientSideCredentialAccessBoundaryFactory class and module. Implement the function to fetch intermediary tokens from STS.

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

…some minor refactoring.

cab-token-generator/pom.xml

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

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

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

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

cab-token-generator/pom.xml

lsirac · 2024-11-13T00:41:27Z

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


 /** Internal utilities for the com.google.auth.oauth2 namespace. */
-class OAuth2Utils {
+public class OAuth2Utils {


Not sure if it makes sense for this to be public @lqiu96

One option for this to see if we can keep this package-private and move some of these constants to STSRequestHandler. It looks like TOKEN_TYPE_ACCESS_BOUNDARY_INTERMEDIARY_TOKEN and TOKEN_EXCHANGE_URL_FORMAT are new additions could live in the individual classes (i.e StsREquestHandler or DownscopedCredentials).

TOKEN_TYPE_ACCESS_TOKEN is used in a few places, but if it makes sense we can move them around. I don't know if TOKEN_TYPE_ACCESS_TOKEN` applies to non-CAB use cases/ if we rather keep it in this Utils class.

Otherwise, I think I'm ok with making it public given the module constraints we have.

I'm going to leave this comment open and keep it as-is for now so I can merge this PR and unblock #1571. I'll address this comment in the next PR.

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

lqiu96 · 2024-11-13T15:22:42Z

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

+    try {
+      this.sourceCredential.refreshIfExpired();
+    } catch (IOException e) {
+      throw new IOException("Unable to refresh the provided source credential.", e);
+    }


Can you remind me why we are re-throwing this exception? If there is no reason, I think we should have refreshIfExpired() throw the exception instead of catching and then re-throw

From @aeitzman: It's just to give more easily readable details about what part of the auth flow broke vs just returning the stack trace. I don't think its strictly necessary but it makes the library easier to debug.

Ok, I'm fine with this for now, but I'd rather have this be captured via logs (that is probably a separate project).

CC: @zhumin8

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

lqiu96 · 2024-11-13T15:38:03Z

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

+    this.tokenExchangeEndpoint = builder.tokenExchangeEndpoint;
+  }
+
+  private void refreshCredentials() throws IOException {


Would you be able to write some quick comments that describes some of the behavior regarding the source access token and the response access token and how they form the intermediate access token?

From what I gather from the code, but it would be helpful for a comment to help confirm:
Intermediate access token is usually the STS response, unless the response doesn't have an expiration time. If not, then a new access token is created with the source access token's expiration time unless that value also doesn't exist.

What happens when both access tokens don't have an expiration time? Will this end up failing downstream? Will this just live forever?

Added documentation to the function. Please let me know if additional clarification is needed.

Thanks, that makes sense! Is there any issue if the intermediary token doesn't have an expiration time?

I believe the intermediary token should still work as expected without any issues even if there's no expiration time.

Sounds good, would it be possible confirm that? If does end up failing, could you document that or link to docs that detail that?

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

lqiu96 · 2024-11-13T16:00:02Z

cab-token-generator/pom.xml

+    <version>1.29.1-SNAPSHOT
+    </version><!-- {x-version-update:google-auth-library-parent:current} -->


Suggested change

<version>1.29.1-SNAPSHOT

</version>

<version>1.30.0</version>

This version is going to change as we cut new releases of auth. Last thing before we merge will be to update this to the latest version.

I'll leave this comment open as a reminder to update the version before the feature is merged to the main branch.

cab-token-generator/pom.xml

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

sonarqubecloud · 2024-11-14T19:41:55Z

Quality Gate failed

Failed conditions
18.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

lqiu96 · 2024-11-14T20:44:20Z

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

+
+      // Ensure source credential's universe domain matches.
+      try {
+        if (!universeDomain.equals(sourceCredential.getUniverseDomain())) {


qq, does CAB plan to restrict the sourceCredentials to a subset of GoogleCredentials (i.e. only Compute or SA)?

Or can any GoogleCredential be used?

Currently, client-side CAB only works with service account tokens. In the future we might add support for other types of tokens.

I think it would be helpful to add validation for that. One reason is that universe_domain retrieval should never throw an IOException for SACredentials. It was added for ComputeEngineCredentials because of MDS.

Also it could be helpful to get an error message earlier, rather than finding out when trying to refreshCredentials

lqiu96

LGTM. This PR's changes are going into the client-side-cab feature branch and not main. The unimplemented methods and additional changes will be addressed in future PRs.

…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]>

product-auto-label bot added the size: m Pull request size is medium. label Oct 30, 2024

feat: Implement ClientSideCredentialAccessBoundaryFactory

d9574da

Set up the ClientSideCredentialAccessBoundaryFactory class and module. Implement the function to fetch intermediary tokens from STS.

nbayati force-pushed the offline-cab branch from 147a542 to d9574da Compare October 31, 2024 17:13

nbayati changed the title ~~Setup client side CAB factory class and fetch intermediary token~~ feat: Implement ClientSideCredentialAccessBoundaryFactory Oct 31, 2024

nbayati marked this pull request as ready for review October 31, 2024 17:16

nbayati requested review from a team as code owners October 31, 2024 17:16

nbayati changed the base branch from main to client-side-cab October 31, 2024 18:39

lqiu96 requested review from lqiu96 and zhumin8 October 31, 2024 20:06

lqiu96 reviewed Nov 1, 2024

View reviewed changes

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

lqiu96 reviewed Nov 1, 2024

View reviewed changes

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

lqiu96 reviewed Nov 1, 2024

View reviewed changes

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

lqiu96 reviewed Nov 1, 2024

View reviewed changes

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

lqiu96 reviewed Nov 1, 2024

View reviewed changes

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

lqiu96 reviewed Nov 1, 2024

View reviewed changes

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

lqiu96 reviewed Nov 1, 2024

View reviewed changes

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

Move ClientSideCredentialAccessBoundaryFactory to its own module and …

6b04cc1

…some minor refactoring.

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Nov 6, 2024

nbayati requested a review from lqiu96 November 6, 2024 02:09

lqiu96 reviewed Nov 6, 2024

View reviewed changes

cab-token-generator/pom.xml Show resolved Hide resolved

lqiu96 reviewed Nov 6, 2024

View reviewed changes

cab-token-generator/pom.xml Show resolved Hide resolved

lqiu96 reviewed Nov 6, 2024

View reviewed changes

cab-token-generator/pom.xml Outdated Show resolved Hide resolved

lqiu96 reviewed Nov 6, 2024

View reviewed changes

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

lqiu96 reviewed Nov 6, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/CredentialAccessBoundary.java Outdated Show resolved Hide resolved

lqiu96 reviewed Nov 6, 2024

View reviewed changes

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

lqiu96 reviewed Nov 6, 2024

View reviewed changes

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

nbayati added 2 commits November 7, 2024 20:50

Add unit test for STS request/response.

c50a10f

Add comma to the copyright header

62b501c

aeitzman reviewed Nov 11, 2024

View reviewed changes

nbayati added 3 commits November 11, 2024 13:56

Rename variable

76d30b9

Add javadocs to classes and methods that were made public.

bd01ab3

Fix dependency and formatting issues.

a64cebd

aeitzman approved these changes Nov 12, 2024

View reviewed changes

Rename function to refreshIntermediateCredentials.

272db77

nbayati requested a review from lqiu96 November 12, 2024 22:47

lsirac reviewed Nov 13, 2024

View reviewed changes

Enhancements: Fix doc error, add method placeholder, rename function.

2056ca3

lqiu96 reviewed Nov 13, 2024

View reviewed changes

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

lqiu96 reviewed Nov 13, 2024

View reviewed changes

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

lqiu96 reviewed Nov 13, 2024

View reviewed changes

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

lqiu96 reviewed Nov 13, 2024

View reviewed changes

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

lqiu96 reviewed Nov 13, 2024

View reviewed changes

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

lqiu96 reviewed Nov 13, 2024

View reviewed changes

cab-token-generator/pom.xml Show resolved Hide resolved

lqiu96 reviewed Nov 13, 2024

View reviewed changes

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

lsirac approved these changes Nov 13, 2024

View reviewed changes

Add documentation, remove versions from pom.xml

5911813

nbayati requested a review from lqiu96 November 14, 2024 19:39

lqiu96 reviewed Nov 14, 2024

View reviewed changes

lqiu96 approved these changes Nov 14, 2024

View reviewed changes

nbayati merged commit 5d32642 into googleapis:client-side-cab Nov 14, 2024
13 of 14 checks passed

nbayati deleted the offline-cab branch January 22, 2025 00:04

		<version>1.29.1-SNAPSHOT
		</version><!-- {x-version-update:google-auth-library-parent:current} -->

	<version>1.29.1-SNAPSHOT
	</version><!-- {x-version-update:google-auth-library-parent:current} -->
	<version>1.30.0</version><!-- {x-version-update:google-auth-library-parent:current} -->

feat: Implement ClientSideCredentialAccessBoundaryFactory #1562

feat: Implement ClientSideCredentialAccessBoundaryFactory #1562

Uh oh!

Conversation

nbayati commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conventional-commit-lint-gcf bot commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 Nov 13, 2024 • 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!

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!

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!

lqiu96 Nov 14, 2024 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Nov 14, 2024

Quality Gate failed

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 left a comment

nbayati commented Oct 30, 2024 •

edited

Loading

conventional-commit-lint-gcf bot commented Oct 30, 2024 •

edited

Loading

lqiu96 Nov 13, 2024 •

edited

Loading

lqiu96 Nov 14, 2024 •

edited

Loading