Skip to content

fix(tls): skip server_name override when dialing an HTTPS proxy#25899

Queued
gwenaskell wants to merge 9 commits into
masterfrom
yoenn.burban/OPA-6001-skip-server-name-for-proxy
Queued

fix(tls): skip server_name override when dialing an HTTPS proxy#25899
gwenaskell wants to merge 9 commits into
masterfrom
yoenn.burban/OPA-6001-skip-server-name-for-proxy

Conversation

@gwenaskell

@gwenaskell gwenaskell commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to the merged tls.server_name OpenSSL verification fix (#25881).

The proxy connector uses one inner HttpsConnector to dial both the destination directly (no_proxy) and, for proxied requests, the proxy itself (hyper-proxy calls self.connector.call(proxy_url)). Its per-connection callback pins the X509 verify parameter to the destination tls.server_name. For an HTTPS proxy that means the proxy's own certificate gets verified against the upstream server_name, failing with a hostname mismatch.

  • apply_connect_configuration gains a skip_server_name flag; when set it behaves as if no server_name were configured (verification falls back to the connection URL host).
  • The connector callback compares the target host against the configured proxy hosts and skips the server_name override when dialing a proxy.
  • build_proxy_connector collects the proxy hosts (when enabled) and passes them to the connector; build_tls_connector passes an empty list so direct callers are unaffected.

Result:

  • Direct / no_proxy destination -> server_name applied (verified against server_name).
  • HTTPS proxy connection -> server_name skipped (proxy cert verified against the proxy host).
  • Tunneled destination TLS through a proxy -> unchanged; server_name still can't be applied there (hyper-proxy limitation), which the existing warn! covers.

Test plan

  • cargo test -p vector-core --lib tls::settings::test — the end-to-end handshake test now also asserts that with the override skipped, connecting by IP with server_name set fails (i.e. verifies against the URL host, as a proxy connection would).
  • cargo check --lib for the vector crate.

Change Type

  • Bug fix
  • New feature
  • Dependencies
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

Does this PR include user facing changes?

  • Yes. A changelog fragment is included. (I actually updated the previous fragment)
  • No. A maintainer will apply the no-changelog label to this PR.

The inner connector's callback runs for both direct destination connections and
the TLS connection to an HTTPS forward proxy. Pinning the verify parameter to the
destination `server_name` on the proxy connection would verify the proxy's own
certificate against the upstream name and fail with a hostname mismatch. Exclude
the configured proxy hosts from the `server_name` override so proxy certificates
are verified against the proxy host.
@github-actions github-actions Bot added the domain: core Anything related to core crates i.e. vector-core, core-common, etc label Jul 20, 2026
@datadog-vectordotdev

datadog-vectordotdev Bot commented Jul 20, 2026

Copy link
Copy Markdown

Pipelines  Tests

⚠️ Warnings

🚦 1 Pipeline job failed

Changelog | validate-changelog   View in Datadog   GitHub Actions

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 939d1cd | Docs | Give us feedback!

There are only ever two proxy hosts (http and https), so a small struct with a
matches() helper is clearer than a Vec.
@gwenaskell

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e15eba97d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/http.rs Outdated
…r_name skip

The previous host-only check could skip the server_name override for a direct
destination connection that merely shared a host with a configured proxy (e.g. a
no_proxy bypass on a different port, or an https destination sharing a host with
an http-scheme proxy). Only track proxies reached over TLS (https:// URLs) and
match the full authority (host and port) so the skip applies solely to the actual
proxy connection.
@gwenaskell

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e818acba96

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/http.rs
@gwenaskell
gwenaskell marked this pull request as ready for review July 20, 2026 09:57
@gwenaskell
gwenaskell requested a review from a team as a code owner July 20, 2026 09:57
Comment thread changelog.d/tls_server_name_openssl_verify.fix.md Outdated
Comment thread src/http.rs Outdated

@vladimir-dd vladimir-dd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some minor suggestions, otherwise makes sense to me.

@gwenaskell
gwenaskell requested a review from guilload July 20, 2026 11:47

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 939d1cdb2c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/http.rs
@pront pront added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Jul 20, 2026

@pront pront left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change makes sense to me. I left one quesiton.

Comment thread src/http.rs
@gwenaskell
gwenaskell enabled auto-merge July 21, 2026 14:13
@gwenaskell
gwenaskell added this pull request to the merge queue Jul 21, 2026
Any commits made after this event will not be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: core Anything related to core crates i.e. vector-core, core-common, etc no-changelog Changes in this PR do not need user-facing explanations in the release changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants