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 23 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
36 changes: 30 additions & 6 deletions source/extensions/quic_listeners/quiche/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,17 @@ 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 = [
":envoy_quic_utils_lib",
"@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 +84,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 +96,32 @@ 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 = [
":envoy_quic_utils_lib",
"@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",
":envoy_quic_utils_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 Expand Up @@ -317,7 +338,10 @@ envoy_cc_library(
name = "envoy_quic_utils_lib",
srcs = ["envoy_quic_utils.cc"],
hdrs = ["envoy_quic_utils.h"],
external_deps = ["quiche_quic_platform"],
external_deps = [
"quiche_quic_platform",
"ssl",
],
tags = ["nofips"],
deps = [
"//include/envoy/http:codec_interface",
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ EnvoyQuicProofSource::GetCertChain(const quic::QuicSocketAddress& server_address
new quic::ProofSource::Chain(chain));
}

void EnvoyQuicProofSource::ComputeTlsSignature(
void EnvoyQuicProofSource::signPayload(
const quic::QuicSocketAddress& server_address, const quic::QuicSocketAddress& client_address,
const std::string& hostname, uint16_t signature_algorithm, quiche::QuicheStringPiece in,
std::unique_ptr<quic::ProofSource::SignatureCallback> callback) {
Expand Down
19 changes: 11 additions & 8 deletions source/extensions/quic_listeners/quiche/envoy_quic_proof_source.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +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,
protected Logger::Loggable<Logger::Id::quic> {
// A ProofSource implementation which supplies a proof instance with certs from filter chain.
class EnvoyQuicProofSource : public EnvoyQuicProofSourceBase {
public:
EnvoyQuicProofSource(Network::Socket& listen_socket,
Network::FilterChainManager& filter_chain_manager,
Expand All @@ -19,14 +19,17 @@ 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;
void ComputeTlsSignature(const quic::QuicSocketAddress& server_address,
const quic::QuicSocketAddress& client_address,
const std::string& hostname, uint16_t signature_algorithm,
quiche::QuicheStringPiece in,
std::unique_ptr<quic::ProofSource::SignatureCallback> callback) override;

protected:
// quic::ProofSource
void signPayload(const quic::QuicSocketAddress& server_address,
const quic::QuicSocketAddress& client_address, const std::string& hostname,
uint16_t signature_algorithm, quiche::QuicheStringPiece in,
std::unique_ptr<quic::ProofSource::SignatureCallback> callback) override;

private:
struct CertConfigWithFilterChain {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#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

#include "extensions/quic_listeners/quiche/envoy_quic_utils.h"

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;
}

std::string error_details;
bssl::UniquePtr<X509> cert = parseDERCertificate(chain->certs[0], &error_details);
if (cert == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add test coverage for the error case. If it can't be tested, should it be changed to an ASSERT?

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

ENVOY_LOG(warn, absl::StrCat("Invalid leaf cert: ", error_details));
quic::QuicCryptoProof proof;
callback->Run(/*ok=*/false, nullptr, proof, nullptr);
return;
}

bssl::UniquePtr<EVP_PKEY> pub_key(X509_get_pubkey(cert.get()));
int sign_alg = deduceSignatureAlgorithmFromPublicKey(pub_key.get(), &error_details);
if (sign_alg == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please 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

ENVOY_LOG(warn, absl::StrCat("Failed to deduce signature algorithm from public key: ",
error_details));
quic::QuicCryptoProof proof;
callback->Run(/*ok=*/false, nullptr, proof, nullptr);
return;
}

auto signature_callback = std::make_unique<SignatureCallback>(std::move(callback), chain);

signPayload(server_address, client_address, hostname, sign_alg,
quiche::QuicheStringPiece(payload.get(), payload_size),
std::move(signature_callback));
}

void EnvoyQuicProofSourceBase::ComputeTlsSignature(
const quic::QuicSocketAddress& server_address, const quic::QuicSocketAddress& client_address,
const std::string& hostname, uint16_t signature_algorithm, quiche::QuicheStringPiece in,
std::unique_ptr<quic::ProofSource::SignatureCallback> callback) {
signPayload(server_address, client_address, hostname, signature_algorithm, in,
std::move(callback));
}

} // namespace Quic
} // namespace Envoy
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,16 @@
#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"
#include "common/common/logger.h"

namespace Envoy {
namespace Quic {
Expand All @@ -38,31 +40,36 @@ 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,
protected Logger::Loggable<Logger::Id::quic> {
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; }

void ComputeTlsSignature(const quic::QuicSocketAddress& server_address,
const quic::QuicSocketAddress& client_address,
const std::string& hostname, uint16_t signature_algorithm,
quiche::QuicheStringPiece in,
std::unique_ptr<quic::ProofSource::SignatureCallback> callback) override;

protected:
virtual void signPayload(const quic::QuicSocketAddress& server_address,
const quic::QuicSocketAddress& client_address,
const std::string& hostname, uint16_t signature_algorithm,
quiche::QuicheStringPiece in,
std::unique_ptr<quic::ProofSource::SignatureCallback> callback) = 0;

private:
// Used by GetProof() to get signature.
class SignatureCallback : public quic::ProofSource::SignatureCallback {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#include "extensions/quic_listeners/quiche/envoy_quic_proof_verifier.h"

#include "extensions/quic_listeners/quiche/envoy_quic_utils.h"

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

namespace Envoy {
namespace Quic {

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++) {
bssl::UniquePtr<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 = std::move(cert);
} else {
sk_X509_push(intermediates.get(), cert.release());
}
}
bool success = context_impl_.verifyCertChain(*leaf, *intermediates, *error_details);
if (!success) {
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 (Extensions::TransportSockets::Tls::ContextImpl::dnsNameMatch(hostname, config_san)) {
return quic::QUIC_SUCCESS;
}
}
*error_details = absl::StrCat("Leaf certificate doesn't match hostname: ", hostname);
return quic::QUIC_FAILURE;
}

} // namespace Quic
} // namespace Envoy
Loading