Add AuthzClientCryptoProvider for authorization client cryptographic operations#3
Conversation
…tory closes #33831 Signed-off-by: mposolda <mposolda@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce cryptographic support into the authorization client by implementing a new CryptoProvider with ECDSA signature conversion utilities. The provider framework is extended with an ordering mechanism to support multiple providers coexisting. Existing crypto providers are updated to implement provider ordering, and the discovery logic is modified to select the highest-priority provider. Changes
Sequence DiagramssequenceDiagram
participant Client
participant AuthzClient
participant CryptoIntegration
participant ServiceLoader
participant AuthzClientCryptoProvider
participant DefaultCryptoProvider
Client->>AuthzClient: create(config)
AuthzClient->>CryptoIntegration: init()
CryptoIntegration->>ServiceLoader: load CryptoProvider instances
ServiceLoader->>AuthzClientCryptoProvider: discover
ServiceLoader->>DefaultCryptoProvider: discover
CryptoIntegration->>CryptoIntegration: sort by order() descending
Note over CryptoIntegration: AuthzClientCryptoProvider(100) < DefaultCryptoProvider(200)
CryptoIntegration->>CryptoIntegration: select DefaultCryptoProvider (highest order)
CryptoIntegration-->>AuthzClient: provider configured
AuthzClient-->>Client: ready
sequenceDiagram
participant Test
participant KeyPairGen
participant Signature
participant AuthzClientCryptoProvider
participant ASN1Decoder
participant ASN1Encoder
Test->>KeyPairGen: generate EC key pair
KeyPairGen-->>Test: ECPublicKey, ECPrivateKey
Test->>Signature: sign(data) with private key
Signature-->>Test: DER-encoded signature
Test->>AuthzClientCryptoProvider: asn1derToConcatenatedRS(derSig)
AuthzClientCryptoProvider->>ASN1Decoder: decode DER sequence
ASN1Decoder-->>AuthzClientCryptoProvider: R, S as BigInteger
AuthzClientCryptoProvider-->>Test: concatenated RS bytes
Test->>AuthzClientCryptoProvider: concatenatedRSToASN1DER(rsBytes)
AuthzClientCryptoProvider->>ASN1Encoder: encode R, S sequence
ASN1Encoder-->>AuthzClientCryptoProvider: DER bytes
AuthzClientCryptoProvider-->>Test: DER signature
Test->>Signature: verify(data, derSig)
Signature-->>Test: valid
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
🤖 Fix all issues with AI agents
In
@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java:
- Around line 93-125: In readTagNumber(int tag) the loop builds tagNo by ORing
and then left-shifting which can overflow an int for maliciously large high-tag
encodings; fix by adding overflow detection before each left shift (or use a
wider accumulator) and throw an IOException if the next shift/OR would exceed
int capacity. Concretely, in readTagNumber replace the blind "tagNo |= (b &
0x7f); tagNo <<= 7;" sequence with logic that either accumulates into a long and
validates it fits into int before assigning back to tagNo, or checks whether
(tagNo >>> (32 - 7)) != 0 (or equivalent) and throws "corrupted stream - tag
number too large" to prevent integer wraparound while parsing.
- Around line 49-63: readSequence currently assumes definite-length; call
readLength() and if it returns -1 (indefinite-length) then explicitly reject it
by throwing an IOException (e.g. "Indefinite-length SEQUENCE not supported") or,
if BER support is required, implement an indefinite-length branch that
repeatedly reads elements (using readNext()/readTag()/readLength() as needed)
until an EOC (tag 0 and length 0) is encountered; update the while loop in
readSequence to handle both the definite-length case (decrementing length by
bytes.length) and the indefinite-length case (loop until EOC), referencing the
readSequence, readLength, readNext and readTag methods to locate and change the
logic.
In
@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java:
- Around line 99-165: Validate inputs and remove dead code: in
concatenatedRSToASN1DER check that signature != null, signLength > 0, signLength
is even, and signature.length == signLength before any System.arraycopy; remove
the unused ASN1Encoder.create().write(...) calls and only build/write the DER
sequence once. In asn1derToConcatenatedRS validate the sequence size==2 (already
present) and ensure the decoded BigInteger byte lengths are not larger than len
in a way that would cause silent truncation — if a DER integer yields more than
len bytes and is not a single leading zero padding byte, throw an IOException.
Harden integerToBytes to validate qLength > 0 and when bytes.length > qLength
allow trimming only if bytes.length == qLength+1 and bytes[0] == 0 (strip the
leading zero), otherwise throw an IOException; keep the existing left-padding
behavior when bytes.length < qLength.
- Around line 60-72: getBouncyCastleProvider() currently returns the JDK
KeyStore provider (e.g., "SUN") instead of a BouncyCastle provider; update
AuthzClientCryptoProvider.getBouncyCastleProvider() so it returns a genuine
BouncyCastle Provider instance (e.g., obtain Security.getProvider("BC") and if
null instantiate org.bouncycastle.jce.provider.BouncyCastleProvider and register
it via Security.addProvider(...) before returning it) so callers like
BouncyIntegration.loadProvider() receive the correct provider; keep the existing
order() value.
In
@authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java:
- Around line 41-43: The test currently creates a generic EC key pair in the
ECDSAAlgorithmTest() constructor (keyPair =
KeyPairGenerator.getInstance("EC").genKeyPair()) which doesn’t guarantee the
curve required by ES256/ES384/ES512; update the tests to generate curve-specific
key pairs instead: create separate key pairs using
KeyPairGenerator.getInstance("EC") initialized with
ECGenParameterSpec("secp256r1"), ECGenParameterSpec("secp384r1"), and
ECGenParameterSpec("secp521r1") respectively (e.g., in per-test setup methods or
by parameterizing the tests) so each ECDSA algorithm uses the correct curve for
ES256, ES384, and ES512 while retaining the existing keyPair variable name or
providing distinct names tied to the algorithm under test.
🧹 Nitpick comments (4)
common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java (1)
64-70: Consider cleaning up the trailing comma in the log message.The current implementation appends
", "after each provider name, resulting in a trailing comma and space in the log output (e.g.,"Ignored crypto providers: Provider1, Provider2, ").♻️ Suggested cleanup for log message
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() + ", "); - } - logger.debugf(builder.toString()); + String ignoredProviders = foundProviders.stream() + .skip(1) + .map(p -> p.getClass().getName()) + .collect(Collectors.joining(", ")); + logger.debugf("Ignored crypto providers: %s", ignoredProviders); }authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java (1)
46-57: Consider adding signature verification to strengthen the test.The current test validates that the conversion round-trip (
DER -> RS -> DER -> RS) produces identical results, which tests the conversion logic but doesn't verify that the signatures themselves remain valid after conversion. Adding signature verification would provide stronger assurance that the conversions preserve cryptographic correctness.✅ Proposed enhancement to add signature verification
private void test(ECDSAAlgorithm algorithm, String curveName) throws Exception { + KeyPairGenerator keyGen = KeyPairGenerator.getInstance("EC"); + keyGen.initialize(new java.security.spec.ECGenParameterSpec(curveName)); + KeyPair keyPair = keyGen.genKeyPair(); + AuthzClientCryptoProvider prov = new AuthzClientCryptoProvider(); byte[] data = "Something to sign".getBytes(StandardCharsets.UTF_8); Signature signature = Signature.getInstance(JavaAlgorithm.getJavaAlgorithm(algorithm.name())); signature.initSign(keyPair.getPrivate()); signature.update(data); byte[] sign = signature.sign(); + + // Verify original signature + Signature verifier = Signature.getInstance(JavaAlgorithm.getJavaAlgorithm(algorithm.name())); + verifier.initVerify(keyPair.getPublic()); + verifier.update(data); + Assert.assertTrue("Original signature should be valid", verifier.verify(sign)); + byte[] rsConcat = prov.getEcdsaCryptoProvider().asn1derToConcatenatedRS(sign, algorithm.getSignatureLength()); byte[] asn1Des = prov.getEcdsaCryptoProvider().concatenatedRSToASN1DER(rsConcat, algorithm.getSignatureLength()); + + // Verify reconstructed DER signature + verifier.initVerify(keyPair.getPublic()); + verifier.update(data); + Assert.assertTrue("Reconstructed DER signature should be valid", verifier.verify(asn1Des)); + byte[] rsConcat2 = prov.getEcdsaCryptoProvider().asn1derToConcatenatedRS(asn1Des, algorithm.getSignatureLength()); Assert.assertArrayEquals(rsConcat, rsConcat2); }authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java (1)
30-99: Tighten API/clarity a bit (package-private class doesn’t needpublicmembers; length-byte write could be clearer).Since
ASN1Encoderis package-private,publicon its methods doesn’t expand access but does add noise; consider droppingpublickeywords. Also consider masking inwriteLength((length >>> i) & 0xFF) for readability.Proposed tweak
- static public ASN1Encoder create() { + static ASN1Encoder create() { return new ASN1Encoder(); } - public ASN1Encoder write(BigInteger value) throws IOException { + ASN1Encoder write(BigInteger value) throws IOException { writeEncoded(INTEGER, value.toByteArray()); return this; } - public ASN1Encoder writeDerSeq(ASN1Encoder... objects) throws IOException { + ASN1Encoder writeDerSeq(ASN1Encoder... objects) throws IOException { writeEncoded(CONSTRUCTED | SEQUENCE, concatenate(objects)); return this; } - for (int i = (size - 1) * 8; i >= 0; i -= 8) { - write((byte) (length >> i)); + for (int i = (size - 1) * 8; i >= 0; i -= 8) { + write((byte) ((length >>> i) & 0xFF)); }authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (1)
74-225: Consider improving error messages for unsupported methods or add defensive test coverage.The unsupported methods (
getCertificateUtils,getPemUtils,getSignature, etc.) are not called in authz-client code paths—onlygetEcdsaCryptoProvider()andgetKeyStore()are used. However, if future code paths or dependencies inadvertently invoke these, the generic exception message will be unhelpful for debugging. Consider either:
- Adding context-specific exception messages (e.g., "ECDSA signature conversions only; full crypto suite not available in authz-client")
- Adding a minimal integration test asserting the supported surface explicitly
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
authz/client/pom.xmlauthz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.javaauthz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.javaauthz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.javaauthz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.javaauthz/client/src/main/resources/META-INF/services/org.keycloak.common.crypto.CryptoProviderauthz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.javacommon/src/main/java/org/keycloak/common/crypto/CryptoIntegration.javacommon/src/main/java/org/keycloak/common/crypto/CryptoProvider.javacrypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.javacrypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.javacrypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java
🧰 Additional context used
🧬 Code graph analysis (3)
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (1)
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java (1)
ASN1Encoder(30-100)
crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java (2)
js/apps/admin-ui/src/authentication/execution-model.ts (1)
order(72-81)core/src/main/java/org/keycloak/crypto/JavaAlgorithm.java (1)
JavaAlgorithm(19-148)
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (2)
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java (1)
ASN1Encoder(30-100)authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (1)
ASN1Decoder(33-202)
🔇 Additional comments (10)
authz/client/src/main/resources/META-INF/services/org.keycloak.common.crypto.CryptoProvider (1)
20-20: LGTM! Service provider registration is correctly formatted.The registration of
AuthzClientCryptoProviderfollows the standard Java SPI pattern and will enable runtime discovery via ServiceLoader.crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java (1)
82-85: LGTM! Provider ordering correctly implemented.The
order()method returns 200, establishingDefaultCryptoProvideras a high-priority provider in the selection mechanism.crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java (1)
113-116: Verify the ordering strategy when multiple providers share the same priority.The
order()method returns 200, which is the same value asDefaultCryptoProviderand the mentionedWildFlyElytronProvider. When multiple providers with identical order values are present on the classpath, the selection becomes dependent onServiceLoaderiteration order, which is not guaranteed to be deterministic.This may be intentional if any of these providers is considered acceptable when multiple are available. However, if there's a preference between
FIPS1402ProviderandDefaultCryptoProvider, consider using distinct order values.common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java (1)
56-58: LGTM! Provider sorting correctly prioritizes higher order values.The descending sort ensures that providers with higher
order()values are evaluated first, implementing the documented precedence correctly.common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java (1)
39-44: All CryptoProvider implementations have been properly updated with theorder()method. Verification confirms:
- DefaultCryptoProvider:
order()returns 200- FIPS1402Provider:
order()returns 200- WildFlyElytronProvider:
order()returns 200- AuthzClientCryptoProvider:
order()returns 100authz/client/pom.xml (1)
62-71: LGTM!Test dependencies are correctly scoped and will inherit versions from the parent POM, which is the standard approach for Maven multi-module projects.
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (2)
127-167: Approve length validation with minor observation.The
readLength()method correctly:
- Handles short-form (< 128), long-form, and indefinite-length encoding
- Validates length is non-negative after reconstruction
- Checks length doesn't exceed the limit
- Restricts long-form to 4 bytes maximum
Note: The indefinite-length return value (-1) requires proper handling by callers, which should be verified.
169-182: LGTM!The
read(int length)method correctly handles partial reads from the underlying stream and provides clear error messages when EOF is encountered prematurely.authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java (1)
95-95: CryptoIntegration.init() is idempotent and safe to call multiple times.The method includes an explicit guard clause (
if (isInitialised()) return;) that checks if the provider has already been initialized. Subsequent calls after the first initialization return immediately without repeating provider discovery, preventing repeated initialization overhead or potential resource issues.crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.java (1)
75-78: Consider documenting the provider ordering strategy and mutual exclusivity of crypto implementations.All three main crypto providers (DefaultCryptoProvider, FIPS1402Provider, WildFlyElytronProvider) return
order() = 200, giving them equal precedence. While the code selects the first provider after sorting by order (reversed), when multiple providers have identical order values, the selection depends on ServiceLoader discovery order, which can vary based on classpath order.This is likely intentional since these providers are designed as mutually exclusive alternatives for different deployment scenarios (default, FIPS-compliant, and WildFly-based crypto implementations). However, if multiple crypto provider modules are accidentally included on the classpath, the selection would be non-deterministic. Consider documenting this design decision and the mutual exclusivity expectation, or adding a warning if multiple providers with equal order are detected at runtime (beyond the current debug logging).
| public List<byte[]> readSequence() throws IOException { | ||
| int tag = readTag(); | ||
| int tagNo = readTagNumber(tag); | ||
| if (tagNo != ASN1Encoder.SEQUENCE) { | ||
| throw new IOException("Invalid Sequence tag " + tagNo); | ||
| } | ||
| int length = readLength(); | ||
| List<byte[]> result = new ArrayList<>(); | ||
| while (length > 0) { | ||
| byte[] bytes = readNext(); | ||
| result.add(bytes); | ||
| length = length - bytes.length; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Handle indefinite-length SEQUENCE encoding.
The readSequence() method only handles definite-length encoding. The readLength() method can return -1 for indefinite-length encoding (when encountering 0x80), but the while (length > 0) loop at line 57 will not execute for indefinite-length sequences, resulting in an empty list being returned incorrectly.
DER encoding typically uses definite length, but if this decoder is intended to handle BER (which allows indefinite length), this is a correctness issue.
🔧 Proposed fix to handle indefinite length or reject it explicitly
Option 1: Reject indefinite-length encoding explicitly (if only DER is supported):
public List<byte[]> readSequence() throws IOException {
int tag = readTag();
int tagNo = readTagNumber(tag);
if (tagNo != ASN1Encoder.SEQUENCE) {
throw new IOException("Invalid Sequence tag " + tagNo);
}
int length = readLength();
+ if (length == -1) {
+ throw new IOException("Indefinite-length SEQUENCE encoding is not supported");
+ }
List<byte[]> result = new ArrayList<>();
while (length > 0) {
byte[] bytes = readNext();
result.add(bytes);
length = length - bytes.length;
}
return result;
}Option 2: Support indefinite-length encoding (if BER support is needed):
public List<byte[]> readSequence() throws IOException {
int tag = readTag();
int tagNo = readTagNumber(tag);
if (tagNo != ASN1Encoder.SEQUENCE) {
throw new IOException("Invalid Sequence tag " + tagNo);
}
int length = readLength();
List<byte[]> result = new ArrayList<>();
+ if (length == -1) {
+ // Indefinite-length: read until end-of-contents octets (0x00 0x00)
+ while (true) {
+ mark();
+ int nextTag = readTag();
+ reset();
+ if (nextTag == 0x00) {
+ read(); // consume first 0x00
+ if (read() != 0x00) {
+ throw new IOException("Invalid end-of-contents octets");
+ }
+ break;
+ }
+ result.add(readNext());
+ }
+ } else {
- while (length > 0) {
- byte[] bytes = readNext();
- result.add(bytes);
- length = length - bytes.length;
- }
+ while (length > 0) {
+ byte[] bytes = readNext();
+ result.add(bytes);
+ length = length - bytes.length;
+ }
+ }
return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<byte[]> readSequence() throws IOException { | |
| int tag = readTag(); | |
| int tagNo = readTagNumber(tag); | |
| if (tagNo != ASN1Encoder.SEQUENCE) { | |
| throw new IOException("Invalid Sequence tag " + tagNo); | |
| } | |
| int length = readLength(); | |
| List<byte[]> result = new ArrayList<>(); | |
| while (length > 0) { | |
| byte[] bytes = readNext(); | |
| result.add(bytes); | |
| length = length - bytes.length; | |
| } | |
| return result; | |
| } | |
| public List<byte[]> readSequence() throws IOException { | |
| int tag = readTag(); | |
| int tagNo = readTagNumber(tag); | |
| if (tagNo != ASN1Encoder.SEQUENCE) { | |
| throw new IOException("Invalid Sequence tag " + tagNo); | |
| } | |
| int length = readLength(); | |
| if (length == -1) { | |
| throw new IOException("Indefinite-length SEQUENCE encoding is not supported"); | |
| } | |
| List<byte[]> result = new ArrayList<>(); | |
| while (length > 0) { | |
| byte[] bytes = readNext(); | |
| result.add(bytes); | |
| length = length - bytes.length; | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In
@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java
around lines 49 - 63, readSequence currently assumes definite-length; call
readLength() and if it returns -1 (indefinite-length) then explicitly reject it
by throwing an IOException (e.g. "Indefinite-length SEQUENCE not supported") or,
if BER support is required, implement an indefinite-length branch that
repeatedly reads elements (using readNext()/readTag()/readLength() as needed)
until an EOC (tag 0 and length 0) is encountered; update the while loop in
readSequence to handle both the definite-length case (decrementing length by
bytes.length) and the indefinite-length case (loop until EOC), referencing the
readSequence, readLength, readNext and readTag methods to locate and change the
logic.
| int readTagNumber(int tag) throws IOException { | ||
| int tagNo = tag & 0x1f; | ||
|
|
||
| // | ||
| // with tagged object tag number is bottom 5 bits, or stored at the start of the content | ||
| // | ||
| if (tagNo == 0x1f) { | ||
| tagNo = 0; | ||
|
|
||
| int b = read(); | ||
|
|
||
| // X.690-0207 8.1.2.4.2 | ||
| // "c) bits 7 to 1 of the first subsequent octet shall not all be zero." | ||
| if ((b & 0x7f) == 0) // Note: -1 will pass | ||
| { | ||
| throw new IOException("corrupted stream - invalid high tag number found"); | ||
| } | ||
|
|
||
| while ((b >= 0) && ((b & 0x80) != 0)) { | ||
| tagNo |= (b & 0x7f); | ||
| tagNo <<= 7; | ||
| b = read(); | ||
| } | ||
|
|
||
| if (b < 0) { | ||
| throw new EOFException("EOF found inside tag value."); | ||
| } | ||
|
|
||
| tagNo |= (b & 0x7f); | ||
| } | ||
|
|
||
| return tagNo; | ||
| } |
There was a problem hiding this comment.
Potential integer overflow in high tag number calculation.
At lines 112-113, the tag number is constructed by repeatedly left-shifting and ORing values:
tagNo |= (b & 0x7f);
tagNo <<= 7;For extremely large tag numbers (which are theoretically possible in ASN.1), this could overflow the int type after approximately 4-5 octets. While such large tag numbers are rare in practice, this could be exploited with malicious input.
🛡️ Proposed fix to add overflow detection
int readTagNumber(int tag) throws IOException {
int tagNo = tag & 0x1f;
//
// with tagged object tag number is bottom 5 bits, or stored at the start of the content
//
if (tagNo == 0x1f) {
tagNo = 0;
int b = read();
// X.690-0207 8.1.2.4.2
// "c) bits 7 to 1 of the first subsequent octet shall not all be zero."
if ((b & 0x7f) == 0) // Note: -1 will pass
{
throw new IOException("corrupted stream - invalid high tag number found");
}
while ((b >= 0) && ((b & 0x80) != 0)) {
+ if (tagNo > (Integer.MAX_VALUE >>> 7)) {
+ throw new IOException("Tag number overflow");
+ }
tagNo |= (b & 0x7f);
tagNo <<= 7;
b = read();
}
if (b < 0) {
throw new EOFException("EOF found inside tag value.");
}
tagNo |= (b & 0x7f);
}
return tagNo;
}🤖 Prompt for AI Agents
In
@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java
around lines 93 - 125, In readTagNumber(int tag) the loop builds tagNo by ORing
and then left-shifting which can overflow an int for maliciously large high-tag
encodings; fix by adding overflow detection before each left shift (or use a
wider accumulator) and throw an IOException if the next shift/OR would exceed
int capacity. Concretely, in readTagNumber replace the blind "tagNo |= (b &
0x7f); tagNo <<= 7;" sequence with logic that either accumulates into a long and
validates it fits into int before assigning back to tagNo, or checks whether
(tagNo >>> (32 - 7)) != 0 (or equivalent) and throws "corrupted stream - tag
number too large" to prevent integer wraparound while parsing.
| @Override | ||
| public Provider getBouncyCastleProvider() { | ||
| try { | ||
| return KeyStore.getInstance(KeyStore.getDefaultType()).getProvider(); | ||
| } catch (KeyStoreException e) { | ||
| throw new IllegalStateException(e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public int order() { | ||
| return 100; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find CryptoProvider interface definition
echo "=== CryptoProvider interface definition ==="
find . -name "*.java" -type f | xargs grep -l "interface.*CryptoProvider\|class.*CryptoProvider" | head -5
# 2) Search for CryptoProvider interface
rg -n "interface\s+CryptoProvider" -A 20Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where getBouncyCastleProvider is called
echo "=== Calls to getBouncyCastleProvider() ==="
rg -n "getBouncyCastleProvider\s*\(" -C 3
# Find where CryptoProvider instances are used
echo "=== CryptoProvider usage ==="
rg -n "CryptoProvider" --max-count=20Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 19605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for ServiceLoader usage with CryptoProvider
echo "=== ServiceLoader + CryptoProvider ==="
rg -n "ServiceLoader.*CryptoProvider\|CryptoProvider.*load" -C 2
# Search for order() method calls
echo "=== order() method implementations ==="
rg -n "\.order\(\)" -C 2Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check AuthzClientCryptoProvider's order() method
echo "=== AuthzClientCryptoProvider.order() ==="
rg -n "class AuthzClientCryptoProvider" -A 50 authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java | grep -A 3 "order()"
# Check other providers' order() for comparison
echo -e "\n=== Order values for other CryptoProviders ==="
rg -n "order\(\)" -B 2 -A 1 crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java
rg -n "order\(\)" -B 2 -A 1 crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java
rg -n "order\(\)" -B 2 -A 1 crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.javaRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full BouncyIntegration context
echo "=== BouncyIntegration context ==="
cat -n common/src/main/java/org/keycloak/common/util/BouncyIntegration.java | head -50Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 1843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if authz-client is lightweight and doesn't use full CryptoIntegration
echo "=== AuthzClientCryptoProvider usage context ==="
grep -r "CryptoIntegration\|AuthzClientCryptoProvider" authz/client/src/ | grep -v ".class" | head -20Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the full AuthzClientCryptoProvider file to understand its design/purpose
echo "=== Full AuthzClientCryptoProvider context ==="
cat -n authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java | head -100Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 4185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what methods actually get called on the selected CryptoProvider in authz-client
echo "=== Methods called from CryptoIntegration.getProvider() in authz-client ==="
grep -n "CryptoIntegration\|getProvider()" authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java -A 3 -B 3Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at CryptoIntegration.detectProvider to see how it selects the provider when multiple exist
echo "=== CryptoIntegration provider selection logic ==="
cat -n common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java | sed -n '55,70p'Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 1249
getBouncyCastleProvider() returns the wrong provider type and violates the documented contract.
The method returns KeyStore.getInstance(KeyStore.getDefaultType()).getProvider(), which returns the JDK keystore provider (typically "SUN"), not BouncyCastle. The CryptoProvider interface contract explicitly documents this method should return "BouncyCastle security provider. Can be either non-FIPS or FIPS based provider."
While this provider has order()=100 (lower than other implementations at 200), meaning it won't be selected in typical deployments where real crypto providers are available, it should still be fixed. If AuthzClientCryptoProvider is the only provider on the classpath (e.g., in a thin client scenario), callers like BouncyIntegration.loadProvider() will receive the wrong provider name, potentially causing cryptographic operations to use incorrect providers.
🤖 Prompt for AI Agents
In
@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java
around lines 60 - 72, getBouncyCastleProvider() currently returns the JDK
KeyStore provider (e.g., "SUN") instead of a BouncyCastle provider; update
AuthzClientCryptoProvider.getBouncyCastleProvider() so it returns a genuine
BouncyCastle Provider instance (e.g., obtain Security.getProvider("BC") and if
null instantiate org.bouncycastle.jce.provider.BouncyCastleProvider and register
it via Security.addProvider(...) before returning it) so callers like
BouncyIntegration.loadProvider() receive the correct provider; keep the existing
order() value.
| @Override | ||
| public ECDSACryptoProvider getEcdsaCryptoProvider() { | ||
| return new ECDSACryptoProvider() { | ||
| @Override | ||
| public byte[] concatenatedRSToASN1DER(byte[] signature, int signLength) throws IOException { | ||
| int len = signLength / 2; | ||
| 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(); | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] asn1derToConcatenatedRS(byte[] derEncodedSignatureValue, int signLength) throws IOException { | ||
| int len = signLength / 2; | ||
|
|
||
| List<byte[]> seq = ASN1Decoder.create(derEncodedSignatureValue).readSequence(); | ||
| if (seq.size() != 2) { | ||
| throw new IOException("Invalid sequence with size different to 2"); | ||
| } | ||
|
|
||
| BigInteger rBigInteger = ASN1Decoder.create(seq.get(0)).readInteger(); | ||
| BigInteger sBigInteger = ASN1Decoder.create(seq.get(1)).readInteger(); | ||
|
|
||
| byte[] r = integerToBytes(rBigInteger, len); | ||
| byte[] s = integerToBytes(sBigInteger, len); | ||
|
|
||
| byte[] concatenatedSignatureValue = new byte[signLength]; | ||
| System.arraycopy(r, 0, concatenatedSignatureValue, 0, len); | ||
| System.arraycopy(s, 0, concatenatedSignatureValue, len, len); | ||
|
|
||
| return concatenatedSignatureValue; | ||
| } | ||
|
|
||
| @Override | ||
| public ECPublicKey getPublicFromPrivate(ECPrivateKey ecPrivateKey) { | ||
| throw new UnsupportedOperationException("Not supported yet."); | ||
| } | ||
|
|
||
| private byte[] integerToBytes(BigInteger s, int qLength) { | ||
| byte[] bytes = s.toByteArray(); | ||
| if (qLength < bytes.length) { | ||
| byte[] tmp = new byte[qLength]; | ||
| System.arraycopy(bytes, bytes.length - tmp.length, tmp, 0, tmp.length); | ||
| return tmp; | ||
| } else if (qLength > bytes.length) { | ||
| byte[] tmp = new byte[qLength]; | ||
| System.arraycopy(bytes, 0, tmp, tmp.length - bytes.length, bytes.length); | ||
| return tmp; | ||
| } | ||
| return bytes; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
ECDSA RS<->DER conversion needs input validation + remove dead code + avoid truncation.
concatenatedRSToASN1DER: no check thatsignature.length == signLength(can crash withSystem.arraycopy), andsignLengthodd/invalid isn’t rejected. Also Lines 114-115 are unused.asn1derToConcatenatedRS/integerToBytes: oversized/negative DER integers can get silently truncated tolenbytes, producing incorrect signatures without an explicit error.
Proposed hardening (minimal, keeps current structure)
@@
import java.security.interfaces.ECPrivateKey;
import java.security.interfaces.ECPublicKey;
import java.security.spec.ECParameterSpec;
import java.util.List;
+import java.util.Arrays;
import javax.crypto.Cipher;
@@
@Override
public byte[] concatenatedRSToASN1DER(byte[] signature, int signLength) throws IOException {
+ if (signature == null) {
+ throw new IOException("Signature must not be null");
+ }
+ if (signLength <= 0 || (signLength & 1) != 0) {
+ throw new IOException("Invalid signLength: " + signLength);
+ }
+ if (signature.length != signLength) {
+ throw new IOException("Invalid signature length " + signature.length + ", expected " + signLength);
+ }
int len = signLength / 2;
- 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);
+ BigInteger rBigInteger = new BigInteger(1, Arrays.copyOfRange(signature, 0, len));
+ BigInteger sBigInteger = new BigInteger(1, Arrays.copyOfRange(signature, len, signLength));
@@
public byte[] asn1derToConcatenatedRS(byte[] derEncodedSignatureValue, int signLength) throws IOException {
+ if (derEncodedSignatureValue == null) {
+ throw new IOException("DER signature must not be null");
+ }
+ if (signLength <= 0 || (signLength & 1) != 0) {
+ throw new IOException("Invalid signLength: " + signLength);
+ }
int len = signLength / 2;
@@
BigInteger rBigInteger = ASN1Decoder.create(seq.get(0)).readInteger();
BigInteger sBigInteger = ASN1Decoder.create(seq.get(1)).readInteger();
+ if (rBigInteger.signum() < 0 || sBigInteger.signum() < 0) {
+ throw new IOException("Invalid DER signature: negative integer");
+ }
@@
private byte[] integerToBytes(BigInteger s, int qLength) {
byte[] bytes = s.toByteArray();
+ // Accept one leading 0x00 sign byte, but reject true overflow beyond qLength.
+ if (bytes.length > qLength + 1 || (bytes.length == qLength + 1 && bytes[0] != 0)) {
+ throw new IllegalArgumentException("Integer does not fit expected length " + qLength);
+ }
if (qLength < bytes.length) {
byte[] tmp = new byte[qLength];
System.arraycopy(bytes, bytes.length - tmp.length, tmp, 0, tmp.length);
return tmp;
} else if (qLength > bytes.length) {
byte[] tmp = new byte[qLength];
System.arraycopy(bytes, 0, tmp, tmp.length - bytes.length, bytes.length);
return tmp;
}
return bytes;
}🤖 Prompt for AI Agents
In
@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java
around lines 99 - 165, Validate inputs and remove dead code: in
concatenatedRSToASN1DER check that signature != null, signLength > 0, signLength
is even, and signature.length == signLength before any System.arraycopy; remove
the unused ASN1Encoder.create().write(...) calls and only build/write the DER
sequence once. In asn1derToConcatenatedRS validate the sequence size==2 (already
present) and ensure the decoded BigInteger byte lengths are not larger than len
in a way that would cause silent truncation — if a DER integer yields more than
len bytes and is not a single leading zero padding byte, throw an IOException.
Harden integerToBytes to validate qLength > 0 and when bytes.length > qLength
allow trimming only if bytes.length == qLength+1 and bytes[0] == 0 (strip the
leading zero), otherwise throw an IOException; keep the existing left-padding
behavior when bytes.length < qLength.
| public ECDSAAlgorithmTest() throws Exception { | ||
| keyPair = KeyPairGenerator.getInstance("EC").genKeyPair(); | ||
| } |
There was a problem hiding this comment.
Specify EC curve to match ECDSA algorithm requirements.
The constructor generates a generic EC key pair without specifying the curve. Each ECDSA algorithm requires a specific curve:
- ES256 requires P-256 (secp256r1)
- ES384 requires P-384 (secp384r1)
- ES512 requires P-521 (secp521r1)
Using a single key pair with an unspecified curve for all three tests may cause failures depending on the JVM's default curve, and doesn't properly test the algorithm-specific behavior.
🔧 Proposed fix to generate curve-specific key pairs
-public class ECDSAAlgorithmTest {
-
- private final KeyPair keyPair;
-
- public ECDSAAlgorithmTest() throws Exception {
- keyPair = KeyPairGenerator.getInstance("EC").genKeyPair();
- }
-
-
- private void test(ECDSAAlgorithm algorithm) throws Exception {
+public class ECDSAAlgorithmTest {
+
+ private void test(ECDSAAlgorithm algorithm, String curveName) throws Exception {
+ KeyPairGenerator keyGen = KeyPairGenerator.getInstance("EC");
+ keyGen.initialize(new java.security.spec.ECGenParameterSpec(curveName));
+ KeyPair keyPair = keyGen.genKeyPair();
+
AuthzClientCryptoProvider prov = new AuthzClientCryptoProvider();
byte[] data = "Something to sign".getBytes(StandardCharsets.UTF_8);
Signature signature = Signature.getInstance(JavaAlgorithm.getJavaAlgorithm(algorithm.name()));
signature.initSign(keyPair.getPrivate());
signature.update(data);
byte[] sign = signature.sign();
byte[] rsConcat = prov.getEcdsaCryptoProvider().asn1derToConcatenatedRS(sign, algorithm.getSignatureLength());
byte[] asn1Des = prov.getEcdsaCryptoProvider().concatenatedRSToASN1DER(rsConcat, algorithm.getSignatureLength());
byte[] rsConcat2 = prov.getEcdsaCryptoProvider().asn1derToConcatenatedRS(asn1Des, algorithm.getSignatureLength());
Assert.assertArrayEquals(rsConcat, rsConcat2);
}
@Test
public void testES256() throws Exception {
- test(ECDSAAlgorithm.ES256);
+ test(ECDSAAlgorithm.ES256, "secp256r1");
}
@Test
public void testES384() throws Exception {
- test(ECDSAAlgorithm.ES384);
+ test(ECDSAAlgorithm.ES384, "secp384r1");
}
@Test
public void testES512() throws Exception {
- test(ECDSAAlgorithm.ES512);
+ test(ECDSAAlgorithm.ES512, "secp521r1");
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java
around lines 41 - 43, The test currently creates a generic EC key pair in the
ECDSAAlgorithmTest() constructor (keyPair =
KeyPairGenerator.getInstance("EC").genKeyPair()) which doesn’t guarantee the
curve required by ES256/ES384/ES512; update the tests to generate curve-specific
key pairs instead: create separate key pairs using
KeyPairGenerator.getInstance("EC") initialized with
ECGenParameterSpec("secp256r1"), ECGenParameterSpec("secp384r1"), and
ECGenParameterSpec("secp521r1") respectively (e.g., in per-test setup methods or
by parameterizing the tests) so each ECDSA algorithm uses the correct curve for
ES256, ES384, and ES512 while retaining the existing keyPair variable name or
providing distinct names tied to the algorithm under test.
2 participants
Test 3
Summary by CodeRabbit
Release Notes
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