Add AuthzClientCryptoProvider for authorization client cryptographic operations#8
Conversation
…tory closes #33831 Signed-off-by: mposolda <mposolda@gmail.com>
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:XL
Sequence DiagramShows the new initialization path where AuthzClient ensures CryptoIntegration is initialized (via ServiceLoader), the highest-order CryptoProvider is selected (AuthzClientCryptoProvider is registered), and the client obtains the ECDSA helper for signature conversions used by the authz client tests. sequenceDiagram
participant TestClient
participant AuthzClient
participant CryptoIntegration
participant ServiceLoader
participant AuthzClientCryptoProvider
TestClient->>AuthzClient: create(configuration)
AuthzClient->>CryptoIntegration: init(classLoader)
CryptoIntegration->>ServiceLoader: load CryptoProvider implementations
ServiceLoader-->>CryptoIntegration: returns providers (with order)
CryptoIntegration->>AuthzClientCryptoProvider: select highest-order provider and set as active
AuthzClient-->>AuthzClientCryptoProvider: getEcdsaCryptoProvider() (used for ASN.1 <-> R|S conversions)
AuthzClientCryptoProvider-->>AuthzClient: ECDSA helper returned
Generated by CodeAnt AI |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| @Override | ||
| public int order() { | ||
| return 100; |
There was a problem hiding this comment.
Suggestion: This lightweight crypto provider is registered as a CryptoProvider service and returns a high priority (order() = 100), but most of its methods throw UnsupportedOperationException; in an environment where both this client JAR and the full server crypto provider are on the classpath, CryptoIntegration.detectProvider() will sort by order() and may incorrectly select this incomplete provider, leading to runtime UnsupportedOperationExceptions in server-side cryptographic operations that expect a full implementation. [logic error]
Severity Level: Critical 🚨
- ❌ CryptoIntegration may select incomplete AuthzClientCryptoProvider implementation.
- ❌ Server cryptographic operations fail with UnsupportedOperationException at runtime.
- ⚠️ Affects apps including authz-client jar on server classpath.| return 100; | |
| // Use a very low priority so this lightweight client provider is only chosen | |
| // when no other CryptoProvider implementations are available on the classpath. | |
| return Integer.MIN_VALUE; |
Steps of Reproduction ✅
1. Package an application (for example, a Keycloak-based server or tool) whose classloader
includes both the `authz/client` module and at least one server crypto module implementing
`CryptoProvider` (e.g. `org.keycloak.crypto.def.DefaultCryptoProvider` in
`crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java`)
together with their `META-INF/services/org.keycloak.common.crypto.CryptoProvider` entries;
note that
`authz/client/src/main/resources/META-INF/services/org.keycloak.common.crypto.CryptoProvider`
already registers
`org.keycloak.authorization.client.util.crypto.AuthzClientCryptoProvider` via
ServiceLoader (seen in Grep output).
2. At application startup, call `CryptoIntegration.init(classLoader)` as intended by
`common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java:29-44`, passing
the application classloader that sees both providers; inside `init`,
`detectProvider(classLoader)` is invoked (`CryptoIntegration.java:55`).
3. In `detectProvider`, ServiceLoader discovers all `CryptoProvider` implementations on
the classpath and sorts them by
`Comparator.comparingInt(CryptoProvider::order).reversed()`
(`CryptoIntegration.java:55-58`); `AuthzClientCryptoProvider.order()` returns `100` as
defined in
`authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java:69-72`,
so if other providers use a lower `order()` value this lightweight provider becomes the
first in `foundProviders` and is set as the global provider
(`CryptoIntegration.java:63-71`).
4. Later, when server-side code calls `CryptoIntegration.getProvider()`
(`CryptoIntegration.java:46-51`) and then uses methods such as `getPemUtils()`,
`getCertificateUtils()`, `getSignature(...)`, or `wrapFactoryForTruststore(...)`, it now
receives the `AuthzClientCryptoProvider` instance whose implementations for these methods
simply throw `UnsupportedOperationException` (`AuthzClientCryptoProvider.java:75-97`,
183-220, 222-225), causing runtime failures in cryptographic operations that expect a full
provider implementation; this failure mode is a direct consequence of the high `order()`
value on an intentionally incomplete client-side provider.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java
**Line:** 71:71
**Comment:**
*Logic Error: This lightweight crypto provider is registered as a `CryptoProvider` service and returns a high priority (`order()` = 100), but most of its methods throw `UnsupportedOperationException`; in an environment where both this client JAR and the full server crypto provider are on the classpath, `CryptoIntegration.detectProvider()` will sort by `order()` and may incorrectly select this incomplete provider, leading to runtime `UnsupportedOperationException`s in server-side cryptographic operations that expect a full implementation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| * | ||
| * The higher number has preference over the lower number | ||
| */ | ||
| int order(); |
There was a problem hiding this comment.
Suggestion: Adding a new abstract method to a public interface is a binary-incompatible change: any existing third-party CryptoProvider implementations compiled against the previous version will not have an order() method, so when CryptoIntegration.detectProvider calls CryptoProvider::order they will trigger an AbstractMethodError at runtime; making order() a default method with a sensible default value preserves compatibility while still allowing newer providers to override it for prioritization. [type error]
Severity Level: Critical 🚨
- ❌ Keycloak fails startup when custom CryptoProvider SPI present.
- ⚠️ CryptoIntegration auto-detection unusable with third-party crypto providers.| int order(); | |
| default int order() { | |
| return 0; | |
| } |
Steps of Reproduction ✅
1. Implement a custom crypto provider in an external module as `public class
CustomCryptoProvider implements org.keycloak.common.crypto.CryptoProvider` compiled
against a Keycloak version where `CryptoProvider` (file
`common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java`) does not yet
declare `int order()`.
2. Register this implementation for ServiceLoader by adding
`org.keycloak.common.crypto.CryptoProvider` with the FQCN of `CustomCryptoProvider` to
`META-INF/services` in that external JAR and put the JAR on the Keycloak server classpath.
3. Upgrade Keycloak to the PR version where `CryptoProvider` declares the new abstract
method `int order()` (line 44 in `CryptoProvider.java`) and start the server, which calls
`CryptoIntegration.init(KeycloakApplication.class.getClassLoader())` from
`services/src/main/java/org/keycloak/services/resources/KeycloakApplication.java:89`.
4. During `CryptoIntegration.init`, `detectProvider` in
`common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java:55-58` executes
`ServiceLoader.load(CryptoProvider.class, classLoader)` and sorts the loaded instances
with `Comparator.comparingInt(CryptoProvider::order).reversed()`, which invokes `order()`
on `CustomCryptoProvider` that has no such method, causing a
`java.lang.AbstractMethodError` and preventing server startup.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java
**Line:** 44:44
**Comment:**
*Type Error: Adding a new abstract method to a public interface is a binary-incompatible change: any existing third-party `CryptoProvider` implementations compiled against the previous version will not have an `order()` method, so when `CryptoIntegration.detectProvider` calls `CryptoProvider::order` they will trigger an `AbstractMethodError` at runtime; making `order()` a default method with a sensible default value preserves compatibility while still allowing newer providers to override it for prioritization.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Nitpicks 🔍
|
|
CodeAnt AI finished running the review. |
2 participants
User description
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
CodeAnt-AI Description
Add ECDSA support and selectable crypto provider to authz-client
What Changed
Impact
✅ Authz client can handle ES256/ES384/ES512 signatures✅ Deterministic crypto provider selection when multiple providers exist✅ ECDSA signature conversion validated by unit tests💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.