tls: make X.509 certificate parsing and verification explicit. #10621

PiotrSikora · 2020-04-02T00:10:40Z

BoringSSL would like to deprecate OpenSSL's X.509 certificate parsing
and verification code. Although the replacements are not yet ready,
we can take a step in this direction by making certificate parsing
and certificate chain validation explicit.

This changes Envoy to use TLS_with_buffers_method(), which removes the
OpenSSL behavior of parsing certificates to an |X509| object implicitly,
as well as the behavior of calling X509_verify_cert() implicitly.
Instead, Envoy now does these things explicitly, as required.

Usefully, operations that require parsing (such as extracting a
certificate's serial number) are now distinguished from operations
that do not require parsing (such as returning the SHA-256 hash of a
certificate's DER encoding).

This makes it easier to take the next step of using a better parser,
when it is available.

Risk Level: Medium
Testing: Existing tests
Docs Changes: N/A (not user-visible change)
Release Notes: N/A (not user-visible change)

Signed-off-by: Matt Braithwaite mab@google.com
Signed-off-by: Piotr Sikora piotrsikora@google.com

BoringSSL would like to deprecate OpenSSL's X.509 certificate parsing and verification code. Although the replacements are not yet ready, we can take a step in this direction by making certificate parsing and certificate chain validation explicit. This changes Envoy to use TLS_with_buffers_method(), which removes the OpenSSL behavior of parsing certificates to an |X509| object implicitly, as well as the behavior of calling X509_verify_cert() implicitly. Instead, Envoy now does these things explicitly, as required. Usefully, operations that require parsing (such as extracting a certificate's serial number) are now distinguished from operations that do not require parsing (such as returning the SHA-256 hash of a certificate's DER encoding). This makes it easier to take the next step of using a better parser, when it is available. Signed-off-by: Matt Braithwaite <mab@google.com> Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2020-04-02T00:11:08Z

cc @SAHF

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2020-04-02T16:14:19Z

/azp run

azure-pipelines · 2020-04-02T16:14:30Z

Azure Pipelines successfully started running 1 pipeline(s), but failed to run 2 pipeline(s).

lizan

This looks good but scares me a bit. Can you add the PR template to the PR description, mark it at least Medium risk level? Would love to get @ggreenway to double check.

source/extensions/transport_sockets/tls/context_impl.cc

ggreenway · 2020-04-03T16:26:41Z

Can you provide more information about the changes happening in BoringSSL, so I have more context on this change? What has happened already, and what is planned for the future?

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

…buffers

PiotrSikora · 2020-04-04T01:50:10Z

@ggreenway BoringSSL comes with X.509 parser and verifier it inherited from OpenSSL, but that codebase is not actively maintained, and Google doesn't use it anymore.

This PR effectively removes X.509 parsing and verification from BoringSSL's internal processing, and moves it to Envoy, which coincidentally uses X.509 parser and verifier that's bundled with BoringSSL, but it can be replaced with others in the future.

The certificate verifier that we should eventually use is hidden inside Chromium's codebase, but it's not yet a standalone project, so it's going to be a while before we can use it in Envoy.

ggreenway

I'm uncomfortable with moving core TLS stuff out of the TLS library (BoringSSL) and into Envoy. If BoringSSL is moving towards pushing this into chromium, can we work on factoring out the chromium code into a re-usable library instead of re-implementing it in Envoy?

source/extensions/transport_sockets/tls/context_impl.cc

PiotrSikora

I'm uncomfortable with moving core TLS stuff out of the TLS library (BoringSSL) and into Envoy. If BoringSSL is moving towards pushing this into chromium, can we work on factoring out the chromium code into a re-usable library instead of re-implementing it in Envoy?

@ggreenway We're not re-implementing certificate verification in Envoy. This PR separates TLS processing (using BoringSSL) from certificate verification (using X.509 verifier from BoringSSL), so that we could use the separate library for certificate verification when it's ready.

I've been asking about moving Chromium's certificate verifier to a standalone library for a while, but I don't have anything to share yet. It's going to be available, eventually...

source/extensions/transport_sockets/tls/context_impl.cc

ggreenway · 2020-04-07T19:58:10Z

We're not re-implementing certificate verification in Envoy.

Right, but in my mind I was including all the byte-level parsing. I'd like a higher-level API. Either an API that takes the raw DER and finds what it needs itself, or an accessor from BoringSSL that finds the bits we need to pass to a verifier. Anytime I see CBS it seems out of place to me; it doesn't seem like Envoy should be parsing at that level.

stale · 2020-04-14T20:03:40Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-04-23T13:07:12Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

PiotrSikora · 2020-05-18T16:01:17Z

Right, but in my mind I was including all the byte-level parsing. I'd like a higher-level API. Either an API that takes the raw DER and finds what it needs itself, or an accessor from BoringSSL that finds the bits we need to pass to a verifier. Anytime I see CBS it seems out of place to me; it doesn't seem like Envoy should be parsing at that level.

The functions in question are part of the aforementioned Chromium's certificate verifier, so it's unlikely that they'll be added to BoringSSL itself, since they're a part of the component that's getting extracted into a separate library.

Also, as you've already noticed, those functions already exist in Envoy (in QuicCertUtilsImpl), so it's unclear why they're fine there, but we're blocking this PR on them. Seems inconsistent.

ggreenway · 2020-05-18T19:27:38Z

The functions in question are part of the aforementioned Chromium's certificate verifier, so it's unlikely that they'll be added to BoringSSL itself, since they're a part of the component that's getting extracted into a separate library.

I don't feel strongly about which library they come from. But I do feel that byte-level parsing certificates (or the protocol) should be coming from some library, not directly in Envoy.

When everything you want to be exported from chromium is exported, what will this code look like? Will it be mostly as it is in this PR, but we'll replace a call to X509_verify_cert() with something else?

As I see it, if BoringSSL removes it's x509 parsing/verifying code, this change will be needed whether I like it or not. I'm trying to get a sense of how much of this is "final" and how much of this is just an intermediate step.

Also, as you've already noticed, those functions already exist in Envoy (in QuicCertUtilsImpl), so it's unclear why they're fine there, but we're blocking this PR on them. Seems inconsistent.

I understand the frustration, but that is somewhat different. That function is part of the quiche API for supporting a platform. I don't think there is any alternative.

PiotrSikora · 2020-05-19T12:00:01Z

I don't feel strongly about which library they come from. But I do feel that byte-level parsing certificates (or the protocol) should be coming from some library, not directly in Envoy.

When everything you want to be exported from chromium is exported, what will this code look like? Will it be mostly as it is in this PR, but we'll replace a call to X509_verify_cert() with something else?

As I see it, if BoringSSL removes it's x509 parsing/verifying code, this change will be needed whether I like it or not. I'm trying to get a sense of how much of this is "final" and how much of this is just an intermediate step.

The certificate-parsing functions would be replaced with calls to their copies in Chromium's certificate verifier (see: ExtractSubjectFromDERCert(), ExtractSPKIFromDERCert()), and X509_verify_cert() would be replaced with CertVerifier::Verify(). But I'm not involved in that work, and there is no timeline, so we might be stuck with functions as in this PR for a while.

Having said that, the switch to CRYPTO_BUFFER APIs (which is majority of this PR) has other benefits, like deduplication of the same certificate when used multiple times in the configuration (intermediate certificates, trusted CAs, etc.). This is not implemented in this PR, but it should be fairly easy to add.

stale · 2020-05-30T10:22:37Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

PiotrSikora · 2020-07-15T21:21:20Z

@ggreenway @alyssawilk is there any way that we can move this forward?

alyssawilk · 2020-07-16T14:30:39Z

The QUIC code you reference is considered to be an untrusted path. As discussed offline, if we need them for QUICHE and Envoy though, maybe we could add them to boringssl directly, since they seem to be general utility functions for crypto handshakes? Then more crypto-savvy folks could own and maintain and we could just consume? cc @agl

source/extensions/transport_sockets/tls/context_impl.cc

ggreenway · 2020-07-22T17:56:40Z

source/extensions/transport_sockets/tls/context_impl.cc

+  }());
+}
+
+bool seekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) {


Can this (and the other similar functions) return optional? I'm assuming CBS is just a pointer and length (not a big struct).

It's a little bit more than that:
https://boringssl.googlesource.com/boringssl/+/74161f485b5d54fe963cbd3d081b718ec84d2e00/include/openssl/bytestring.h#39
let me know how do you want to proceed.

Yes, let's make it return an optional.

ggreenway · 2020-07-28T19:15:12Z

/wait

stale · 2020-08-08T23:29:41Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

PiotrSikora · 2020-08-09T00:15:46Z

Not stale.

…buffers Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

test/extensions/transport_sockets/tls/ssl_socket_test.cc

PiotrSikora · 2020-08-10T11:39:48Z

/retest

repokitteh-read-only · 2020-08-10T11:39:52Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Check envoy-presubmit (Linux-x64 compile_time_options) didn't fail.

🐱

Caused by: a #10621 (comment) was created by @PiotrSikora.

see: more, trace.

ggreenway · 2020-08-10T16:25:19Z

source/extensions/transport_sockets/tls/context_impl.cc

+  }());
+}
+
+bool seekToSubject(CRYPTO_BUFFER* cert, CBS* tbs_certificate) {


Yes, let's make it return an optional.

ggreenway · 2020-08-10T16:59:43Z

source/extensions/transport_sockets/tls/context_impl.cc

+
+  ContextImpl* impl = reinterpret_cast<ContextImpl*>(SSL_CTX_get_app_data(SSL_get_SSL_CTX(ssl)));
+  const Network::TransportSocketOptions* transport_socket_options =
+      static_cast<const Network::TransportSocketOptions*>(SSL_get_app_data(ssl));


I think this would be clearer as a reinterpret_cast

ggreenway · 2020-08-10T17:23:23Z

source/extensions/transport_sockets/tls/context_impl.cc

+  //      subject              Name,
+  //      subjectPublicKeyInfo SubjectPublicKeyInfo,
+  //      ...
+  if (!CBS_get_asn1(&der, &certificate, CBS_ASN1_SEQUENCE) ||


Please add some test coverage for this. At least the case of of trailing junk after cert, and an invalid cert (either complete junk, or a cert that's in some way invalid (missing a required field, etc)). I want to make sure that the code is exercised a bit since the error path is reachable by client-controlled data.

stale · 2020-08-23T02:26:14Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-08-30T03:21:24Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

PiotrSikora requested review from agl and lizan April 2, 2020 00:10

PiotrSikora requested a review from ggreenway April 2, 2020 00:27

review: fix casing of function names.

4c386f5

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

htuch assigned ggreenway Apr 2, 2020

lizan reviewed Apr 3, 2020

View reviewed changes

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

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

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

PiotrSikora added 4 commits April 4, 2020 00:38

review: drop static.

5744ac8

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use absl::optional<absl::string_view>.

b7fa99a

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: remove unnecessary check.

e23fc68

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Merge remote-tracking branch 'origin/master' into PiotrSikora/crypto_…

8bc267d

…buffers

ggreenway requested changes Apr 6, 2020

View reviewed changes

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

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

PiotrSikora commented Apr 7, 2020

View reviewed changes

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

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

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 14, 2020

stale bot closed this Apr 23, 2020

PiotrSikora reopened this May 18, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 18, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 30, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 15, 2020

ggreenway mentioned this pull request Jul 21, 2020

quiche: implement certificate verification #12063

Merged

ggreenway requested changes Jul 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 28, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 8, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 9, 2020

PiotrSikora added 8 commits August 10, 2020 04:01

Merge remote-tracking branch 'origin/master' into PiotrSikora/crypto_…

db7eb63

…buffers Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: rename leafCertificate() to localLeafCertificate().

66db644

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use X509_parse_from_buffer().

91ecf00

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: reject corrupted peer certificates.

46a37d3

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: guard against cases that cannot happen.

c1685bf

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: address some of Greg's comments.

13af8c0

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use TLS_with_buffers_method().

9611e50

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: fix comment.

577c26f

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

repokitteh-read-only bot removed the waiting label Aug 10, 2020

PiotrSikora commented Aug 10, 2020

View reviewed changes

test/extensions/transport_sockets/tls/ssl_socket_test.cc Show resolved Hide resolved

ggreenway requested changes Aug 10, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 23, 2020

stale bot closed this Aug 30, 2020

davidben mentioned this pull request Dec 3, 2021

Wildcard Let's Encrypt certs do not validate after DST Root CA expiration #18510

Closed

PiotrSikora mentioned this pull request Feb 22, 2022

possible optimization: repetitive private key reads #20045

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: make X.509 certificate parsing and verification explicit. #10621

tls: make X.509 certificate parsing and verification explicit. #10621

PiotrSikora commented Apr 2, 2020 •

edited

Loading

PiotrSikora commented Apr 2, 2020

PiotrSikora commented Apr 2, 2020

azure-pipelines bot commented Apr 2, 2020

lizan left a comment

ggreenway commented Apr 3, 2020

PiotrSikora commented Apr 4, 2020

ggreenway left a comment

PiotrSikora left a comment

ggreenway commented Apr 7, 2020

stale bot commented Apr 14, 2020

stale bot commented Apr 23, 2020

PiotrSikora commented May 18, 2020

ggreenway commented May 18, 2020

PiotrSikora commented May 19, 2020

stale bot commented May 30, 2020

PiotrSikora commented Jul 15, 2020

alyssawilk commented Jul 16, 2020

ggreenway Jul 22, 2020

PiotrSikora Aug 10, 2020

ggreenway Aug 10, 2020

ggreenway commented Jul 28, 2020

stale bot commented Aug 8, 2020

PiotrSikora commented Aug 9, 2020

PiotrSikora commented Aug 10, 2020

repokitteh-read-only bot commented Aug 10, 2020

ggreenway Aug 10, 2020

ggreenway Aug 10, 2020

ggreenway Aug 10, 2020

stale bot commented Aug 23, 2020

stale bot commented Aug 30, 2020

tls: make X.509 certificate parsing and verification explicit. #10621

tls: make X.509 certificate parsing and verification explicit. #10621

Conversation

PiotrSikora commented Apr 2, 2020 • edited Loading

PiotrSikora commented Apr 2, 2020

PiotrSikora commented Apr 2, 2020

azure-pipelines bot commented Apr 2, 2020

lizan left a comment

Choose a reason for hiding this comment

ggreenway commented Apr 3, 2020

PiotrSikora commented Apr 4, 2020

ggreenway left a comment

Choose a reason for hiding this comment

PiotrSikora left a comment

Choose a reason for hiding this comment

ggreenway commented Apr 7, 2020

stale bot commented Apr 14, 2020

stale bot commented Apr 23, 2020

PiotrSikora commented May 18, 2020

ggreenway commented May 18, 2020

PiotrSikora commented May 19, 2020

stale bot commented May 30, 2020

PiotrSikora commented Jul 15, 2020

alyssawilk commented Jul 16, 2020

ggreenway Jul 22, 2020

Choose a reason for hiding this comment

PiotrSikora Aug 10, 2020

Choose a reason for hiding this comment

ggreenway Aug 10, 2020

Choose a reason for hiding this comment

ggreenway commented Jul 28, 2020

stale bot commented Aug 8, 2020

PiotrSikora commented Aug 9, 2020

PiotrSikora commented Aug 10, 2020

repokitteh-read-only bot commented Aug 10, 2020

ggreenway Aug 10, 2020

Choose a reason for hiding this comment

ggreenway Aug 10, 2020

Choose a reason for hiding this comment

ggreenway Aug 10, 2020

Choose a reason for hiding this comment

stale bot commented Aug 23, 2020

stale bot commented Aug 30, 2020

PiotrSikora commented Apr 2, 2020 •

edited

Loading