Add AuthzClientCryptoProvider for authorization client cryptographic operations #3
Conversation
…tory closes #33831 Signed-off-by: mposolda <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR introduces a new AuthzClientCryptoProvider to provide specialized cryptographic capabilities for Keycloak's authorization client module. The implementation focuses primarily on ECDSA signature format conversion between ASN.1 DER encoding and concatenated R|S format, which is essential for JWT signature processing in authorization flows.
Key Changes:
- New ASN.1 utilities: Added
ASN1EncoderandASN1Decoderclasses to handle ASN.1 DER encoding/decoding operations without external dependencies like BouncyCastle - AuthzClientCryptoProvider: A minimal
CryptoProviderimplementation that only supports ECDSA operations and basic keystore functionality, throwingUnsupportedOperationExceptionfor most other crypto operations - Provider ordering system: Enhanced the
CryptoProviderinterface with anorder()method to enable prioritized provider selection when multiple providers exist on the classpath - Integration updates: Modified
CryptoIntegrationto sort providers by order (descending) and select the highest priority provider, plus initialized crypto integration inAuthzClient.create() - Comprehensive testing: Added
ECDSAAlgorithmTestto validate signature format conversions for ES256, ES384, and ES512 algorithms
The new provider has an order value of 100, making it lower priority than existing providers (DefaultCryptoProvider, FIPS1402Provider, WildFlyElytronProvider all at 200). This architectural approach allows the authorization client to have lightweight crypto capabilities while ensuring full-featured providers take precedence in server environments. The implementation enables the authz-client to handle JWT signature format conversions independently without requiring heavy cryptographic dependencies.
Confidence score: 2/5
- This PR has several critical implementation issues that could cause runtime failures and security vulnerabilities
- The
getBouncyCastleProvider()method incorrectly returns a keystore provider instead of BouncyCastle, the ASN.1 parsing has potential buffer overflow vulnerabilities, and there are integer overflow risks in length calculations - Files needing attention:
AuthzClientCryptoProvider.java,ASN1Decoder.java,ASN1Encoder.java
12 files reviewed, 5 comments
| throw new EOFException("EOF found reading length"); | ||
| } | ||
|
|
||
| length = (length << 8) + next; |
There was a problem hiding this comment.
logic: bit shifting could cause integer overflow for large length values, consider using Math.addExact() or similar overflow checking
| * | ||
| * The higher number has preference over the lower number | ||
| */ | ||
| int order(); |
There was a problem hiding this comment.
logic: method lacks default implementation which will break existing CryptoProvider implementations that don't override this method
| int order(); | |
| default int order() { | |
| return 0; | |
| } |
| @Override | ||
| public Provider getBouncyCastleProvider() { | ||
| try { | ||
| return KeyStore.getInstance(KeyStore.getDefaultType()).getProvider(); | ||
| } catch (KeyStoreException e) { | ||
| throw new IllegalStateException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: incorrect provider returned - this returns the default keystore provider instead of BouncyCastle provider, which will break cryptographic operations expecting BC functionality
| ASN1Encoder.create().write(rBigInteger); | ||
| ASN1Encoder.create().write(sBigInteger); |
There was a problem hiding this comment.
style: these encoder calls create objects that are immediately discarded - remove these unused lines
| ASN1Encoder.create().write(rBigInteger); | |
| ASN1Encoder.create().write(sBigInteger); | |
| return ASN1Encoder.create() | |
| .writeDerSeq( | |
| ASN1Encoder.create().write(rBigInteger), | |
| ASN1Encoder.create().write(sBigInteger)) | |
| .toByteArray(); |
| if (foundProviders.size() > 1) { | ||
| StringBuilder builder = new StringBuilder("Ignored crypto providers: "); | ||
| for (int i = 1 ; i < foundProviders.size() ; i++) { | ||
| builder.append(foundProviders.get(i).getClass().getName() + ", "); |
There was a problem hiding this comment.
logic: Trailing comma will be added to the logged string for all ignored providers
2 participants
Test 3