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

OCSP Stapling was not working #984

Merged
merged 1 commit into from Mar 23, 2018
Merged

OCSP Stapling was not working #984

merged 1 commit into from Mar 23, 2018

Conversation

ghost
Copy link

@ghost ghost commented Nov 19, 2017

The nginx.tmpl was looking for the chain certificate with the file extension of .crt when in fact the chain certificate has a .pem extension. Adjusted the template to ensure SSL stapling was correctly enabled by detecting the correct file.

… was not working due to the incorrect file extension.
@ghost ghost changed the title SSL Stapling was not working OCSP Stapling was not working Nov 19, 2017
@ghost
Copy link
Author

ghost commented Nov 19, 2017

Confirmed with SSL labs before and after and confirmed it is now working ok.

image

@buchdag
Copy link
Member

buchdag commented Dec 6, 2017

I confirm that due to this (.crt instead of .pem) OCSP stapling does not work with projects like letsencrypt-nginx-proxy-companion.

@buchdag
Copy link
Member

buchdag commented Dec 27, 2017

@jwilder , @kamermans could you take a look at this ?

@kamermans
Copy link
Contributor

Thanks @sydoveton and @buchdag, looks good to me! @buchdag, correct me if I'm wrong, but you're saying that if this PR is merged, the fact that the .pem extension is now used will make it easier to use OCSP stapling in the letsencrypt-nginx-proxy-companion?

@kamermans
Copy link
Contributor

Also, I could swear that there was some verbiage in the README about enabling OCSP stapling, but now I don't see it. Maybe you guys recall seeing it? In any case, we should add a section like this to the README after SNI if it is indeed missing:

#### OCSP Stapling
To enable OCSP Stapling for a domain, `nginx-proxy` looks for a PEM certificate containing the trusted
CA certificate chain at `/etc/nginx/certs/<domain>.chain.pem`, where `<domain>` is the domain name in
the `VIRTUAL_HOST` directive.  The format of this file is a concatenation of the public PEM CA
certificates starting with the intermediate CA most near the SSL certificate, down to the root CA.  This is
often referred to as the "SSL Certificate Chain".  If found, this filename is passed to the [NGINX
`ssl_trusted_certificate` directive](http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_trusted_certificate)
and OCSP Stapling is enabled.

@buchdag
Copy link
Member

buchdag commented Jan 4, 2018

@kamermans it will make OCSP work out of the box compared to not working at all, letsencrypt-nginx-proxy-companion automatically generate chain files and creates symlinks to them as /etc/nginx/certs/<domain>.chain.pem

It could be the other way around (modifying letsencrypt-nginx-proxy-companion to create /etc/nginx/certs/<domain>.chain.crt symlinks instead), but the de-facto extension for chain files seem to be .pem (that's what cerbot does).

@buchdag
Copy link
Member

buchdag commented Jan 28, 2018

ping @jwilder

@buchdag
Copy link
Member

buchdag commented Mar 15, 2018

@jwilder again, could you please take a look at this and consider it for merging ? I'm sorry to insist but this is just an incredibly small, six characters change that would just make something work out of the box for letsencrypt-nginx-proxy-companion users instead of not working at all.

It's actually the same person that contributed OCSP support to both nginx-proxy and letsencrypt-nginx-proxy-companion, but the owner of the later decided to change the .crt to .pem while nginx-proxy kept .crt

@jwilder jwilder merged commit 1dce981 into nginx-proxy:master Mar 23, 2018
@jwilder
Copy link
Collaborator

jwilder commented Mar 23, 2018

Thanks @buchdag @kamermans @sydoveton!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants