ocsp: add parsing utilities for ASN.1 OCSP responses #12307
Conversation
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
daniel-goldstein
force-pushed
the
ocsp-response-parsing
branch
from
d0230fa to
5f904ad
Compare
|
@PiotrSikora Can you take a first pass on this? You're the most knowledgable regarding parsing ASN.1. |
|
@PiotrSikora @ggreenway Bumping this since it's been sitting green without a review for a few days. |
|
As a design alternative, can this parsing be done with the openssl/boringssl |
I investigated my own question. Looks like OpenSSL has support for this, but BoringSSL (which we use) does not. |
|
Thanks for the feedback! I left the TODOs because it seemed like the PR was too long as is, but am happy to lump them into this PR.
Yes this is mainly making up for BoringSSL having removed the OCSP data structures, and recommending that clients work with the CBS API for ASN.1 parsing. |
|
I'm getting this whole feature paged back into my brain now. The utilities here are for the purpose of validating that a config-provided OCSP staple, which Envoy will serve to downstream clients during TLS handshakes, is valid (not expired, etc), correct? If so, disregard my comment on checking the signature. I think the goal would be to prevent likely user config errors, not verify trust. Things like using an expired value, or one for the wrong cert (things which you're checking already). Is my understanding correct? |
|
Yes, that's true, responses are only coming from configs. Should I instead leave a note explaining why we don't need to inspect the signature but how it should be verified if responses are fetched dynamically in the future? |
Yes to leaving a comment, but even if we are fetching them directly, I don't think we need to verify it. The client will always verify it; all we're doing here is trying to prevent operator-error misconfigurations (like supplying an OCSP staple for the wrong cert). |
In this case, should I remove/skip the signature-related fields since they won't be processed? |
Yes, I think so, with commenting (as discussed) on why we're skipping them. Also, please make it clear (through naming, comments, whatever) that this code is not intended for verifying an upstream OCSP staple from a server. That requires a different set of validations than what you're doing here. |
|
Thinking through the use-case more as we've been talking about it, I think you can probably do a minimal set of validations: maybe the serial number and hostname and valid time (although even for time, it's not clear what to do if it's not valid right now, or if it's going to become invalid soon (like within a few seconds). The rest of the fields can be skipped. The goal here is to help the operator not make common mistakes. If you disagree, let's discuss :) |
By hostname, do you mean the name of the certificate issuer? If so then I would agree, since that is technically necessary to uniquely identify the certificate. I am fine skipping other superfluous fields to keep things simple, namely:
I think the time period checking at config time is a reasonable assertion, specifically because the validity end time is optional in OCSP responses, and a missing end time implies the response is always "expired" and not suited for stapling. This seems like a good use-case to fail quickly and alert the user that they need to ensure the responses they are delivering to Envoy have a valid window. |
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
Signed-off-by: Daniel Goldstein <[email protected]>
|
@PiotrSikora I think everything else is set here if you could take a look |
PR envoyproxy#12307 added a handrolled OCSP parser, but handled the version field incorrectly. ASN.1 tags are more than just a number. They also have a class (universal, context-specific, application, or private). Class zero is "universal", which is for the built-in types like INTEGER or SEQUENCE. A tag written like [1] is not universal but "context-specific". That means one needs to write CBS_ASN1_CONTEXT_SPECIFIC | 1, not plain 1. Plain 1 is BOOLEAN. ASN.1 TLVs additionally are either constructed or primitive. Explicitly-tagged types are always constructed (they contain other TLVs). The constructed bit is encoded alongside the tag, so CBS/CBB treat it as part of the tag. See https://letsencrypt.org/docs/a-warm-welcome-to-asn1-and-der/ for further background. BoringSSL's CBS/CBB APIs assume familiarity with ASN.1, so reviewers for future ASN.1 PRs in Envoy should read over it. An OCSP response contains the following field: version [0] EXPLICIT Version DEFAULT v1, With Version defined as: Version ::= INTEGER { v1(0) } The outermost tag should be written as CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0. ocsp.cc was using 0. This PR fixes it. (Other tags in ocsp.cc appear to have been correct.) It also fixes the encoding in the test to reflect the EXPLICIT tagging. The test was written as if the version used IMPLICIT tagging. The old code would not have successfully parsed any OCSP response with an explicit version number. Likely the reason this wasn't noticed before is because the field is DEFAULT v1 and v1 is the only defined version. That means any OCSPv1 resolver is not only allowed to omit the version, but *required* to do so under DER. As Envoy only implements v1, it should be rejecting the field, rather than skipping it, but I've left the (originally intended) behavior as-is for now. Finally, this PR fixes some tests in asn1_utility_test.cc to use more plausible test data. The test used tag [UNIVERSAL 0] rather than [0]. ([0] is encoded 0xa0 when constructed.) This PR is necessary to avoid Envoy breaking with future versions of BoringSSL. Tag [UNIVERSAL 0] is particularly fraught because it is reserved for use by the encoding. Notably, it is part of BER indefinite-length EOC markers. Using it as an actual tag has some weird conseequences and future versions of BoringSSL will no longer parse it. Signed-off-by: David Benjamin <[email protected]>
PR #12307 added a handrolled OCSP parser, but handled the version field incorrectly. ASN.1 tags are more than just a number. They also have a class (universal, context-specific, application, or private). Class zero is "universal", which is for the built-in types like INTEGER or SEQUENCE. A tag written like [1] is not universal but "context-specific". That means one needs to write CBS_ASN1_CONTEXT_SPECIFIC | 1, not plain 1. Plain 1 is BOOLEAN. ASN.1 TLVs additionally are either constructed or primitive. Explicitly-tagged types are always constructed (they contain other TLVs). The constructed bit is encoded alongside the tag, so CBS/CBB treat it as part of the tag. See https://letsencrypt.org/docs/a-warm-welcome-to-asn1-and-der/ for further background. BoringSSL's CBS/CBB APIs assume familiarity with ASN.1, so reviewers for future ASN.1 PRs in Envoy should read over it. An OCSP response contains the following field: version [0] EXPLICIT Version DEFAULT v1, With Version defined as: Version ::= INTEGER { v1(0) } The outermost tag should be written as CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0. ocsp.cc was using 0. This PR fixes it. (Other tags in ocsp.cc appear to have been correct.) It also fixes the encoding in the test to reflect the EXPLICIT tagging. The test was written as if the version used IMPLICIT tagging. The old code would not have successfully parsed any OCSP response with an explicit version number. Likely the reason this wasn't noticed before is because the field is DEFAULT v1 and v1 is the only defined version. That means any OCSPv1 resolver is not only allowed to omit the version, but *required* to do so under DER. As Envoy only implements v1, it should be rejecting the field, rather than skipping it, but I've left the (originally intended) behavior as-is for now. Finally, this PR fixes some tests in asn1_utility_test.cc to use more plausible test data. The test used tag [UNIVERSAL 0] rather than [0]. ([0] is encoded 0xa0 when constructed.) This PR is necessary to avoid Envoy breaking with future versions of BoringSSL. Tag [UNIVERSAL 0] is particularly fraught because it is reserved for use by the encoding. Notably, it is part of BER indefinite-length EOC markers. Using it as an actual tag has some weird conseequences and future versions of BoringSSL will no longer parse it. Signed-off-by: David Benjamin <[email protected]>
Signed-off-by: Daniel Goldstein [email protected]
Commit Message: add parsing utilities for ASN.1 OCSP responses
Additional Description: This is the first step in implementing OCSP stapling as described here, adding the capability on top of BoringSSL to parse and validate DER-encoded OCSP responses. It extracts enough to determine the revocation status of a single SSL certificate and the window of time for which the OCSP response can be considered valid. OCSP extensions are not currently processed.
Risk Level: Low/Medium (unused but part of larger feature related to TLS)
Testing: unit tests that include testing on openssl-generated certs and ocsp responses. There should be only two lines that don't have test coverage and they are both a result of BoringSSL potentially returning a nullptr which I have not yet been able to trigger.
Docs Changes:
Release Notes: N/A