quiche: make cert verification check extended key usage of the cert by danzh2010 · Pull Request #18309 · envoyproxy/envoy

danzh2010 · 2021-09-29T18:35:06Z

Restrict the set of certificates Envoy, as a TLS client, accepts from the peer to only those certificates that contain extendedKeyUsage: serverAuth. The change only affects QUIC.

Risk Level: low, QUIC support is in alpha
Testing: added unit tests in QUIC proof verifier

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-09-29T19:04:58Z

/retest

repokitteh-read-only · 2021-09-29T19:05:01Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18309 (comment) was created by @danzh2010.

see: more, trace.

lizan · 2021-09-30T03:53:11Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2021-09-30T03:53:14Z

@envoyproxy/first-pass-reviewers assignee is @wbpcode

🐱

Caused by: a #18309 (comment) was created by @lizan.

see: more, trace.

RyanTheOptimist · 2021-09-30T04:00:02Z

source/extensions/transport_sockets/tls/context_impl.cc

    return false;
  }
+  // Currently only EnvoyQuicProofVerifier, which is used by the client code, calls this method. So
+  // hard-code "ssl_server" for now.


I think that since this method is always called to verify a chain of certs presented by a server, it is fine to use ssl_server here. In other words I don't think we need to mention EnvoyQuicProofVerifier in the comment and instead can simply explain that this method is always call to verify server certs.

What do you think?

I rephrased the comment.

RyanTheOptimist · 2021-09-30T04:00:58Z

test/common/quic/envoy_quic_proof_verifier_test.cc

 }

+TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureNonServerAuthEKU) {
+  root_ca_cert_ = R"(-----BEGIN CERTIFICATE-----


Do we have documentation somewhere that explains how these certs were generated?

Under test/config/integration/certs/ there are bunch of cert configs and certs.sh to generate various kinda cert. I just modified servercert.cfg a bit and run the script.

Commented about where this cert comes from.

Perfect, thanks!

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist

Looks great. Thanks, Dan!

RyanTheOptimist · 2021-09-30T22:58:03Z

test/common/quic/envoy_quic_proof_verifier_test.cc

 }

+TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureNonServerAuthEKU) {
+  root_ca_cert_ = R"(-----BEGIN CERTIFICATE-----


Perfect, thanks!

wbpcode

Thanks for your contributions and sorry for the delayed review comments. Add some minor comments to the PR.

In addition, can you add a little more detailed background info about why we need this PR? Although I probably understand the purpose of your PR. But if there is some clear description, it will be more convenient for others to view this part of the code in the future.

wbpcode · 2021-10-01T06:40:06Z

source/extensions/transport_sockets/tls/context_impl.cc

+    error_details = "Failed to verify certificate chain: X509_STORE_CTX_set_default";
+    return false;


Can we add a case to test this if branch?

TBH, it's not clear to me how X509_STORE_CTX_set_default() would return 0 with "ssl_server". Probably this is impossible if the name is hard-coded to "ssl_server". But I'm not a boring SSL expert.

Even if it is impossible with the hard-coded parameter, I would still prefer to handle the case where it returns 0 so that future BoringSSL change doesn't break Envoy in a unexpected way.

Get it. But it seems that without this test, the test coverage is not up to the requirements. We can let the maintainer decide whether we can reduce the coverage requirement of this module.

added a few irrelevant tls unit tests to raise coverage.

Correct, this is impossible with a hard-coded parameter. To synthetically generate code coverage, you'd have to allow the name ("ssl_server") to be injectable by the test, and then supply an invalid/unregistered name.

wbpcode · 2021-10-01T06:43:59Z

test/common/quic/envoy_quic_proof_verifier_test.cc

 }

+TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureNonServerAuthEKU) {
+  // Override the CA cert with test/config/integration/certs/ca.pem


spelling error: ca.pem -> cacert.pem
and
minor suggestion: May be Override the CA cert with cert copied from test/config/integration/certs/cacert.pem. is more clear.

wbpcode · 2021-10-01T06:53:49Z

test/common/quic/envoy_quic_proof_verifier_test.cc

+  // This is a cert same as test/config/integration/certs/servercert.pem but with extKeyUsage:
+  // clientAuth.


I think here should be a more accurate description of the process of generating this cert.

May be: This is a cert generated with the test/config/integration/certs/certs.sh. And the config that used to generate this cert is same as test/config/integration/certs/servercert.cfg but with 'extKeyUsage: clientAuth'.

Signed-off-by: Dan Zhang <danzh@google.com>

wbpcode · 2021-10-02T07:09:38Z

Thanks, it's look good overall for me. However, it seems that because the newly added code is not covered by the test, the coverage does not meet the requirements.

@wbpcode Can we add a case to test this if branch?
@danzh2010 TBH, it's not clear to me how X509_STORE_CTX_set_default() would return 0 with "ssl_server". Probably this is impossible if the name is hard-coded to "ssl_server". But I'm not a boring SSL expert.
@danzh2010 Even if it is impossible with the hard-coded parameter, I would still prefer to handle the case where it returns 0 so that future BoringSSL change doesn't break Envoy in a unexpected way.
@wbpcode Get it. But it seems that without this test, the test coverage is not up to the requirements. We can let the maintainer decide whether we can reduce the coverage requirement of this module.

@envoyproxy/envoy-maintainers

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-10-05T22:11:06Z

/retest

repokitteh-read-only · 2021-10-05T22:11:10Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18309 (comment) was created by @danzh2010.

see: more, trace.

wbpcode · 2021-10-06T02:00:37Z

/retest

repokitteh-read-only · 2021-10-06T02:00:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18309 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2021-10-06T02:23:59Z

source/extensions/transport_sockets/tls/ssl_socket.h

-  bool isThreadSafe() const {
-    return callbacks_ != nullptr && callbacks_->connection().dispatcher().isThreadSafe();
-  }


Why we removed this method?

It is only called in one place in ASSERT() which is not ignored in coverage tests. In order to raise the coverage, I inlined it instead.

I see. But I personally don't think this is good choice. 🤣

Can we just add some other tests to raise up cov or just edit the cov requirements?

c296145 already added some unit tests which is irrelevant to this PR. And now the coverage CI is happy.
As to inlining isThreadSafe() it makes much sense to me as it was a private method called only in one place which is an ASSERT().

I know it's OK to inline this method. But besides improving cov, what other useful effects does this removing have? 🤔

I mean, maybe we shouldn't make this kind of modification that has nothing to do with this PR or feature itself just in order to improve cov. Of course, this is just a personal thought.

Anyway, I have already approve this PR for the maintainer to do further review. Thanks very much for you contributions. 😃

wbpcode

Thanks. I will give an approve to push the PR to the next step.

It is only called in one place in ASSERT() which is not ignored in coverage tests. In order to raise the coverage, I inlined it instead.

I personally think this is not a good choice but keep it for the further review of @envoyproxy/senior-maintainers

danzh2010 · 2021-10-07T16:00:35Z

/assign @alyssawilk

sleevi · 2021-10-07T17:56:13Z

source/extensions/transport_sockets/tls/context_impl.cc

+  if (!X509_STORE_CTX_set_default(ctx.get(), "ssl_server")) {
+    error_details = "Failed to verify certificate chain: X509_STORE_CTX_set_default";
+    return false;
+  }


Question: Should you also be inheriting the params from the SSL_CTX? I don't see this file manipulating them in any way, but it's an edge case I wanted to call out. Specifically, the following elements of the SSL_CTX are not carried over, so if they were ever used to configure the SSL_CTX, they wouldn't be propagated to the Quiche path:

SSL{_CTX}_set_verify_depth

SSL{_CTX}_set1_param

SSL{_CTX}_set_purpose

SSL{_CTX}_set_trust

Because this code already grabs the SSL_CTX_set_cert_store path (line 1169), perhaps it makes sense to do so here as well?

Suggested change

}

}

if (!X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(ctx.get()),

SSL_CTX_get0_param(ssl_ctx))) {

error_details = "Failed to verify certificate chain: X509_VERIFY_PARAM_set1";

return false;

}

This will copy the X509_VERIFY_PARAM config from the SSL_CTX into the X509_STORE_CTX's verify param as appropriate, and matches the SSL/TLS layer implementation.

To address the testing/coverage question: This is also an "impossible failure" scenario; the only way for this to fail is for SSL_CTX_get0_param to return an invalid parameter (since it will not return nullptr)

Because X509_VERIFY_PARAM is an opaque struct, whose setters validate the incoming parameters, to get an invalid parameter into the SSL_CTX_get0_param, you must have first used one of the X509_VERIFY_PARAM_set functions, which would have similarly validated the parameters then. So this leaves the only possible room for failure being a malloc failure, which presumably will blow things up spectacularly anyways out of necessity :)

Envoy doesn't config those aspects of SSL_CTX objects AFAIK. But sure that we can add X509_VERIFY_PARAM_set1() just in case.

Q please, with X509_VERIFY_PARAM_set1(), do I still need to call X509_STORE_CTX_set_default()?

Yes. The combination - of setting purpose, and then inheriting from the SSL_CTX, more or less matches how BoringSSL/OpenSSL's verifier works (source (ignoring RFC 6125 name matching, which I believe you separately implement)

sleevi · 2021-10-07T17:58:10Z

source/extensions/transport_sockets/tls/context_impl.cc

+    error_details = "Failed to verify certificate chain: X509_STORE_CTX_set_default";
+    return false;


Correct, this is impossible with a hard-coded parameter. To synthetically generate code coverage, you'd have to allow the name ("ssl_server") to be injectable by the test, and then supply an invalid/unregistered name.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-10-07T21:33:45Z

/assign @yanavlasov

danzh2010 · 2021-10-08T15:29:22Z

/retest

repokitteh-read-only · 2021-10-08T15:29:26Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18309 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-10-08T15:55:11Z

@sleevi @yanavlasov PTAL

sleevi

LGTM from a Boring/OpenSSL perspective.

set default on X509_STORE_CTX

b3cace9

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from asraa, ggreenway and lizan as code owners September 29, 2021 18:35

added test

e634d47

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the certverify branch from 6713370 to e634d47 Compare September 29, 2021 18:39

repokitteh-read-only bot assigned wbpcode Sep 30, 2021

RyanTheOptimist reviewed Sep 30, 2021

View reviewed changes

danzh1989 added 2 commits September 30, 2021 14:43

comment

691701b

Signed-off-by: Dan Zhang <danzh@google.com>

comments

50cda56

Signed-off-by: Dan Zhang <danzh@google.com>

RyanTheOptimist previously approved these changes Sep 30, 2021

View reviewed changes

jmarantz dismissed RyanTheOptimist’s stale review via 50cda56 October 1, 2021 00:42

wbpcode reviewed Oct 1, 2021

View reviewed changes

danzh1989 added 2 commits October 1, 2021 11:51

comment

6f575ea

Signed-off-by: Dan Zhang <danzh@google.com>

spelling

bf4fb3b

Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 4 commits October 4, 2021 16:11

add test for coverage

455c88d

Signed-off-by: Dan Zhang <danzh@google.com>

test coverage

c296145

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into certverify

e4b23d5

Signed-off-by: Dan Zhang <danzh@google.com>

format

9e791bd

Signed-off-by: Dan Zhang <danzh@google.com>

wbpcode reviewed Oct 6, 2021

View reviewed changes

wbpcode previously approved these changes Oct 7, 2021

View reviewed changes

repokitteh-read-only bot assigned alyssawilk Oct 7, 2021

sleevi reviewed Oct 7, 2021

View reviewed changes

X509_VERIFY_PARAM_set1

7073029

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed wbpcode’s stale review via 7073029 October 7, 2021 21:26

repokitteh-read-only bot assigned yanavlasov Oct 7, 2021

sleevi approved these changes Oct 8, 2021

View reviewed changes

yanavlasov approved these changes Oct 8, 2021

View reviewed changes

yanavlasov merged commit 351c0ca into envoyproxy:main Oct 8, 2021

		error_details = "Failed to verify certificate chain: X509_STORE_CTX_set_default";
		return false;

		// This is a cert same as test/config/integration/certs/servercert.pem but with extKeyUsage:
		// clientAuth.

-  }
+  }
+  if (!X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(ctx.get()),
+                                                        SSL_CTX_get0_param(ssl_ctx))) {
+    error_details = "Failed to verify certificate chain: X509_VERIFY_PARAM_set1";
+    return false;
+  }

Conversation

danzh2010 commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Sep 29, 2021

Uh oh!

repokitteh-read-only bot commented Sep 29, 2021

Uh oh!

lizan commented Sep 30, 2021

Uh oh!

repokitteh-read-only bot commented Sep 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Oct 5, 2021

Uh oh!

repokitteh-read-only bot commented Oct 5, 2021

Uh oh!

wbpcode commented Oct 6, 2021

Uh oh!

repokitteh-read-only bot commented Oct 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

danzh2010 commented Sep 29, 2021 •

edited

Loading

wbpcode left a comment •

edited

Loading

wbpcode commented Oct 2, 2021 •

edited

Loading

danzh2010 Oct 7, 2021 •

edited

Loading

wbpcode Oct 7, 2021 •

edited

Loading

wbpcode Oct 7, 2021 •

edited

Loading

wbpcode left a comment •

edited

Loading