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 switch #3151

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

BatSmacker84
Copy link

With this PR Dead, Proxy, and Redirect Hosts are now able to have OCSP Stapling enabled via the webgui with a simple toggle in the SSL tab.

The two config options added when OCSP Stapling is enabled are:

ssl_stapling on;
ssl_stapling_verify on;

The file used for OCSP Stapling is already provided by certbot (chain.pem), so all this PR does is allow for that stapling to occur by enabling those two options in each site's .conf file.
This is already possible in the current version of NPM by putting the two options into the Advanced Settings, but this PR allows for the config to look nicer and makes it easier for users to implement across their proxies.

The database does have to be migrated (a new migration file is included) in order to store if the Host has OCSP Stapling enabled or not.

dead, proxy, and redirect hosts have the option to enable OCSP Stapling in the webgui
@nginxproxymanagerci
Copy link

Docker Image for build 1 is available on DockerHub as jc21/nginx-proxy-manager:github-pr-3151

Note: ensure you backup your NPM instance before testing this PR image! Especially if this PR contains database changes.

@jc21 jc21 added the requires-verification Waiting for one or more people to confirm the fix label Aug 31, 2023
Comment on lines +4 to +5
ssl_stapling on;
ssl_stapling_verify on;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ssl_stapling on;
ssl_stapling_verify on;
ssl_stapling on;
ssl_stapling_verify on;
{% if certificate.provider == "letsencrypt" %}
ssl_trusted_certificate /etc/letsencrypt/live/npm-{{ certificate_id }}/chain.pem;
{% else %}
ssl_trusted_certificate /data/custom_ssl/npm-{{ certificate_id }}/fullchain.pem;
{% endif %}

Should ssl_trusted_certificate be included as well?

From the nginx docs:

For verification to work, the certificate of the server certificate issuer, the root certificate, and all intermediate certificates should be configured as trusted using the ssl_trusted_certificate directive.

Please note that, in my suggestion, I'm unsure about the custom SSL chain 🙂

Copy link

github-actions bot commented Apr 7, 2024

PR is now considered stale. If you want to keep it open, please comment 👍

@github-actions github-actions bot added the stale label Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires-verification Waiting for one or more people to confirm the fix stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants