quic: support cert selection by SNI, non-PEM formats #32260

ggreenway · 2024-02-07T21:29:17Z

Commit Message: Added support for QUIC listeners to choose certificates based on SNI and load certificates from formats other than PEM, such as pkcs12. This brings feature parity between quic and non-quic TLS use for certificate selection and loading formats.
Additional Description:
Risk Level: Medium
Testing: existing tests
Docs Changes: none
Release Notes: added
Platform Specific Features:
Runtime guard: envoy.restart_features.quic_handle_certs_with_shared_tls_code
[Optional Fixes #Issue]

This brings feature parity between quic and non-quic TLS use for certificate selection. Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only · 2024-02-07T21:29:22Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #32260 was opened by ggreenway.

see: more, trace.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist

Looks awesome! One small nit.

RyanTheOptimist · 2024-02-13T18:07:15Z

source/common/quic/quic_server_transport_socket_factory.cc

+  // we can reuse all the code that loads from different formats, allows using passwords on the key,
+  // etc.
+  std::string certs_str;
+  auto process_one_cert = [&](X509* cert) {


nit: Would it make sense to just make this function in the anonymous namespace?

I prefer having it in the function that uses it to make it clear the scope in which it's used, and it can capture a variable instead of needing a parameter. But it's just a small style preference. I'll change it if you insist.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

source/extensions/transport_sockets/tls/context_impl.cc

danzh2010 · 2024-02-13T19:56:13Z

source/common/quic/quic_server_transport_socket_factory.cc

+    ASSERT(result == 1);
+    BUF_MEM* buf_mem = nullptr;
+    result = BIO_get_mem_ptr(bio.get(), &buf_mem);
+    certs_str.append(buf_mem->data, buf_mem->length);


Can you directly populate a std::vector<std::string> instead of concatenating the pem string here and then calling CertificateView::LoadPemFromStream() to split them apart?

I took at stab at this in latest commit; PTAL and let me know if this is what you had in mind.

My understanding is that each string in this vector should be a pem-encoded (base64) string of a cert, without newlines or the begin/end block. Is there a boringssl API to get that directly? I didn't find a more direct way than what's in my latest commit.

source/extensions/transport_sockets/tls/context_impl.cc

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Some paths can't be hit anymore because they're caught earlier when constructing the ContextImpl, but they should still remain in the code for sanity Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2024-03-01T22:24:37Z

/retest

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist · 2024-03-05T14:57:45Z

Needs main merge again?
/wait

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010

LGTM

ggreenway · 2024-03-05T21:41:10Z

/retest

* origin: (34 commits) update CODEOWNER (envoyproxy#32457) Delete unused runtime flag. (envoyproxy#32739) mobile: Use direct ByteBuffer to pass data between C++ and Java (envoyproxy#32715) quic: support cert selection by SNI, non-PEM formats (envoyproxy#32260) mobile: Replace std::thread with Envoy::Thread::PosixThread (envoyproxy#32610) grpc: Add support for max frame length in gPRC frame decoding (envoyproxy#32511) build(deps): bump google.golang.org/protobuf from 1.32.0 to 1.33.0 (envoyproxy#32728) build(deps): bump the examples-golang-network group in /examples/golang-network/simple with 1 update (envoyproxy#32732) build(deps): bump google.golang.org/protobuf from 1.32.0 to 1.33.0 in /contrib/golang/filters/http/test/test_data/property (envoyproxy#32731) build(deps): bump otel/opentelemetry-collector from `246dfe9` to `71ac13c` in /examples/opentelemetry (envoyproxy#32730) build(deps): bump the examples-grpc-bridge group in /examples/grpc-bridge/server with 2 updates (envoyproxy#32720) build(deps): bump the contrib-golang group in /contrib/golang/router/cluster_specifier/test/test_data/simple with 1 update (envoyproxy#32721) build(deps): bump the contrib-golang group in /contrib/golang/filters/http/test/test_data/echo with 1 update (envoyproxy#32722) build(deps): bump the examples-ext-authz group in /examples/ext_authz/auth/grpc-service with 1 update (envoyproxy#32723) build(deps): bump the contrib-golang group in /contrib/golang/filters/http/test/test_data/routeconfig with 1 update (envoyproxy#32724) build(deps): bump the examples-load-reporting group in /examples/load-reporting-service with 1 update (envoyproxy#32726) build(deps): bump the contrib-golang group in /contrib/golang/filters/http/test/test_data/buffer with 1 update (envoyproxy#32727) build(deps): bump the examples-golang-http group in /examples/golang-http/simple with 1 update (envoyproxy#32729) opentelemetrytracer: Add User-Agent header to OTLP trace exporters (envoyproxy#32659) build: remove incorrect cc_library after tls code move (envoyproxy#32714) ...

Commit Message: the test failed after #32260 because the test server uses a mocked MockTransportSocketFactoryContext which doesn't return a real ContextManager object in sslContextManager(). The mocked object returns nullptr in createSslServerContext(), thus fail to initialize the new member ssl_ctx_ in QuicServerTransportSocketFactory. Additional Description: bazel test //test/java/io/envoyproxy/envoymobile/engine/testing:quic_test_server_test passes in this PR Risk Level: low, test only Testing: existing tests pass Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Dan Zhang <danzh@google.com>

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

quic: support multiple certs, selected by SNI

fd93527

This brings feature parity between quic and non-quic TLS use for certificate selection. Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway added 9 commits February 8, 2024 08:50

comments and whitespace

c6e5291

Signed-off-by: Greg Greenway <ggreenway@apple.com>

wip: try to fix integration tests

534667a

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/main' into quic-multi-cert

1a343fa

fix quic handoff hotrestart test

05d5e62

Signed-off-by: Greg Greenway <ggreenway@apple.com>

integration test fixes

34b39f6

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix tls transport socket tests

aa980d1

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix quic tests

ba6799f

Signed-off-by: Greg Greenway <ggreenway@apple.com>

re-use already loaded certs

0e105cd

Signed-off-by: Greg Greenway <ggreenway@apple.com>

changelog

e6bab00

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway marked this pull request as ready for review February 13, 2024 00:03

ggreenway requested a review from lizan as a code owner February 13, 2024 00:03

ggreenway assigned danzh2010 and RyanTheOptimist Feb 13, 2024

ggreenway changed the title ~~quic: support multiple certs, selected by SNI~~ quic: support cert selection by SNI, non-PEM formats Feb 13, 2024

ggreenway added 6 commits February 12, 2024 16:46

fix format

6d77bc1

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix spelling

176791c

Signed-off-by: Greg Greenway <ggreenway@apple.com>

remove some unneeded stuff

5db400b

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/main' into quic-multi-cert

f9b4a25

fix another test

f4ac1ae

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix test

16c8bbd

Signed-off-by: Greg Greenway <ggreenway@apple.com>

RyanTheOptimist reviewed Feb 13, 2024

View reviewed changes

fix compile-time-options

e7355bb

Signed-off-by: Greg Greenway <ggreenway@apple.com>

danzh2010 reviewed Feb 13, 2024

View reviewed changes

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

danzh2010 reviewed Feb 13, 2024

View reviewed changes

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

danzh2010 reviewed Feb 13, 2024

View reviewed changes

ggreenway added 3 commits February 14, 2024 08:46

fix asan error

fac9719

Signed-off-by: Greg Greenway <ggreenway@apple.com>

slightly lower coverage for quic

4c095a1

Some paths can't be hit anymore because they're caught earlier when constructing the ContextImpl, but they should still remain in the code for sanity Signed-off-by: Greg Greenway <ggreenway@apple.com>

create chain more directly

0849a2f

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed RyanTheOptimist’s stale review via 1d0a7b7 March 1, 2024 17:13

repokitteh-read-only bot removed the waiting label Mar 1, 2024

ggreenway added 4 commits March 4, 2024 13:11

switch from assert to exception

5c1c95d

Signed-off-by: Greg Greenway <ggreenway@apple.com>

factor out common code; use latched runtime value

92f49f5

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/main' into quic-multi-cert

53cde4d

remove conflict marker

ce2a2f8

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot added the waiting label Mar 5, 2024

Merge remote-tracking branch 'upstream/main' into quic-multi-cert

48393d1

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Mar 5, 2024

danzh2010 approved these changes Mar 5, 2024

View reviewed changes

RyanTheOptimist approved these changes Mar 6, 2024

View reviewed changes

ggreenway merged commit d82b5a0 into envoyproxy:main Mar 6, 2024
54 checks passed

danzh2010 mentioned this pull request Mar 6, 2024

mobile: fix quic_test_server_test #32755

Merged

sayboras added a commit to cilium/proxy that referenced this pull request Jul 3, 2024

cilium: Adjust cilium related changes

dbc7e45

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 3, 2024

cilium: Adjust cilium related changes

982850f

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 3, 2024

cilium: Adjust cilium related changes

5245adc

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 4, 2024

cilium: Adjust cilium related changes

10530d3

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 4, 2024

cilium: Adjust cilium related changes

68df01f

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 17, 2024

cilium: Adjust cilium related changes

a11d3a8

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 17, 2024

cilium: Adjust cilium related changes

6631f1f

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 17, 2024

cilium: Adjust cilium related changes

773d3b7

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

sayboras added a commit to cilium/proxy that referenced this pull request Jul 23, 2024

cilium: Adjust cilium related changes

f26fc7b

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

github-merge-queue bot pushed a commit to cilium/proxy that referenced this pull request Jul 23, 2024

cilium: Adjust cilium related changes

54affde

Relates: envoyproxy/envoy#31841 Relates: envoyproxy/envoy#32203 Relates: envoyproxy/envoy#32260 Relates: envoyproxy/envoy#33019

alyssawilk mentioned this pull request Oct 15, 2024

envoy_restart_features_quic_handle_certs_with_shared_tls_code deprecation #36612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: support cert selection by SNI, non-PEM formats #32260

quic: support cert selection by SNI, non-PEM formats #32260

ggreenway commented Feb 7, 2024 •

edited

Loading

repokitteh-read-only bot commented Feb 7, 2024

RyanTheOptimist left a comment

RyanTheOptimist Feb 13, 2024

ggreenway Feb 13, 2024

danzh2010 Feb 13, 2024

ggreenway Feb 14, 2024

ggreenway commented Mar 1, 2024

RyanTheOptimist commented Mar 5, 2024

danzh2010 left a comment

ggreenway commented Mar 5, 2024

quic: support cert selection by SNI, non-PEM formats #32260

quic: support cert selection by SNI, non-PEM formats #32260

Conversation

ggreenway commented Feb 7, 2024 • edited Loading

repokitteh-read-only bot commented Feb 7, 2024

RyanTheOptimist left a comment

Choose a reason for hiding this comment

RyanTheOptimist Feb 13, 2024

Choose a reason for hiding this comment

ggreenway Feb 13, 2024

Choose a reason for hiding this comment

danzh2010 Feb 13, 2024

Choose a reason for hiding this comment

ggreenway Feb 14, 2024

Choose a reason for hiding this comment

ggreenway commented Mar 1, 2024

RyanTheOptimist commented Mar 5, 2024

danzh2010 left a comment

Choose a reason for hiding this comment

ggreenway commented Mar 5, 2024

ggreenway commented Feb 7, 2024 •

edited

Loading