enhancement(tls): add configurable minimum and maximum TLS versions - #26265
enhancement(tls): add configurable minimum and maximum TLS versions#26265sainad2222 wants to merge 5 commits into
Conversation
Vector's negotiated TLS versions were fixed by the library defaults and could not be changed. `TlsSettings::acceptor` builds from `SslAcceptor::mozilla_intermediate` -- Mozilla's v4 intermediate profile -- which permits TLS v1.0 and v1.1 and explicitly sets `SSL_OP_NO_TLSv1_3`, and `tls_connector_builder` sets no minimum protocol version at all. This is the compliance problem reported in vectordotdev#11959. Add `min_tls_version` and `max_tls_version` to `TlsConfig`, accepting `TLSv1`, `TLSv1.1`, `TLSv1.2` and `TLSv1.3`. Both are unset by default, so existing configurations negotiate exactly the same versions as before. vectordotdev#17191 attempted to fix this by moving the acceptor to `mozilla_intermediate_v5` outright and was closed with the direction to make the behavior configurable instead. Enforcement lives in `apply_context_base`, which both the acceptor and the connector already funnel through, so every component that reads the `tls` block picks it up. OpenSSL treats the `SSL_OP_NO_*` options as a veto outranking `set_min_proto_version`/`set_max_proto_version`, so the option is cleared for each version inside the requested window before the bounds are applied; otherwise the acceptor's `SSL_OP_NO_TLSv1_3` would keep TLS v1.3 excluded and `min_tls_version: TLSv1.3` would yield a context with no usable version. Components that pass the certificates to a third-party TLS stack cannot honor these options, so they warn rather than ignore them silently: the `mqtt` source and sink, the `gcp_pubsub` source, and the `greptimedb_metrics` sink.
Regenerated by `make generate-component-docs` after adding `min_tls_version` and `max_tls_version` to `TlsConfig`. Every component that exposes a `tls` block gains the two options in its generated Cue reference.
github-actions
Bot
added
docs review on hold
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4172f3bada
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Review found three more components that accept `tls.min_tls_version` and `tls.max_tls_version` -- so the options appear in their generated reference -- but never apply them, because they hand the configuration to a third-party TLS implementation rather than to an OpenSSL context: - `kafka` source and sink: `KafkaAuthConfig::apply` maps only verification, CA, certificate, key and password onto librdkafka properties. - `nats` source and sink: `from_tls_auth_config` applies only `require_tls`, root certificates and the client certificate pair. - `amqp` source and sink: `AmqpConfig::connect` builds lapin's `OwnedTLSConfig` from `cert_chain` and `identity` alone. None of these expose protocol version selection through the API surface Vector uses, so the bounds cannot be forwarded. Since they are a security control, warn rather than accept them silently. Adds a shared `warn_unenforceable_protocol_versions` helper and a `TlsConfig` counterpart to the existing `TlsSettings::has_protocol_version_bounds`, and routes the previously inline `mqtt` and `gcp_pubsub` warnings through it so all six sites emit one message. Also corrects the changelog fragment. It claimed the AWS SDK-based sinks do not read the `tls` block; they do, via `aws::connector` -> `MaybeTlsSettings::tls_client` -> `build_tls_connector` -> `tls_connector_builder`, which reaches `apply_context_base` and enforces the bounds. It made the same claim about `kafka`, which reads the block and maps its other fields but cannot map these two.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f887d07660
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`set_min_proto_version(None)` compiles to `SSL_CTX_set_min_proto_version(ctx, 0)`, which does not mean "leave unchanged" -- 0 selects the lowest version OpenSSL supports, clearing any bound already in force. That includes a bound applied from the host's OpenSSL configuration (`MinProtocol` in `openssl.cnf`) when the context is created. Because both setters were called whenever either option was set, a config specifying only `max_tls_version` erased a host-configured minimum and re-enabled the very protocol versions that policy forbids. Call each setter only when Vector has an explicit bound for that side. The `SSL_OP_NO_*` handling had the same flaw: it cleared the flag for every version inside the requested window, so `min_tls_version: TLSv1.0` would relax `NO_TLSV1`/`NO_TLSV1_1` had anything else set them. Only `NO_TLSV1_3` is cleared now, and only when the window contains TLS v1.3 -- that is the one flag Vector's own acceptor profile (`SslAcceptor::mozilla_intermediate`) sets, so it is the only one there is standing to undo. Adds four tests: a pre-set bound on the opposite side survives a one-sided config (both directions), externally disabled versions stay disabled, and a window excluding TLS v1.3 leaves `NO_TLSV1_3` in place. They set the bound explicitly rather than relying on host configuration, so they hold on machines with no system-wide policy -- which is why the original tests missed this.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5be1ab6809
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Some(max) = self.max_tls_version { | ||
| context | ||
| .set_max_proto_version(Some(max.as_ssl_version())) | ||
| .context(SetTlsVersionSnafu)?; |
There was a problem hiding this comment.
On hosts whose OpenSSL configuration supplies the opposite bound, this can silently create an unusable context: for example, a system MinProtocol = TLSv1.3 combined with only max_tls_version: TLSv1.2 leaves the builder with a minimum greater than its maximum, yet both OpenSSL setters succeed and Vector starts before every handshake fails. Fresh evidence in this revision is that the setters now preserve the opposite host bound, while the earlier range validation still compares only the two Vector options; validate the builder's effective minimum and maximum after applying them.
Useful? React with 👍 / 👎.
| min_tls_version: { | ||
| description: """ | ||
| Minimum TLS protocol version to negotiate. | ||
|
|
||
| Peers that cannot negotiate at least this version are rejected during the handshake. |
There was a problem hiding this comment.
When a Databend sink sets either newly documented bound, the value is silently ignored: a repo-wide search of src/sinks/databend finds that DatabendConfig::tls is only declared and is never read by validation or build code, so this sink never constructs TlsSettings or emits the unsupported-stack warning. Although the parent field is deprecated, the generated reference now explicitly promises that these accepted suboptions constrain negotiation; hide them for this component or warn that they have no effect.
Useful? React with 👍 / 👎.
Preserving a host-supplied bound instead of clearing it means Vector's own two options can be self-consistent while the window actually in force is empty: a host `MinProtocol = TLSv1.3` combined with `max_tls_version: TLSv1.2` leaves a minimum above the maximum. Both OpenSSL setters report success, so Vector would start and then fail every handshake, with nothing in the configuration to explain why. Read the builder's effective bounds back after applying and fail when the minimum exceeds the maximum. The existing `InvalidTlsVersionRange` check only compares the two Vector options and cannot see this, so the new `EmptyTlsVersionWindow` error names the host's OpenSSL configuration as the other possible source of a bound. `SslVersion` is not ordered and does not expose its underlying value, so comparison goes through a new `TlsVersion::from_ssl_version`; versions Vector does not model are skipped rather than guessed at.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5599da78f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // only one side silently re-enable versions the host policy forbids. | ||
| if let Some(min) = self.min_tls_version { | ||
| context | ||
| .set_min_proto_version(Some(min.as_ssl_version())) |
There was a problem hiding this comment.
When the host already enforces MinProtocol = TLSv1.2, configuring min_tls_version: TLSv1 replaces that minimum and re-enables versions the host forbids; max_tls_version can similarly raise an existing host maximum. Preserving only the unconfigured side is insufficient—the configured bounds should be intersected with the context's existing bounds so Vector cannot weaken system TLS policy.
Useful? React with 👍 / 👎.
| if self.window_contains(TlsVersion::Tls13) { | ||
| context.clear_options(SslOptions::NO_TLSV1_3); |
There was a problem hiding this comment.
For outgoing connections on a host whose OpenSSL configuration sets NO_TLSV1_3, configuring a window that includes TLS 1.3 clears that host restriction because this helper is also called through apply_context(..., false). The clearing is only needed to undo Vector's mozilla_intermediate acceptor profile, so it should be restricted to server contexts rather than relaxing policy on every connector.
Useful? React with 👍 / 👎.
| if tls.has_protocol_version_bounds() { | ||
| vector_lib::tls::warn_unenforceable_protocol_versions("lapin"); |
There was a problem hiding this comment.
When an AMQP sink configures either bound, this warning runs for every call to AmqpConfig::connect; the deadpool channel manager calls that method whenever it creates or replaces a pooled channel. Pool expansion and especially repeated channel recreation during broker failures can therefore emit the identical configuration warning continuously, so it should be emitted during component build/validation or otherwise rate-limited rather than from the connection hot path.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds
min_tls_versionandmax_tls_versionto Vector's sharedtlsconfiguration block, so userscan constrain which TLS protocol versions Vector negotiates. Accepted values are
TLSv1,TLSv1.1,TLSv1.2andTLSv1.3.Today the negotiated version is fixed by the library defaults and cannot be changed:
TlsSettings::acceptorbuilds fromSslAcceptor::mozilla_intermediate,which is Mozilla's v4 intermediate profile. It permits TLS v1.0 and v1.1, and explicitly sets
SSL_OP_NO_TLSv1_3, so TLS v1.3 is unavailable. (That helper is marked// FIXME remove in next major versionin theopensslcrate.)tls_connector_builderusesSslConnector::builder, which sets nominimum protocol version at all.
This is the problem reported in #11959: compliance scanners flag Vector's listening sources for
accepting TLS v1.0/v1.1, and there is no way to turn them off.
PR #17191 previously proposed swapping the acceptor for
mozilla_intermediate_v5. It was closed withthe direction to make the behavior configurable and go through a deprecation process rather than
change it outright, which is the approach taken here: both options are unset by default, so
existing configurations negotiate exactly the same versions as before.
Implementation notes
Both the acceptor and the connector funnel through
TlsSettings::apply_context_base, soenforcement lives in one place — a new
apply_protocol_versions— and every component that readsthe
tlsblock picks it up.The subtlety is that OpenSSL treats the
SSL_OP_NO_*options as a veto that outranksSSL_CTX_set_min_proto_version/set_max_proto_version. Because the acceptor profile hard-setsSSL_OP_NO_TLSv1_3, setting the bounds alone would leave TLS v1.3 excluded, andmin_tls_version: TLSv1.3would produce a context with no usable version at all. The option istherefore cleared for every version inside the requested window before the bounds are applied. A
unit test pins this against the profile so it cannot regress silently.
An inverted range (
min_tls_versiongreater thanmax_tls_version) is rejected at config loadrather than producing an unusable context.
Components that cannot honor these options
Some components accept a
tlsblock but hand the PEM material to a third-party TLS stack insteadof applying it to an OpenSSL context, so these settings cannot take effect there. Rather than
ignore a security setting silently, they now warn when either option is set:
mqttsource and sink (rumqttc)gcp_pubsubsource (tonic)greptimedb_metricssink — folded into its existing unsupported-options warning; that sinkdestructures
TlsConfigexhaustively, so it needed updating regardless.Components that never read Vector's
tlsblock (kafkavia librdkafka, the AWS SDK-based sinks)are unaffected and unchanged.
Deliberately not included
The deprecation half of the #17191 review — warning when a connection negotiates below TLS v1.2,
and eventually defaulting
min_tls_versiontoTLSv1.2— is a behavior change with release-timingimplications, so I left it out of this PR. Happy to add it here or as a follow-up, whichever the
maintainers prefer.
References
Closes: #11959
Related: #17191
Vector configuration
The new options on any component that exposes a
tlsblock:Omitting both options preserves today's behavior exactly. Setting
min_tls_version: TLSv1.2asabove refuses TLS v1.0/v1.1 handshakes and, as a side effect of the
SSL_OP_NO_*handlingdescribed above, makes TLS v1.3 available on the listener.
How did you test this PR?
Six tests added in
lib/vector-core/src/tls/settings.rs, run withcargo test -p vector-core --lib tls::(27 passed, 0 failed):TLSv1,TLSv1.1,TLSv1.2,TLSv1.3) andrenders back to it
protocol versions are unchanged from the library defaults
SslAcceptor::mozilla_intermediate— and asserting as a precondition that theprofile really does set
SSL_OP_NO_TLSv1_3— applying a window that includes TLS v1.3 clears itmin/maxof TLS v1.2 negotiates exactlyTLSv1.2with a default client, and a client configured withmin_tls_version: TLSv1.3failsthe handshake with a protocol version alert
Also run locally:
make fmt,cargo clippy -p vector-core --all-targets(clean),vdev check generated-docs,vdev check changelog-fragments,vdev check fmt,vdev check markdown— all passing.The behavior is reproducible by hand against the config above with:
Note that some distributions set a system-wide
MinProtocolinopenssl.cnf, which can refuseTLS v1.0/v1.1 before Vector is consulted; overriding that is necessary to observe the old
permissive default.
Is this a breaking change?
Both options default to unset, which preserves the currently negotiated versions exactly. This is
covered by the no-regression test listed above.
Does this PR include user facing changes?
no-changeloglabel to this PR.Changelog fragment:
changelog.d/11959_tls_min_max_version.enhancement.md. The generated Cuereference is regenerated in a separate commit (74 files, additions only).
Contributor Guidelines
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook (template) or run the following locally before pushing:make fmtmake check-clippy(auto-fix withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.