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

Add --must-staple flag #2667

Merged
merged 3 commits into from May 13, 2016
Merged

Add --must-staple flag #2667

merged 3 commits into from May 13, 2016

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Mar 15, 2016

Fixes #2626

@jsha jsha force-pushed the must-staple branch 3 times, most recently from 9a1e68e to 09a38c7 Compare March 16, 2016 03:43
@pde
Copy link
Member

pde commented Mar 16, 2016

Reading the help for this flag, one might naively expect it to integrate with the Apache (and Nginx?) plugins to actually do the stapling. If not it should be clearly documented that it simply changes a property of the obtained cert and that (for now) the user will have to set things up manually.

@pde
Copy link
Member

pde commented Mar 16, 2016

@sagi would you be interested in implementing the plugin-side of a must staple enhancement?

@pde pde added this to the 0.6.0 milestone Mar 16, 2016
@jsha
Copy link
Contributor Author

jsha commented Mar 16, 2016

Clarified the help text to indicate it doesn't do the autoconfiguration.

@sagi
Copy link
Member

sagi commented Mar 17, 2016

@pde yes! thanks. I'll start working on it this weekend :)

@pde pde assigned sagi Mar 17, 2016
@jsha
Copy link
Contributor Author

jsha commented Mar 18, 2016

The relevant issue for the enhancement part is at #930. Leaving this PR open for now in case it requires modification as part of that work.

@pde
Copy link
Member

pde commented Mar 22, 2016

@sagi have you started looking at this yet? Any thoughts on whether we should merge this PR as is, or wait for possible tweaks to match an Apache OCSP stapling component?

@sagi
Copy link
Member

sagi commented Mar 22, 2016

@pde yes. Lets call the extension X, the minimum apache version that supports OCSP Stapling V, Letsencrypt client L, the --must-staple flag F and a flag that disables X by NX

My thoughts are:

The gist is that X should be enabled by default and that X adds both the must staple to the CSR and defines the apache config for OCSP Stapling (see this for more context).

pseudocode:

If apache >= V:
    if NX is off:
        L disregards F and applies X.
    else:
        L regards F and doesn't apply X.
else:
    L disregards NX, regards F and doesn't apply X.

What are your thoughts @jsha, @pde ?

@jsha
Copy link
Contributor Author

jsha commented Mar 22, 2016

Strongly disagree. Must Staple is still very new, and many servers (including Apache) are likely to have problems. I think the Must Staple extension should only be by request from adventurous users, but Stapling can be turned on by default because it's very low risk.

@sagi
Copy link
Member

sagi commented Mar 22, 2016

I erred. I agree with @jsha.

@jsha
Copy link
Contributor Author

jsha commented Apr 8, 2016

Now that 0.5.0 is released, can we land this?

@jsha jsha unassigned sagi May 6, 2016
@pde
Copy link
Member

pde commented May 11, 2016

This never got a "LGTM" from Sagi. Sagi, should we merge it?

@pde pde assigned sagi May 11, 2016
@pde
Copy link
Member

pde commented May 11, 2016

And @jsha, want to merge master?

@pde pde modified the milestones: 0.6.0, 0.7.0 May 11, 2016
@sagi
Copy link
Member

sagi commented May 12, 2016

LGTM.

@pde pde merged commit 01a528c into master May 13, 2016
@pde pde deleted the must-staple branch July 14, 2016 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants