quic: support cert selection by SNI, non-PEM formats #32260
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| #include "envoy/extensions/transport_sockets/quic/v3/quic_transport.pb.validate.h" | ||
|
|
||
| #include "source/common/quic/envoy_quic_utils.h" | ||
| #include "source/common/runtime/runtime_features.h" | ||
| #include "source/extensions/transport_sockets/tls/context_config_impl.h" | ||
|
|
||
|
|
@@ -13,7 +14,7 @@ namespace Quic { | |
| Network::DownstreamTransportSocketFactoryPtr | ||
| QuicServerTransportSocketConfigFactory::createTransportSocketFactory( | ||
| const Protobuf::Message& config, Server::Configuration::TransportSocketFactoryContext& context, | ||
| const std::vector<std::string>& server_names) { | ||
| auto quic_transport = MessageUtil::downcastAndValidate< | ||
| const envoy::extensions::transport_sockets::quic::v3::QuicDownstreamTransport&>( | ||
| config, context.messageValidationVisitor()); | ||
|
|
@@ -26,11 +27,77 @@ QuicServerTransportSocketConfigFactory::createTransportSocketFactory( | |
|
|
||
| auto factory = std::make_unique<QuicServerTransportSocketFactory>( | ||
| PROTOBUF_GET_WRAPPED_OR_DEFAULT(quic_transport, enable_early_data, true), | ||
| context.statsScope(), std::move(server_config), context.sslContextManager(), server_names); | ||
| factory->initialize(); | ||
| return factory; | ||
| } | ||
|
|
||
| namespace { | ||
| void initializeQuicCertAndKey(Ssl::TlsContext& context, | ||
| const Ssl::TlsCertificateConfig& /*cert_config*/) { | ||
| // Convert the certificate chain loaded into the context into PEM, as that is what the QUICHE | ||
| // API expects. By using the version already loaded, instead of loading it from the source, | ||
| // 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) { | ||
|
There was a problem hiding this comment. nit: Would it make sense to just make this function in the anonymous namespace? There was a problem hiding this comment. 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. |
||
| bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem())); | ||
| int result = PEM_write_bio_X509(bio.get(), cert); | ||
| 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); | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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. |
||
| }; | ||
|
|
||
| process_one_cert(SSL_CTX_get0_certificate(context.ssl_ctx_.get())); | ||
|
|
||
| STACK_OF(X509)* chain_stack = nullptr; | ||
| int result = SSL_CTX_get0_chain_certs(context.ssl_ctx_.get(), &chain_stack); | ||
| ASSERT(result == 1); | ||
| for (size_t i = 0; i < sk_X509_num(chain_stack); i++) { | ||
| process_one_cert(sk_X509_value(chain_stack, i)); | ||
| } | ||
|
|
||
| std::istringstream pem_stream(certs_str); | ||
| std::vector<std::string> chain = quic::CertificateView::LoadPemFromStream(&pem_stream); | ||
| quiche::QuicheReferenceCountedPointer<quic::ProofSource::Chain> cert_chain( | ||
| new quic::ProofSource::Chain(chain)); | ||
|
|
||
| std::string error_details; | ||
| bssl::UniquePtr<EVP_PKEY> pub_key(X509_get_pubkey(context.cert_chain_.get())); | ||
| int sign_alg = deduceSignatureAlgorithmFromPublicKey(pub_key.get(), &error_details); | ||
| if (sign_alg == 0) { | ||
| throwEnvoyExceptionOrPanic( | ||
| absl::StrCat("Failed to deduce signature algorithm from public key: ", error_details)); | ||
| } | ||
|
|
||
| context.quic_cert_ = std::move(cert_chain); | ||
|
|
||
| bssl::UniquePtr<EVP_PKEY> privateKey( | ||
| bssl::UpRef(SSL_CTX_get0_privatekey(context.ssl_ctx_.get()))); | ||
| std::unique_ptr<quic::CertificatePrivateKey> pem_key = | ||
| std::make_unique<quic::CertificatePrivateKey>(std::move(privateKey)); | ||
| if (pem_key == nullptr) { | ||
| throwEnvoyExceptionOrPanic("Failed to load QUIC private key."); | ||
| } | ||
|
|
||
| context.quic_private_key_ = std::move(pem_key); | ||
| } | ||
| } // namespace | ||
|
|
||
| QuicServerTransportSocketFactory::QuicServerTransportSocketFactory( | ||
| bool enable_early_data, Stats::Scope& scope, Ssl::ServerContextConfigPtr config, | ||
| Envoy::Ssl::ContextManager& manager, const std::vector<std::string>& server_names) | ||
| : QuicTransportSocketFactoryBase(scope, "server"), manager_(manager), stats_scope_(scope), | ||
| config_(std::move(config)), server_names_(server_names), | ||
| ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_, | ||
|
There was a problem hiding this comment. I think we need a RUNTIME_GUARD to protect all the interaction with ssl_ctx_ in this class and keep the old interface till the RUNTIME_GUARD gets deprecated. There was a problem hiding this comment. @danzh2010 This is going to be difficult to runtime-guard, due to the changes across a few different files/classes. Are you concerned about behavior changes (related to which cert is selected), or bugs? If it's bugs, is there additional test coverage we could add to alleviate concern? There was a problem hiding this comment. I'm more concerned about bugs. I think you can use a bool in this class to latch the value of a RUNTIME_GUARD and use it to guard both this class and the proof source. Please see my other comment. |
||
| initializeQuicCertAndKey)), | ||
| enable_early_data_(enable_early_data) {} | ||
|
|
||
| QuicServerTransportSocketFactory::~QuicServerTransportSocketFactory() { | ||
| manager_.removeContext(ssl_ctx_); | ||
| } | ||
|
|
||
| ProtobufTypes::MessagePtr QuicServerTransportSocketConfigFactory::createEmptyConfigProto() { | ||
| return std::make_unique< | ||
| envoy::extensions::transport_sockets::quic::v3::QuicDownstreamTransport>(); | ||
|
|
@@ -46,6 +113,46 @@ void QuicServerTransportSocketFactory::initialize() { | |
| } | ||
| } | ||
|
|
||
| std::pair<quiche::QuicheReferenceCountedPointer<quic::ProofSource::Chain>, | ||
| std::shared_ptr<quic::CertificatePrivateKey>> | ||
| QuicServerTransportSocketFactory::getTlsCertificateAndKey(absl::string_view sni, | ||
| bool* cert_matched_sni) const { | ||
| // onSecretUpdated() could be invoked in the middle of checking the existence of , and using, | ||
| // ssl_ctx. Capture ssl_ctx_ into a local variable so that we check and use the same ssl_ctx. | ||
| Envoy::Ssl::ServerContextSharedPtr ssl_ctx; | ||
| { | ||
| absl::ReaderMutexLock l(&ssl_ctx_mu_); | ||
| ssl_ctx = ssl_ctx_; | ||
| } | ||
| if (!ssl_ctx) { | ||
| ENVOY_LOG(warn, "SDS hasn't finished updating Ssl context config yet."); | ||
| stats_.downstream_context_secrets_not_ready_.inc(); | ||
| return {}; | ||
| } | ||
| auto ctx = | ||
| std::dynamic_pointer_cast<Extensions::TransportSockets::Tls::ServerContextImpl>(ssl_ctx); | ||
| auto [tls_context, ocsp_staple_action] = ctx->findTlsContext( | ||
| sni, true /* TODO: ecdsa_capable */, false /* TODO: ocsp_capable */, cert_matched_sni); | ||
ggreenway marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Thread safety note: accessing the tls_context requires holding a shared_ptr to the ``ssl_ctx``. | ||
| // Both of these members are themselves reference counted, so it is safe to use them after | ||
| // ``ssl_ctx`` goes out of scope after the function returns. | ||
| return {tls_context.quic_cert_, tls_context.quic_private_key_}; | ||
| } | ||
|
|
||
| void QuicServerTransportSocketFactory::onSecretUpdated() { | ||
| ENVOY_LOG(debug, "Secret is updated."); | ||
| auto ctx = manager_.createSslServerContext(stats_scope_, *config_, server_names_, | ||
| initializeQuicCertAndKey); | ||
| { | ||
| absl::WriterMutexLock l(&ssl_ctx_mu_); | ||
| std::swap(ctx, ssl_ctx_); | ||
| } | ||
| manager_.removeContext(ctx); | ||
|
|
||
| stats_.context_config_update_by_sds_.inc(); | ||
| } | ||
|
|
||
| REGISTER_FACTORY(QuicServerTransportSocketConfigFactory, | ||
| Server::Configuration::DownstreamTransportSocketConfigFactory); | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing in a nullable function pointer, why not just pass in a bool and initialize quic cert per string and private key in ServerContextImpl constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to keep as much quic-specific code in
source/common/quicis possible.