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

proposal: crypto/x509: implement OCSP verifier #40017

Open
FiloSottile opened this issue Jul 3, 2020 · 15 comments
Open

proposal: crypto/x509: implement OCSP verifier #40017

FiloSottile opened this issue Jul 3, 2020 · 15 comments
Assignees
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-Hold
Milestone

Comments

@FiloSottile
Copy link
Contributor

Although we have a golang.org/x/crypto/ocsp package, we don't in fact have an OCSP verifier. The existing package provides serialization and parsing, but not APIs for "get me an OCSP response for this certificate" and "given this certificate and this OCSP response tell me if it's revoked". (They are separate because you only want the latter when checking stapled responses.)

There is a lot of complexity, subtlety, and dark knowledge involved in OCSP unfortunately. Here are a few notes on things the verifier needs to do (from reading this thread):

  • check that the response is signed directly by the issuer (without needing the OCSP EKU) or that it's signed by a Delegated Responder issued directly by the issuer (with the OCSP EKU)
  • for Delegated Responders, not require the EKU to be enforced up the chain
  • check that the signer has the correct KeyUsage
  • for Delegated Responders, require them to be End Entity certificates (i.e. not a CA; this is an off-spec Mozilla check that protected them from the mess of CAs giving the OCSP EKU away to intermediates)
  • for Delegated Responders, maybe check that the id-pkix-ocsp-nocheck extension is present (this is a BR requirement, but if it's not an IETF requirement we might want to skip it)

There are definitely a lot more things to consider (for example, the id-pkix-ocsp-nocheck extension needs to be processed itself), the list above are just notes of things I learned from that one incident.

A difficult question is where to put the code, and how to surface it. We'll want to use it in crypto/tls for Must-Staple (#22274) but it feels like the wrong place for the code to live. An obvious answer would be golang.org/x/crypto/ocsp, but then we can't use it from crypto/x509 without an import loop. Should x509.VerifyOptions have an option to verify a stapled response? Probably, I feel like we'd regret doing OCSP verification separately from path building and certificate verification anyway. What about the API to fetch a response? Maybe that can stay in golang.org/x/crypto/ocsp, separating the concerns of obtaining responses and verifying them.

We should also look around the ecosystem, because surely someone had to implement this, and we should compare results.

@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jul 3, 2020
@gopherbot gopherbot added this to the Proposal milestone Jul 3, 2020
@rolandshoemaker
Copy link
Member

I think this would go a long way towards getting rid of the numerous traps x/crypto/ocsp currently presents users. It definitely feels like response/certificate validation should happen in Cerifitcate.Verify, we'll already have all the requisite information and it feels unlikely that users will want to check OCSP at some other time (either pre or post chain building).

For delegated responders it probably makes sense to ignore whether a delegated responder contains id-pkix-ocsp-nocheck, but it's something response fetching code would probably want to take into consideration. There is also the question of what to do if the EE being verified has id-pkix-ocsp-nocheck but the user provided an OCSP response to verify against (this is a weird corner case, since presumably if the CA put the ext in a leaf it wouldn't be generating an OCSP response for it, but I wouldn't put it past anyone).

Fetching code probably should live in x/crypto/ocsp (or a paired down version of the API could make its way into crypto/x509 as a way of dissuading users from using the more dangerous bits that lurk in x/crypto/ocsp entirely) since the request generation code is already mostly complete and all that would need to be added would be a wrapper around a http.Client which generates the request and extracts the responder URL from the AIA extension in the certificate to check.

There is a bit of a chicken and egg problem with the fetching code and the verification code. For generating the OCSP request you need the leaf issuer, which means you might want to do chain building before you request the OCSP response (otherwise you need to do some pre-processing to determine the issuer, or you just assume the second cert in any chain you were sent is the issuer...), but as previously mentioned it'd probably be nicer to be able to do OCSP verification during chain building. The other less desirable option would be to to add a new Certificate.VerifyOCSP (or something) method which only verifies the OCSP response, so you could do Verify chain -> Fetch OCSP -> Verify OCSP...

@vanbroup
Copy link
Contributor

vanbroup commented Jul 3, 2020

I vote to implement this in Cerifitcate.Verify during the chain building as @rolandshoemaker suggested. If you would go for something like Certificate.VerifyOCSP at some point you would get Certificate.VerifyCRL as not all PKI is based on OCSP.

Ideally VerifyOptions would have an option CheckStatus or similar to let it check the current certificate status either using OCSP or CRL whatever is available in the certificate.

The id-pkix-ocsp-nocheck should be taken into account by the OCSP client when verifying the response but can be ignored when validating certificates of the chain itself. This because, if the responder is using a CA signed response, it should not include the id-pkix-ocsp-nocheck extension and the CA certificate who signed the response should be checked for revocation with it's issuer.

We might also want to cache the response according to the NotAfter, Expires or Cache-Control information.

@sleevi
Copy link

sleevi commented Jul 6, 2020

There is a bit of a chicken and egg problem with the fetching code and the verification code. For generating the OCSP request you need the leaf issuer, which means you might want to do chain building before you request the OCSP response (otherwise you need to do some pre-processing to determine the issuer, or you just assume the second cert in any chain you were sent is the issuer...), but as previously mentioned it'd probably be nicer to be able to do OCSP verification during chain building. The other less desirable option would be to to add a new Certificate.VerifyOCSP (or something) method which only verifies the OCSP response, so you could do Verify chain -> Fetch OCSP -> Verify OCSP...

Right, as Roland suggests, doing it during chain building is the 'desired' implementation.

RFC 4158, Section 3.5.9 discusses some of the trade-offs here. Note that, depending on how you want to handle nocheck, revocation is potentially ideally implemented via a 'reverse' path search. That is, as reflected in RFC 5280, 6.1.3, you revocation check the root, then the intermediate, then the leaf. If, during revocation checking the leaf, you encounter a delegated responder for the intermediate, you can either check just that responder (with the intermediate) or via CRL. Since you can get into responder bounces, you probably have to have a limit, or force a fallback to CRL, or just refuse anything with nocheck (as it seems Microsoft is doing, as best I can tell)

That is, if you have

Intermediate ----> Responder 1
              \--> Responder 2
               \-> Responder 3
                \> Server

It's possible for Server to be responded by Responder 1, who when you check status (i.e. it lacks nocheck), gets a response from Responder 2, which also lacks nocheck and gets a response from Responder 3... etc. So either requiring nocheck or profiling that down seems sensible, or requiring a CRL if nocheck isn't present.

@rolandshoemaker
Copy link
Member

It feels like there are two different paths we can go down here, implementing verification with out-of-band fetching (i.e. verifying using a stapled, or otherwise fetched, response) or verification with in-band fetching (i.e. fetching responses during verification) These can share a good bit of code (basically the actual verification logic) but have different implementation complexities.

It seems like probably the path with the least friction, for now anyway, is implementation out-of-band verification first and then deciding on the more complex problem of adding in-band verification once that is complete.

@tialaramex
Copy link

Fetching OCSP can introduce both privacy and usability surprises. Privacy: Your client is now talking to somebody unexpected (based on an HTTP URL in the AIA OCSP item) and it's telling them which certificate you're currently interested in. It is obliged to use plaintext HTTP for this transaction and although the answers should be signed (so a MITM can't fake the response) the query can be read by anybody on the path.

Usability: Sometimes (too often) OCSP servers aren't reachable. Sometimes this is because they are on the Internet and you can't reach some or all of the public Internet from where you are. Sometimes though it's just the OCSP server is down.

In contrast the Stapled scenario is pretty much worry free.

As a result I'd strongly urge that any opt-in / opt-out type behaviour focuses on these network behaviours (whether to fetch stuff, over HTTP, from some server we discovered in the AIA records) not on disabling / enabling OCSP checking itself.

In particular it's desirable that on the one hand a quiet piece of software doesn't end up broadcasting its existence to the world because it saw some random Let's Encrypt certificate, while on the other hand a certificate with OCSP-must-staple set is always rejected by a Go program if that OCSP response is missing.

@FiloSottile
Copy link
Contributor Author

Yes, however we end up structuring the API, a verification by default will not contact the network.

I'd still like to put fetching in a separate package, but it sounds like it would require a chain, and the chain comes from Verify, but the response would be fed to Verify.

@xCoderly
Copy link

you should keep in mind that not just Verification ( Valid / Invalid ) that's needed, a complete OCSP-stapling diagnose is needed, for example someone want to verify Stapling if activated on any web server or no, and to be able to get information from the OCSP-must-staple response from web server ..

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Nov 11, 2020
@rsc
Copy link
Contributor

rsc commented Nov 18, 2020

Is this really for crypto/x509 and not x/crypto?

@rsc
Copy link
Contributor

rsc commented Nov 18, 2020

Maybe we should put it in x/crypto and figure out how to deal with import loops between the standard library and x repos?

@FiloSottile
Copy link
Contributor Author

It's not (just) a matter of import loops. A good OCSP verifier needs a chain builder to do its job well for all cases, and the thing that has a chain builder is x509.Verify, so the natural place to put the OCSP response to be validated is x509.VerifyOptions. It's also a better and safer API, because Verify just returns an error if the certificate is revoked instead of making you go check something else while holding a chain you might mistake for valid.

OTOH, fetching an OCSP response if it's not stapled requires the chain, which is the output of Verify. This makes it awkward to fetch a OCSP response to put into VerifyOptions if you don't already have a stapled one. How other verifiers solved it has been by making connections to the Internet during Verify, which we are not going to do. We might just accept that this is for stapling primarily.

@rsc
Copy link
Contributor

rsc commented Dec 2, 2020

I am not sure I see a proposal here. Do you have concrete API you are proposing? If not, maybe we should put this on hold until there is API to evaluate.

@rsc rsc moved this from Incoming to Active in Proposals (old) Dec 2, 2020
@rolandshoemaker
Copy link
Member

I think the current API proposal is basically just adding a OCSPResponses field to x509.VerifyOptions.

The main question currently is do we either use a [][]byte, which means we need to re-implement OCSP parsing in crypto/x509, or ocsp.Response, which means we probably need to move the existing x/crypto/ocsp code into crypto/x509 because of the import loops.

@FiloSottile
Copy link
Contributor Author

I think there are multiple open questions on how the fetch-verify cycle looks like. I'm ok with putting this on hold to take it off the active proposal radar, I expect it will take a while.

@rsc
Copy link
Contributor

rsc commented Dec 9, 2020

Putting on hold until there is API to review.

@nightlyone
Copy link
Contributor

@FiloSottile the import loop has a well known solution: The bundler.

That is how you automatically get a version of golang.org/x/net/http2 into the net/http package to support transparent http2 support.

Not the most beautiful solution but a pragmatic, existing and proven one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-Hold
Projects
Status: Hold
Development

No branches or pull requests

9 participants