SNI-based cert selection during TLS handshake #22036

LuyaoZhong · 2022-07-06T10:48:11Z

Envoy supports selecting certs by selecting filter chain based on SNI.
But it is possible that we access different services via one filter
chain, which requires SNI-based cert selection in one single filter
chain during handshake.

Risk Level: Medium
Testing: unit tests
Docs Changes: ssl
Release Notes: yes
Fixes #21739

Signed-off-by: Luyao Zhong [email protected]

ggreenway

/wait

docs/root/intro/arch_overview/security/ssl.rst

source/extensions/transport_sockets/tls/context_impl.cc

ggreenway

/wait

docs/root/intro/arch_overview/security/ssl.rst

source/extensions/transport_sockets/tls/context_impl.cc

source/extensions/transport_sockets/tls/context_impl.h

source/extensions/transport_sockets/tls/utility.cc

LuyaoZhong · 2022-07-22T14:07:42Z

@ggreenway Do we need to add release notes for this?

ggreenway · 2022-07-27T01:02:46Z

I've been busy and haven't had time to review this yet; thanks for your patience.

LuyaoZhong · 2022-07-27T01:36:39Z

I've been busy and haven't had time to review this yet; thanks for your patience.

It's ok. :) Thanks for your response.

repokitteh-read-only · 2022-08-01T06:51:03Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #22036 was synchronize by LuyaoZhong.

see: more, trace.

LuyaoZhong · 2022-08-01T12:20:40Z

/retest

repokitteh-read-only · 2022-08-01T12:20:44Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #22036 (comment) was created by @LuyaoZhong.

see: more, trace.

alyssawilk · 2022-08-01T14:07:34Z

@ggreenway would you be able to take a look today?

ggreenway

/wait

source/extensions/transport_sockets/tls/context_impl.cc

source/extensions/transport_sockets/tls/context_impl.h

source/extensions/transport_sockets/tls/context_impl.cc

ggreenway

Please add more test coverage:

Multiple certs with multiple SANs; matching works correctly
CN is not used if SANs are present
Wildcard in SANs is matched properly
Wildcard does not match if an exact match is present
Wildcard only matches 1 level, eg *.example.com does not match a.b.example.com
Config fails to load with conflicting SANs in certs, both exact and wildcard
Any other new behavior you've added in this PR that isn't tested.

/wait

repokitteh-read-only · 2022-11-29T15:49:19Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #22036 (comment) was created by @LuyaoZhong.

see: more, trace.

mattklein123 · 2022-11-29T16:49:42Z

Please fix DCO (you may need to squash the entire thing and force push). Then we can do a final pass. Thanks.

/wait

Envoy supports selecting certs by selecting filter chain based on SNI. BUt it is possible that we access different services via one filter chain, which requires SNI-based cert selection in one single filter chain during handshake. Signed-off-by: Luyao Zhong <[email protected]>

LuyaoZhong · 2022-11-30T06:30:31Z

/retest

repokitteh-read-only · 2022-11-30T06:30:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #22036 (comment) was created by @LuyaoZhong.

see: more, trace.

LuyaoZhong · 2022-11-30T08:25:23Z

Please fix DCO (you may need to squash the entire thing and force push). Then we can do a final pass. Thanks.

/wait

Done

ggreenway

This looks good. Thanks for all the work on this!

yanavlasov · 2022-12-09T15:42:28Z

source/extensions/transport_sockets/tls/context_impl.cc

  if (config.tlsCertificates().empty() && !config.capabilities().provides_certificates) {
    throw EnvoyException("Server TlsCertificates must have a certificate specified");
  }

+  for (auto& ctx : tls_contexts_) {
+    bssl::UniquePtr<EVP_PKEY> public_key(X509_get_pubkey(ctx.cert_chain_.get()));


This crashes for cases when cert_chain is nullptr. It is missing nullptr check present in ContextImpl::getCertChainInformation()

It already gets checked in loading phase. If the cert_chain is nullptr, envoy will log and raise exception, you could check following code path

envoy/source/extensions/transport_sockets/tls/context_impl.cc

Lines 198 to 204 in 8433082

if (!tls_certificate.pkcs12().empty()) {

ctx.loadPkcs12(tls_certificate.pkcs12(), tls_certificate.pkcs12Path(),

tls_certificate.password());

} else {

ctx.loadCertificateChain(tls_certificate.certificateChain(),

tls_certificate.certificateChainPath());

}

So I don't think this will cause crash, and we can see similar code in constructor of its parent class, it checks nothing since it gets checked earlier.

envoy/source/extensions/transport_sockets/tls/context_impl.cc

Lines 215 to 216 in 8433082

bssl::UniquePtr<EVP_PKEY> public_key(X509_get_pubkey(ctx.cert_chain_.get()));

const int pkey_id = EVP_PKEY_id(public_key.get());

I saw this change was reverted by #24475 , but no enough context for me, could you elaborate the example config that it will crash?

cc @ggreenway @KBaichoo

Revert 22036. Signed-off-by: Kevin Baichoo <[email protected]>

Envoy supports selecting certs by selecting filter chain based on SNI. But it is possible that we access different services via one filter chain, which requires SNI-based cert selection in one single filter chain during handshake. This change is merged by envoyproxy#22036 and reverted by envoyproxy#24475. Signed-off-by: Luyao Zhong <[email protected]>

Envoy supports selecting certs by selecting filter chain based on SNI. But it is possible that we access different services via one filter chain, which requires SNI-based cert selection in one single filter chain during handshake. This change is merged by #22036 and reverted by #24475. Signed-off-by: Luyao Zhong <[email protected]>

LuyaoZhong requested review from lizan and ggreenway as code owners July 6, 2022 10:48

LuyaoZhong marked this pull request as draft July 7, 2022 10:12

ggreenway requested changes Jul 7, 2022

View reviewed changes

docs/root/intro/arch_overview/security/ssl.rst Outdated Show resolved Hide resolved

source/extensions/transport_sockets/tls/context_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added waiting and removed waiting labels Jul 7, 2022

ggreenway requested changes Jul 12, 2022

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jul 12, 2022

LuyaoZhong mentioned this pull request Jul 13, 2022

TLS bumping: decrypting communications between internal and external services #18928

Open

LuyaoZhong requested a review from ggreenway July 19, 2022 05:43

LuyaoZhong marked this pull request as ready for review July 21, 2022 01:55

LuyaoZhong changed the title ~~[WIP] SNI-based cert selection during TLS handshake~~ SNI-based cert selection during TLS handshake Jul 21, 2022

phlax assigned ggreenway Jul 21, 2022

repokitteh-read-only bot added the api label Aug 1, 2022

repokitteh-read-only bot assigned lizan Aug 1, 2022

ggreenway requested changes Aug 1, 2022

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Aug 1, 2022

LuyaoZhong requested review from ggreenway and zuercher August 2, 2022 11:10

ggreenway requested changes Aug 3, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 3, 2022

repokitteh-read-only bot added the waiting label Nov 29, 2022

LuyaoZhong force-pushed the sni-based-cert-selection branch from c019fef to e62edc4 Compare November 30, 2022 02:48

repokitteh-read-only bot removed the waiting label Nov 30, 2022

ggreenway approved these changes Nov 30, 2022

View reviewed changes

mattklein123 approved these changes Nov 30, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Nov 30, 2022

ggreenway merged commit 9653024 into envoyproxy:main Nov 30, 2022

LuyaoZhong mentioned this pull request Dec 1, 2022

[backport] tls: SNI-based cert selection during TLS handshake intel/envoy#59

Closed

LuyaoZhong deleted the sni-based-cert-selection branch December 1, 2022 08:48

ggreenway mentioned this pull request Dec 7, 2022

tls: allow multiple certs with the same SAN #24421

Merged

yanavlasov reviewed Dec 9, 2022

View reviewed changes

KBaichoo mentioned this pull request Dec 9, 2022

Rollback #22036 due to crash #24475

Merged

KBaichoo added a commit that referenced this pull request Dec 10, 2022

Rollback #22036 due to crash (#24475)

8011f48

Revert 22036. Signed-off-by: Kevin Baichoo <[email protected]>

LuyaoZhong mentioned this pull request Dec 11, 2022

tls: SNI-based cert selection during TLS handshake (#22036) #24483

Merged

yanavlasov mentioned this pull request Mar 6, 2023

Simpler listener configuration for multiple domains with TLS support #16174

Open

alyssawilk mentioned this pull request Aug 17, 2023

envoy_reloadable_features_no_full_scan_certs_on_sni_mismatch deprecation #29096

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNI-based cert selection during TLS handshake #22036

SNI-based cert selection during TLS handshake #22036

LuyaoZhong commented Jul 6, 2022 •

edited

Loading

ggreenway left a comment

ggreenway left a comment

LuyaoZhong commented Jul 22, 2022

ggreenway commented Jul 27, 2022

LuyaoZhong commented Jul 27, 2022

repokitteh-read-only bot commented Aug 1, 2022

LuyaoZhong commented Aug 1, 2022

repokitteh-read-only bot commented Aug 1, 2022

alyssawilk commented Aug 1, 2022

ggreenway left a comment

ggreenway left a comment

repokitteh-read-only bot commented Nov 29, 2022

mattklein123 commented Nov 29, 2022

LuyaoZhong commented Nov 30, 2022

repokitteh-read-only bot commented Nov 30, 2022

LuyaoZhong commented Nov 30, 2022

ggreenway left a comment

yanavlasov Dec 9, 2022

LuyaoZhong Dec 11, 2022 •

edited

Loading

	if (!tls_certificate.pkcs12().empty()) {
	ctx.loadPkcs12(tls_certificate.pkcs12(), tls_certificate.pkcs12Path(),
	tls_certificate.password());
	} else {
	ctx.loadCertificateChain(tls_certificate.certificateChain(),
	tls_certificate.certificateChainPath());
	}

	bssl::UniquePtr<EVP_PKEY> public_key(X509_get_pubkey(ctx.cert_chain_.get()));
	const int pkey_id = EVP_PKEY_id(public_key.get());

SNI-based cert selection during TLS handshake #22036

SNI-based cert selection during TLS handshake #22036

Conversation

LuyaoZhong commented Jul 6, 2022 • edited Loading

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

LuyaoZhong commented Jul 22, 2022

ggreenway commented Jul 27, 2022

LuyaoZhong commented Jul 27, 2022

repokitteh-read-only bot commented Aug 1, 2022

LuyaoZhong commented Aug 1, 2022

repokitteh-read-only bot commented Aug 1, 2022

alyssawilk commented Aug 1, 2022

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Nov 29, 2022

mattklein123 commented Nov 29, 2022

LuyaoZhong commented Nov 30, 2022

repokitteh-read-only bot commented Nov 30, 2022

LuyaoZhong commented Nov 30, 2022

ggreenway left a comment

Choose a reason for hiding this comment

yanavlasov Dec 9, 2022

Choose a reason for hiding this comment

LuyaoZhong Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

LuyaoZhong commented Jul 6, 2022 •

edited

Loading

LuyaoZhong Dec 11, 2022 •

edited

Loading