Skip to content

Conversation

@shilmyhasan
Copy link
Contributor

@shilmyhasan shilmyhasan commented Nov 4, 2025

Purpose

Add mtls support while connecting to a backend through proxy.

Summary by CodeRabbit

  • Refactor
    • Consolidated HTTP client construction and SSL/HTTPS handling to improve security configuration and maintainability.
    • Enhanced proxy configuration support with improved credential handling and route planning capabilities.
    • Streamlined internal SSL context resolution logic while preserving per-host customization options.

@shilmyhasan shilmyhasan requested a review from chanikag as a code owner November 4, 2025 05:27
@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025

Walkthrough

The OAuthClient's HTTP client construction and SSL handling underwent a comprehensive refactor, consolidating multiple legacy paths into a unified flow with explicit SSL context resolution, hostname verification, and proxy configuration support.

Changes

Cohort / File(s) Change Summary
OAuth client SSL/proxy refactoring
modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java
Refactored getSecureClient method to unify HTTP client construction: replaced ad-hoc RequestConfig with multi-step SSL context resolution via ClientConnFactoryBuilder, introduced hostname verifier selection (with custom DefaultAndLocalhost option), added SSLConnectionSocketFactory and connection Registry setup, implemented conditional connection manager selection (PoolingHttpClientConnectionManager for proxy, BasicHttpClientConnectionManager otherwise), enhanced proxy configuration with DefaultProxyRoutePlanner and credentials support, removed legacy trust-store and hostname verification helper methods, retained per-host SSL context override capability via getSSLContextWithUrl.

Sequence Diagram(s)

sequenceDiagram
    actor caller as Caller
    participant oauth as OAuthClient
    participant builder as ClientConnFactoryBuilder
    participant ssl as SSLContext
    participant verifier as HostnameVerifier
    participant connMgr as ConnectionManager
    participant client as HttpClient

    caller->>oauth: getSecureClient(url, config)
    oauth->>builder: read SSL config from Axis2
    builder->>ssl: create SSLContext with default fallback
    ssl-->>oauth: SSLContext
    
    oauth->>oauth: select HostnameVerifier based on system properties
    oauth->>verifier: create verifier instance (DefaultAndLocalhost or standard)
    verifier-->>oauth: HostnameVerifier
    
    oauth->>oauth: create SSLConnectionSocketFactory(SSLContext, verifier)
    oauth->>oauth: build Registry for HTTP/HTTPS
    
    alt proxy enabled
        oauth->>connMgr: create PoolingHttpClientConnectionManager
    else no proxy
        oauth->>connMgr: create BasicHttpClientConnectionManager
    end
    connMgr-->>oauth: ConnectionManager
    
    oauth->>oauth: wire SSL socket factory into ConnectionManager
    oauth->>oauth: apply RequestConfig (includeProxy if needed)
    oauth->>client: build HttpClient with ConnectionManager + RequestConfig
    client-->>oauth: HttpClient
    oauth-->>caller: secure HttpClient
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • SSL/security-critical logic: Refactored hostname verification, SSL context resolution, and fallback handling require careful security review
  • Connection manager selection: Conditional logic between PoolingHttpClientConnectionManager and BasicHttpClientConnectionManager paths needs validation
  • Proxy configuration: New DefaultProxyRoutePlanner integration and credential handling must be verified for correctness
  • Removed legacy methods: Ensure no remaining code paths depend on removed helper methods
  • Per-host SSL context override: Verify that getSSLContextWithUrl integration remains functional and backward-compatible

Poem

🐰 Hops of joy for cleaner flows,
SSL paths consolidated with care,
No more legacy ghosts in the code,
Just unified trust, hostname verified fair,
Proxy routes now dance in the air!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is incomplete. It only includes a Purpose section with minimal detail; all other required sections (Goals, Approach, User stories, Release note, Documentation, Training, Certification, Marketing, Automation tests, Security checks, Samples, Related PRs, Migrations, Test environment, Learning) are missing. Complete the description by adding the remaining sections from the template: Goals, Approach, User stories, Release note, Documentation, Training, Certification, Marketing, Automation tests, Security checks, Samples, Related PRs, Migrations, Test environment, and Learning.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add mtls support for proxy connections' directly and clearly summarizes the main change, which is enabling mutual TLS support when connecting through a proxy.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java (1)

249-339: TrustStoreConfigs regression breaks custom TLS/mTLS setups

getSecureClient(..., trustStoreConfigs) no longer consumes the trustStoreConfigs argument, even though the public overload exists precisely to let callers supply endpoint-specific key/trust material. After this refactor those credentials are silently ignored, so any deployment that relied on that hook (including the newly advertised mTLS-over-proxy scenario) will fail the TLS handshake. Please reinstate the logic that merges trustStoreConfigs into the SSLContext before you build the SSLConnectionSocketFactory—load the provided key/trust stores and layer them over the Axis2 defaults so the client certificate and custom trust anchors are honored.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e3ebe4 and b4a9339.

📒 Files selected for processing (1)
  • modules/core/src/main/java/org/apache/synapse/endpoints/auth/oauth/OAuthClient.java (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build

@AnuGayan
Copy link
Contributor

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Dec 22, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chanikag chanikag requested a review from tharindu1st December 22, 2025 09:01
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