enhancement(tls settings): support of SNI when connecting to remote server #21365

anil-db · 2024-09-27T00:47:41Z

we have a usecase where server is behind a reverse proxy and it selects the right backend based on SNI provided.

This PR adds options to set SNI which sending to other IP/domain.

test configuration:

{
    "sinks": {
	    "grpc_aggregator": {
            "address": "https://127.0.0.1:443",
            "inputs": [
                "http"
            ],
            "tls": {
	        "ca_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/testCA.pem",
                "crt_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostcrt.pem",
                "key_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostkey.pem",	    
                "enabled": true,
                "verify_certificate": true,
                "verify_hostname": false,
		"server_name": "www.example.com"
            },
            "type": "vector"
        },
	    "aggregator": {
            "uri": "https://127.0.0.1:444",
	    "encoding": {
                  "codec": "json"             },
            "inputs": [
                "http"
            ],
            "tls": {
                "ca_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/testCA.pem",
                "crt_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostcrt.pem",
                "key_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostkey.pem",
                "verify_certificate": true,
                "verify_hostname": false,
		"server_name": "www.example.com"
            },
            "type": "http"
        },
	"print": {
            "encoding": {
                "codec": "json"
            },
            "inputs": [
                "http2",
		"agent"
            ],
            "type": "console"
        }
    },
    "sources": {
	"agent": {
            "address": "0.0.0.0:443",
            "tls": {
                "alpn_protocols": [
                    "h2"
                ],
		"ca_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/testCA.pem",
                "crt_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostcrt.pem",
                "enabled": true,
                "key_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostkey.pem",
                "verify_certificate": true,
                "verify_hostname": false
            },
            "type": "vector"
        },
        "http": {
            "address": "0.0.0.0:6000",
            "framing": {
                "method": "bytes"
            },
            "headers": [

            ],
            "keepalive": {
                "max_connection_age_secs": 300
            },
            "strict_path": false,
            "tls": {
                "alpn_protocols": [
                    "h2"
                ],
                "ca_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/testCA.pem",
                "crt_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostcrt.pem",
                "enabled": true,
                "key_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostkey.pem",
                "verify_certificate": false
            },
            "type": "http_server"
        },
	"http2": {
            "address": "0.0.0.0:444",
            "framing": {
                "method": "bytes"
            },
            "headers": [

            ],
            "keepalive": {
                "max_connection_age_secs": 300
            },
            "strict_path": false,
            "tls": {
                "alpn_protocols": [
                    "h2"
                ],
                "ca_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/testCA.pem",
                "crt_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostcrt.pem",
                "enabled": true,
                "key_file": "/Users/anil.gupta/universe/woodchuck/regtests/resources/localhostkey.pem",
                "verify_certificate": false
            },
            "type": "http_server"
        }
    }
}

sent log data to http source using curl -k "https://localhost:6000" -d "hi"
and captured client hello frame from wrieshark on triggered by http sink and other triggered by vector sink

jszwedko

Thanks for this enhancement @anil-db ! I left one comment inline. Could you also:

Add a changelog entry per https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md
Run make generate-component-docs to regerenate the docs to add this new option?

lib/vector-core/src/tls/settings.rs

anil-db · 2024-09-27T20:53:45Z

Thanks for this enhancement @anil-db ! I left one comment inline. Could you also:

Add a changelog entry per https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md

Run make generate-component-docs to regerenate the docs to add this new option?

Added.

jszwedko

Awesome, thank you! Just a few more minor comments.

.ruby-version

lib/vector-core/src/tls/mod.rs

jszwedko · 2024-09-27T21:02:32Z

src/http.rs

+                        .unwrap()
+                        .downcast_ref::<openssl::error::ErrorStack>()
+                        .unwrap();


Rather than unwrap could we use expect? It's not immediately clear to me why these unwraps are safe and I think expect would make that clearer since you can provide a reason.

changed it to expect and provided reason why it is expected.

jszwedko · 2024-10-03T17:06:33Z

src/http.rs

+                Err(error) => {
+                    error
+                        .source()
+                        .expect("was expecting to have a source in tlsError variant returned from apply_connect_configuration(). See SetSniSnafu.")
+                        .downcast_ref::<openssl::error::ErrorStack>()
+                        .expect("was expecting the source error to be of type openssl::error::ErrorStack from apply_connect_configuration(). See SetSniSnafu.");
+                }
+            }


Thanks for adding the expects. I'm not a huge fan of asserting that a specific enum variant from a Snafu error was returned. I tried to refactor this but wasn't able to push to this branch. Could you apply this patch?

9a5fabeb0e - (HEAD -> anil-db/support-sni) Refactor to get rid of `expect`s (23 seconds ago) <Jesse Szwedko> diff --git a/lib/vector-core/src/tls/mod.rs b/lib/vector-core/src/tls/mod.rs index 96b9253f4f..9e065cb212 100644 --- a/lib/vector-core/src/tls/mod.rs +++ b/lib/vector-core/src/tls/mod.rs @@ -191,7 +191,9 @@ fn tls_connector(settings: &MaybeTlsSettings) -> Result<ConnectConfiguration> { .context(TlsBuildConnectorSnafu)?; let tls_setting = settings.tls().cloned(); if let Some(tls_setting) = &tls_setting { - tls_setting.apply_connect_configuration(&mut configure)?; + tls_setting + .apply_connect_configuration(&mut configure) + .context(SetSniSnafu)?; } Ok(configure) } diff --git a/lib/vector-core/src/tls/settings.rs b/lib/vector-core/src/tls/settings.rs index 1e68563011..1dcdc932b8 100644 --- a/lib/vector-core/src/tls/settings.rs +++ b/lib/vector-core/src/tls/settings.rs @@ -20,8 +20,8 @@ use super::{ AddCertToStoreSnafu, AddExtraChainCertSnafu, CaStackPushSnafu, DerExportSnafu, EncodeAlpnProtocolsSnafu, FileOpenFailedSnafu, FileReadFailedSnafu, MaybeTls, NewCaStackSnafu, NewStoreBuilderSnafu, ParsePkcs12Snafu, Pkcs12Snafu, PrivateKeyParseSnafu, Result, - SetAlpnProtocolsSnafu, SetCertificateSnafu, SetPrivateKeySnafu, SetSniSnafu, - SetVerifyCertSnafu, TlsError, TlsIdentitySnafu, X509ParseSnafu, + SetAlpnProtocolsSnafu, SetCertificateSnafu, SetPrivateKeySnafu, SetVerifyCertSnafu, TlsError, + TlsIdentitySnafu, X509ParseSnafu, }; pub const PEM_START_MARKER: &str = "-----BEGIN "; @@ -343,12 +343,15 @@ impl TlsSettings { Ok(()) } - pub fn apply_connect_configuration(&self, connection: &mut ConnectConfiguration) -> Result<()> { + pub fn apply_connect_configuration( + &self, + connection: &mut ConnectConfiguration, + ) -> std::result::Result<(), openssl::error::ErrorStack> { connection.set_verify_hostname(self.verify_hostname); if let Some(server_name) = &self.server_name { // Prevent native TLS lib from inferring default SNI using domain name from url. connection.set_use_server_name_indication(false); - connection.set_hostname(server_name).context(SetSniSnafu)?; + connection.set_hostname(server_name)?; } Ok(()) } diff --git a/src/http.rs b/src/http.rs index 9c30bdea49..43bd10eb88 100644 --- a/src/http.rs +++ b/src/http.rs @@ -14,7 +14,6 @@ use hyper_openssl::HttpsConnector; use hyper_proxy::ProxyConnector; use rand::Rng; use serde_with::serde_as; -use snafu::Error; use snafu::{ResultExt, Snafu}; use std::{ fmt, @@ -205,19 +204,10 @@ pub fn build_tls_connector( let settings = tls_settings.tls().cloned(); https.set_callback(move |c, _uri| { if let Some(settings) = &settings { - match settings.apply_connect_configuration(c) { - Ok(()) => (), - Err(error) => { - error - .source() - .expect("was expecting to have a source in tlsError variant returned from apply_connect_configuration(). See SetSniSnafu.") - .downcast_ref::<openssl::error::ErrorStack>() - .expect("was expecting the source error to be of type openssl::error::ErrorStack from apply_connect_configuration(). See SetSniSnafu."); - } - } + settings.apply_connect_configuration(c) + } else { + Ok(()) } - - Ok(()) }); Ok(https) }

This makes more sense. applied.

jszwedko

Thanks @anil-db !

github-actions · 2024-10-04T01:37:54Z

Regression Detector Results

Run ID: e53da310-03fb-46fa-9bb0-1e1baf64ade0 Metrics dashboard

Baseline: 077dca8
Comparison: e11b155

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI	links
✅	file_to_blackhole	egress throughput	+7.42	[+0.27, +14.57]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
✅	file_to_blackhole	egress throughput	+7.42	[+0.27, +14.57]
➖	splunk_hec_route_s3	ingress throughput	+3.04	[+2.72, +3.36]
➖	syslog_regex_logs2metric_ddmetrics	ingress throughput	+0.99	[+0.84, +1.15]
➖	http_elasticsearch	ingress throughput	+0.38	[+0.22, +0.53]
➖	http_to_http_noack	ingress throughput	+0.13	[+0.06, +0.21]
➖	http_to_http_json	ingress throughput	+0.04	[-0.00, +0.09]
➖	splunk_hec_indexer_ack_blackhole	ingress throughput	+0.01	[-0.07, +0.09]
➖	splunk_hec_to_splunk_hec_logs_noack	ingress throughput	-0.00	[-0.09, +0.09]
➖	splunk_hec_to_splunk_hec_logs_acks	ingress throughput	-0.01	[-0.12, +0.10]
➖	datadog_agent_remap_blackhole	ingress throughput	-0.20	[-0.30, -0.11]
➖	syslog_loki	ingress throughput	-0.23	[-0.32, -0.15]
➖	syslog_log2metric_humio_metrics	ingress throughput	-0.28	[-0.40, -0.16]
➖	http_to_s3	ingress throughput	-0.45	[-0.72, -0.18]
➖	syslog_splunk_hec_logs	ingress throughput	-0.50	[-0.61, -0.40]
➖	fluent_elasticsearch	ingress throughput	-0.54	[-1.03, -0.04]
➖	http_to_http_acks	ingress throughput	-0.55	[-1.77, +0.66]
➖	otlp_http_to_blackhole	ingress throughput	-0.81	[-0.92, -0.69]
➖	syslog_humio_logs	ingress throughput	-0.82	[-0.93, -0.72]
➖	datadog_agent_remap_datadog_logs_acks	ingress throughput	-1.01	[-1.19, -0.82]
➖	syslog_log2metric_splunk_hec_metrics	ingress throughput	-1.14	[-1.28, -1.01]
➖	http_text_to_http_json	ingress throughput	-1.37	[-1.49, -1.25]
➖	otlp_grpc_to_blackhole	ingress throughput	-1.77	[-1.88, -1.66]
➖	datadog_agent_remap_datadog_logs	ingress throughput	-1.86	[-2.09, -1.63]
➖	syslog_log2metric_tag_cardinality_limit_blackhole	ingress throughput	-2.07	[-2.16, -1.99]
➖	socket_to_socket_blackhole	ingress throughput	-2.68	[-2.77, -2.60]
➖	datadog_agent_remap_blackhole_acks	ingress throughput	-3.71	[-3.82, -3.61]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

github-actions · 2024-10-04T02:39:00Z

Regression Detector Results

Run ID: 4d6d09ff-85a2-44a9-a747-f96bd869c0ab Metrics dashboard

Baseline: 04d21fb
Comparison: 4588cec

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI	links
➖	file_to_blackhole	egress throughput	-3.84	[-10.50, +2.82]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	syslog_humio_logs	ingress throughput	+4.34	[+4.22, +4.45]
➖	syslog_log2metric_splunk_hec_metrics	ingress throughput	+1.00	[+0.91, +1.10]
➖	syslog_regex_logs2metric_ddmetrics	ingress throughput	+0.61	[+0.48, +0.74]
➖	otlp_http_to_blackhole	ingress throughput	+0.29	[+0.17, +0.41]
➖	http_to_s3	ingress throughput	+0.14	[-0.13, +0.41]
➖	http_to_http_noack	ingress throughput	+0.13	[+0.06, +0.21]
➖	http_to_http_json	ingress throughput	+0.03	[-0.01, +0.07]
➖	splunk_hec_indexer_ack_blackhole	ingress throughput	+0.03	[-0.05, +0.11]
➖	splunk_hec_to_splunk_hec_logs_noack	ingress throughput	+0.01	[-0.08, +0.11]
➖	splunk_hec_to_splunk_hec_logs_acks	ingress throughput	+0.00	[-0.12, +0.12]
➖	http_to_http_acks	ingress throughput	-0.05	[-1.26, +1.17]
➖	http_elasticsearch	ingress throughput	-0.24	[-0.39, -0.08]
➖	datadog_agent_remap_datadog_logs	ingress throughput	-0.31	[-0.50, -0.13]
➖	syslog_loki	ingress throughput	-0.36	[-0.44, -0.27]
➖	syslog_log2metric_tag_cardinality_limit_blackhole	ingress throughput	-0.43	[-0.52, -0.35]
➖	fluent_elasticsearch	ingress throughput	-0.71	[-1.20, -0.22]
➖	otlp_grpc_to_blackhole	ingress throughput	-0.78	[-0.89, -0.66]
➖	syslog_log2metric_humio_metrics	ingress throughput	-1.26	[-1.38, -1.14]
➖	datadog_agent_remap_blackhole	ingress throughput	-1.31	[-1.42, -1.21]
➖	http_text_to_http_json	ingress throughput	-1.38	[-1.48, -1.27]
➖	splunk_hec_route_s3	ingress throughput	-1.59	[-1.88, -1.30]
➖	datadog_agent_remap_blackhole_acks	ingress throughput	-1.81	[-1.92, -1.70]
➖	syslog_splunk_hec_logs	ingress throughput	-1.87	[-1.97, -1.78]
➖	datadog_agent_remap_datadog_logs_acks	ingress throughput	-1.92	[-2.10, -1.74]
➖	socket_to_socket_blackhole	ingress throughput	-2.65	[-2.70, -2.60]
➖	file_to_blackhole	egress throughput	-3.84	[-10.50, +2.82]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

anil-db requested a review from a team as a code owner September 27, 2024 00:47

github-actions bot added the domain: core Anything related to core crates i.e. vector-core, core-common, etc label Sep 27, 2024

anil-db force-pushed the anil-db/support-sni branch 2 times, most recently from 8dd1a5a to 06b56a0 Compare September 27, 2024 05:38

anil-db mentioned this pull request Sep 27, 2024

Anil db/v0.39 databricks/vector#39

Merged

support of SNI when connecting to remote server

0a73680

anil-db force-pushed the anil-db/support-sni branch from 06b56a0 to 0a73680 Compare September 27, 2024 15:45

jszwedko reviewed Sep 27, 2024

View reviewed changes

lib/vector-core/src/tls/settings.rs Outdated Show resolved Hide resolved

anil-db added 2 commits September 27, 2024 10:24

incorporate comments

910f228

bubble up error

1ac3dd8

anil-db requested review from a team as code owners September 27, 2024 20:52

github-actions bot added the domain: external docs Anything related to Vector's external, public documentation label Sep 27, 2024

anil-db requested a review from jszwedko September 27, 2024 20:54

jszwedko reviewed Sep 27, 2024

View reviewed changes

brett0000FF approved these changes Sep 27, 2024

View reviewed changes

change unwrap to expect and other comment incorporation

b8e49db

anil-db requested a review from jszwedko September 27, 2024 22:29

jszwedko reviewed Oct 3, 2024

View reviewed changes

refactor to remove expect

ed0cf4f

anil-db requested a review from jszwedko October 3, 2024 20:18

jszwedko approved these changes Oct 3, 2024

View reviewed changes

jszwedko enabled auto-merge October 3, 2024 20:34

fix change log file

3170c70

auto-merge was automatically disabled October 3, 2024 21:07
Head branch was pushed to by a user without write access

jszwedko enabled auto-merge October 3, 2024 21:09

jszwedko added this pull request to the merge queue Oct 3, 2024

anil-db mentioned this pull request Oct 4, 2024

add error handling. databricks/vector#40

Merged

Merged via the queue into vectordotdev:master with commit 4588cec Oct 4, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement(tls settings): support of SNI when connecting to remote server #21365

enhancement(tls settings): support of SNI when connecting to remote server #21365

anil-db commented Sep 27, 2024 •

edited

Loading

jszwedko left a comment

anil-db commented Sep 27, 2024

jszwedko left a comment

jszwedko Sep 27, 2024

anil-db Sep 27, 2024

jszwedko Oct 3, 2024

anil-db Oct 3, 2024

jszwedko left a comment

github-actions bot commented Oct 4, 2024

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

github-actions bot commented Oct 4, 2024

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

enhancement(tls settings): support of SNI when connecting to remote server #21365

enhancement(tls settings): support of SNI when connecting to remote server #21365

Conversation

anil-db commented Sep 27, 2024 • edited Loading

jszwedko left a comment

Choose a reason for hiding this comment

anil-db commented Sep 27, 2024

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko Sep 27, 2024

Choose a reason for hiding this comment

anil-db Sep 27, 2024

Choose a reason for hiding this comment

jszwedko Oct 3, 2024

Choose a reason for hiding this comment

anil-db Oct 3, 2024

Choose a reason for hiding this comment

jszwedko left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 4, 2024

Regression Detector Results

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

github-actions bot commented Oct 4, 2024

Regression Detector Results

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

anil-db commented Sep 27, 2024 •

edited

Loading