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 OCSP support #1475

Merged
merged 1 commit into from
Oct 5, 2017
Merged

Add OCSP support #1475

merged 1 commit into from
Oct 5, 2017

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Oct 4, 2017

This PR adds support for OCSP transparently. This means the ingress controller will check the configured SSL certificate for missing intermediate certs and generate PEM file with the complete chain (this is required in NGINX to support OCSP)

No change is required in the existing certificates

fixes #416
fixes kubernetes-retired/contrib#1949

Test image: quay.io/aledbf/nginx-ingress-controller:0.253

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 4, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 4, 2017
@ConnorJC3
Copy link

I can confirm this works with kube-lego; however, I'm going to copy-paste my potential concerns from Slack:

Although, is there a fallback to not staple?
As if the ca doesn't support ocsp it can't be stapled

It's kinda hard for me to test that, because all my certs are let's encrypt, but I can see that being a problem

@aledbf
Copy link
Member Author

aledbf commented Oct 5, 2017

As if the ca doesn't support ocsp it can't be stapled

In that case nginx disables ocsp.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 5, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 43.413% when pulling e7f8488 on aledbf:ocsp into 1c6ff88 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 43.616% when pulling f6ba3ab on aledbf:ocsp into 1c6ff88 on kubernetes:master.

@aledbf aledbf merged commit cd6d90d into kubernetes:master Oct 5, 2017
@aledbf aledbf deleted the ocsp branch October 5, 2017 03:38
@rikatz
Copy link
Contributor

rikatz commented Oct 9, 2017

How this impacts in the following situations:

  1. Ingress machine (as we use bare metal) accepts incoming connections, but doesn't accepts outgoing connections? Everytime an SSL connection is made, NGINX tries to fetch SSL stapling from server?

  2. What happens if the user uses only the Key and Cert, and not the full chain?

  3. Is that possible we make this configurable through ConfigMap? Is this the case? So we do not have a new unexpected behaviour with this new feature.

@mrrandrade @jcmoraisjr please be advised of this before upgrading our Ingress Controller.

@aledbf
Copy link
Member Author

aledbf commented Oct 9, 2017

  1. Ingress machine (as we use bare metal) accepts incoming connections, but doesn't accepts outgoing connections?

Good point. I think we need this http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_stapling_file to avoid the request

  1. What happens if the user uses only the Key and Cert, and not the full chain?

The controller inspect the certificate and downloads the missing intermediate certificates. In case of errors the ocsp stapling is disabled by nginx.

@rikatz
Copy link
Contributor

rikatz commented Oct 9, 2017

But using a file requires that you download or made it available for the server, and assumes that you update it each hour (as it happens with CRL). Probably it would be better to make this not available, and enable it through ConfigMap.

The usage of a Responder is the best approach anyway, I'm just trying to figure out if there's some more impact in this change :)

@rossedman
Copy link

@aledbf @rikatz attempting to implement this. appears docs weren't updated with these changes, but if I understand the changes correctly you still use --default-ssl-certificate flag for ingress-controller but the file must be named namespace/*-full-chain.pem. Is this correct?

@aledbf
Copy link
Member Author

aledbf commented Nov 16, 2017

@rossedman please check https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/cli-arguments.md

--default-ssl-certificate string Name of the secret that contains a SSL certificate to be used as default for a HTTPS catch-all server. Takes the form <namespace>/<secret name>.

@rossedman
Copy link

@aledbf thanks for the follow up, is there a way to do this on a specific endpoint through ingress rules or is it at the controller level only

@aledbf
Copy link
Member Author

aledbf commented Nov 16, 2017

@rossedman what part? disable the ocsp?
The next version will include a flag to disable the intermediate CA certificate resolution #1699

@leonklingele
Copy link

leonklingele commented Feb 8, 2020

Has anyone managed to get this working with Let's Encrypt (certs issued by cert-manager-v0.13.0) on v0.28.0?

I've tried to inject the OCSP directives directly to an Ingress with:

  annotations:
    nginx.ingress.kubernetes.io/server-snippet: |
      ssl_stapling on;
      ssl_stapling_verify on;

and using the Helm values.yaml file:

controller:
  config:
    http-snippet: |
      ssl_stapling on;
      ssl_stapling_verify on;

but to no avail :/ OCSP simply isn't offered. Any idea?

It seems that this is working as expected (see #4651 (comment)). Also, --enable-dynamic-certificates has been deprecated (always enabled) which breaks OCSP stapling (Currently does not support OCSP stapling, https://kubernetes.github.io/ingress-nginx/user-guide/cli-arguments/)
Seriously?!

EDIT:
This is currently being worked on! 🎉 #4758 / #4868

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[nginx] Support ssl ocsp stapling [Nginx ingress controller] Additional SSL options
8 participants