Add AuthzClientCryptoProvider for authorization client cryptographic operations

[ShashankFC]

Test 3nn---nReplicated from ai-code-review-evaluation/keycloak-greptile#3

[GitHoobar]

@claude

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 48s —— View job

---

Code Review Complete

I've reviewed this PR which adds a lightweight crypto provider (AuthzClientCryptoProvider) for the authorization client with ECDSA signature conversion support. Here are my findings:

---

Critical Issues

1. Security: Potential Buffer Overflow in integerToBytes

Location: AuthzClientCryptoProvider.java:151-163

The integerToBytes method has a critical bug when handling BigInteger values larger than expected:

if (qLength < bytes.length) {
    byte[] tmp = new byte[qLength];
    System.arraycopy(bytes, bytes.length - tmp.length, tmp, 0, tmp.length);
    return tmp;
}

This silently truncates the BigInteger if it's too large, which could result in signature corruption and security vulnerabilities. This should throw an exception instead.

Fix this →

2. Resource Management: Missing try-with-resources in ASN1Decoder

Location: ASN1Decoder.java:39-43

The ByteArrayInputStream is never explicitly closed. While ByteArrayInputStream doesn't hold system resources, using try-with-resources is a best practice:

ASN1Decoder(byte[] bytes) {
    is = new ByteArrayInputStream(bytes);  // Never closed
    count = 0;
    limit = bytes.length;
}

Consider making this closeable or documenting that the stream doesn't need closing.

---

High Priority Issues

3. Code Quality: Inefficient Object Creation in ECDSA Conversion

Location: AuthzClientCryptoProvider.java:114-121

The code creates multiple unnecessary ASN1Encoder instances:

ASN1Encoder.create().write(rBigInteger);  // Discarded
ASN1Encoder.create().write(sBigInteger);  // Discarded

return ASN1Encoder.create()
        .writeDerSeq(
                ASN1Encoder.create().write(rBigInteger),
                ASN1Encoder.create().write(sBigInteger))
        .toByteArray();

The first two encoder creations are completely unused. Remove lines 114-115.

Fix this →

4. Error Handling: Incomplete Validation in ASN1Decoder.readSequence

Location: ASN1Decoder.java:49-63

The sequence reading logic doesn't validate that the entire sequence was consumed:

while (length > 0) {
    byte[] bytes = readNext();
    result.add(bytes);
    length = length - bytes.length;
}

If length becomes negative due to malformed data, this could cause issues. Should validate length >= bytes.length before subtraction.

5. Test Coverage: Limited Test Scenarios

Location: ECDSAAlgorithmTest.java

The test only validates round-trip conversion but doesn't test:

• Edge cases (all zeros, maximum values)
• Invalid/malformed ASN.1 data
• Signature verification after conversion
• Different key sizes for each algorithm

---

Medium Priority Issues

6. Code Quality: Magic Numbers

Location: AuthzClientCryptoProvider.java:104-110

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);

The magic number 1 for array offset/size adjustment isn't documented. This relates to BigInteger's sign byte handling and should be commented.

7. Code Duplication: Repeated Encoder Creation Pattern

Location: AuthzClientCryptoProvider.java:117-120

The nested encoder creation pattern is repeated. Consider extracting to a helper method:

private byte[] encodeInteger(BigInteger value) throws IOException {
    return ASN1Encoder.create().write(value);
}

8. API Design: Inconsistent Exception Handling

Location: AuthzClientCryptoProvider.java:61-67

Most methods throw UnsupportedOperationException, but getBouncyCastleProvider() wraps KeyStoreException in IllegalStateException. This inconsistency could confuse users about which operations are truly unsupported vs. failed.

9. Documentation: Missing Javadoc

Location: ASN1Encoder.java and ASN1Decoder.java

Both classes lack class-level and method-level documentation explaining:

• The ASN.1 encoding scheme used (DER)
• Supported tag types
• Expected input/output formats
• Thread safety

---

Low Priority / Suggestions

10. Code Style: Package-Private Classes Should Be Final

Location: ASN1Encoder.java:30 and ASN1Decoder.java:33

Both utility classes are package-private but not final. Since they're not designed for extension, mark them as final.

11. Performance: String Concatenation in Loop

Location: Multiple locations using IOException messages

Consider using formatted strings instead of concatenation:

throw new IOException(String.format("Invalid sequence size: expected 2, got %d", seq.size()));

12. Dependency Management: Missing Explicit Crypto Dependencies

Location: pom.xml

The code uses org.keycloak.crypto and org.keycloak.common.crypto packages but the POM changes only show test dependencies. Ensure crypto module dependencies are explicit.

---

Positive Observations

✅ Good: The implementation correctly handles BigInteger sign bytes in both encoding and decoding
✅ Good: Proper validation of tag numbers and lengths in ASN.1 parsing
✅ Good: Clear separation of concerns with dedicated encoder/decoder classes
✅ Good: Service loader integration follows Keycloak's provider pattern
✅ Good: Test covers all three ECDSA algorithms (ES256, ES384, ES512)

---

Summary

This PR adds necessary ECDSA signature conversion functionality for the authz-client, but has one critical security issue (silent truncation in integerToBytes) that must be fixed before merging. The code also has efficiency issues with unnecessary object creation and would benefit from better error handling and test coverage.

Recommendation: Address the critical issue (#1) and high-priority issues (#3-5) before merging.

---