Add AuthzClientCryptoProvider for authorization client cryptographic operations#3
Conversation
…tory closes #33831 Signed-off-by: mposolda <mposolda@gmail.com>
WalkthroughThis change introduces a new crypto provider for the authorization client, including ASN.1 encoding/decoding utilities and ECDSA signature format conversion. It updates the crypto provider discovery logic to prioritize providers using a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as AuthzClient.create()
participant Crypto as CryptoIntegration
participant Provider as AuthzClientCryptoProvider
Client->>Crypto: init()
Crypto->>Crypto: detectProvider(classLoader)
Crypto->>Provider: (if selected) initialize provider
Crypto-->>Client: (provider ready)
Client-->>Client: return new AuthzClient instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java (1)
64-70: Minor improvement: Remove trailing comma in debug message.The current implementation will produce a trailing comma and space in the debug 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()); - } + if (foundProviders.size() > 1) { + 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 (2)
41-43: Consider specifying EC curve parameters explicitly.The current implementation uses default EC parameters. For more predictable behavior across different JVM implementations, consider specifying the curve explicitly.
public ECDSAAlgorithmTest() throws Exception { - keyPair = KeyPairGenerator.getInstance("EC").genKeyPair(); + KeyPairGenerator keyGen = KeyPairGenerator.getInstance("EC"); + keyGen.initialize(new java.security.spec.ECGenParameterSpec("secp256r1")); + keyPair = keyGen.generateKeyPair(); }
46-57: Consider adding signature verification to the test.While the test verifies format conversion correctness, it doesn't verify that the converted signatures remain valid.
private void test(ECDSAAlgorithm algorithm) throws Exception { 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); + + // Verify the converted signature is still valid + signature.initVerify(keyPair.getPublic()); + signature.update(data); + Assert.assertTrue("Converted signature should be valid", signature.verify(asn1Des)); }authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java (2)
66-83: DER length encoding implementation is correct but could be optimized.The implementation correctly handles both short form (≤127) and long form (>127) length encoding according to DER rules. However, there's potential for optimization in the bit shifting logic.
Consider this more readable implementation:
void writeLength(int length) throws IOException { if (length > 127) { - int size = 1; - int val = length; - - while ((val >>>= 8) != 0) { - size++; - } + int size = (Integer.SIZE - Integer.numberOfLeadingZeros(length) + 7) / 8; write((byte) (size | 0x80)); for (int i = (size - 1) * 8; i >= 0; i -= 8) { write((byte) (length >> i)); } } else { write((byte) length); } }
93-99: Object concatenation method is inefficient for large sequences.The method creates a new ByteArrayOutputStream and copies data from multiple encoders, which could be memory-intensive for large sequences.
Consider pre-calculating total size to avoid internal array resizing:
byte[] concatenate(ASN1Encoder... objects) throws IOException { + int totalSize = 0; + for (ASN1Encoder object : objects) { + totalSize += object.toByteArray().length; + } - ByteArrayOutputStream tmp = new ByteArrayOutputStream(); + ByteArrayOutputStream tmp = new ByteArrayOutputStream(totalSize); for (ASN1Encoder object : objects) { tmp.write(object.toByteArray()); } return tmp.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)
common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java (1)
39-44: Verified CryptoProvider interface implementations updatedAll direct implementers of
CryptoProvidernow include the neworder()method, so the interface change is fully addressed:
- crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.java
- crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java
- crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java
- authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java
authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java (1)
95-95: LGTM - Proper crypto integration initialization.The addition of
CryptoIntegration.init()ensures that the newAuthzClientCryptoProviderand related cryptographic utilities are properly loaded when creating AuthzClient instances. Using the AuthzClient class loader is appropriate for this context.authz/client/pom.xml (1)
62-71: LGTM - Standard test dependencies added.The addition of JUnit and Hamcrest dependencies with test scope is appropriate for supporting the new ECDSA algorithm tests. These are standard, well-maintained testing libraries.
crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.java (1)
75-78: LGTM - Consistent priority implementation.The
order()method implementation returning 200 is consistent with other crypto providers and gives this provider appropriate high priority in the selection mechanism.crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java (1)
113-116: LGTM - Consistent FIPS provider priority implementation.The
order()method implementation returning 200 maintains consistency with other crypto providers and ensures the FIPS provider has appropriate high priority in the selection mechanism.crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java (1)
82-85: LGTM!The implementation of the
order()method is correct and aligns with the new priority mechanism for crypto providers.authz/client/src/main/resources/META-INF/services/org.keycloak.common.crypto.CryptoProvider (1)
1-21: Service provider registration looks good.The file correctly registers
AuthzClientCryptoProvideras aCryptoProviderimplementation following the Java SPI pattern.authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Encoder.java (4)
32-34: ASN.1 tag constants are correct.The ASN.1 tag values are properly defined according to ITU-T X.690 standard: INTEGER (0x02), SEQUENCE (0x10), and CONSTRUCTED (0x20).
38-44: Factory method pattern implementation is sound.The private constructor with public factory method provides good encapsulation and clear instantiation semantics.
46-49: BigInteger encoding correctly delegates to writeEncoded.The method properly encodes BigInteger values as ASN.1 INTEGER using
toByteArray()which handles two's complement representation correctly.
51-54: SEQUENCE encoding implementation is correct.The method properly constructs ASN.1 SEQUENCE with CONSTRUCTED bit set and concatenates nested objects correctly.
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (3)
125-144: ASN.1 DER to concatenated RS conversion looks correct.The method properly parses the ASN.1 sequence, extracts the two BigInteger values, and converts them to fixed-length byte arrays. The error handling for invalid sequence size is appropriate.
198-200: KeyStore implementation is correct.The method properly delegates to the standard KeyStore.getInstance() with the format name, which is the expected behavior for this minimal crypto provider.
104-112: Verify ECDSA DER Conversion Against Test Vectors
I wasn’t able to find existing unit tests invokingconcatenatedRSToASN1DERto validate how the leading-zero byte andBigIntegersign handling behave across different R/S MSB values. Please confirm or add tests using known ECDSA signature vectors (covering both MSB=0 and MSB=1 cases) to ensure correct DER encoding.• File: authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java
Lines: 104–112
| 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.
Validate remaining length in readSequence to prevent negative values.
The method subtracts bytes.length from length without checking if it would go negative, which could happen with corrupted data.
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();
+ if (bytes.length > length) {
+ throw new IOException("Sequence element length exceeds remaining sequence length");
+ }
result.add(bytes);
length = length - bytes.length;
}
+ if (length != 0) {
+ throw new IOException("Sequence length mismatch: " + length + " bytes remaining");
+ }
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(); | |
| List<byte[]> result = new ArrayList<>(); | |
| while (length > 0) { | |
| byte[] bytes = readNext(); | |
| if (bytes.length > length) { | |
| throw new IOException("Sequence element length exceeds remaining sequence length"); | |
| } | |
| result.add(bytes); | |
| length = length - bytes.length; | |
| } | |
| if (length != 0) { | |
| throw new IOException("Sequence length mismatch: " + length + " bytes remaining"); | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java
between lines 49 and 63, the readSequence method subtracts bytes.length from
length without validating if length remains non-negative, risking negative
values with corrupted data. Add a check after subtracting bytes.length to ensure
length does not become negative, and if it does, throw an IOException indicating
corrupted or invalid ASN.1 data to prevent further processing.
| int readLength() throws IOException { | ||
| int length = read(); | ||
| if (length < 0) { | ||
| throw new EOFException("EOF found when length expected"); | ||
| } | ||
|
|
||
| if (length == 0x80) { | ||
| return -1; // indefinite-length encoding | ||
| } |
There was a problem hiding this comment.
Handle indefinite-length encoding or throw an exception.
The code detects indefinite-length encoding (0x80) but returns -1 without proper handling in calling methods.
int readLength() throws IOException {
int length = read();
if (length < 0) {
throw new EOFException("EOF found when length expected");
}
if (length == 0x80) {
- return -1; // indefinite-length encoding
+ throw new IOException("Indefinite-length encoding not supported");
}📝 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.
| int readLength() throws IOException { | |
| int length = read(); | |
| if (length < 0) { | |
| throw new EOFException("EOF found when length expected"); | |
| } | |
| if (length == 0x80) { | |
| return -1; // indefinite-length encoding | |
| } | |
| int readLength() throws IOException { | |
| int length = read(); | |
| if (length < 0) { | |
| throw new EOFException("EOF found when length expected"); | |
| } | |
| if (length == 0x80) { | |
| throw new IOException("Indefinite-length encoding not supported"); | |
| } | |
| // ... rest of method unchanged ... | |
| } |
🤖 Prompt for AI Agents
In
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java
around lines 127 to 135, the method readLength detects indefinite-length
encoding by returning -1 but does not properly handle this case in the calling
methods. Update the code to either fully support indefinite-length encoding by
implementing the necessary logic or throw a specific exception when encountering
0x80 to prevent silent failures. Ensure that calling methods are updated
accordingly to handle this new behavior.
| void mark() { | ||
| count = 0; | ||
| is.mark(is.available()); | ||
| } | ||
|
|
||
| int reset() { | ||
| int tmp = count; | ||
| is.reset(); | ||
| return tmp; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add mark support validation.
The code uses mark/reset without checking if the stream supports marking.
void mark() {
+ if (!is.markSupported()) {
+ throw new IllegalStateException("Mark not supported by the underlying stream");
+ }
count = 0;
is.mark(is.available());
}🤖 Prompt for AI Agents
In
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java
between lines 184 and 193, the mark() and reset() methods use the input stream's
mark and reset functions without verifying if the stream supports marking. To
fix this, add a check using is.markSupported() before calling is.mark() and
is.reset(). If marking is not supported, handle it appropriately, such as
throwing an exception or avoiding the mark/reset calls to prevent runtime
errors.
| @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.
Misleading method name and implementation.
The getBouncyCastleProvider() method doesn't actually return a BouncyCastle provider - it returns the default KeyStore provider. This could cause confusion and unexpected behavior when cryptographic operations expect BouncyCastle-specific functionality.
Consider either:
- Renaming the method to reflect its actual behavior
- Implementing proper BouncyCastle provider detection
- Throwing UnsupportedOperationException like other unimplemented methods
@Override
public Provider getBouncyCastleProvider() {
- try {
- return KeyStore.getInstance(KeyStore.getDefaultType()).getProvider();
- } catch (KeyStoreException e) {
- throw new IllegalStateException(e);
- }
+ throw new UnsupportedOperationException("BouncyCastle provider not supported in AuthzClient");
}📝 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.
| @Override | |
| public Provider getBouncyCastleProvider() { | |
| try { | |
| return KeyStore.getInstance(KeyStore.getDefaultType()).getProvider(); | |
| } catch (KeyStoreException e) { | |
| throw new IllegalStateException(e); | |
| } | |
| } | |
| @Override | |
| public Provider getBouncyCastleProvider() { | |
| throw new UnsupportedOperationException("BouncyCastle provider not supported in AuthzClient"); | |
| } |
🤖 Prompt for AI Agents
In
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java
between lines 60 and 67, the method getBouncyCastleProvider() misleadingly
returns the default KeyStore provider instead of a BouncyCastle provider. To fix
this, either rename the method to reflect it returns the default provider,
implement proper detection and return of the BouncyCastle provider, or throw an
UnsupportedOperationException to indicate the method is unimplemented,
consistent with other similar methods.
| 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(); | ||
| } |
There was a problem hiding this comment.
Critical bug in ECDSA signature conversion - multiple ASN1Encoder instances created.
The method creates multiple ASN1Encoder instances unnecessarily and the standalone write() calls on lines 114-115 are discarded, not contributing to the final result.
Apply this fix to use a single encoder instance properly:
@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();
}📝 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 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[] 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); | |
| return ASN1Encoder.create() | |
| .writeDerSeq( | |
| ASN1Encoder.create().write(rBigInteger), | |
| ASN1Encoder.create().write(sBigInteger)) | |
| .toByteArray(); | |
| } |
🤖 Prompt for AI Agents
In
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java
lines 103 to 122, the method creates multiple ASN1Encoder instances and calls
write() on some that are discarded, causing a critical bug. Fix this by creating
a single ASN1Encoder instance at the start, use it to write both BigIntegers,
and then call writeDerSeq on that same instance before returning the byte array,
ensuring all writes contribute to the final encoded output.
| 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.
🛠️ Refactor suggestion
Integer to bytes conversion has potential truncation risk.
The method truncates bytes when qLength < bytes.length, which could lose significant bits if the BigInteger is larger than expected. This should be validated or throw an exception for invalid cases.
Add validation to prevent silent truncation:
private byte[] integerToBytes(BigInteger s, int qLength) {
byte[] bytes = s.toByteArray();
if (qLength < bytes.length) {
+ // Check if we're only truncating leading zero bytes
+ for (int i = 0; i < bytes.length - qLength; i++) {
+ if (bytes[i] != 0) {
+ throw new IllegalArgumentException("BigInteger too large for target 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
between lines 151 and 163, the integerToBytes method currently truncates the
byte array when qLength is less than the byte array length, risking loss of
significant bits. To fix this, add validation to check if qLength is smaller
than bytes.length and if so, throw an IllegalArgumentException or similar to
prevent silent truncation and ensure the caller handles this case explicitly.
2 participants
Test 3
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores