Add trust-only SSL HttpClient to oltu-jdk21 to prevent OAuth2 client-cert dual-auth - #1398
Open
YasasRangika wants to merge 4 commits into
Open
Add trust-only SSL HttpClient to oltu-jdk21 to prevent OAuth2 client-cert dual-auth#1398YasasRangika wants to merge 4 commits into
YasasRangika wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary
WalkthroughAdded the Sequence Diagram(s)sequenceDiagram
participant OAuthCaller
participant TrustOnlyURLConnectionClient
participant TrustOnlySslUtils
participant OAuthEndpoint
OAuthCaller->>TrustOnlyURLConnectionClient: execute OAuth request
TrustOnlyURLConnectionClient->>TrustOnlySslUtils: apply trust-only SSL for HTTPS
TrustOnlyURLConnectionClient->>OAuthEndpoint: send request
OAuthEndpoint-->>TrustOnlyURLConnectionClient: return response stream
TrustOnlyURLConnectionClient-->>OAuthCaller: return OAuthClientResponse
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (2)
oltu-jdk21/1.0.0.wso2v2/pom.xml (2)
110-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit compiler release setting.
The module declares no
maven.compiler.release(or source/target). The produced bytecode level then depends on the local JDK and the inherited default. For a module namedoltu-jdk21, set the level explicitly to keep builds reproducible.♻️ Proposed change
<properties> + <maven.compiler.release>21</maven.compiler.release> <oltu.version>1.0.0</oltu.version>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oltu-jdk21/1.0.0.wso2v2/pom.xml` around lines 110 - 116, Update the Maven properties in the module POM to define an explicit maven.compiler.release value of 21, ensuring oltu-jdk21 consistently produces Java 21 bytecode regardless of the build environment.
81-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpgrade
maven-bundle-pluginto a JDK 21-compatible version.
oltu-jdk21builds as a bundle but still pinsorg.apache.felix:maven-bundle-plugin:1.4.0; the JDK 21tomcat-el-api-jdk21modules use5.1.2or2.4.0. A newer bundle plugin is required to avoid parsing JDK 21 bytecode during packaging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@oltu-jdk21/1.0.0.wso2v2/pom.xml` around lines 81 - 85, Update the maven-bundle-plugin configuration in the Maven build to use a JDK 21-compatible newer version, matching the established compatible version used by the tomcat-el-api-jdk21 modules where appropriate, and replace the outdated 1.4.0 declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@oltu-jdk21/1.0.0.wso2v2/src/main/java/org/wso2/carbon/oltu/ssl/TrustOnlyURLConnectionClient.java`:
- Around line 62-70: Update TrustOnlyURLConnectionClient’s connection setup to
apply configurable connect and read timeouts before connect() is invoked, using
sensible defaults that prevent indefinite blocking. Preserve the existing
URLConnectionClient behavior and HTTPS trust handling, and expose timeout
configuration through the constructor if required for parity and caller opt-in.
- Around line 118-125: Update the request-body writing logic in
TrustOnlyURLConnectionClient to use an OutputStreamWriter with
StandardCharsets.UTF_8 instead of the platform-default
PrintWriter(OutputStream), and wrap the writer in try-with-resources so it is
always closed. Preserve the existing POST/PUT behavior and requestBody output.
- Around line 93-100: In the response handling around responseCode and
inputStream, guard the nullable result of httpURLConnection.getErrorStream() for
SC_BAD_REQUEST and SC_UNAUTHORIZED before passing it to
OAuthUtils.saveStreamAsString. Preserve normal input-stream processing and
ensure responses without an error body produce an appropriate empty or fallback
response body instead of invoking the helper with null.
---
Nitpick comments:
In `@oltu-jdk21/1.0.0.wso2v2/pom.xml`:
- Around line 110-116: Update the Maven properties in the module POM to define
an explicit maven.compiler.release value of 21, ensuring oltu-jdk21
consistently produces Java 21 bytecode regardless of the build environment.
- Around line 81-85: Update the maven-bundle-plugin configuration in the Maven
build to use a JDK 21-compatible newer version, matching the established
compatible version used by the tomcat-el-api-jdk21 modules where appropriate,
and replace the outdated 1.4.0 declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec8658c7-ed29-4261-863a-f8b91258d0da
📒 Files selected for processing (3)
oltu-jdk21/1.0.0.wso2v2/pom.xmloltu-jdk21/1.0.0.wso2v2/src/main/java/org/wso2/carbon/oltu/ssl/TrustOnlySslUtils.javaoltu-jdk21/1.0.0.wso2v2/src/main/java/org/wso2/carbon/oltu/ssl/TrustOnlyURLConnectionClient.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@oltu-jdk21/1.0.0.wso2v2/src/main/java/org/wso2/carbon/oltu/ssl/TrustOnlyURLConnectionClient.java`:
- Around line 65-69: Update the TrustOnlyURLConnectionClient constructor to
validate connectTimeoutMillis and readTimeoutMillis before assigning them,
rejecting negative values immediately while preserving zero if it represents the
supported infinite-timeout behavior. Ensure invalid arguments fail during
construction rather than when execute() configures the URLConnection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d9c1a62-fe15-4a77-a205-6ebec11a51ed
📒 Files selected for processing (2)
oltu-jdk21/1.0.0.wso2v2/pom.xmloltu-jdk21/1.0.0.wso2v2/src/main/java/org/wso2/carbon/oltu/ssl/TrustOnlyURLConnectionClient.java
🚧 Files skipped from review as they are similar to previous changes (1)
- oltu-jdk21/1.0.0.wso2v2/pom.xml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Outbound OAuth2/OIDC token calls made via Oltu's stock
URLConnectionClientuse the JVM's defaultSSLContext/SSLSocketFactory, with no explicit client identity. If the JVM-wide default happens to carry a client certificate (e.g. because some other component in the process setjavax.net.ssl.keyStore*and something touchedSSLContext.getDefault()before it did), that certificate gets presented on the connection even though the caller only intends to authenticate viaclient_secret. Some identity providers reject this as an invalid dual client-authentication attempt (invalid_request, The client MUST NOT use more than one authentication method).This adds
org.wso2.carbon.oltu.ssl—TrustOnlyURLConnectionClient(an OltuHttpClientSPI implementation) andTrustOnlySslUtils— which apply a trust-only (no clientKeyManager)SSLSocketFactoryscoped to a single connection. Consumers opt in by usingTrustOnlyURLConnectionClientinstead of the stockURLConnectionClient; nothing about the existingorg.apache.oltu.oauth2.client.URLConnectionClientclass or any other upstream class changes, so this has no effect on any existing consumer that doesn't explicitly adopt it.