fix: route TLS trust probe through IDE HTTP proxy (CRW-12333) - #361
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces ChangesTLS probing and trust evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
50b5a29 to
f3f7133
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #361 +/- ##
==========================================
+ Coverage 0.00% 35.04% +35.04%
==========================================
Files 4 113 +109
Lines 26 4916 +4890
Branches 0 945 +945
==========================================
+ Hits 0 1723 +1723
- Misses 26 2941 +2915
- Partials 0 252 +252 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt (2)
174-197: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe
Authenticatorrequest hardcodesBasicand ignores the advertised realm.
basicProxyAuthorizationpasses an empty prompt and the schemeBasicwithout reading theProxy-Authenticateheader from the 407 response. A proxy that advertises a realm, or a credential store keyed by realm, then returns no credentials. Capture theProxy-Authenticatevalue inreadConnectStatusand pass the parsed realm and scheme here.Also note that
PasswordAuthentication.getPassword()returns a copy, sopassword.fillclears the copy only.🤖 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 `@src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt` around lines 174 - 197, Update readConnectStatus to capture the 407 response’s Proxy-Authenticate header, parse its advertised scheme and realm, and pass both into basicProxyAuthorization instead of using an empty prompt and hardcoded Basic. Preserve the existing credential encoding, but remove the misleading password.fill cleanup or otherwise avoid implying it clears the authenticator’s stored password because getPassword() returns a copy.
78-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reporting failures to the proxy selector.
connectnever callsproxySelector.connectFailedwhen a proxy route fails. IDE and JVM selectors use that callback to deprioritize a broken proxy for later requests. Adding the call in theIOExceptionbranch ofconnectkeeps proxy failover state consistent with the other HTTP paths inIdeHttpProxy.🤖 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 `@src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt` around lines 78 - 101, The connect flow should report failed proxy routes through ProxySelector.connectFailed. Update the IOException handling in connect to invoke the callback with the failed proxy’s address and the thrown exception, while preserving the existing exception propagation and direct-connection behavior.
🤖 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
`@src/main/kotlin/com/redhat/devtools/gateway/auth/tls/DefaultTlsTrustManager.kt`:
- Around line 164-178: In the SSLHandshakeException handler, update the captured
chain validation before trustAnchor is assigned so both null and empty
serverCertificateChain results rethrow the original exception. Keep the existing
CapturedCertInfo construction unchanged for non-empty chains, using the chain
value associated with CapturingTrustManager.
In `@src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt`:
- Around line 146-172: Bound both CONNECT response dimensions: update
readAsciiLine to enforce a maximum line length and fail when the limit is
exceeded, including lines that reach EOF without a newline; update
readConnectStatus to read at most a fixed maximum number of header lines while
preserving termination on an empty line or EOF. Define or reuse clear constants
for the line and header limits, and use an indexed while loop rather than break
inside repeat.
- Around line 199-203: Update TlsConnectionProbe.handshakeOver to set the
created SSLSocket's SSLParameters.endpointIdentificationAlgorithm to "HTTPS"
before startHandshake(). Preserve CapturingTrustManager certificate capture and
add regression tests covering both hostname-matching success and
hostname-mismatch failure cases.
In
`@src/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeTest.kt`:
- Line 92: Suppress detekt’s EmptyFunctionBlock rule for the anonymous
ProxySelector objects containing the empty connectFailed overrides, or add an
intentionality comment inside each body. Apply the fix to all four connectFailed
implementations referenced in TlsConnectionProbeTest.
- Around line 42-58: Replace the non-thread-safe tasks mutableListOf used by the
proxy accept thread and tearDown with a concurrent collection that supports safe
concurrent appends and iteration. Update the tasks declaration and retain the
existing cancellation behavior in tearDown, ensuring worker-added futures cannot
be skipped or trigger ConcurrentModificationException.
---
Nitpick comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt`:
- Around line 174-197: Update readConnectStatus to capture the 407 response’s
Proxy-Authenticate header, parse its advertised scheme and realm, and pass both
into basicProxyAuthorization instead of using an empty prompt and hardcoded
Basic. Preserve the existing credential encoding, but remove the misleading
password.fill cleanup or otherwise avoid implying it clears the authenticator’s
stored password because getPassword() returns a copy.
- Around line 78-101: The connect flow should report failed proxy routes through
ProxySelector.connectFailed. Update the IOException handling in connect to
invoke the callback with the failed proxy’s address and the thrown exception,
while preserving the existing exception propagation and direct-connection
behavior.
🪄 Autofix
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: 58b7d331-9f0b-45ad-8c36-7b7d9bb6ba4b
⛔ Files ignored due to path filters (2)
src/test/resources/tls/server-cert.pemis excluded by!**/*.pemsrc/test/resources/tls/server-key.pemis excluded by!**/*.pem
📒 Files selected for processing (7)
src/main/kotlin/com/redhat/devtools/gateway/auth/tls/DefaultTlsTrustManager.ktsrc/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.ktsrc/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsProbe.ktsrc/main/kotlin/com/redhat/devtools/gateway/util/IdeHttpProxy.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/DefaultTlsTrustManagerTrustTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeTest.ktsrc/test/resources/tls/README.md
💤 Files with no reviewable changes (1)
- src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsProbe.kt
|
@azatsarynnyy, @vrubezhny: we should have this PR merged before we release the new version 0.0.19 |
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
`@src/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeTest.kt`:
- Around line 284-286: Update the authorization check in the request validation
around the authorized predicate to require the Basic header value to equal the
Base64-encoded token for proxy-user:proxy-pass, rather than accepting any header
with the Basic scheme. Preserve case-insensitive matching for the header name
and ensure invalid credentials cause the test to reject the request.
🪄 Autofix
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: 0344d870-50c4-4fb2-bc67-8cefef6e0f54
⛔ Files ignored due to path filters (4)
src/test/resources/tls/mismatch-cert.pemis excluded by!**/*.pemsrc/test/resources/tls/mismatch-key.pemis excluded by!**/*.pemsrc/test/resources/tls/server-cert.pemis excluded by!**/*.pemsrc/test/resources/tls/server-key.pemis excluded by!**/*.pem
📒 Files selected for processing (3)
src/main/kotlin/com/redhat/devtools/gateway/auth/tls/DefaultTlsTrustManager.ktsrc/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/kotlin/com/redhat/devtools/gateway/auth/tls/DefaultTlsTrustManager.kt
- src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@src/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeRetryTest.kt`:
- Line 45: Replace the released-port setup in TlsConnectionProbeRetryTest with a
deterministic local proxy fixture that remains bound while
TlsConnectionProbe.connect executes and accepts then immediately closes the
connection. Update the test lifecycle to start and clean up this fixture,
preserving the intended proxy-failure and retry assertions without relying on
deadProxyPort.
In
`@src/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeTestFixtures.kt`:
- Around line 35-37: Make the tasks collection in TlsConnectionProbeTestFixtures
safe for concurrent registration and iteration by using CopyOnWriteArrayList or
equivalent synchronization. Ensure proxy accept task additions and close()
teardown cannot race, while preserving existing task cancellation and executor
shutdown behavior.
- Around line 151-153: Update the authorized check in the proxy fixture’s
request validation to compare the complete Proxy-Authorization Basic value for
proxy-user:proxy-pass, rather than accepting any header with the Basic prefix.
Preserve case-insensitive header-name matching while requiring the credential
value to match exactly.
🪄 Autofix
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: 739a8d4d-f630-41cd-abc3-315c6bc671a0
📒 Files selected for processing (5)
src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeDirectTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeProxyTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeRetryTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeTestFixtures.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt
43102e2 to
33a148b
Compare
vrubezhny
left a comment
vrubezhny
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.kt`:
- Around line 152-160: Update writeConnectRequest to build a bracketed authority
when host contains “:” (IPv6), while leaving non-IPv6 hosts unchanged; reuse
that authority in both the CONNECT request line and Host header. Add a
regression test using a proxy that verifies the received CONNECT authority for
an IPv6 target.
In
`@src/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeRetryTest.kt`:
- Around line 42-63: Strengthen both retry tests around
TlsConnectionProbe.connect by recording proxy activity in the fixture and
asserting that the configured proxy routes were actually used. Update the
failing-proxy and successful-proxy test setups to capture a proxy connection or
CONNECT request, then verify those records after the connection attempt,
including the intended retry/no-direct-fallback behavior.
🪄 Autofix
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: 77cdab68-f9cd-40da-b4a1-8895c5a3cd28
📒 Files selected for processing (5)
src/main/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbe.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeDirectTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeProxyTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeRetryTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/auth/tls/TlsConnectionProbeTestFixtures.kt
TlsConnectionProbe opened a raw SSLSocket and bypassed IdeHttpProxy, so proxy-only clusters timed out even when Check connection worked. Use the IDE ProxySelector (HTTP CONNECT, Basic on 407) and surface connect failures clearly instead of treating them as trust prompts. Signed-off-by: Andre Dietisheim <adietish@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
fixes https://redhat.atlassian.net/browse/CRW-12333
TlsProbe opened a raw SSLSocket and bypassed IdeHttpProxy, so proxy-only clusters timed out even when Check connection worked. Use the IDE ProxySelector (HTTP CONNECT, Basic on 407) and surface connect failures clearly instead of treating them as trust prompts.