Add AuthzClientCryptoProvider for authorization client cryptographic operations#8
Conversation
…tory closes #33831 Signed-off-by: mposolda <mposolda@gmail.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR adds ECDSA cryptography support to the authorization client by introducing ASN.1 encoding/decoding utilities, a new AuthzClientCryptoProvider implementation, and provider ordering capability. The CryptoProvider interface gains an order() method, implemented across existing and new providers, enabling prioritized provider selection when multiple implementations are available. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as AuthzClient
participant CryptoInt as CryptoIntegration
participant SM as ServiceLoader
participant DefProv as DefaultCryptoProvider
participant AuthzProv as AuthzClientCryptoProvider
Client->>CryptoInt: init()
CryptoInt->>SM: load all CryptoProvider impls
SM-->>CryptoInt: [AuthzClientCryptoProvider(order=100),<br/>DefaultCryptoProvider(order=200), ...]
rect rgb(200, 220, 240)
Note over CryptoInt: Sort by order() descending
CryptoInt->>CryptoInt: sort(providers, order DESC)
end
CryptoInt->>AuthzProv: getEcdsaCryptoProvider()
AuthzProv-->>CryptoInt: ECDSACryptoProvider instance
Note over CryptoInt: Log: "Detected: AuthzClientCryptoProvider"<br/>"Ignored: DefaultCryptoProvider, ..."
CryptoInt-->>Client: CryptoIntegration ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java (1)
6-6: Provider detection via ordered ServiceLoader is reasonable; consider clarifying tie casesSorting discovered
CryptoProviders byorder()(descending) and selecting the first one is a clean way to express precedence, and the debug log of ignored providers is useful when more than one is present.Two small points to consider:
- All built‑in providers currently return the same
order()(200), so in mixed environments the effective choice still depends onServiceLoaderiteration order. If you intend a deterministic preference (e.g., FIPS > Elytron > Default), you might want to give them distinct order values.- When multiple providers are present, logging their order alongside class names (and maybe flagging equal‑priority ties) would make misconfiguration easier to spot when reading logs.
These are behavioral/diagnostic refinements; the current logic is otherwise sound.
Also applies to: 56-58, 64-70
authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java (1)
41-72: ECDSA round‑trip tests look good; minor naming / assertion tweaks optionalThe round‑trip coverage for ES256/384/512 via
asn1derToConcatenatedRSandconcatenatedRSToASN1DERis solid and exercises the new provider correctly. Two small polish ideas:
- Rename
asn1Des(Line 54) toasn1Derfor clarity.- Optionally assert
rsConcat.length == algorithm.getSignatureLength()to catch mismatchedsignLength/signature sizes early.authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (1)
127-167: Indefinite‑length (0x80) handling is incomplete; better to fail fast
readLength()returns-1for 0x80 (indefinite length), butreadSequence()/readInteger()assume a non‑negative length and don’t special‑case-1. For DER, indefinite lengths are invalid anyway, so silently returning-1risks odd behavior if this decoder is ever reused beyond ECDSA signatures.Consider rejecting indefinite lengths explicitly, e.g.:
- if (length == 0x80) { - return -1; // indefinite-length encoding - } + if (length == 0x80) { + throw new IOException("Indefinite-length encoding not supported"); + }authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (1)
100-164: Harden ECDSA DER↔R||S helpers with length checks and avoid redundant encoder instancesThe ECDSA conversion logic is essentially correct, but a couple of improvements would make it more robust and cleaner:
concatenatedRSToASN1DERassumessignature.length == signLengthand will throwArrayIndexOutOfBoundsExceptionif misused. It’s safer to validate upfront and fail with a clear message.- Lines 114–115 create
ASN1Encoderinstances whose results are discarded;writeDerSeqthen creates yet more encoders. You can build ther/sencoders once and reuse them.integerToBytessilently truncates whenbytes.length > qLength. For stronger validation, you could check for unexpected oversize values (e.g., more thanqLength + 1bytes or a non‑zero leading byte) and throw anIOExceptioninstead of dropping high‑order bits.Example for the first two points:
@Override public byte[] concatenatedRSToASN1DER(byte[] signature, int signLength) throws IOException { int len = signLength / 2; + if (signature.length != signLength) { + throw new IOException("Expected signature of length " + signLength + + " but got " + signature.length); + } int arraySize = len + 1; byte[] r = new byte[arraySize]; byte[] s = new byte[arraySize]; System.arraycopy(signature, 0, r, 1, len); System.arraycopy(signature, len, s, 1, len); BigInteger rBigInteger = new BigInteger(r); BigInteger sBigInteger = new BigInteger(s); - ASN1Encoder.create().write(rBigInteger); - ASN1Encoder.create().write(sBigInteger); - - return ASN1Encoder.create() - .writeDerSeq( - ASN1Encoder.create().write(rBigInteger), - ASN1Encoder.create().write(sBigInteger)) - .toByteArray(); + ASN1Encoder rEnc = ASN1Encoder.create().write(rBigInteger); + ASN1Encoder sEnc = ASN1Encoder.create().write(sBigInteger); + return ASN1Encoder.create() + .writeDerSeq(rEnc, sEnc) + .toByteArray(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
authz/client/pom.xml(1 hunks)authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java(2 hunks)authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java(1 hunks)authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java(1 hunks)authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java(1 hunks)authz/client/src/main/resources/META-INF/services/org.keycloak.common.crypto.CryptoProvider(1 hunks)authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java(1 hunks)common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java(2 hunks)common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java(2 hunks)crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java(2 hunks)crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.java(1 hunks)crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java(3 hunks)
🔇 Additional comments (14)
authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java (1)
33-33: Wire-up of CryptoIntegration in AuthzClient factory looks appropriateImporting
CryptoIntegrationand callingCryptoIntegration.init(AuthzClient.class.getClassLoader())increate(Configuration)is a sensible place to bootstrap the crypto SPI: it keeps initialization lazy, idempotent, and uses the module’s classloader so theAuthzClientCryptoProviderservice entry is visible. Just be aware this makesAuthzClient.create(Configuration)dependent on aCryptoProviderbeing present on the classpath, which is fine for this tests-only module.Also applies to: 95-96
authz/client/pom.xml (1)
61-71: Test dependencies are scoped and aligned with usageAdding
junitandhamcrestwithtestscope is appropriate for the new test suite and keeps runtime dependencies unchanged. Relying on the parent/BOM for versions is consistent with the rest of the Keycloak build.If you haven't already, you may want to confirm via
mvn dependency:tree -Dscope=testthat there are no conflicting JUnit/Hamcrest versions pulled in transitively.crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.java (1)
75-78: order() implementation satisfies new SPI contractProviding
order()with value200correctly implements the newCryptoProvidercontract and keeps WildFlyElytronProvider aligned with the other core providers in the same priority tier.crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java (2)
82-85: DefaultCryptoProvider correctly participates in provider orderingThe new
order()implementation returning200cleanly satisfies the updatedCryptoProviderinterface and keeps DefaultCryptoProvider in the same priority class as the other main providers.
186-186: Whitespace-only changeThis line change appears to be formatting-only around
getSignatureand has no behavioral impact.common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java (1)
95-95: AES‑GCM method in interface matches provider implementationsThe
getAesGcmCipherdeclaration, including its checked exceptions, is consistent with the implementations in the default and FIPS providers and doesn’t introduce new concerns.crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java (5)
113-116: FIPS1402Provider correctly implements provider orderingThe
order()implementation returning200brings FIPS1402Provider into compliance with the updatedCryptoProviderSPI and keeps it in the same priority band as the other core providers.
199-203: Whitespace-only adjustment around AES‑GCM cipherThis hunk appears to be formatting-only around
getAesGcmCipherand does not affect behavior.
204-208: Whitespace-only adjustment around SecretKeyFactoryChange here is purely structural/whitespace around
getSecretKeyFactand has no runtime effect.
226-230: Whitespace-only adjustment around CertPathBuilderNon-functional change around
getCertPathBuilder; behavior remains the same.
231-235: Whitespace-only adjustment around getSignatureAgain, only formatting around
getSignature; no semantic changes.authz/client/src/main/resources/META-INF/services/org.keycloak.common.crypto.CryptoProvider (1)
1-20: Service descriptor correctly exposes AuthzClientCryptoProviderThe META‑INF/services entry is properly formatted (comments via
#, then the FQCN) and will allowServiceLoaderto discoverAuthzClientCryptoProviderwhenCryptoIntegrationis initialized with the authz client’s classloader.authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java (1)
30-100: ASN.1 DER encoder implementation looks correctTag constants, length encoding, and sequence construction are consistent with DER and with how the decoder and ECDSA provider use this class. No issues spotted.
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (1)
69-225: Correct the provider ordering logic and clarify whenAuthzClientCryptoProvideris actually selectedThe review comment misstates how provider ordering works. In
CryptoIntegration.detectProvider(), providers are sorted by.comparingInt(CryptoProvider::order).reversed()—meaning higherorder()values take priority. SinceAuthzClientCryptoProviderreturns 100 whileDefaultCryptoProvider,FIPS1402Provider, andWildFlyElytronProvidereach return 200, the latter three will win when multiple providers are on the classpath.However, the underlying risk is real for standalone authz-client usage: when this library is used in isolation (without the full Keycloak server),
AuthzClientCryptoProvidermay be the only registered provider and will be selected. Any code then calling unsupported methods likegetKeyPairGen(),getKeyFactory(),getSignature(), etc., will fail withUnsupportedOperationException.The registered
META-INF/services/org.keycloak.common.crypto.CryptoProviderentry confirms the provider is discoverable. To mitigate:
- Raise
order()above 200, or- Implement the commonly-used methods (key factories, signatures, ciphers), or
- Add guards to prevent selection when insufficient functionality is available.
Likely an incorrect or invalid review comment.
| /** | ||
| * Order of this provider. This allows to specify which CryptoProvider will have preference in case that more of them are on the classpath. | ||
| * | ||
| * The higher number has preference over the lower number | ||
| */ | ||
| int order(); | ||
|
|
There was a problem hiding this comment.
New order() method breaks SPI binary compatibility for existing providers
Defining int order(); as a new abstract method on CryptoProvider means any third-party implementations compiled against the previous version will not implement it and will hit AbstractMethodError once CryptoIntegration invokes order() during provider detection.
To preserve SPI compatibility while still enabling ordering, consider making this a default method:
default int order() {
// Baseline for older providers; newer ones can override with >0
return 0;
}This keeps existing implementations working, while letting built‑in providers (Default, Elytron, FIPS, Authz client) override with their desired priority.
🤖 Prompt for AI Agents
In common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java around
lines 39 to 45, the new abstract int order() method breaks SPI binary
compatibility for existing third‑party providers; change it to a default method
that returns a baseline value (e.g. 0) so older implementations remain
compatible while allowing newer providers to override with higher priorities,
and update the Javadoc to document that providers may override order() to
influence preference.
2 participants
Test 3
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#3