Skip to content

Add AuthzClientCryptoProvider for authorization client cryptographic operations#8

Open
zaibkhan wants to merge 1 commit into
feature-authz-crypto-baselinefrom
feature-authz-crypto-implementation
Open

Add AuthzClientCryptoProvider for authorization client cryptographic operations#8
zaibkhan wants to merge 1 commit into
feature-authz-crypto-baselinefrom
feature-authz-crypto-implementation

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

…tory

closes #33831

Signed-off-by: mposolda <mposolda@gmail.com>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Initialize crypto with safe classloader, ensure provider discovery
What’s good: Initializing CryptoIntegration in AuthzClient ensures crypto providers are available for client operations; adding provider order clarifies selection precedence.
Review Status: ❌ Requires changes
Overall Priority: High
Review Update:
• Coverage: Reviewed all 12 files across 2 batches

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Incorrect DER length bounds check in ASN1Decoder …/crypto/ASN1Decoder.java
Incorrect bounds check may reject valid DER or accept malformed inputs. The check compares content length against the total buffer size rather than the remaining bytes in the stream. Use the remaining capacity (limit - count) to ensure the declared length fits within what is left to read.
High Compatibility — Use TCCL fallback for CryptoIntegration.init …/client/AuthzClient.java
Using the AuthzClient classloader can miss SPI providers in environments where services are exposed via the Thread Context ClassLoader (e.g., application servers). Prefer TCCL with a fallback to ensure robust provider discovery.

Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Use the thread context classloader (with fallback) when initializing CryptoIntegration to avoid SPI discovery failures in containerized/classloader-rich environments.
  • Testing: Consider a test that sets a custom Thread Context ClassLoader before AuthzClient.create(Configuration) and verifies that SPI providers (e.g., the new client provider) are discovered and ECDSA operations work. This guards against regressions in classloader-dependent environments.
  • Documentation: A brief note in AuthzClient Javadoc about initializing crypto providers and the classloader used would help integrators understand discovery behavior in app servers.
  • Compatibility: Initializing CryptoIntegration during AuthzClient.create may alter which CryptoProvider is selected if multiple providers are present and ordered; verify this doesn’t change behavior for existing clients running with Elytron/default providers.
  • Performance: Initialization should be cheap and idempotent; if not, repeated create() calls could add overhead. Confirm init's idempotency or memoize to avoid repeated work.
  • Security: No direct security issues identified in the provided changes; ensuring correct provider discovery is important for cryptographic correctness.
  • Open questions: Is CryptoIntegration.init idempotent across different classloaders? If multiple AuthzClient instances are created, do we risk re-initialization or provider reordering?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

throw new IOException("corrupted stream - negative length found");
}

if (length >= limit) // after all we must have read at least 1 byte

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: Incorrect bounds check may reject valid DER or accept malformed inputs. The check compares content length against the total buffer size rather than the remaining bytes in the stream. Use the remaining capacity (limit - count) to ensure the declared length fits within what is left to read.

Suggested change
if (length >= limit) // after all we must have read at least 1 byte
if (length > (limit - count)) {
throw new IOException("corrupted stream - out of bounds length found");
}

@Override
public Provider getBouncyCastleProvider() {
try {
return KeyStore.getInstance(KeyStore.getDefaultType()).getProvider();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: getBouncyCastleProvider should return a BC provider when present; returning the default KeyStore provider is misleading and can break callers that expect BC-specific capabilities. Prefer returning the BC provider if available, falling back to the default only when BC is not installed.

@@ -91,6 +92,7 @@ public static AuthzClient create(InputStream configStream) throws RuntimeExcepti
* @return a new instance
*/
public static AuthzClient create(Configuration configuration) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: Using the AuthzClient classloader can miss SPI providers in environments where services are exposed via the Thread Context ClassLoader (e.g., application servers). Prefer TCCL with a fallback to ensure robust provider discovery.

Suggested change
public static AuthzClient create(Configuration configuration) {
ClassLoader cl = Thread.currentThread().getContextClassLoader();
CryptoIntegration.init(cl != null ? cl : AuthzClient.class.getClassLoader());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants