Fix a bug where SNI doesn't reflect the :authority of a request
#6522
Conversation
WalkthroughThe changes modify endpoint normalization and TLS/SNI handling. HttpChannelPool now preserves trailing dots in endpoints. HttpClientDelegate derives server names from authority headers for proper SNI when endpoints contain only IP addresses and normalizes trailing dots for TLS compatibility. DefaultClientRequestContext refactors header computation into a reusable helper method. A new test validates Authority header handling with custom TLS trust. Changes
Sequence DiagramsequenceDiagram
participant Client as HttpClientDelegate
participant Pool as HttpChannelPool
participant Auth as Authority Parser
participant SNI as TLS/SNI Handler
Client->>Client: Detect IP-only endpoint + TLS
Client->>Auth: authorityToServerName(authority)
Auth-->>Client: server name (if valid)
Client->>SNI: Normalize host (remove trailing dot)
Client->>Client: Set endpoint host to server name
Client->>Pool: Create PoolKey with updated endpoint
Pool->>Pool: Store endpoint as-is (no normalization)
Pool-->>Client: Return channel from pool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6522 +/- ##
============================================
- Coverage 74.46% 74.14% -0.32%
- Complexity 22234 23366 +1132
============================================
Files 1963 2100 +137
Lines 82437 87514 +5077
Branches 10764 11492 +728
============================================
+ Hits 61385 64889 +3504
- Misses 15918 17145 +1227
- Partials 5134 5480 +346 ☔ View full report in Codecov by Sentry. 🚀 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: 0
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java (1)
260-274: Consider wrapping authority parsing in try-catch for robustness.
SchemeAndAuthority.of(null, authority)may throw an exception for malformed authority strings. Since this method is used to derive an optional server name for SNI, a parsing failure shouldn't abort the request—it should gracefully returnnull.@Nullable private static String authorityToServerName(@Nullable String authority) { if (authority == null) { return null; } - String serverName = SchemeAndAuthority.of(null, authority).host(); + final String serverName; + try { + serverName = SchemeAndAuthority.of(null, authority).host(); + } catch (Exception e) { + // Malformed authority; fall back to no server name. + return null; + } if (NetUtil.isValidIpV4Address(serverName) || NetUtil.isValidIpV6Address(serverName)) { return null; } - serverName = serverName.trim(); - if (serverName.isEmpty()) { + final String trimmed = serverName.trim(); + if (trimmed.isEmpty()) { return null; } - return serverName; + return trimmed; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java(3 hunks)core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java(3 hunks)core/src/test/java/com/linecorp/armeria/client/TrailingDotSniTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/test/java/com/linecorp/armeria/client/TrailingDotSniTest.javacore/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.javacore/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.javacore/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java
🧬 Code graph analysis (3)
core/src/test/java/com/linecorp/armeria/client/TrailingDotSniTest.java (1)
core/src/main/java/com/linecorp/armeria/common/HttpHeaderNames.java (1)
HttpHeaderNames(53-1099)
core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java (1)
core/src/main/java/com/linecorp/armeria/common/HttpHeaderNames.java (1)
HttpHeaderNames(53-1099)
core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java (1)
core/src/main/java/com/linecorp/armeria/internal/common/SchemeAndAuthority.java (1)
SchemeAndAuthority(32-151)
⏰ 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). (2)
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (6)
core/src/test/java/com/linecorp/armeria/client/TrailingDotSniTest.java (1)
78-96: LGTM! Test validates the new SNI behavior correctly.This test verifies that when connecting to an IP-based endpoint, the
:authorityheader set via a decorator is used for SNI during TLS handshake. The certificate is forexample.com, and setting the authority header allows the TLS handshake to succeed with the correct server name.core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java (1)
624-628: Trailing dot normalization moved to HttpClientDelegate.The removal of
endpoint.withoutTrailingDot()here is intentional since the normalization now occurs inHttpClientDelegate.acquireConnectionAndExecute0()(line 231) right before PoolKey creation. This ensures SNI determination happens at the correct point in the request lifecycle.core/src/main/java/com/linecorp/armeria/client/HttpClientDelegate.java (1)
219-231: Logic correctly derives SNI from authority for IP-only TLS endpoints.The implementation properly:
- Checks if the protocol is TLS and endpoint contains only an IP address
- Derives a hostname from the
:authorityheader when available- Strips trailing dots for SNI compliance (per RFC requirements)
This ensures SNI reflects the effective
:authorityat request time, as described in the PR objectives.core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java (3)
585-600: Clean refactoring of internal header computation.The new
computeInternalHeadershelper consolidates the logic for settingSCHEME,AUTHORITY, andORIGINheaders. The method correctly:
- Sets the scheme based on session protocol
- Sets authority from the endpoint when available
- Conditionally fills the Origin header based on
autoFillOriginHeadersettingThis improves maintainability by eliminating code duplication between
updateEndpointandfailEarly.
512-517: LGTM!The
updateEndpointmethod now delegates tocomputeInternalHeaders, maintaining the same behavior while improving code organization.
569-576: LGTM!The
failEarlymethod correctly usescomputeInternalHeaderswith anullendpoint parameter, ensuring the scheme header is still set even when there's no endpoint available.
3 participants
Motivation:
This is part of an ongoing effort to apply per-request TLS: #6516
Currently, SNI is determined as follows:
Endpointhas a hostname, the hostname is used:authorityat the time of setting theEndpointis usedarmeria/core/src/main/java/com/linecorp/armeria/internal/client/DefaultClientRequestContext.java
Lines 584 to 593 in ee1836f
When using the
:authority, it does not consider headers that might have been set after the endpoint is determined (e.g. in the decorators)I propose that the final SNI is determined right before
PoolKeygeneration. This will be useful in the upcoming PR to determine the exactTLSspecification associated with a HTTP request.Modifications:
HttpClientDelegateOriginheaderResult: