Keycloak / PR #3

Add AuthzClientCryptoProvider for authorization client cryptographic operations

keycloak/keycloak#33832
Golden recall: 100% Extra findings: 7

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Returns wrong provider (default keystore instead of BouncyCastle) — getBouncyCastleProvider() returns the default KeyStore provider (typically SunJSSE) instead of BouncyCastle HIGH ✓ Found FR: "returns the default KeyStore provider (typically SunJSSE) instead of a BouncyCastle provider. This breaks expectations of callers." Same severity.
#2 Dead code in concatenatedRSToASN1DER: two ASN1Encoder.create().write() instances are created, their results written then immediately discarded — the return creates new instances LOW ✓ Found FR: "Two ASN1Encoder.create().write() calls discard their return values. Remove lines 113-114 to avoid wasted CPU." Upgraded to MEDIUM.

Supplementary Findings (10 findings)

Finding File Severity Legitimate?
Non-deterministic provider selection: all 3 providers return order=200, making sort useless — FIPS should be highest (order=300) DefaultCryptoProvider.java:82 HIGH ✓ Critical configuration bug
Indefinite-length encoding (0x80) in readLength() returns -1 without being handled by callers ASN1Decoder.java:103 HIGH ✓ DER mandates definite-length — must throw IOException
Bounds check uses limit (total buffer size) instead of remaining bytes, allowing out-of-bounds reads on malformed TLVs ASN1Decoder.java:136 HIGH ✓ Potential vulnerability
readSequence() does not validate all bytes are consumed after parsing inner elements ASN1Decoder.java:45 MEDIUM ✓ Missing validation
Test does not verify full DER→ConcatenatedRS→DER round-trip — missing assertArrayEquals(sign, asn1Des) ECDSAAlgorithmTest.java:56 MEDIUM ✓ Incomplete test
CryptoIntegration is now a direct import but keycloak-common is only a transitive dependency — fragile authz/client/pom.xml:23 MEDIUM ✓ Should be direct dependency
WildFlyElytronProvider uses PKCS5Padding for AES/CBC while other providers use PKCS7Padding — interoperability risk WildFlyElytronProvider.java:151 MEDIUM ✓ Inconsistency between providers
ASN1Decoder not thread-safe due to mutable count field ASN1Decoder.java:25 LOW ✓ Should document or refactor
Non-standard modifier order: static public instead of public static ASN1Encoder.java:37 LOW ✓ Java convention
Missing documentation on choice of AuthzClient.class.getClassLoader() vs Thread.currentThread().getContextClassLoader() AuthzClient.java:95 LOW ✓ Design decision undocumented

Honest Analysis

FriendlyReviewer detected both golden bugs. The wrong provider bug (#1) was correctly identified and given the same HIGH severity. The dead code in concatenatedRSToASN1DER (#2) was also found and upgraded from LOW to MEDIUM.

The 10 supplementary findings include 3 HIGH severity bugs: non-deterministic provider ordering (all providers claim order=200, making selection arbitrary), mishandled indefinite-length encoding in the ASN.1 decoder, and a bounds check vulnerability in the same decoder. These are genuine security and correctness bugs in code that the golden set did not cover.

The findings on incomplete test coverage, transitive dependency fragility, and padding inconsistency between providers show FriendlyReviewer evaluating the change holistically — not just the diff, but how it integrates with the existing provider ecosystem.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
4 high, 5 medium, 3 low

This MR introduces ECDSA signature support in the authz-client module with a custom ASN.1 DER encoder/decoder and a lightweight AuthzClientCryptoProvider. It also adds a provider ordering mechanism to CryptoProvider to enable deterministic selection among multiple implementations. While the overall direction is positive, several critical issues were identified: non-deterministic provider ordering due to identical order values across all implementations, a getBouncyCastleProvider() method that returns a non-BouncyCastle provider, dead code in concatenatedRSToASN1DER, and multiple bugs in the ASN.1 decoder (indefinite-length handling, weak bounds check). Additionally, test coverage is incomplete and several consistency concerns exist across provider implementations. The review recommends addressing these before merging.

These might need a close look

  • 🔴 crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java (L82)
    Non-deterministic provider selection: All three main providers (DefaultCryptoProvider, WildFlyElytronProvider, FIPS1402Provider) return order=200, making the sort ineffective. FIPS provider should have highest order (e.g., 300) to be preferred in FIPS environments.
  • 🔴 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (L68)
    getBouncyCastleProvider() returns the default KeyStore provider (typically SunJSSE) instead of a BouncyCastle provider. This breaks expectations of callers like BouncyIntegration.PROVIDER. Either rename or provide a genuine BC provider.
  • 🔴 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (L103)
    Indefinite-length encoding (0x80) is silently mishandled: readLength() returns -1, which callers do not handle gracefully. Since DER mandates definite-length, throw IOException for 0x80.
  • 🔴 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (L136)
    Bounds check uses limit (total buffer capacity) instead of limit - position (remaining readable bytes). A crafted TLV with a length value designed to exactly match the buffer limit will pass the check even if the remaining data is insufficient, leading to an out-of-bounds read. Use if (length < 0 || length > available()) throw new IOException("Invalid length").

Worth checking

  • 🟡 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (L45)
    readSequence() does not verify that all bytes are consumed after reading internal elements. If there are trailing bytes after the expected elements, they are silently ignored. This could mask malformed input.
  • 🟡 authz/client/src/test/java/org/keycloak/authorization/client/util/crypto/ECDSAAlgorithmTest.java (L56)
    The test verifies the DER→ConcatenatedRS direction but not the round-trip (DER→ConcatenatedRS→DER). Adding assertArrayEquals(sign, asn1Des) after re-encoding would validate round-trip correctness.
  • 🟡 authz/client/pom.xml (L23)
    CryptoIntegration is imported from keycloak-common, which is only a transitive dependency (included via keycloak-core). If the core module changes its dependency chain, this will break. Add keycloak-common as an explicit direct dependency.
  • 🟡 crypto/wildfly/src/main/java/org/keycloak/crypto/wildfly/WildFlyElytronProvider.java (L151)
    Uses "AES/CBC/PKCS5Padding" while DefaultCryptoProvider and FIPS1402Provider use "AES/CBC/PKCS7Padding". Although PKCS5 is effectively PKCS7 in Java, this inconsistency is confusing and risks interop issues with non-Java clients expecting the standard PKCS7 name.

Small things (take or leave)

  • 🔵 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (L25)
    ASN1Decoder has a mutable count field and is not thread-safe. Since it's instantiated per-use this is likely safe in practice, but should be documented or refactored to use local variables.
  • 🔵 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java (L37)
    Non-standard modifier order: static public instead of public static. Reorder for consistency.
  • 🔵 authz/client/src/main/java/org/keycloak/authorization/client/util/AuthzClient.java (L95)
    The classloader choice (AuthzClient.class.getClassLoader() vs Thread.currentThread().getContextClassLoader()) is a design decision that should be documented, especially for the crypto provider lookup logic.