fix(gdch): support EC private keys#1896
Conversation
|
Acceso
El martes, 24 de marzo de 2026, Lawrence ***@***.***>
escribió:
… ***@***.**** commented on this pull request.
------------------------------
In oauth2_http/java/com/google/auth/oauth2/GdchCredentials.java
<#1896 (comment)>
:
> private static final String PARSE_ERROR_PREFIX = "Error parsing token refresh response. ";
+ @VisibleForTesting static final String SUPPORTED_FORMAT_VERSION = "1";
+
+ private static final String SERVICE_ACCOUNT_TOKEN_TYPE =
any documentation regarding this specific Uniform Resource Name and the
k8s variant? Why does GDCH require this one?
—
Reply to this email directly, view it on GitHub
<#1896?email_source=notifications&email_token=B5MKPO3KWUKTX3EMNGA4SAT4SK7YTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBQGA4TIMRTGQ3KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4000942346>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B5MKPO4GWQZHDG3EL3IWC734SK7YTAVCNFSM6AAAAACWHGYLYWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMBQHE2DEMZUGY>
.
You are receiving this because you commented.Message ID:
***@***.***
com>
|
| private static final String SA_PRIVATE_KEY_PKCS8 = | ||
| ServiceAccountCredentialsTest.PRIVATE_KEY_PKCS8; | ||
| private static final String GDCH_SA_FORMAT_VERSION = GdchCredentials.SUPPORTED_FORMAT_VERSION; | ||
| private static final String GDCH_SA_FORMAT_VERSION = |
There was a problem hiding this comment.
can the tests just use GdchCredentials.SUPPORTED_JSON_FORMAT_VERSION constant directly?
There was a problem hiding this comment.
Good point, done.
| // SEQUENCE length doesn't match actual length | ||
| byte[] invalidDer = new byte[] {0x30, 0x05, 0x02, 0x01, 0x01, 0x02, 0x01, 0x02}; | ||
| GoogleAuthException e = | ||
| assertThrows( | ||
| GoogleAuthException.class, () -> GdchCredentials.transcodeDerToConcat(invalidDer, 64)); | ||
| assertEquals("Invalid DER signature length.", e.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| void transcodeDerToConcat_invalidRInteger() { | ||
| // Missing INTEGER for R | ||
| byte[] invalidDer = new byte[] {0x30, 0x06, 0x03, 0x01, 0x01, 0x02, 0x01, 0x02}; | ||
| GoogleAuthException e = | ||
| assertThrows( | ||
| GoogleAuthException.class, () -> GdchCredentials.transcodeDerToConcat(invalidDer, 64)); | ||
| assertEquals("Expected INTEGER for R.", e.getMessage()); | ||
| } | ||
|
|
||
| @Test | ||
| void transcodeDerToConcat_invalidSInteger() { | ||
| // Missing INTEGER for S |
There was a problem hiding this comment.
For these test cases, can the comment show where it's missing the INTEGER value?
// Missing INTEGER for R
Is it missing something from byte[] invalidDer = new byte[] {0x30, 0x06, 0x03, 0x01, 0x01, 0x02, 0x01, 0x02};?
There was a problem hiding this comment.
Added a more specific comment
oauth2_http/javatests/com/google/auth/oauth2/GoogleCredentialsTest.java
Outdated
Show resolved
Hide resolved
lqiu96
left a comment
There was a problem hiding this comment.
Changes LGTM. added a few comments.
| if (algorithm == Pkcs8Algorithm.EC) { | ||
| reader = new StringReader(privateKeyPkcs8); | ||
| section = PemReader.readFirstSectionAndClose(reader, "EC PRIVATE KEY"); | ||
| if (section != null) { | ||
| return privateKeyFromSec1(section.getBase64DecodedBytes()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is PKCS#8 or SEC1 two different algorithms/ flows? Also Is this logic only needed for GDCH?
If so, can this logic exists inside GDCH itself?
e.g.
- Check the first section and determine if we should call pckc8 or sec1 method
- call privateKeyFromPkcs8 or privateKeyFromSec1
I think it would better to separate these concerns since it looks like privateKeyFromPkcs8 is now also making a call to privateKeyFromSec1. I think we have added proper checks, but since this resides in OAuth2Utils I'm worried this behavior may have unknown impact.
There was a problem hiding this comment.
Done. I still kept the algorithm parameter in OAuth2Utils to parse EC keys. WDYT?
|
* fix: allow for ES algorithm in GdchCredentials * test: partially adapt tests * test: finish adjusting tests * chore: format * fix: restore credential name * docs: restore license * fix: restore removed code * test: increase coverage * test(gdch): parameterize test * test: remove unused var * chore: remove unused throw clause * test: parameterize more * fix: remove unused parameter * fix: make variables final as intended * fix: remove unused throw clause * fix: remove unused throw clause * fix: make OAuth2Credentials clock package private for production code * fix: use non deprecated base 64 encoder * test: parameterize flagged tests * chore: format * test: fix assertion * build: remove unused dependency * test: run linux gce only on linux envs * fix: sonarqube flags (use java.util.Base64) * fix: improve error message template * fix: keep overload of "with audience" that takes an URI * fix: restore public getter of getApiAudience * docs: add javadoc for signing logic * test: test private signature and decode methods * fix: add null and empty check for audience string * docs: add javadoc for audience getters * fix: use enum for possible algorithms * fix: use obsolete javadoc instead of @deprecated * refactor: use OAuth2Utils validate methods in GdchCredentials * fix: restore GoogleAuthException throwing in GdchCredentials * refactor: downgrade Pkcs8Algorithm and privateKeyFromPkcs8 to package-private * refactor: split parseBody into parseJson and parseQuery in test utilities * refactor: remove validation reflection by making signUsingEsSha256 package-private * test: use hardcoded string literal for gdch api audience in test * test: refactor to use assertThrows in GdchCredentialsTest and remove host OS check in DefaultCredentialsProviderTest * fix: add comment about EC algorithm support in GdchCredentials * fix: update GDCH audience error message to be more descriptive * refactor: rename getApiAudienceString to getGdchAudience * fix: Remove unused import * docs: update GDCH audience getter javadocs * test: add null-checks to builder and corresponding tests * refactor: consolidate token type constants using OAuth2Utils * refactor: throw GoogleAuthException for signing and transcoding errors * docs: add javadoc to related test utils * fix: use GoogleAuthException * test: use assertThrows where applicable * refactor: replace Preconditions with Strings.isNullOrEmpty for audience checks * fix: consistent exception message * chore: format * test: use lowercase os name * chore: address review comments for PR #1896 * chore: format * Finalizing GDCH credentials support by addressing reviewer comments * chore: format * fix: parse EC private keys with SEC1 algorithm * chore: format * fix: separate PKCs8 vs SEC1 logic in GdchCredentials * fix: improved exception message, added comments to extractPrivateKeyValue Original-PR: googleapis/google-auth-library-java#1896


Context: b/488439640
Implementation originally proposed in b/431924643#comment9
The primary objective is to enable support for Elliptic Curve (EC) keys and non-URI audience formats, aligning the Java SDK with the behavior of the Python and Go implementations. Additionally, the GDCH key creation tool creates EC keys only, meaning the GDCH implementation was not following the convention.
Key Changes
apiAudiencefield from aURIto aStringto accommodate "magic" non-URI strings (e.g., specific administrative audiences) required by certain GDCH services.privateKeyFromPkcs8to accept an algorithm parameter, allowing the library to parse EC keys instead of defaulting exclusively to RSA.Testing
GdchCredentialsTestto include test cases for EC key parsing and token signing.