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/tls: add SetOCSPStaple function #14878

Open
gregory-m opened this issue Mar 19, 2016 · 9 comments

Comments

@gregory-m
Copy link
Contributor

@gregory-m gregory-m commented Mar 19, 2016

OCSPStaple should be updated by server after it expiration.
Today we can't update it directly without data races, and the only one workaround is use tls.Config.GetCertificate and return certificate with OCSPStaple populated.

Can we add SetOCSPStaple function with mutex to tls.Certificate (just like SetSessionTicketKeys) to simplify this?

Thanks.

@gregory-m gregory-m changed the title tls.Certificate: Add SetOCSPStaple function Proposal tls.Certificate: Add SetOCSPStaple function Mar 19, 2016
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Mar 21, 2016

/cc @agl

@rakyll rakyll added the Proposal label Mar 22, 2016
@rakyll rakyll changed the title Proposal tls.Certificate: Add SetOCSPStaple function proposal: tls.Certificate: Add SetOCSPStaple function Mar 22, 2016
@bradfitz bradfitz modified the milestone: Unplanned Apr 7, 2016
@adg

This comment has been minimized.

Copy link
Contributor

@adg adg commented Aug 15, 2016

Ping @agl. Does this make sense?

@agl agl self-assigned this Aug 19, 2016
@adg

This comment has been minimized.

Copy link
Contributor

@adg adg commented Sep 26, 2016

Ping

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Sep 30, 2016

Yes, this makes sense. I had originally expected people to atomically switch the tls.Config for this sort of thing, but that doesn't appear to be how things are working out.

@adg adg modified the milestones: Go1.9, Unplanned Oct 31, 2016
@bradfitz bradfitz changed the title proposal: tls.Certificate: Add SetOCSPStaple function crypto/tls: add SetOCSPStaple function Oct 31, 2016
@kreichgauer

This comment has been minimized.

Copy link
Contributor

@kreichgauer kreichgauer commented Dec 8, 2016

Chatted with @agl about this. tls.Certificate is occasionally passed by value (e.g., tls.LoadX509KeyPair returns a Certificate; tls.Config.Certificates is a []tls.Certificate). So we would end up copying mutexes in a few places. AFAICT this is technically okay because copies would be made shortly after initialization (i.e. before anyone could reasonably call SetOCSPStaple). Though I don't know if this would pass review.

Alternatively, we could add a GetOCSPStable callback, returning a staple for a given certificate. But would either solution really add much convenience over using GetCertificate? Maybe improving documentation of Certificate.OCSPStaple is a better solution.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Jul 18, 2017

How's it going with this issue? There hasn't been much activity since early December 2016, but the proposal was accepted, or are we too late in the cycle to do anything?

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jul 22, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Unplanned Jun 6, 2018
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Dec 3, 2019

This didn't get implemented, and GetCertificate and GetConfigForClient became the standard way to update anything in the server configuration. Caddy for example uses it to dynamically set OCSPStaple.

I think we should not add complexity to tls.Certificate, nor yet another callback. Reverting the Proposal-Accepted and passing it to the proposal committee.

@gregory-m

This comment has been minimized.

Copy link
Contributor Author

@gregory-m gregory-m commented Dec 4, 2019

I also poked the code after it was accepted, and I don't see good way to implement it without rewriting huge part of code because of problems mentioned by @kreichgauer.

@rsc rsc added this to Incoming in Proposals Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2019

Based on the lack of movement since the acceptance 3 years ago and the implementation difficulties noted above, and also the general lack of activity on this issue, this now seems like a likely decline.

Leaving open for a week for final comments.

@rsc rsc changed the title crypto/tls: add SetOCSPStaple function proposal: crypto/tls: add SetOCSPStaple function Dec 4, 2019
@rsc rsc moved this from Incoming to Likely Decline in Proposals Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.