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 support for CRL when using TLS Auth #1514

Closed
jniebuhr opened this issue Oct 10, 2017 · 24 comments · Fixed by #3164
Closed

Add support for CRL when using TLS Auth #1514

jniebuhr opened this issue Oct 10, 2017 · 24 comments · Fixed by #3164

Comments

@jniebuhr
Copy link

Is this a request for help?: No

What keywords did you search in NGINX Ingress controller issues before filing this one?: CRL (also looked for the nginx directive in the code, wasn't there)


Is this a BUG REPORT or FEATURE REQUEST? (choose one): FEATURE REQUEST

NGINX Ingress controller version: Latest

What you expected to happen: When putting a ca.crl file in the secret that contains the ca.crt file, I'd expect the controller to also copy that file to the filesystem and put the ssl_crl directive into the nginx config.

@rikatz
Copy link
Contributor

rikatz commented Oct 15, 2017

@jniebuhr Thanks for your report. Actually NGINX Ingress supports OCSP stappling.

The CRL supports needs to be improved, but it's a gap in major Open Source HTTP / Proxy servers. They relies in a file being downloaded times to times to the server, and then the server needs to be reloaded.

As this is an easy to implement solution in small environments, on some environments that have hundreds of trustable CAs this tends to be impossible, as you have to know each of the CAs and their CRL locations.

A desired behaviour for those servers (not ingress, itself) is to download those files from the specified CRL inside the CA, as Microsoft IIS server does. So when you use a Certification Authority, it does download the CRL (and caches it) before, not relying if you do have or not them phisically in your server.

A recent PR that happened here downloads the whole chain of a Certificate (as happens here ) but this needs also some kind of caution, as this might impact in your server performance (we could also make some kind of timer or anything else that triggers this to CRL, but don't know if this is something easy to implement).

Anyway, your concern of using CRL is right. Doing as you specified in What you expect to happen isn't anything pretty hard, but you'll have to keep your CRL secret synced as CRLs tends to change a lot of times per day / week :)

Hope I could explain to you the bigger problem here, and if you have any idea in how to solve this, I'm here to help.

My bet is a NGINX module that changes the behaviour and makes this pretty much the same behaviour as IIS Servers, but I'm not close to be a C developer :)

@jniebuhr
Copy link
Author

I have a Self signed authority that I create my client certificates from. When revoking a client certificate (like when an employee leaves the company), OpenSSL will make a CRL for me. I would like to put that into the same secret my CA is in for client with. That should be pretty easy but sadly I’m not really fluent in Go.

@rikatz
Copy link
Contributor

rikatz commented Oct 15, 2017

@jniebuhr Isn't this the case to enable OCSP in your OpenSSL CA, emit a new CA (don't know what's the level of 'production' your CA is) and use OCSP stapling in NGINX? :)

Anyway I'll think in something here, as I also do need some kind of CRL downloading (as made with the full chain), but I'm without much time to do this by now :(

@jniebuhr
Copy link
Author

Just read about OCSP. Well we don't have any OCSP server yet. Right now, the CA is only available in the secret. I guess it would make the setup much easier if one could just put the CRL in the secret with the CA cert and kick the ingress pods.

@zeeZ
Copy link
Contributor

zeeZ commented Jan 3, 2018

To chime in my opinion, I think this shouldn't be overcomplicated and, at least for me, the expected behaviour would be just as it works in nginx configuration: You point it to a static file and every time you make a change to this file nginx needs to be reloaded for it to take effect.

This should simply map to the server's configuration and does not have to do any magic.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 3, 2018
@ghost
Copy link

ghost commented Apr 10, 2018

/remove-lifecycle stale

I support what @zeeZ said. I would like to point to a resource (secret, if that is what everyone wants) and every time it is touched, the ingress reloads.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 10, 2018
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 9, 2018
@bputt
Copy link

bputt commented Jul 16, 2018

/remove-lifecycle stale

Is there anyway this could get added to the baseline? I agree that adding the file as a secret is sufficient and an auto reload once it changes would be perfect.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 16, 2018
@rikatz
Copy link
Contributor

rikatz commented Jul 25, 2018

Hi,

I'll take a look into the implementation of this (too much stuff have changed since last time I've been here) and see if it's possible to make as the follow:

  • Fetch/sync the secrets for the CA Authentication from the very same CA Secret used (here)[https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/annotations/authtls/main.go#L91] but probably from a field other than 'ca.crt' (probably ca.crl)

  • If the value if not null and it's validated as a working CRL, will do the rest (create the CRL file, point the vhost into this file)

Don't know how much time will take, and if this does have some impact into the work being done by @aledbf and @ElvinEfendi to remove the 'reloads' from ingress, but let's see it later.

If someone did understood what's the way here and want's to code this feature before me, just let me know and I'll help reviewing and so.

Tks!

@zeeZ
Copy link
Contributor

zeeZ commented Jul 25, 2018

Here's a dirty hack that works right now:

  1. Use a custom template where you duplicate the ssl_client_certificate line that points to the CA file and change it to ssl_crl
  2. Append the CRL to the ca.crt in your auth-tls-secret

This may or may not be the source for "cert already in hash table error" alerts, but hey.

@stanislavvv
Copy link

I can't use OCSP with self-signed CA working for several years in production...

How about add new line ssl_ca_crl (not use if empty) and let's it update by external scripts?
It will be added to nginx configuration
It can be a good start.

Next point - something like ssl_ca_crl_update_url (something like "https://ca.company.tld/crl.pem")

@Queuecumber
Copy link

As far as I can tell, nginx doesn't even support checking client certificate status using ocsp. Which is what we're talking about right? (If I'm wrong someone please correct me because that's the better solution)

@ntavares
Copy link

ntavares commented Sep 9, 2018

Although I like @jniebuhr 's "What you expected to happen" approach, I believe it might turn out to be a bit obscure. I see a very simple approach here: just allow another secret (or a configmap) specifically for the CSR, just as with the 'auth-tls-secret' annotation. If it is specified, then ssl_crt gets inserted in nginx.conf, otherwise nothing is done. It would then be up to the user to keep the secret/configmap up-to-date, and we could rely on nginx to reload automatically when the secret changed;

@bputt
Copy link

bputt commented Sep 10, 2018

I agree with ntavares, we don't mind managing the file / update frequency on our side, we just need the option to configure it in the ingress controller

@ssboisen
Copy link

We're looking at using certificates to protect internal applications and would love to be able to set a ca.crl part in a secret passed via ingress annotation. The suggested approach in this thread seems good as it's in line with how the ca crt is provided to the ingress.

@rikatz
Copy link
Contributor

rikatz commented Sep 28, 2018

Hi,

I'm working in this issue, I'm pretty busy but soon will have some PR to be reviewed

@Szymongib
Copy link

Hello,

I see PR is still open, are you going to drive that topic forward?

@rikatz
Copy link
Contributor

rikatz commented Dec 18, 2018 via email

@Szymongib
Copy link

@rikatz Thank you for the response, looking forward to that feature!

@bputt
Copy link

bputt commented Feb 26, 2019

any updates on this? Is the PR going to be included in the next release?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 27, 2019
@Queuecumber
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 27, 2019
@pakoAku
Copy link

pakoAku commented Jun 7, 2019

Hey,

I wanted to ask if there are any news regarding the PR.

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 a pull request may close this issue.