SNI-based cert selection during TLS handshake by LuyaoZhong · Pull Request #22036 · envoyproxy/envoy

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

LuyaoZhong · 2022-11-29T15:49:09Z

/retest

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

  }

+  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 ggreenway and lizan 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

	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());

Conversation

LuyaoZhong commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LuyaoZhong commented Jul 22, 2022

Uh oh!

ggreenway commented Jul 27, 2022

Uh oh!

LuyaoZhong commented Jul 27, 2022

Uh oh!

repokitteh-read-only bot commented Aug 1, 2022

Uh oh!

LuyaoZhong commented Aug 1, 2022

Uh oh!

repokitteh-read-only bot commented Aug 1, 2022

Uh oh!

alyssawilk commented Aug 1, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

LuyaoZhong commented Nov 29, 2022

Uh oh!

repokitteh-read-only bot commented Nov 29, 2022

Uh oh!

mattklein123 commented Nov 29, 2022

Uh oh!

LuyaoZhong commented Nov 30, 2022

Uh oh!

repokitteh-read-only bot commented Nov 30, 2022

Uh oh!

LuyaoZhong commented Nov 30, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

LuyaoZhong Dec 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

LuyaoZhong commented Jul 6, 2022 •

edited

Loading

LuyaoZhong Dec 11, 2022 •

edited

Loading