Keycloak PR #23 — Live Benchmark

Golden Comments (2/2 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	Returns wrong provider (default keystore instead of BouncyCastle)	HIGH	✓ Found	"`getBouncyCastleProvider()` returns the provider of `KeyStore.getDefaultType()` (typically `SunJSSE`) rather than BouncyCastle, violating the interface contract implied by the method name. Return `null` like `WildFlyElytronProvider` (or a real BC provider) to avoid latent failures if shared core code uses this value" (util/crypto/AuthzClientCryptoProvider.java L62)
#2	Dead code exists where ASN1Encoder instances are created and written to, but their results are immediately discarded. The actual encoding is performed by new ASN1Encoder instances created in the subsequent return statement, rendering the earlier operations useless.	LOW	✓ Found	"Dead code: `ASN1Encoder.create().write(rBigInteger)` and `ASN1Encoder.create().write(sBigInteger)` have no observable effect; the actual output comes from the subsequent `writeDerSeq(...).toByteArray()` call. Remove these two statements" (util/crypto/AuthzClientCryptoProvider.java L114)

Supplementary Findings (10 findings)

Finding	File	Severity	Legitimate?
Non-deterministic tie-breaking between equal-order providers — the stable sort falls back to `ServiceLoader` iteration order when multiple providers return the same order, and the previous guard that loudly rejected multiple providers was removed	CryptoIntegration.java:56	MEDIUM	✓ Real non-determinism with the loud guard gone; arbitrary pick silently masks a double-provider classpath
First-init-wins defeats the `order()` mechanism across classloader scopes — `init()` only detects a provider when `cryptoProvider == null` and permanently latches it, so a client-first init with a classloader that does not expose the server provider would latch the lower-order provider	CryptoIntegration.java:33	MEDIUM	✓ Real classloader limitation of the ranking mechanism
The new abstract `order()` method has no `default` — a source/binary-breaking change for any external `CryptoProvider` implementor, relevant since the change is intended to be synced to `keycloak-client`	CryptoProvider.java:42	MEDIUM	✓ Real API compatibility break for third-party implementors
Indefinite-length (0x80) DER is accepted and returns -1 instead of throwing an `IOException` — DER forbids indefinite-length, and malformed input can silently produce empty lists or negative array sizes	ASN1Decoder.java:135	MEDIUM	✓ Real laxness on malformed DER input
`AuthzClient.create()` now makes provider detection mandatory (throws `IllegalStateException` if none is discovered) and, due to once-only init, silently uses the server's provider inside a Keycloak server; it also uses the class's own classloader rather than the context classloader	AuthzClient.java:95	MEDIUM	✓ Real behavioral change with classloader implications; undocumented
Magic order value 200 hardcoded identically across `DefaultCryptoProvider`, `WildFlyElytronProvider` and `FIPS1402Provider` (100 in `AuthzClientCryptoProvider`) with no shared constant or explanation	DefaultCryptoProvider.java:83	LOW	✓ Borderline: magic value, functionally consistent, maintainability concern
The ignored-providers debug message is built unconditionally and malformed — always performs string concatenation, leaves a trailing `", "`, and is passed as a format pattern	CryptoIntegration.java:64	LOW	✓ Real minor log defect; guard with `isDebugEnabled()`
The "out of bounds" length guard compares against the total buffer size (`limit`), not the remaining stream length — harmless for ECDSA signatures but semantically imprecise	ASN1Decoder.java:162	LOW	✓ Borderline: harmless for current usage, imprecise guard
The new `org.hamcrest:hamcrest` test dependency is unused by `ECDSAAlgorithmTest` and may not be managed by any BOM — version resolution depends on `jboss-parent`	pom.xml:66	LOW	✓ Real unused dependency with unmanaged version; fragile build
The test proves only self-consistency of the encoder/decoder round-trip (a shared bug would not be caught) and exercises the provider directly rather than through `CryptoIntegration`/`ServiceLoader`, so the `order()`, service file and init wiring are never tested	ECDSAAlgorithmTest.java:52	LOW	✓ Real self-consistency-only test; the wiring that matters is never exercised

Honest Analysis

FriendlyReviewer found both golden bugs of this PR — recall 100%. G1 (the wrong provider) is identified verbatim: getBouncyCastleProvider() returns the provider of KeyStore.getDefaultType() — typically SunJSSE — and the review names the failure mode (latent failures if shared core code uses the value) and the suggested fix (return null like WildFlyElytronProvider). G2 is also found, as dead code in the "small things" bucket: the two ASN1Encoder.create().write(...) statements have no observable effect because the output comes from the later writeDerSeq(...) call.

The severity calibration deserves a question: the dataset rates G1 High, but the review places it in "Worth checking" (🟡) rather than "These might need a close look", and the checklist marks it non-blocking. A method named getBouncyCastleProvider() that returns SunJSSE is a contract violation at the exact seam where a crypto client could silently stop using BouncyCastle; the review's own framing ("latent failures") concedes the risk while ranking it below the tie-break and classloader items. That ranking is defensible — no current caller is broken — but it is one notch under the dataset's assessment.

The ten supplementary findings are the strongest extra set of the run, all real: the non-deterministic tie-break with the loud guard removed, the first-init-wins classloader latch that can defeat the ranking, the abstract order() breaking external implementors, the indefinite-length DER acceptance, and the mandatory detection in AuthzClient.create(). Even the lows (magic 200, malformed debug log, unmanaged hamcrest, self-consistency-only test) are legitimate rather than padding — the test-coverage item is arguably the most consequential of the lows, since a wiring test through CryptoIntegration would have caught G1's provider confusion in one run.

The main tension is the review's own verdict: it asserts "no blocking correctness bug in the happy path" and marks the feature "Looks good" while simultaneously flagging a wrong-provider return, a non-deterministic provider selection and an untested wiring path. For a compatibility-sensitive PR meant to be synced to keycloak-client, the caveats are closer to the surface than the summary suggests — but the review does state each of them explicitly, which is what makes the summary caveated rather than wrong.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

⛅ Tech	🌤️ Feat
6 medium, 6 low	1 ok, 1 mitigated

Core change is sound: adding order() to CryptoProvider and letting CryptoIntegration select the max-order provider is functionally correct, and the new client-only AuthzClientCryptoProvider (order 100) correctly ranks below the server providers (order 200). However, several robustness, compatibility, and maintainability concerns surfaced. The selection mechanism is non-deterministic when providers share the same order (all server providers return 200) and the previous guard that loudly rejected multiple providers was removed. The first-init-wins static latch means the order() ranking only considers providers visible to the first caller's classloader, which can defeat the mechanism across classloader boundaries. The new abstract order() method is a source/binary-breaking interface change for external CryptoProvider implementors if synced to keycloak-client; a default would preserve compatibility. AuthzClient.create() now mandates provider detection and uses the class's own classloader rather than the context classloader, and can silently fall back to the server's provider. In the ASN.1 code, dead statements remain, getBouncyCastleProvider() returns an unrelated default KeyStore provider, and indefinite-length DER is accepted instead of rejected. Tests prove only self-consistency and do not exercise the CryptoIntegration wiring. No blocking correctness bug in the happy path, but the issues should be addressed before merge.

Worth checking

🟡 common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java (L56)
Non-deterministic tie-breaking between equal-order providers. The stable sort falls back to ServiceLoader iteration order when multiple providers return the same order (all server providers return 200). The previous guard that threw on multiple providers was removed, so an accidental double-provider classpath is silently masked by an arbitrary pick. Resolve ties explicitly or document/assert a single highest-order provider.
🟡 common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java (L33)
First-init-wins defeats the order() mechanism across classloader scopes. init() only detects a provider when cryptoProvider == null and permanently latches it. If AuthzClient.create() initializes with a classloader that does not expose the server provider before the server's own init, the lower-order AuthzClientCryptoProvider (100) would be latched and the higher-order server provider (200) never selected. Document this limitation or re-evaluate when init() is called with a different classloader.
🟡 common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java (L42)
The new abstract order() method has no default, which is a source/binary-breaking change for any external/third-party CryptoProvider implementors — relevant since this change is intended to be synced to keycloak-client. Consider declaring default int order() { return 0; } to preserve compatibility. The Javadoc should also specify tie-break behavior.
🟡 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (L62)
getBouncyCastleProvider() returns the provider of KeyStore.getDefaultType() (typically SunJSSE) rather than BouncyCastle, violating the interface contract implied by the method name. Return null like WildFlyElytronProvider (or a real BC provider) to avoid latent failures if shared core code uses this value.
🟡 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (L135)
Indefinite-length (0x80) is accepted and returns -1 instead of throwing an IOException. DER forbids indefinite-length; current behavior can silently produce empty lists or negative array sizes on malformed input. Throw new IOException("indefinite length not allowed in DER") instead.
🟡 authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java (L95)
AuthzClient.create() now calls CryptoIntegration.init(AuthzClient.class.getClassLoader()), making provider detection mandatory (throws IllegalStateException if no provider is discovered) and, due to once-only init, silently uses the server's provider when running inside a Keycloak server. Using AuthzClient.class.getClassLoader() may also miss the application's context classloader; prefer Thread.currentThread().getContextClassLoader(). Consider documenting this behavioral change.

Small things (take or leave)

🔵 crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java (L83)
Magic order value 200 is hardcoded identically across DefaultCryptoProvider, WildFlyElytronProvider, and FIPS1402Provider (and 100 in AuthzClientCryptoProvider) with no shared constant or explanation. Add a shared constant on CryptoProvider or a Javadoc note explaining the ranking scheme. Functionally consistent, but a maintainability concern.
🔵 common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java (L64)
Minor: the ignored-providers debug message is built unconditionally and is malformed — it always performs string concatenation, leaves a trailing ", ", and is passed as a format pattern. Guard with logger.isDebugEnabled() and build a clean comma-separated list.
🔵 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (L114)
Dead code: ASN1Encoder.create().write(rBigInteger) and ASN1Encoder.create().write(sBigInteger) have no observable effect; the actual output comes from the subsequent writeDerSeq(...).toByteArray() call. Remove these two statements.
🔵 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (L162)
The "out of bounds" length guard compares against the total buffer size (limit), not the remaining stream length. Harmless for ECDSA signatures but semantically imprecise. Tighten to the remaining length and/or verify the sequence consumed all input to deterministically reject malformed input.
🔵 authz/client/pom.xml (L66)
The new org.hamcrest:hamcrest test dependency is unused by ECDSAAlgorithmTest and may not be managed by any BOM (version resolution depends on jboss-parent). Remove it or pin an explicit version to avoid a fragile/breaking build.
🔵 authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java (L52)
The test proves only self-consistency of the encoder/decoder round-trip; a shared bug would not be caught. Also, the provider is exercised directly rather than through CryptoIntegration/ServiceLoader, so the order(), service file, and AuthzClient.create init wiring are never tested. Add known-good reference assertions and wiring coverage.

✅ Feature-level checklist

Looks good

✅ Add AuthzClientCryptoProvider to authz-client (intended to be synced to keycloak-client later).
The provider is implemented with order 100, registered via META-INF/services, and integrated into AuthzClient.create(). Minor caveats (e.g. mandatory provider detection and the misleading getBouncyCastleProvider() implementation) are non-blocking and noted in technical findings.

Partially covered

⚠️ Add order() method to CryptoProvider to allow multiple CryptoProvider instances on classpath, with server providers outranking the client provider (approach 2).
The method is added and the ranking works in a typical server deployment, but tie-breaking between equal-order providers is non-deterministic, the first-init-wins latch limits the effect across classloaders, and the abstract method is a breaking change for external implementors. These caveats are captured in technical findings.

Add AuthzClientCryptoProvider for authorization client cryptographic operations