Skip to content

Conversation

vverman
Copy link

@vverman vverman commented Oct 13, 2025

Documenting the custom credential supplier for authenticating:

  1. AWS Workloads.

  2. Okta Workloads.

@vverman vverman requested review from a team as code owners October 13, 2025 19:05
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Oct 13, 2025
@vverman vverman force-pushed the chore/document-custom-credential-suppliers branch from d6f790b to 49d17f5 Compare October 13, 2025 19:33
lqiu96
lqiu96 previously approved these changes Oct 13, 2025
lsirac
lsirac previously approved these changes Oct 20, 2025
// 2. GCP_WORKLOAD_AUDIENCE:
// The audience for the workload identity federation. This is the full resource name of the
// Workload Identity Pool Provider, in the following format:
// //iam.googleapis.com/projects/<project-number>/locations/global/workloadIdentityPools/<pool-id>/providers/<provider-id>
Copy link
Member

Choose a reason for hiding this comment

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

qq, in //iam.googleapis.com, is the // needed or a typo?

Copy link
Author

Choose a reason for hiding this comment

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

Nope that is how the aucience is structured -> Refer

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, thanks! Can you wrap it in `` or quotes so that it doesn't look like a typo?

public String getSubjectToken(ExternalAccountSupplierContext context) throws IOException {
// Check if the current token is still valid (with a 60-second buffer).
boolean isTokenValid =
this.accessToken != null && System.currentTimeMillis() < this.expiryTime - 60 * 1000;
Copy link
Member

Choose a reason for hiding this comment

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

expiryTime looks like it's set in fetchOktaAccessToken(), which is called later

Copy link
Author

@vverman vverman Oct 23, 2025

Choose a reason for hiding this comment

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

This is used to demonstrate caching and token expiry in case of multiple calls to the storage endpoints which isn't handled by the library in case of custom credential suppliers.

Hence I think we should keep this logic.

Copy link
Member

Choose a reason for hiding this comment

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

I think the logic is fine. From what I see, expiryTime is only set inside fetchOktaAccessToken(), so it may not be set on this first invocation (and has a default value of 0).

private final String clientId;
private final String clientSecret;
private String accessToken;
private long expiryTime;
Copy link
Member

@lqiu96 lqiu96 Oct 23, 2025

Choose a reason for hiding this comment

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

Instead of using a long, is it possible to use java.time.Instant instead?

If possible, I would prefer to use Instant.now() instead of System.currentTimeMillis()

public String getSubjectToken(ExternalAccountSupplierContext context) throws IOException {
// Check if the current token is still valid (with a 60-second buffer).
boolean isTokenValid =
this.accessToken != null && System.currentTimeMillis() < this.expiryTime - 60 * 1000;
Copy link
Member

Choose a reason for hiding this comment

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

Can you create a constant for 60 * 1000 (make it less of a magic number)?

Comment on lines +196 to +197
// Okta authorization server. Multiple scopes can be requested by space-separating them
// (e.g., "scope1 scope2").
Copy link
Member

Choose a reason for hiding this comment

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

Can the example showcase multiple scopes?

Would it be scope=gcp.test.read gcp.test.write or would the spcae need to be encoded?

*/
private void fetchOktaAccessToken() throws IOException {
URL url = new URL(this.oktaTokenUrl);
HttpURLConnection conn = (HttpURLConnection) url.openConnection();
Copy link
Member

Choose a reason for hiding this comment

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

Can you call disconnect() at the end to close the connection?

Comment on lines +67 to +76
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>auth</artifactId>
<version>2.20.27</version>
</dependency>
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>regions</artifactId>
<version>2.20.27</version>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we import their BOM? and have the bom manage these versions together?

I know above we seem to have a bom but manage some deps separately (I'm not too sure why that's the case).

Comment on lines +61 to +68
// To set up the Okta application for this flow:
// a. In your Okta developer console, create a new Application of type "Machine-to-Machine
// (M2M)".
// b. Under the "General" tab, ensure that "Client Credentials" is an allowed grant type.
// c. Note the "Client ID" and "Client Secret" for your application.
// d. Navigate to "Security" > "API" and select your authorization server (e.g., "default").
// e. Under the "Scopes" tab, add a custom scope (e.g., "gcp.test.read").
// f. Ensure your M2M application is granted this scope.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I am fine with this for the samples. I'm wondering if Okta has any public docs that also describes this. I'm just a bit worried that this flow or UI may change in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: l Pull request size is large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants