Fix SNI support with `tls_use_host_header` #15384

ian28223 · 2023-07-26T12:14:41Z

What does this PR do?

Uses the Host header for SNI. This is a variation of requests_toolbelt.adapters.host_header_ssl that includes an SNI fix requests/toolbelt#293

Motivation

Additional Notes

I think we can drop requests_toolbelt library and just use below but I'm not sure how to handle licenses involved.

from requests.adapters import HTTPAdapter

class HostHeaderSSLSNIAdapter(HTTPAdapter):

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
PR title must be written as a CHANGELOG entry (see why)
Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
PR must have changelog/ and integration/ labels attached
If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

codecov · 2023-07-26T12:18:41Z

Codecov Report

Merging #15384 (50ccdfb) into master (b7b6b2e) will decrease coverage by 1.00%.
Report is 570 commits behind head on master.
The diff coverage is n/a.

Flag	Coverage Δ
active_directory	`100.00% <ø> (+17.64%)`	⬆️
activemq_xml	`82.31% <ø> (ø)`
amazon_msk	`89.07% <ø> (ø)`
ambari	`85.75% <ø> (ø)`
apache	`95.08% <ø> (ø)`
arangodb	`98.23% <ø> (ø)`
argocd	`88.04% <ø> (ø)`
aspdotnet	`100.00% <ø> (ø)`
avi_vantage	`91.35% <ø> (ø)`
azure_iot_edge	`82.08% <ø> (ø)`
boundary	`100.00% <ø> (ø)`
btrfs	`82.91% <ø> (ø)`
cacti	`87.90% <ø> (ø)`
calico	`84.61% <ø> (+0.19%)`	⬆️
cert_manager	`77.41% <ø> (ø)`
cisco_aci	`95.27% <ø> (ø)`
citrix_hypervisor	`87.50% <ø> (ø)`
cloud_foundry_api	`96.35% <ø> (+0.12%)`	⬆️
cloudera	`99.49% <ø> (+<0.01%)`	⬆️
cockroachdb	`91.52% <ø> (ø)`
consul	`91.65% <ø> (ø)`
coredns	`94.57% <ø> (ø)`
crio	`89.79% <ø> (ø)`
datadog_checks_dev	`82.60% <ø> (+0.04%)`	⬆️
datadog_checks_downloader	`81.65% <ø> (ø)`
datadog_cluster_agent	`90.19% <ø> (ø)`
dcgm	`97.33% <ø> (ø)`
ddev	`99.36% <ø> (+0.10%)`	⬆️
directory	`95.87% <ø> (+0.43%)`	⬆️
disk	`89.23% <ø> (-2.14%)`	⬇️
dns_check	`93.90% <ø> (ø)`
dotnetclr	`91.39% <ø> (+12.90%)`	⬆️
druid	`98.47% <ø> (ø)`
ecs_fargate	`82.91% <ø> (-0.16%)`	⬇️
eks_fargate	`94.05% <ø> (ø)`
envoy	`95.30% <ø> (+0.62%)`	⬆️
etcd	`95.56% <ø> (ø)`
external_dns	`89.28% <ø> (ø)`
fluentd	`94.77% <ø> (ø)`
foundationdb	`78.50% <ø> (ø)`
gitlab	`92.10% <ø> (+1.21%)`	⬆️
go_expvar	`92.73% <ø> (ø)`
gunicorn	`92.10% <ø> (-0.76%)`	⬇️
harbor	`80.04% <ø> (ø)`
hazelcast	`92.39% <ø> (ø)`
hdfs_datanode	`89.74% <ø> (ø)`
hdfs_namenode	`86.72% <ø> (ø)`
http_check	`96.09% <ø> (+2.15%)`	⬆️
ibm_ace	`91.79% <ø> (ø)`
ibm_i	`81.91% <ø> (ø)`
impala	`97.97% <ø> (ø)`
istio	`77.43% <ø> (+0.55%)`	⬆️
kube_apiserver_metrics	`97.85% <ø> (ø)`
kube_controller_manager	`96.00% <ø> (ø)`
kube_dns	`95.97% <ø> (ø)`
kube_metrics_server	`94.87% <ø> (ø)`
kube_proxy	`96.80% <ø> (ø)`
kube_scheduler	`96.53% <ø> (ø)`
kubelet	`91.00% <ø> (+0.01%)`	⬆️
kubernetes_state	`89.18% <ø> (ø)`
kyototycoon	`85.96% <ø> (ø)`
lighttpd	`83.64% <ø> (ø)`
linkerd	`85.14% <ø> (+1.14%)`	⬆️
linux_proc_extras	`96.22% <ø> (ø)`
mapr	`82.70% <ø> (ø)`
mapreduce	`81.81% <ø> (ø)`
marathon	`83.43% <ø> (ø)`
marklogic	`96.51% <ø> (+0.04%)`	⬆️
mcache	`93.50% <ø> (ø)`
mesos_master	`89.75% <ø> (ø)`
mesos_slave	`93.63% <ø> (ø)`
mysql	`87.25% <ø> (-0.07%)`	⬇️
nagios	`89.01% <ø> (ø)`
network	`93.63% <ø> (+0.73%)`	⬆️
nfsstat	`95.20% <ø> (ø)`
nginx	`95.24% <ø> (+0.54%)`	⬆️
nginx_ingress_controller	`98.36% <ø> (ø)`
openldap	`96.79% <ø> (ø)`
openmetrics	`98.08% <ø> (ø)`
openstack	`51.45% <ø> (ø)`
openstack_controller	`91.12% <ø> (ø)`
pgbouncer	`91.33% <ø> (ø)`
php_fpm	`90.25% <ø> (+0.84%)`	⬆️
postfix	`88.04% <ø> (ø)`
powerdns_recursor	`96.65% <ø> (ø)`
process	`85.42% <ø> (ø)`
prometheus	`94.17% <ø> (ø)`
proxysql	`98.97% <ø> (ø)`
pulsar	`100.00% <ø> (ø)`
rabbitmq	`96.04% <ø> (ø)`
rethinkdb	`97.93% <ø> (ø)`
riak	`99.22% <ø> (ø)`
sap_hana	`91.64% <ø> (ø)`
silk	`93.82% <ø> (ø)`
singlestore	`90.81% <ø> (ø)`
snmp	`51.35% <ø> (-7.34%)`	⬇️
snowflake	`96.61% <ø> (ø)`
sqlserver	`86.65% <ø> (ø)`
squid	`100.00% <ø> (ø)`
statsd	`87.36% <ø> (+1.05%)`	⬆️
strimzi	`89.70% <ø> (ø)`
supervisord	`90.14% <ø> (-2.55%)`	⬇️
system_core	`90.90% <ø> (ø)`
system_swap	`98.30% <ø> (ø)`
tcp_check	`91.58% <ø> (-1.35%)`	⬇️
teamcity	`88.74% <ø> (+3.21%)`	⬆️
temporal	`100.00% <ø> (ø)`
teradata	`94.06% <ø> (ø)`
tls	`92.18% <ø> (+0.82%)`	⬆️
tokumx	`58.40% <ø> (ø)`
torchserve	`97.33% <ø> (ø)`
traffic_server	`96.13% <ø> (ø)`
twemproxy	`79.45% <ø> (ø)`
twistlock	`79.62% <ø> (ø)`
varnish	`84.39% <ø> (+0.26%)`	⬆️
weaviate	`76.27% <ø> (ø)`
win32_event_log	`86.40% <ø> (+0.27%)`	⬆️
windows_performance_counters	`98.36% <ø> (ø)`
wmi_check	`92.91% <ø> (ø)`
yarn	`89.50% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

github-actions · 2023-07-26T13:16:54Z

Test Results

    994 files     994 suites 7h 43m 13s ⏱️
  5 828 tests   5 760 ✔️     68 💤 0 ❌
24 543 runs 20 278 ✔️ 4 265 💤 0 ❌

Results for commit 50ccdfb.

♻️ This comment has been updated with latest results.

iliakur

n00b question: where and how is this used? How are we testing this fix?

ian28223 · 2023-08-08T06:52:03Z

n00b question: where and how is this used? How are we testing this fix?

This is essentially like ssl_server_name used by sock.connect but for the RequestsWrapper. Used on endpoints that require SNI.

To test, we need something like below config to report http.can_connect OK. Although, I'm not quite sure create the test for it.

  - name: SNI
    tls_verify: true
    url: https://fallback.badssl.com/
    ssl_server_name: sha256.badssl.com
    tls_use_host_header: true
    headers:
      Host: sha256.badssl.com

iliakur

Thanks for the explanation. I'm still a bit unclear why we have to vendor this class instead of upgrading the dependency.

License

According to this we can't have Apache v2 licensed code inside our BSD3-licensed codebase. I'm double-checking this though.

Tests

If we really want to include this in our code, I think we need a test.

The docstring for the class suggests a test case:

Make a request to an IP address with the domain as Host header
Observe that we're not failing.

You can take inspiration from these tests.

ian28223 · 2023-10-19T13:47:43Z

Thanks for the explanation. I'm still a bit unclear why we have to vendor this class instead of upgrading the dependency.

Oh, seems they fixed it upstream and merged in August 2023 (after 3 years). It would be better indeed to update the dependency when it gets released (but not sure when).

iliakur · 2023-10-19T14:11:01Z

Thanks for the explanation. I'm still a bit unclear why we have to vendor this class instead of upgrading the dependency.

Oh, seems they fixed it upstream and merged in August 2023 (after 3 years). It would be better indeed to update the dependency when it gets released (but not sure when).

haha, I didn't look at the timestamps on the PR, just saw that it was merged 🙈

You wanna ping them about cutting a new release? I could also do it, but probably tmr.

iliakur · 2023-10-24T11:42:29Z

@ian28223 do you mind if I close this PR?

ian28223 · 2023-10-24T12:38:39Z

fixed upstream requests/toolbelt#293. waiting for new release

Support SNI

21947cd

ian28223 requested a review from a team as a code owner July 26, 2023 12:14

ghost added the base_package label Jul 26, 2023

ian28223 changed the title ~~Support SNI~~ Fix SNI support with tls_use_host_header Jul 26, 2023

ian28223 added the changelog/Fixed label Jul 26, 2023

iliakur reviewed Aug 4, 2023

View reviewed changes

Use HostHeaderSSLSNIAdapter

50ccdfb

ofek removed the changelog/Fixed label Sep 26, 2023

ian28223 requested a review from iliakur October 17, 2023 05:57

iliakur requested changes Oct 19, 2023

View reviewed changes

ian28223 closed this Oct 24, 2023

dd-devflow bot deleted the ian.bucad/requests_sni branch February 7, 2024 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SNI support with `tls_use_host_header` #15384

Fix SNI support with `tls_use_host_header` #15384

ian28223 commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023 •

edited

Loading

iliakur left a comment

ian28223 commented Aug 8, 2023

iliakur left a comment

ian28223 commented Oct 19, 2023

iliakur commented Oct 19, 2023

iliakur commented Oct 24, 2023

ian28223 commented Oct 24, 2023

Fix SNI support with tls_use_host_header #15384

Fix SNI support with tls_use_host_header #15384

Conversation

ian28223 commented Jul 26, 2023 • edited Loading

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

codecov bot commented Jul 26, 2023 • edited Loading

Codecov Report

github-actions bot commented Jul 26, 2023 • edited Loading

Test Results

iliakur left a comment

Choose a reason for hiding this comment

ian28223 commented Aug 8, 2023

iliakur left a comment

Choose a reason for hiding this comment

License

Tests

ian28223 commented Oct 19, 2023

iliakur commented Oct 19, 2023

iliakur commented Oct 24, 2023

ian28223 commented Oct 24, 2023

Fix SNI support with `tls_use_host_header` #15384

Fix SNI support with `tls_use_host_header` #15384

ian28223 commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023 •

edited

Loading