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

crypto/tls: implement OCSP Must-Staple #22274

Open
nhooyr opened this issue Oct 14, 2017 · 8 comments
Open

crypto/tls: implement OCSP Must-Staple #22274

nhooyr opened this issue Oct 14, 2017 · 8 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Oct 14, 2017

A bit over a year ago, someone created an issue to add support for RFC 7633 TLS Feature Extension into crypto/tls but it was closed by @agl because he felt it was premature and that OCSP stapling wasn't really supported as a client in Go (not sure if that has changed?).

See #13074

It's been more than a year so I'm creating this issue to see what he thinks of it now.

@odeke-em odeke-em changed the title crypto/tls: Implement RFC 7633 TLS Feature Extension Proposal: crypto/tls: Implement RFC 7633 TLS Feature Extension Oct 14, 2017
@gopherbot gopherbot added this to the Proposal milestone Oct 14, 2017
@odeke-em
Copy link
Member

Another one for you s'il vous plait @agl.

@agl
Copy link
Contributor

agl commented Oct 14, 2017

Firstly, there's nothing stopping you from using ExtraExtensions and Extensions to implement this if you wish in crypto/x509, the question is really whether OCSP stapling, must-staple etc should be wound throughout the code. Probably as both a client and server if we're going to do a good job of it.

Firefox supports must-staple now and Chrome has Expect-stable support. I think Cloudflare have it on blog.cloudflare.com.

So it's plausible, but a bigger job than can reasonably happen in the 1.10 cycle.

@ianlancetaylor
Copy link
Member

Proposal accepted based on @agl's comment. Please go ahead and send in an implementation.

@ianlancetaylor ianlancetaylor added Proposal-Accepted help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Mar 19, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Mar 19, 2018
@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Nov 22, 2019
@FiloSottile FiloSottile changed the title Proposal: crypto/tls: Implement RFC 7633 TLS Feature Extension crypto/tls: implement OCSP Must-Staple Jan 21, 2020
@FiloSottile
Copy link
Contributor

Retitled to reflect that this is about implementing OCSP Must-Staple across crypto/tls and crypto/x509.

@divjotarora
Copy link

@FiloSottile @agl Until this is done, what are my options to perform OCSP verification in a client (non-HTTP client, if that matters). For my use case, I need to be able to determine whether or not the certificate presented by the server has a Must-Staple extension, the stapled response returned by the server, if any, and the OCSP responder endpoint for the certificate presented by the server.

I've seen two recommended options so far:

  1. Use the VerifyPeerCertificate callback in the tls.Config. I like this idea because crypto/tls will abort the handshake and report a proper error if OCSP verification fails, but I cannot figure out how to access the server's stapled OCSP response.

  2. Let the handshake complete normally and then use the tls.Conn.ConnectionState method to access the ConnectionState, which has the certificate chain presented by the server and the stapled OCSP response. I saw this approach in this talk. It could work, but it might report that the handshake has been successfully completed in server logs.

Any suggestions?

@rolandshoemaker
Copy link
Member

Randomly stumbled across this and thought it might be an interesting issue to tackle (that is if nobody else is already working on it!).

It seems like there are two real units of work here:

  1. in crypto/tls on the client side enforcing that if the presented leaf certificate contains a tls feature extension with status_request that the ServerHello message contained an OCSP response in the certificate status message, and perhaps adding some safeguards to the server side so you can't present a cert containing the extension if you haven't provided a OCSP response.
  2. in crypto/x509 adding OCSP response validation, probably to Certificate.Verify.

Support for RFC 7633 can be added by just accomplishing (1), but that ends up off-loading significant responsibility to the user who might not really be paying attention. On the other hand bundling the two changes together makes a significantly more complex changeset.

@rolandshoemaker
Copy link
Member

Actually (2) probably doesn't make much sense, the existing x/crypto/ocsp logic can just be called directly from crypto/tls (and trying to introduced that into crypto/x509 would create a fun circular dependency anyway). Overall that makes the change somewhat less complicated.

@ancarda
Copy link

ancarda commented Sep 9, 2020

Firstly, there's nothing stopping you from using ExtraExtensions and Extensions to implement this if you wish

Would anyone be open to a patch to crypto/x509 that checks for this extension and exposes it as a bool in the Certificate struct? Kinda like IsCA, but for checking if OCSP Must Staple is set?

type Certificate struct {
    ...
    // If this certificate requires OCSP Stapling.
    MustStapleOCSP bool
    ...
}

It seems a lot easier than checking for the extension by its ID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

No branches or pull requests

9 participants