Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quiche: implement certificate verification #12063

Merged
merged 34 commits into from
Aug 7, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
0045de4
add verifier impl and proof source details
danzh1989 Jul 1, 2020
105e714
no empty filter chain
danzh1989 Jul 2, 2020
811a141
Merge branch 'master' into certverify
danzh1989 Jul 3, 2020
a0d8975
revert cert verification
danzh1989 Jul 4, 2020
6e599bf
add back cert verification
danzh1989 Jul 4, 2020
67fad57
comment
danzh1989 Jul 4, 2020
af27dd7
Merge branch 'master' into certverify
danzh1989 Jul 13, 2020
eae447a
Merge branch 'master' into certverify
danzh1989 Jul 13, 2020
010661c
add comment
danzh1989 Jul 13, 2020
7ac6b81
spell
danzh1989 Jul 14, 2020
562bbd2
remove redudant check
danzh1989 Jul 14, 2020
66e0383
fix asan
danzh1989 Jul 14, 2020
5c0dd47
fix sigalgs
danzh1989 Jul 14, 2020
ae75699
format
danzh1989 Jul 14, 2020
1b85e44
fix windows
danzh1989 Jul 15, 2020
bcad52c
sign alg
danzh1989 Jul 15, 2020
2675461
share sign function
danzh1989 Jul 20, 2020
f19000a
wildcard
danzh1989 Jul 20, 2020
4c2d222
update comment
danzh1989 Jul 20, 2020
80a24d9
move around X509 stuff
danzh1989 Jul 23, 2020
1406524
Merge branch 'master' into certverify
danzh1989 Jul 24, 2020
ca7978f
use dnsNameMatch
danzh1989 Jul 28, 2020
cf62882
refactor string compare
danzh1989 Jul 28, 2020
bb4ec8a
fail upon invalid pkey
danzh1989 Jul 28, 2020
03f319e
Merge branch 'master' into certverify
danzh1989 Jul 28, 2020
c60059a
fix unuse param
danzh1989 Jul 29, 2020
29e1f29
fix sig alg deduction
danzh1989 Jul 29, 2020
bb9b501
use string_view
danzh1989 Jul 30, 2020
333b896
corner cases
danzh1989 Jul 31, 2020
99aa268
format
danzh1989 Aug 3, 2020
fc121e7
fix test failure
danzh1989 Aug 3, 2020
c3229ed
add more test
danzh1989 Aug 5, 2020
405fdb9
Merge branch 'master' into certverify
danzh1989 Aug 5, 2020
0f7f7b8
fix test name
danzh1989 Aug 6, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions source/extensions/quic_listeners/quiche/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,16 @@ envoy_cc_library(
)

envoy_cc_library(
name = "envoy_quic_fake_proof_source_lib",
hdrs = ["envoy_quic_fake_proof_source.h"],
name = "envoy_quic_proof_source_base_lib",
srcs = ["envoy_quic_proof_source_base.cc"],
hdrs = ["envoy_quic_proof_source_base.h"],
external_deps = ["quiche_quic_platform"],
tags = ["nofips"],
deps = [
"@com_googlesource_quiche//:quic_core_crypto_certificate_view_lib",
"@com_googlesource_quiche//:quic_core_crypto_crypto_handshake_lib",
"@com_googlesource_quiche//:quic_core_crypto_proof_source_interface_lib",
"@com_googlesource_quiche//:quic_core_data_lib",
"@com_googlesource_quiche//:quic_core_versions_lib",
],
)
Expand All @@ -79,7 +83,7 @@ envoy_cc_library(
external_deps = ["ssl"],
tags = ["nofips"],
deps = [
":envoy_quic_fake_proof_source_lib",
":envoy_quic_proof_source_base_lib",
":envoy_quic_utils_lib",
":quic_io_handle_wrapper_lib",
":quic_transport_socket_factory_lib",
Expand All @@ -91,16 +95,30 @@ envoy_cc_library(
)

envoy_cc_library(
name = "envoy_quic_proof_verifier_lib",
hdrs = ["envoy_quic_fake_proof_verifier.h"],
name = "envoy_quic_proof_verifier_base_lib",
srcs = ["envoy_quic_proof_verifier_base.cc"],
hdrs = ["envoy_quic_proof_verifier_base.h"],
external_deps = ["quiche_quic_platform"],
tags = ["nofips"],
deps = [
"@com_googlesource_quiche//:quic_core_crypto_certificate_view_lib",
"@com_googlesource_quiche//:quic_core_crypto_crypto_handshake_lib",
"@com_googlesource_quiche//:quic_core_versions_lib",
],
)

envoy_cc_library(
name = "envoy_quic_proof_verifier_lib",
srcs = ["envoy_quic_proof_verifier.cc"],
hdrs = ["envoy_quic_proof_verifier.h"],
external_deps = ["quiche_quic_platform"],
tags = ["nofips"],
deps = [
":envoy_quic_proof_verifier_base_lib",
"//source/extensions/transport_sockets/tls:context_lib",
],
)

envoy_cc_library(
name = "spdy_server_push_utils_for_envoy_lib",
srcs = ["spdy_server_push_utils_for_envoy.cc"],
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@

#include "server/connection_handler_impl.h"

#include "extensions/quic_listeners/quiche/envoy_quic_fake_proof_source.h"
#include "extensions/quic_listeners/quiche/envoy_quic_proof_source_base.h"
#include "extensions/quic_listeners/quiche/quic_transport_socket_factory.h"

namespace Envoy {
namespace Quic {

class EnvoyQuicProofSource : public EnvoyQuicFakeProofSource,
// A ProofSource implementation which supplies a proof instance with certs from filter chain.
class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase,
protected Logger::Loggable<Logger::Id::quic> {
public:
EnvoyQuicProofSource(Network::Socket& listen_socket,
Expand All @@ -19,6 +20,7 @@ class EnvoyQuicProofSource : public EnvoyQuicFakeProofSource,

~EnvoyQuicProofSource() override = default;

// quic::ProofSource
quic::QuicReferenceCountedPointer<quic::ProofSource::Chain>
GetCertChain(const quic::QuicSocketAddress& server_address,
const quic::QuicSocketAddress& client_address, const std::string& hostname) override;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#include "extensions/quic_listeners/quiche/envoy_quic_proof_source_base.h"

#pragma GCC diagnostic push

// QUICHE allows unused parameters.
#pragma GCC diagnostic ignored "-Wunused-parameter"
#include "quiche/quic/core/quic_data_writer.h"

#pragma GCC diagnostic pop

namespace Envoy {
namespace Quic {

void EnvoyQuicProofSourceBase::GetProof(const quic::QuicSocketAddress& server_address,
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
const quic::QuicSocketAddress& client_address,
const std::string& hostname,
const std::string& server_config,
quic::QuicTransportVersion /*transport_version*/,
quiche::QuicheStringPiece chlo_hash,
std::unique_ptr<quic::ProofSource::Callback> callback) {
quic::QuicReferenceCountedPointer<quic::ProofSource::Chain> chain =
GetCertChain(server_address, client_address, hostname);
ggreenway marked this conversation as resolved.
Show resolved Hide resolved

size_t payload_size = sizeof(quic::kProofSignatureLabel) + sizeof(uint32_t) + chlo_hash.size() +
server_config.size();
auto payload = std::make_unique<char[]>(payload_size);
quic::QuicDataWriter payload_writer(payload_size, payload.get(),
quiche::Endianness::HOST_BYTE_ORDER);
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
bool success =
payload_writer.WriteBytes(quic::kProofSignatureLabel, sizeof(quic::kProofSignatureLabel)) &&
payload_writer.WriteUInt32(chlo_hash.size()) && payload_writer.WriteStringPiece(chlo_hash) &&
payload_writer.WriteStringPiece(server_config);
Comment on lines +31 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems similar to quic::ProofSourceX509::GetProof, is it possible to reuse that code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

payload generation is actually shared across all ProofSource implementation not just with X509 ProofSource. I think it makes more sense for GetProof() to take the generated payload as argument. But this requires large upstream code refactoring which I don't think worth doing just for the sake of code sharing.

if (!success) {
quic::QuicCryptoProof proof;
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
callback->Run(/*ok=*/false, nullptr, proof, nullptr);
return;
}

// TODO(danzh) Get the signature algorithm from leaf cert.
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
auto signature_callback = std::make_unique<SignatureCallback>(std::move(callback), chain);
ComputeTlsSignature(server_address, client_address, hostname, SSL_SIGN_RSA_PSS_RSAE_SHA256,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling ComputeTlsSignature here looks a bit confusing when reading the code. I realize that GetProof and ComputeTlsSignature both do similar things (and ComputeTlsSignature doesn't need to do any processing on the input before signing), but having a helper method for the shared signing code would improve readability here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

quiche::QuicheStringPiece(payload.get(), payload_size),
std::move(signature_callback));
}

} // namespace Quic
} // namespace Envoy
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@
#pragma GCC diagnostic ignored "-Wunused-parameter"
#include "quiche/quic/core/crypto/proof_source.h"
#include "quiche/quic/core/quic_versions.h"

#include "quiche/quic/core/crypto/crypto_protocol.h"
#include "quiche/quic/platform/api/quic_reference_counted.h"
#include "quiche/quic/platform/api/quic_socket_address.h"
#include "quiche/common/platform/api/quiche_string_piece.h"
#pragma GCC diagnostic pop

#include "openssl/ssl.h"
#include "envoy/network/filter.h"
#include "quiche/quic/platform/api/quic_reference_counted.h"
#include "quiche/quic/platform/api/quic_socket_address.h"
#include "quiche/common/platform/api/quiche_string_piece.h"
#include "server/backtrace.h"

namespace Envoy {
namespace Quic {
Expand All @@ -38,28 +39,19 @@ class EnvoyQuicProofSourceDetails : public quic::ProofSource::Details {
const Network::FilterChain& filter_chain_;
};

// A fake implementation of quic::ProofSource which uses RSA cipher suite to sign in GetProof().
// TODO(danzh) Rename it to EnvoyQuicProofSource once it's fully implemented.
class EnvoyQuicFakeProofSource : public quic::ProofSource {
// A partial implementation of quic::ProofSource which uses RSA cipher suite to sign in GetProof().
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
class EnvoyQuicProofSourceBase : public quic::ProofSource {
public:
~EnvoyQuicFakeProofSource() override = default;
~EnvoyQuicProofSourceBase() override = default;

// quic::ProofSource
// Returns a certs chain and its fake SCT "Fake timestamp" and TLS signature wrapped
// in QuicCryptoProof.
void GetProof(const quic::QuicSocketAddress& server_address,
const quic::QuicSocketAddress& client_address, const std::string& hostname,
const std::string& server_config, quic::QuicTransportVersion /*transport_version*/,
quiche::QuicheStringPiece /*chlo_hash*/,
std::unique_ptr<quic::ProofSource::Callback> callback) override {
quic::QuicReferenceCountedPointer<quic::ProofSource::Chain> chain =
GetCertChain(server_address, client_address, hostname);
quic::QuicCryptoProof proof;
// TODO(danzh) Get the signature algorithm from leaf cert.
auto signature_callback = std::make_unique<SignatureCallback>(std::move(callback), chain);
ComputeTlsSignature(server_address, client_address, hostname, SSL_SIGN_RSA_PSS_RSAE_SHA256,
server_config, std::move(signature_callback));
}
quiche::QuicheStringPiece chlo_hash,
std::unique_ptr<quic::ProofSource::Callback> callback) override;

TicketCrypter* GetTicketCrypter() override { return nullptr; }

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
#include "extensions/quic_listeners/quiche/envoy_quic_proof_verifier.h"

#include "quiche/quic/core/crypto/certificate_view.h"

namespace Envoy {
namespace Quic {

static X509* parseDERCertificate(const std::string& der_bytes, std::string* error_details) {
const uint8_t* data;
const uint8_t* orig_data;
orig_data = data = reinterpret_cast<const uint8_t*>(der_bytes.data());
bssl::UniquePtr<X509> cert(d2i_X509(nullptr, &data, der_bytes.size()));
if (!cert.get()) {
*error_details = "d2i_X509";
return nullptr;
}
if (data < orig_data || static_cast<size_t>(data - orig_data) != der_bytes.size()) {
// Trailing garbage.
return nullptr;
}
return cert.release();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this function into to quiche and use it from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseDERCertificate() is not used anywhere else in QUICHE. So it would look like a dead function.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't want the boringssl X509 code in quiche.


quic::QuicAsyncStatus EnvoyQuicProofVerifier::VerifyCertChain(
const std::string& hostname, const uint16_t /*port*/, const std::vector<std::string>& certs,
const std::string& /*ocsp_response*/, const std::string& /*cert_sct*/,
const quic::ProofVerifyContext* /*context*/, std::string* error_details,
std::unique_ptr<quic::ProofVerifyDetails>* /*details*/,
std::unique_ptr<quic::ProofVerifierCallback> /*callback*/) {
if (certs.empty()) {
return quic::QUIC_FAILURE;
}
bssl::UniquePtr<STACK_OF(X509)> intermediates(sk_X509_new_null());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can avoid using the boringssl X.509 code? (It's a mess and should be replaced. The chromium cert verifier is a candidate replacement.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I make the interface takes X509_STORE_CTX and X509 objects because ContextImpl::doVerifyCertChain() heavily uses X509 code. We can move the conversion of const std::vector<std::string>& certs to X509 into ContextImpl, but basically there is no way to avoid using these boringssl interfaces unless we don't reuse envoy cert verification code. But I think it makes sense to be consistent with tcp SSL socket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a separate issue being sorted out for how to move envoy off of this boringssl code. The current status, AFAIK, is that the chromium cert verifier isn't in a state that can be consumed by other projects yet, so there isn't any readily available replacement. @PiotrSikora and I are discussing, and he has posted #10621 as an option.

I think for this PR, you should leave this as-is using boringssl, and this can be updated when we address the issue in envoy as a whole.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree you should reuse the existing envoy cert verification code.

bssl::UniquePtr<X509> leaf;
for (size_t i = 0; i < certs.size(); i++) {
X509* cert = parseDERCertificate(certs[i], error_details);
if (!cert) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test coverage for the error case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return quic::QUIC_FAILURE;
}
if (i == 0) {
leaf.reset(cert);
} else {
sk_X509_push(intermediates.get(), cert);
}
}

bssl::UniquePtr<X509_STORE_CTX> ctx(X509_STORE_CTX_new());
// It doesn't matter which SSL context is used, because they share the same
// cert validation config.
X509_STORE* store = SSL_CTX_get_cert_store(context_impl_.chooseSslContexts());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything here involving X509_STORE_CTX and X509_STORE seems like an implementation detail of ContextImpl and should belong in ContextImpl::doVerifyCertChain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the interface to take a leaf X509 and intermetiates X509 STACK

if (!X509_STORE_CTX_init(ctx.get(), store, leaf.get(), intermediates.get())) {
*error_details = "Failed to verify certificate chain: X509_STORE_CTX_init";
return quic::QUIC_FAILURE;
}

int res = context_impl_.doVerifyCertChain(ctx.get(), nullptr, std::move(leaf), nullptr);
if (res <= 0) {
const int n = X509_STORE_CTX_get_error(ctx.get());
const int depth = X509_STORE_CTX_get_error_depth(ctx.get());
*error_details = absl::StrCat("X509_verify_cert: certificate verification error at depth ",
depth, ": ", X509_verify_cert_error_string(n));
return quic::QUIC_FAILURE;
}

std::unique_ptr<quic::CertificateView> cert_view =
quic::CertificateView::ParseSingleCertificate(certs[0]);
ASSERT(cert_view != nullptr);
for (const absl::string_view config_san : cert_view->subject_alt_name_domains()) {
if (config_san == hostname) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that config_san is a wildcard domain? If so, should we do regex match instead of ==?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think SAN can be wildcard. But better to leave it to Nick to confirm.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it could be a wildcard. A regex is the wrong way to match the wildcard. For a given hostname, there is a single wildcard entry that could cover it: replace the leftmost label in the hostname with *. This could be changed to if (config_san == hostname || config_san == wildcard) where wildcard is the substitution described above (assuming there's more than one label in the hostname).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return quic::QUIC_SUCCESS;
}
}
*error_details = absl::StrCat("Leaf certificate doesn't match hostname: ", hostname);
return quic::QUIC_FAILURE;
}

} // namespace Quic
} // namespace Envoy
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#pragma once

#include "extensions/quic_listeners/quiche/envoy_quic_proof_verifier_base.h"
#include "extensions/transport_sockets/tls/context_impl.h"

namespace Envoy {
namespace Quic {

// A quic::ProofVerifier implementation which verifies cert chain using SSL
// client context config.
class EnvoyQuicProofVerifier : public EnvoyQuicProofVerifierBase {
public:
EnvoyQuicProofVerifier(Stats::Scope& scope, const Envoy::Ssl::ClientContextConfig& config,
TimeSource& time_source)
: context_impl_(scope, config, time_source) {}

// EnvoyQuicProofVerifierBase
quic::QuicAsyncStatus
VerifyCertChain(const std::string& hostname, const uint16_t port,
const std::vector<std::string>& certs, const std::string& ocsp_response,
const std::string& cert_sct, const quic::ProofVerifyContext* context,
std::string* error_details, std::unique_ptr<quic::ProofVerifyDetails>* details,
std::unique_ptr<quic::ProofVerifierCallback> callback) override;

private:
Extensions::TransportSockets::Tls::ClientContextImpl context_impl_;
};

} // namespace Quic
} // namespace Envoy
Loading