feat: enable self-signed JWTs by default in ServiceOptions#13338
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for self-signed JWTs in the base ServiceOptions class, defaulting to true, and overrides it to false in BigQueryOptions. The reviewer raised two important points: first, defaulting to true in the base class could break other inheriting services that do not support self-signed JWTs, so defaulting to false and opting in is recommended. Second, to avoid unnecessary object creation, the code should check if JWT access is already enabled on the credentials before calling createWithUseJwtAccessWithScope.
| protected boolean useSelfSignedJwt() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Enabling self-signed JWTs by default in the base ServiceOptions class will affect all inheriting Google Cloud services (e.g., Storage, Pub/Sub, Datastore, Spanner, etc.). If any of these services do not support self-signed JWTs, they will break unless they explicitly override this method to return false.
A safer and more defensive approach is to default to false in ServiceOptions and let individual services that support and benefit from self-signed JWTs opt-in by overriding this method to return true. Alternatively, if this is intended to be opt-out, please ensure that all inheriting services in the repository have been verified and updated if they do not support self-signed JWTs.
| if (useSelfSignedJwt() && credentialsToReturn instanceof ServiceAccountCredentials) { | ||
| credentialsToReturn = | ||
| ((ServiceAccountCredentials) credentialsToReturn).createWithUseJwtAccessWithScope(true); | ||
| } |
There was a problem hiding this comment.
To avoid unnecessary object creation and copying, we can check if isUseJwtAccessWithScope() is already true before calling createWithUseJwtAccessWithScope(true).
if (useSelfSignedJwt() && credentialsToReturn instanceof ServiceAccountCredentials) {
ServiceAccountCredentials sac = (ServiceAccountCredentials) credentialsToReturn;
if (!sac.isUseJwtAccessWithScope()) {
credentialsToReturn = sac.createWithUseJwtAccessWithScope(true);
}
}References
- Reuse pre-configured objects from options classes directly instead of creating new ones and copying settings.
whowes
force-pushed
the
whowes/ssjwt
branch
5 times, most recently
from
9bfa1f9 to
40cb42c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration option useJwtAccessWithScope in ServiceOptions to control whether self-signed JWTs with scopes are used for service account credentials. This option defaults to true in ServiceOptions but is explicitly disabled by default in BigQueryOptions and StorageOptions. The review feedback correctly points out that the current implementation only propagates this setting to the credentials when it is true. To ensure that a false configuration is strictly honored and to prevent potential issues if credential defaults change, the reviewer suggests always propagating the value of getUseJwtAccessWithScope() to the credentials.
| if (getUseJwtAccessWithScope() && credentialsToReturn instanceof ServiceAccountCredentials) { | ||
| credentialsToReturn = | ||
| ((ServiceAccountCredentials) credentialsToReturn).createWithUseJwtAccessWithScope(true); | ||
| } |
There was a problem hiding this comment.
Currently, if getUseJwtAccessWithScope() is false, the code does not modify the credentials. However, if the passed-in ServiceAccountCredentials already has useJwtAccessWithScope set to true (either explicitly or if the default in google-auth-library changes to true in the future), it will not be overridden to false as configured in ServiceOptions (e.g., for BigQueryOptions or StorageOptions).
To ensure that the configuration in ServiceOptions is strictly honored and to prevent unexpected behavior if defaults change, we should always propagate the value of getUseJwtAccessWithScope() to the credentials. Since ServiceAccountCredentials.createWithUseJwtAccessWithScope(boolean) returns this if the value is already the same, there is no performance overhead or unnecessary object creation.
| if (getUseJwtAccessWithScope() && credentialsToReturn instanceof ServiceAccountCredentials) { | |
| credentialsToReturn = | |
| ((ServiceAccountCredentials) credentialsToReturn).createWithUseJwtAccessWithScope(true); | |
| } | |
| if (credentialsToReturn instanceof ServiceAccountCredentials) { | |
| credentialsToReturn = | |
| ((ServiceAccountCredentials) credentialsToReturn).createWithUseJwtAccessWithScope(getUseJwtAccessWithScope()); | |
| } |
|
|
|
|
||
| private Builder() {} | ||
| private Builder() { | ||
| setUseJwtAccessWithScope(false); |
There was a problem hiding this comment.
For BQ and Storage, are there any internal tracking tickets that we can link for these exceptions (e.g. default false)? IIRC, these are more so temporary and not permanent as we want this for default. I think would be helpful for future context
There was a problem hiding this comment.
Good suggestion, I added tracking tickets to the PR description.
2 participants
This should improve efficiency/reliability by avoiding OAuth token exchange - see https://google.aip.dev/auth/4111.
BigQueryOptions (b/523372553) and StorageOptions (b/523372960) are disabled by default for now.