tls: Add CRL support by adunham-stripe · Pull Request #2255 · envoyproxy/envoy

adunham-stripe · 2017-12-22T00:20:05Z

Description:
Adds support for CRLs. Fixes issue #1496. Duplicate version of #2077.

API Changes:
See Data Plane PR: envoyproxy/data-plane-api#358

Risk Level: Medium (TLS is scary!)

Testing:
Wrote some automated tests that test that revoked certs can't connect while non-revoked certs can.

PiotrSikora · 2017-12-22T01:01:58Z

This is (common?) interface and not implementation, so I don't think you should use BoringSSL-specific types here.

So, there was a request in the previous PR to have the class hold a bssl::UniquePtr<X509_CRL> so that invalid data would be caught sooner. Agreed that it's a bit ugly to have BoringSSL-specific types in the interface, so happy to make whichever changes you / @ggreenway would prefer.

I agree with @PiotrSikora that we shouldn't expose boringssl at this interface level. I haven't fully reviewed the change but can we find a way to hide this? We might need to add a CRL interface also.

So, here's three possible options:

Have ContextConfig contain a DataSource, or just the raw bytes for a CRL.
Pros: we don't expose BoringSSL in this interface; simple.
Cons: if the CRL is invalid, we won't find out until we try to instantiate a listener (since that's when we turn the bytes into a CRL).

Have ContextConfig contain the BoringSSL types.
Pros: we parse the CRL ~immediately and thus can error at config load time; already exists.
Cons: doesn't feel like ContextConfig should have BoringSSL types.

Introduce a CRL interface (or something similar), parse the CRL up-front, wrap into the interface and use that here.
Pros: probably can get the best of both worlds (verifying the CRL up-front and hiding the concrete type).
Cons: complicated; need to make sure that whatever parses the config does so into a format that the ContextConfig implementation understands (i.e. there's coupling between config loading and the ContextConfigImpl).

Thoughts?

Yeah agreed this is a little tricky. Some random ideas:

Variant of (1) where we expose the raw bytes, but also verify that it's valid inside the config and throw if not. (Basically we would end up loading the CRL twice, but I think that's probably not a big deal).

Some type of visitor pattern, where the ContextImpl calls back into the config and the config sets up the SSL context. This would require some dynamic_cast to get at the SSL inside the config impl, but we are already doing that in various places inside config impl for similar reasons, so I think it's probably OK.

FWIW, #2248 is just loading and exposing raw bytes, without verifying the content. We could start loading them twice, but this will start being costly at scale, and will hurt loading times.

I think the most pragmatic solution is to just expose raw bytes in this interface, and catch any data-validation errors in the ContextImpl. We already catch many similar types of errors there.

PiotrSikora · 2017-12-22T01:02:03Z

v1 APIs are deprecated, so I'm not sure if we're supposed to keep adding things here? cc @mattklein123 @htuch

At this point v2 is required, v1 is optional but not blocked. If folks want to add and doc no problem.

PiotrSikora · 2017-12-22T01:04:38Z

You should probably wait for ContextConfigImpl::readDataSource() from #2248.

+1

If not though, change to "if (!crl_conf.filename().empty()) {"

PiotrSikora · 2017-12-22T01:04:46Z

You might want to look at similar code in #2248, and use the list iterator from bssl::UniquePtr.

Ooh, that's good to know - I didn't realize there was an iterator on STACKs. Also, regarding the similar code: as I read it, #2248 will load CRLs from the file passed as a CA cert; I don't want to duplicate work, but $WORK definitely needs to be able to load CRLs from a different file from CAs (we essentially fetch CRLs on a cronjob, whereas the CA certs are placed with config management). What are your thoughts on reducing duplication here?

Yeah, CRLs will be loaded from trusted_ca as well, since that's what the filesystem-based API is currently doing, and I didn't want to change the behavior and suddenly start ignoring CRLs from the trusted_ca file... Having said that, I don't think that's a common use case and handling CRLs via separate field is definitely OK.

PiotrSikora · 2017-12-22T01:04:59Z

You should use X509_CRL_up_ref() instead of this hack.

Thanks for the tip - switched to that!

mattklein123 · 2017-12-22T01:29:49Z

@ggreenway do you mind also taking a look?

ggreenway

Overall looks good, except I think this should wait for readDataSource(), as @PiotrSikora suggested.

ggreenway · 2018-01-02T17:24:04Z

Reminder: Point at your real data-plane-api commit

Done, thanks! I just pointed at the current commit on master.

(rebased to fix merge conflict)

ggreenway · 2018-01-02T17:27:20Z

I think the most pragmatic solution is to just expose raw bytes in this interface, and catch any data-validation errors in the ContextImpl. We already catch many similar types of errors there.

ggreenway · 2018-01-02T17:30:23Z

+1

If not though, change to "if (!crl_conf.filename().empty()) {"

ggreenway · 2018-01-02T17:32:37Z

Please also add a test case for invalid crl (empty file maybe?)

Done! I think an empty file is actually okay (list with 0 elements), so I added a file with invalid PEM data.

adunham-stripe · 2018-01-02T22:18:33Z

Okay, I believe that I've made all the review changes that have been requested, except for using ContextConfigImpl::readDataSource() and putting the CRL bytes in the context. I'll wait for #2248 to land, and then rebase on that and use readDataSource to read the CRL into the context as bytes.

ggreenway · 2018-01-12T18:23:47Z

#2248 is waiting on some other things, so it will be at least another week. @adunham-stripe would you rather wait on that, or proceed without it? Or maybe you can check with @PiotrSikora and just add readDataSource() in this PR to unblock this?

adunham-stripe · 2018-01-12T21:15:50Z

I'm happy to take either approach! We don't have an immediate need for CRLs yet, so I'm okay waiting and seeing how #2248 lands first. Though, if you'd rather get this PR moving sooner, and if @PiotrSikora is onboard, happy to add readDataSource() here too.

ggreenway · 2018-01-12T21:30:37Z

I'm fine with however you want to proceed; I don't think anyone is blocked on CRLs. I just wanted to let you know the status of things and give you options.

adunham-stripe · 2018-01-12T21:33:23Z

Thank you! 😁 I'm going to hold off for now, then; I've subscribed to the other PR and will follow up after it gets merged 👍

mattklein123 · 2018-01-23T00:30:51Z

@adunham-stripe the other PR merged if you want to finish this up. Thank you!

adunham-stripe · 2018-01-23T00:37:46Z

Yep, saw that - nice job on the quick turnaround from you folks! I've rebased on master and am just running tests on the changes that @ggreenway requested. Hoping to push the changes by EOD.

adunham-stripe · 2018-01-23T05:44:06Z

@mattklein123 - Okay, I just pushed the changes that @ggreenway requested in their review, and fixed tests. Let me know if there's anything else you folks would like!

mattklein123

Quick skim LGTM, small comment.

@PiotrSikora can you review? @ggreenway is out this week. Thank you!

mattklein123 · 2018-01-23T18:24:41Z

We won't ever support inline in v1. I would remove this TODO. Did you add docs for v1 also in data-plane-api? I can't remember.

Removed! And yes, I did add documentation here.

PiotrSikora · 2018-01-24T18:59:42Z

Could you move this (and all other changes, really) to right after handing of trusted CA? It pretty much complements it, and the code is so similar that it doesn't make sense to separate it by handling of certificate chain and private key.

Erm, I meant to move the certificateRevocationList / certificateRevocationListPath handling in all the files, not just this one (i.e. you should also move all the new code in context_config.h, tls_context_json.cc, context_config_impl.h and context_impl.cc).

Ah, gotcha - moved the rest!

PiotrSikora · 2018-01-24T19:01:44Z

Nit: s/cs/store_ctx/

PiotrSikora · 2018-01-24T19:01:56Z

X509_V_FLAG_CRL_CHECK|X509_V_FLAG_CRL_CHECK_ALL

Do we actually want to enable X509_V_FLAG_CRL_CHECK_ALL? I had strange failures when turning that flag on during testing, and there's at least one bug still open about it.

I'm pretty sure we want it. What was the error, do you recall?

PiotrSikora · 2018-01-24T19:03:29Z

Could you replace this with EXPECT_THROW_WITH_MESSAGE and proper message, to make sure it fails because of invalid CA and not for other reason (like CRL defined, but no trusted CA)?

PiotrSikora · 2018-01-24T19:04:37Z

Could you add test for v2 with inlined CRL as well? See similar tests in test/server/listener_manager_impl_test.cc , (...which probably should be moved to this file at some point).

PiotrSikora · 2018-01-24T19:05:50Z

Do we need this file?

PiotrSikora · 2018-01-24T19:06:01Z

Do we need this file?

PiotrSikora · 2018-01-24T19:06:36Z

Could you replace this with inline test?

I don't think I can do that easily since the v1 config doesn't support inline data, AFAIK. Happy to write out a temporary file during the test, perhaps?

It's fine to replace the existing v1 test with v2 test, since that's the recommended syntax anyway...

Also, there should be v2 tests for success / invalid content (i.e. replacement for this file) / invalid file path.

Added success / failure cases to the v2 tests - do you want me to just remove the v1 tests in test/common/ssl/context_impl_test.cc?

Up to you, I'm fine either way.

PiotrSikora · 2018-01-24T19:09:15Z

We could simply revoke one of the existing certs and add it to the CRL.

If you prefer revoked_cert.pem, that's fine with me, but perhaps reuse san_with_dns.cfg to produce cert instead of adding another copy of the same .cfg file?

Good point - I just pushed a commit that revokes an existing cert and tweaks the tests.

PiotrSikora · 2018-01-24T19:09:38Z

s/revoked_cert.crl/ca_cert.crl

…nfiguration Signed-off-by: Andrew Dunham <adunham@stripe.com>

Signed-off-by: Andrew Dunham <adunham@stripe.com>

adunham-stripe · 2018-01-24T23:10:14Z

@PiotrSikora - Okay, I rebased to fix a merge conflict, and added a couple commits that address almost all of your feedback. I'm going to test the X509_V_FLAG_CRL_CHECK_ALL flag now, though the rest of the changes should be good for re-review.

Signed-off-by: Andrew Dunham <adunham@stripe.com>

adunham-stripe · 2018-01-25T01:57:32Z

Well, after much testing, it appears that I can't reproduce the previous error I was seeing, so I just pushed a commit to add the X509_V_FLAG_CRL_CHECK_ALL flag.

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora · 2018-01-25T22:04:39Z


 rm *csr
 rm *srl
+rm -r crl


s/rm -r crl/rm crl_*/

PiotrSikora · 2018-01-25T22:05:01Z

@@ -0,0 +1,36 @@
+[req]


This file is no longer being used.

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora · 2018-01-25T22:22:38Z

+          fmt::format("Failed to load CRL from {}", config.certificateRevocationListPath()));
+    }
+
+    X509_STORE* store_ctx = SSL_CTX_get_cert_store(ctx_.get());


Sorry, I know that I suggested it, but this should be actually store, not store_ctx.

Also, I know that's not your code (and I cannot comment on it), but in the CA handling, there is a call to SSL_CTX_get_cert_store() on each iteration, instead of before the loop, like you're doing here.

Could you update the old code to match yours? Thanks!

PiotrSikora · 2018-01-25T22:25:34Z


 rm *csr
 rm *srl
+rm -r crl_*


You don't need -r anymore.

PiotrSikora · 2018-01-25T22:26:07Z

Up to you, I'm fine either way.

…' from script Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora · 2018-01-25T22:34:32Z

      ca_cert_(readDataSource(config.validation_context().trusted_ca(), true)),
      ca_cert_path_(getDataSourcePath(config.validation_context().trusted_ca())),
+      certificate_revocation_list_(readDataSource(config.validation_context().crl(), true)),
+      certificate_revocation_list_path_(getDataSourcePath(config.validation_context().crl())),


There is one more thing missing here. You should throw an exception (down in the function body) if CRL is set, but there is no CA. All v1 tests are doing this, which obviously shouldn't work.

Yeah, good point - I've pushed a commit that adds this, along with some tests.

…icate Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora · 2018-01-26T01:26:19Z

  // TODO(htuch): Support multiple hashes.
  ASSERT(config.validation_context().verify_certificate_hash().size() <= 1);
+  if (ca_cert_.empty() && !certificate_revocation_list_.empty()) {
+    throw EnvoyException("Cannot have a CRL without a CA certificate");


Sigh, it looks that this comment got lost somehow.

This should be:

throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA certificates", config.certificateRevocationListPath()));

to match other errors.

@PiotrSikora - Hah, no worries! Just pushed a commit that changes this and updates the tests.

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora · 2018-01-26T01:48:18Z

+  )EOF";
+
+  EXPECT_THROW_WITH_REGEX(loadConfigJson(json), EnvoyException,
+                          "Failed to load CRL from .*/not_a_crl.crl$");


Nit: missing ^ to indicate start of the message (you're using it in other places).

PiotrSikora

One last thing (I promise!), you might want to add some comment to test/common/ssl/test_data/README.md to mention (probably in CA's description) that there is CRL that revokes san_with_dns_cert.pem.

Signed-off-by: Andrew Dunham <adunham@stripe.com>

adunham-stripe · 2018-01-26T01:57:33Z

@PiotrSikora - Nit fixed, and comment added!

PiotrSikora · 2018-01-26T02:00:40Z

 - **CA**: Certificate Authority for **No SAN**, **SAN With URI** and **SAN With
  DNS**. It has the self-signed certificate *ca_cert.pem*. *ca_key.pem* is its
-  private key.
+  private key.  Additionally, we create a CRL for this CA (*ca_cert.crl*) that


One space should be enough for everybody ;)

FWIW, this also breaks ci/circleci: format check.

Aaaaaaagh 😛

Okay, fixed.

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora

LGTM, @mattklein123 please check style and language.

mattklein123

LGTM. Thank you @adunham-stripe and @PiotrSikora!

comments fixed

mattklein123 · 2018-01-26T05:30:09Z

@adunham-stripe sorry one last thing, it would be cool if you could do a quick doc PR to this page: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/ssl to mention that we now support CRLs (with link to config option). Thank you if you have time. cc @PiotrSikora

adunham-stripe · 2018-01-26T20:55:49Z

Thanks very much for helping get this merged, everyone! I submitted the docs PR over at envoyproxy/data-plane-api#437

adunham-stripe mentioned this pull request Dec 22, 2017

tls: Add CRL support [WIP] #2077

Closed

adunham-stripe force-pushed the adunham/add-crl-support branch 2 times, most recently from 4d76125 to 181790a Compare December 22, 2017 00:37

PiotrSikora suggested changes Dec 22, 2017

View reviewed changes

mattklein123 requested a review from ggreenway December 22, 2017 01:29

ggreenway previously requested changes Jan 2, 2018

View reviewed changes

adunham-stripe force-pushed the adunham/add-crl-support branch from d852d77 to b4252e3 Compare January 2, 2018 22:00

mattklein123 assigned ggreenway Jan 12, 2018

adunham-stripe force-pushed the adunham/add-crl-support branch 2 times, most recently from 966eb23 to fbc693a Compare January 23, 2018 04:37

mattklein123 reviewed Jan 23, 2018

View reviewed changes

mattklein123 assigned PiotrSikora Jan 23, 2018

PiotrSikora suggested changes Jan 24, 2018

View reviewed changes

adunham-stripe force-pushed the adunham/add-crl-support branch from 453872b to 0da2edd Compare January 24, 2018 22:55

adunham-stripe added 7 commits January 24, 2018 14:57

Add support for 'crl' field in CertificateValidationContext and v1 co…

d4f4c64

…nfiguration Signed-off-by: Andrew Dunham <adunham@stripe.com>

Load CRLs from context

26a9abc

Signed-off-by: Andrew Dunham <adunham@stripe.com>

Add CRLs to X509_STORE if any are present

40e9985

Signed-off-by: Andrew Dunham <adunham@stripe.com>

Add test data for CRLs

3599a9d

Signed-off-by: Andrew Dunham <adunham@stripe.com>

Add test that verifies our configuration parses

c1b339b

Signed-off-by: Andrew Dunham <adunham@stripe.com>

Add test that verifies that a revoked certificate + CRL works

99abed1

Signed-off-by: Andrew Dunham <adunham@stripe.com>

Formatting fixes

6d15f1f

Signed-off-by: Andrew Dunham <adunham@stripe.com>

Review feedback: move CRL configuration into ca_cert.cfg

5b53667

Signed-off-by: Andrew Dunham <adunham@stripe.com>

adunham-stripe force-pushed the adunham/add-crl-support branch from 9181a27 to 204fa7c Compare January 25, 2018 01:56

Review feedback: add the X509_V_FLAG_CRL_CHECK_ALL flag

ee2530f

Signed-off-by: Andrew Dunham <adunham@stripe.com>

adunham-stripe force-pushed the adunham/add-crl-support branch from 204fa7c to ee2530f Compare January 25, 2018 01:59

PiotrSikora reviewed Jan 25, 2018

View reviewed changes

adunham-stripe added 2 commits January 25, 2018 14:07

Review feedback: fix test data script and remove unnecessary file

c1c66ac

Signed-off-by: Andrew Dunham <adunham@stripe.com>

Review feedback: move remaining CRL code to immediately after CA code

f1045df

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora suggested changes Jan 25, 2018

View reviewed changes

Review feedback: Clean up X509_STORE* code and remove unnecessary '-r…

8ce73bf

…' from script Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora reviewed Jan 25, 2018

View reviewed changes

Review feedback: throw an exception if we have a CRL but no CA certif…

d4c3ccd

…icate Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora reviewed Jan 26, 2018

View reviewed changes

Review feedback: change exception message to match other errors

7e07bf0

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora reviewed Jan 26, 2018

View reviewed changes

PiotrSikora suggested changes Jan 26, 2018

View reviewed changes

Review feedback: fix missing '^' and add test data comment

fa2dfb7

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora reviewed Jan 26, 2018

View reviewed changes

Review feedback: formatting fixes

cdbb2b9

Signed-off-by: Andrew Dunham <adunham@stripe.com>

PiotrSikora approved these changes Jan 26, 2018

View reviewed changes

mattklein123 approved these changes Jan 26, 2018

View reviewed changes

mattklein123 merged commit 61a9741 into envoyproxy:master Jan 26, 2018

adunham-stripe mentioned this pull request Jan 26, 2018

Add CRL support to architecture overview envoyproxy/data-plane-api#437

Merged

adunham-stripe deleted the adunham/add-crl-support branch August 29, 2018 02:59

Conversation

adunham-stripe commented Dec 22, 2017

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!

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!

mattklein123 commented Dec 22, 2017

Uh oh!

ggreenway left a comment

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!

adunham-stripe commented Jan 2, 2018

Uh oh!

ggreenway commented Jan 12, 2018

Uh oh!

adunham-stripe commented Jan 12, 2018

Uh oh!

ggreenway commented Jan 12, 2018

Uh oh!

adunham-stripe commented Jan 12, 2018

Uh oh!

mattklein123 commented Jan 23, 2018

Uh oh!

adunham-stripe commented Jan 23, 2018

Uh oh!

adunham-stripe commented Jan 23, 2018

Uh oh!

mattklein123 left a comment

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

PiotrSikora Jan 24, 2018 •

edited

Loading

PiotrSikora Jan 24, 2018 •

edited

Loading