Initial support for CRL in Ingress Controller #3164

rikatz · 2018-09-30T23:03:28Z

What this PR does / why we need it: This PR adds support for CRL into Ingress Controller

Which issue this PR fixes : fixes #1514

Special notes for your reviewer: This is a WIP. Tests still need to be improved. Also this PR doesn't support dynamic ssl configuration (yet), needs @ElvinEfendi review ;) I couldn't make further e2e tests yet

rikatz · 2018-10-03T14:46:36Z

@aledbf @ElvinEfendi I'm not pretty aware how Dynamic Certs works internally, so I've made this PR and tested with it disabled.

We can deal with Dynamic Cert in this PR, or make another PR (as this got merged) just for the Dynamic Cert scenario.

Thanks!

aledbf · 2018-12-22T19:43:07Z

@rikatz apologies for the delay.

We can deal with Dynamic Cert in this PR, or make another PR (as this got merged) just for the Dynamic Cert scenario.

Please do that in this PR. I want to switch to dynamic ssl certificates as default.

bputt · 2019-02-26T21:41:52Z

any idea when this will get released?

johanfleury · 2019-04-11T19:55:55Z

docs/examples/auth/client-certs/README.md

+4. If you want to use a Certificate Revocation List, the secret must be created containing both ca.crt and ca.crl as the following:
+
+`kubectl create secret generic auth-tls-chain --from-file=ca.crt --namespace=default`


You could maybe also add the CRL file in that command:

-kubectl create secret generic auth-tls-chain --from-file=ca.crt --namespace=default +kubectl create secret generic auth-tls-chain --from-file=ca.crt --from-file=ca.crl --namespace=default

fejta-bot · 2019-07-10T19:56:07Z

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

bputt · 2019-07-10T22:54:49Z

bad bot, keeping this alive

johanfleury · 2019-07-11T18:52:28Z

/remove-lifecycle stale

aledbf · 2019-09-03T00:41:04Z

@rikatz any chance you can finish this PR?

rikatz · 2019-09-03T01:04:09Z

Hi there! Sorry for being away so much time. Yes, I can take a look again into it. The problem is that I don’t know Lua and how evolve this. I’m OOO this week, but will try to figure out what changed since I’ve first started this PR and try to improve this. Anyway, if is there someone willing to help, please raise your hand and I’ll be more than happy to at least explain how I see this feature :) Em seg, 2 de set de 2019 às 21:41, Manuel Alejandro de Brito Fontes < [email protected]> escreveu:

…

@rikatz <https://github.com/rikatz> any chance you can finish this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3164?email_source=notifications&email_token=ABWZQBPB5I2Y54P7ZTUQURTQHWQB3A5CNFSM4FYEZYC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5WWS7Y#issuecomment-527264127>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABWZQBLH3QGBPEGQNUST5YTQHWQB3ANCNFSM4FYEZYCQ> .

rikatz · 2019-09-03T12:01:10Z

@aledbf I've started to look again into ingress code. Just to see if I'm the right direction, I'm changing the certificates.lua code to contain the following into call:

  local crl_file, crl_err = get_crl_file(hostname)
  if not crl_err then
    lua_ssl_crl = crl_file
  end

I'll also create this get_crl_file function to check wether the API contains a secret, and if the file pointed by this secret exists into the FS (and if that can be read). Is that ok?

If this is the path, I can also take a look into the CA authentication later :)

aledbf · 2019-09-03T12:40:54Z

by this secret exists into the FS (and if that can be read). Is that ok?

No. We don't write certificates to the disk anymore. We have to tables now https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/lua/configuration.lua#L38
hostname -> uuid (of the secret) then uid -> ssl certificate

This approach removes duplication of secrets and also the use of storage of files.

ElvinEfendi · 2019-09-03T13:12:12Z

Hey @rikatz, thanks for resuming work on this. IMHO we don't need any Lua change for this feature. The way I see it is this feature is an extension of TLS client authentication. We currently extra TLS client certificate from the secret and store it on disk. And then configure respective Nginx directives. When the content of the certificate change, we reload Nginx. I think we should just do the same thing for CRL file: introduce a new field as you were planning to, extract it from the secret, and write to disk. Then finally configure ssl_crl respectively.

rikatz · 2019-09-03T13:16:31Z

Hey @rikatz, thanks for resuming work on this. IMHO we don't need any Lua change for this feature. The way I see it is this feature is an extension of TLS client authentication. We currently extra TLS client certificate from the secret and store it on disk. And then configure respective Nginx directives. When the content of the certificate change, we reload Nginx. I think we should just do the same thing for CRL file: introduce a new field as you were planning to, extract it from the secret, and write to disk. Then finally configure ssl_crl respectively.

Right :)

I've just stopped 'cause at that time NGINX Ingress was moving to dynamic certs, and didn't know how to deal with that.

If you and @aledbf are OK with that, I can resume the work with CRL and CA being written on disk (as it is today) and can think later how to move that to Lua :)

I think the bigger problem we try to solve here is to not reload the nginx configuration when we've some change into CA or CRL, so if Lua is able to everytime re-read the CRL file without reloading NGINX I think this is solved, BUT will probably have some impact into the I/O of the server.

WDYT?

rikatz · 2019-09-03T17:26:25Z

So better late than never, I've refactored the whole PR.

I'll make some further tests, if someone is also willing to test it and provide feedback, please reach me up here or into slack :)

rikatz · 2019-09-03T17:26:54Z

/assign @aledbf
/assign @ElvinEfendi

aledbf · 2019-09-03T17:27:18Z

/ok-to-test

ElvinEfendi · 2019-09-03T17:30:15Z

I think the bigger problem we try to solve here is to not reload the nginx configuration when we've some change into CA or CRL, so if Lua is able to everytime re-read the CRL file without reloading NGINX I think this is solved, BUT will probably have some impact into the I/O of the server.

In my comment above I'm suggesting to not try to avoid reloads when CRL change. This is similar to TLS client certificate, where we do not avoid reload either. We have dynamic reload only for TLS certificates for handshaking. Avoiding reloads for TLS client certificate and CRL certificate requires us to dynamically handle TLS client verification using Lua too, we currently do not do it and to my knowledge lua-nginx-module does not provide any API for that. With a little bit of search closest thing I could find was https://github.com/Kong/lua-kong-nginx-module.

I think writing CRL certificate to disk just like TLS client certificate is fine given we do not have a good/easy way to use Lua for that.

ElvinEfendi · 2019-09-03T17:34:31Z

internal/net/ssl/ssl_test.go

+	if err != nil {
+		t.Fatalf("unexpected error creating CRL file: %v", err)
+	}
+	if sslCert.CRLFileName == "" {


Can we assert expected filename instead?

Yes we can. I was thinking about that, just didn't because of the previous test doing the same.

I'll correct this AND the other test.

ElvinEfendi · 2019-09-03T17:37:21Z

Looks good to me, just one comment.

Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> Add support to CRL Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]>

rikatz · 2019-09-03T17:44:55Z

Looks good to me, just one comment.

Tks :)

I'm running some tests here on Kind just to check if it works fine. I've also changed the Tests to reflect also the filename.

aledbf · 2019-09-03T18:20:12Z

/retest

aledbf · 2019-09-03T18:49:55Z

/retest

aledbf · 2019-09-03T18:50:03Z

/approve

rikatz · 2019-09-03T18:53:48Z

Tested here, it worked as expected.

aledbf · 2019-09-03T18:55:31Z

Just in case, the e2e errors we see here are not related to the PR itself but to timeouts running the suite. I need to increase this value.

rikatz · 2019-09-03T18:56:27Z

Just in case, the e2e errors we see here are not related to the PR itself but to timeouts running the suite. I need to increase this value.

Prow hates me all across the ORG anyway :P

ElvinEfendi · 2019-09-03T19:02:46Z

/lgtm

k8s-ci-robot · 2019-09-03T19:03:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi, rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ElvinEfendi,aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aledbf · 2019-09-03T19:24:45Z

/retest

aledbf · 2019-09-03T19:51:53Z

/retest

aledbf · 2019-09-03T20:19:51Z

/retest

k8s-ci-robot · 2019-09-03T20:46:10Z

@rikatz: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-ingress-nginx-e2e-1-12	`51a1e88`	link	`/test pull-ingress-nginx-e2e-1-12`
pull-ingress-nginx-e2e-1-13	`51a1e88`	link	`/test pull-ingress-nginx-e2e-1-13`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

aledbf · 2019-09-03T20:47:47Z

@rikatz thanks!

ntavares · 2020-06-12T15:58:20Z

@ElvinEfendi @rikatz @aledbf Hi! In regards to @ElvinEfendi comment here:

In my comment above I'm suggesting to not try to avoid reloads when CRL change.

I think this was missed. Currently, the controller (0.32.0) is not reloading after CRL changes (secret updated).... shall I open a separate issue?

Edit: I missed that #4904 was there already.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 30, 2018

k8s-ci-robot requested review from aledbf and antoineco September 30, 2018 23:03

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 1, 2018

rikatz changed the title ~~[WIP] - Initial support for CRL in Ingress Controller~~ Initial support for CRL in Ingress Controller Oct 3, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2018

aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2018

johanfleury reviewed Apr 11, 2019

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 10, 2019

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 11, 2019

rikatz force-pushed the crl-support branch from 87621ef to eb8f78b Compare September 3, 2019 17:25

k8s-ci-robot assigned aledbf and ElvinEfendi Sep 3, 2019

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 3, 2019

ElvinEfendi reviewed Sep 3, 2019

View reviewed changes

Add support to CRL

51a1e88

Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]> Add support to CRL Signed-off-by: Ricardo Pchevuzinske Katz <[email protected]>

rikatz force-pushed the crl-support branch from eb8f78b to 51a1e88 Compare September 3, 2019 17:44

aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 3, 2019

aledbf merged commit 9c51676 into kubernetes:master Sep 3, 2019

		4. If you want to use a Certificate Revocation List, the secret must be created containing both ca.crt and ca.crl as the following:

		`kubectl create secret generic auth-tls-chain --from-file=ca.crt --namespace=default`

Initial support for CRL in Ingress Controller #3164

Initial support for CRL in Ingress Controller #3164

Conversation

rikatz commented Sep 30, 2018 • edited Loading

rikatz commented Oct 3, 2018

aledbf commented Dec 22, 2018

bputt commented Feb 26, 2019

johanfleury Apr 11, 2019

Choose a reason for hiding this comment

fejta-bot commented Jul 10, 2019

bputt commented Jul 10, 2019

johanfleury commented Jul 11, 2019

aledbf commented Sep 3, 2019

rikatz commented Sep 3, 2019 via email

rikatz commented Sep 3, 2019

aledbf commented Sep 3, 2019

ElvinEfendi commented Sep 3, 2019

rikatz commented Sep 3, 2019

rikatz commented Sep 3, 2019

rikatz commented Sep 3, 2019

aledbf commented Sep 3, 2019

ElvinEfendi commented Sep 3, 2019

ElvinEfendi Sep 3, 2019

Choose a reason for hiding this comment

rikatz Sep 3, 2019

Choose a reason for hiding this comment

ElvinEfendi commented Sep 3, 2019

rikatz commented Sep 3, 2019

aledbf commented Sep 3, 2019

aledbf commented Sep 3, 2019

aledbf commented Sep 3, 2019

rikatz commented Sep 3, 2019

aledbf commented Sep 3, 2019

rikatz commented Sep 3, 2019

ElvinEfendi commented Sep 3, 2019

k8s-ci-robot commented Sep 3, 2019

aledbf commented Sep 3, 2019

aledbf commented Sep 3, 2019

aledbf commented Sep 3, 2019

k8s-ci-robot commented Sep 3, 2019

aledbf commented Sep 3, 2019

ntavares commented Jun 12, 2020 • edited Loading

rikatz commented Sep 30, 2018 •

edited

Loading

ntavares commented Jun 12, 2020 •

edited

Loading