OCPBUGS-6661: Manage mTLS CRLs #447

rfredette · 2023-02-07T20:26:39Z

This PR makes the router parse the certificates supplied in ROUTER_MUTUAL_TLS_AUTH_CA, download CRLs from any CRL distribution points that are found, and write them to /crls/crls.pem. That file is supplied to haproxy to use for mTLS client auth.

This PR requires that the router deployment include an EmptyDir mount for the /crls directory

pkg/router/crl/crl.go

Miciah · 2023-02-15T19:14:20Z

pkg/router/crl/crl.go

+			if nextUpdate.IsZero() {
+				// TODO: Make this a longer interval (for example, hourly) or implement backoff logic?
+				log.V(1).Info("Couldn't determine next CRL update time. Falling back to updating every 5 minutes.")
+				nextUpdate = time.Now().Add(5 * time.Minute)


Every five minutes seems rather frequent, and CRLs can be quite large. I think daily is more typical for CRLs, right?

In any case, can you add a named constant for the default update interval?

const ( // defaultUpdateInterval is the interval to wait before updating CRLs // when the CRLs do not specify any next update time. defaultUpdateInterval = 1 * time.Day // ... )

Suggested change

nextUpdate = time.Now().Add(5 * time.Minute)

nextUpdate = time.Now().Add(defaultUpdateInterval)

I currently have it set to 5 minutes to aid in testing, since I manually update the CRL files, and sometimes I've missed the actual expiration time. I will add a constant with the appropriate fallback interval before this is ready to merge.

If the failure was transient should we wait for another day before downloading again? I could see a case where we do a number of retries before falling back to downloading once a day. Otherwise (and assuming a transient download failure) the only way to get a new list (in a hurry) is to delete the pod.

pkg/router/crl/crl.go

pkg/router/template/router.go

openshift-ci-robot · 2023-02-17T17:54:29Z

@rfredette: This pull request references Jira Issue OCPBUGS-6661, which is invalid:

expected the bug to target the "4.13.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR makes the router parse the certificates supplied in ROUTER_MUTUAL_TLS_AUTH_CA, download CRLs from any CRL distribution points that are found, and write them to /crls/crls.pem. That file is supplied to haproxy to use for mTLS client auth.

This PR requires that the router deployment include an EmptyDir mount for the /crls directory

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.

Makefile

images/router/haproxy/Dockerfile.rhel8

pkg/util/util.go

Makefile

pkg/util/util.go

pkg/router/crl/crl.go

frobware · 2023-02-28T13:11:24Z

pkg/router/crl/crl.go

+			if nextUpdate.IsZero() {
+				// TODO: Make this a longer interval (for example, hourly) or implement backoff logic?
+				log.V(1).Info("Couldn't determine next CRL update time. Falling back to updating every 5 minutes.")
+				nextUpdate = time.Now().Add(5 * time.Minute)


If the failure was transient should we wait for another day before downloading again? I could see a case where we do a number of retries before falling back to downloading once a day. Otherwise (and assuming a transient download failure) the only way to get a new list (in a hurry) is to delete the pod.

pkg/router/crl/crl.go

pkg/router/template/router.go

Miciah

I like the latest changes. The cleanup of stale mTLS config looks much more straightforward now. Nice improvement!

However, several comments from my previous review don't appear to have been addressed. Can you make sure that past review comments haven't been overlooked? It's fine if you decide to reject some of the suggestions, but please reply in those cases so that I know a comment wasn't inadvertently missed.

pkg/router/crl/crl.go

pkg/router/template/router.go

pkg/util/util.go

pkg/router/crl/crl.go

rfredette · 2023-03-14T00:30:31Z

I plan to remove the build changes I made (in Makefile and the haproxy Dockerfile.rhel8), set the retry timeout to a final value, probably larger than 5 minutes, and squash the commits before this merges. It still needs some cleanup in the tags, but just in case:

/hold

frobware · 2023-03-14T07:12:22Z

/retest

gcs278

Overall look pretty good. A bit hard to understand what's happening, but I think I got the essence of it.

Makefile

images/router/haproxy/Dockerfile.rhel8

gcs278 · 2023-03-20T21:56:34Z

pkg/router/crl/crl.go

+		for {
+			updated := false
+			if nextUpdate.IsZero() {
+				log.V(4).Info("no nextUpdate. only watching for CA updates")


nit Should we have these log messages describe where these are coming from? As they stand on their own, it might be confusing to a dev in the future when they turn on verbose logging. e.g.

Suggested change

log.V(4).Info("no nextUpdate. only watching for CA updates")

log.V(4).Info("ManageCRLs: no nextUpdate. only watching for CA updates")

pkg/router/crl/crl.go

gcs278 · 2023-03-21T17:28:35Z

pkg/router/crl/crl.go

+	crls := make(map[string]*x509.RevocationList)
+	updated := false
+	now := time.Now()
+	for len(clientCAData) > 0 {


This logic is duplicated with ShouldHaveCRLs. Is there a way to clean this up? Seems like a maintenance burden.

Seems like you could just wait to read the clientCAData (it's done early in writeCRLFile, but not necessary), and then pass caBundleFilename to ShouldHaveCRLs.

gcs278 · 2023-03-21T17:46:43Z

pkg/router/crl/crl.go

+	return nil
+}
+
+// ShouldHaveCRLs returns true if any of the certificates in caBundleFilename specify a CRL distribution point


nit and it returns an error

gcs278 · 2023-03-21T17:48:55Z

images/router/haproxy/conf/haproxy-config.template

@@ -290,8 +292,8 @@ frontend fe_sni
    {{- "" }} crt-list /var/lib/haproxy/conf/cert_config.map accept-proxy
    {{- with (env "ROUTER_MUTUAL_TLS_AUTH") }}
      {{- "" }} verify {{. }}
-    {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CA") }} ca-file {{. }} {{ else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}
-    {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CRL") }} crl-file {{. }} {{ end }}
+	{{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{ else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}


Just a thought, /var/lib/haproxy/mtls/latest/ca-bundle.pem could also be GO template variables, since they are already defined in crl.go

pkg/router/crl/crl.go

gcs278 · 2023-03-21T19:40:05Z

pkg/router/crl/crl.go

+// caUpdateChannel (indicating the CA bundle has been updated), or when any existing CRL expires. Whenever either the CA
+// bundle or the CRL file has changed, updateCallback is called, with a boolean indicating whether crl-file needs to be
+// specified in the HAProxy config.
+func ManageCRLs(caBundleFilename string, caUpdateChannel <-chan struct{}, updateCallback func(bool)) {


I think we should have some unit testing for at least the functions that aren't a part of templateRouter struct. Might have to be a little creative with the tests, but I think it's do-able.

Most (if not all) of the functions I've added either deal with downloading data from another source, or read and write files in fixed locations on the filesystem, so isolating them to run in a unit test scenario would take some work. If I'm able to get good enough coverage with e2e tests, what do you think about me circling back to figure out unit tests in a later PR?

gcs278 · 2023-04-05T14:18:13Z

Commandeering this CI to test if haproxy22 RPM is still available
/test images

rfredette · 2023-04-20T21:34:36Z

/jira refresh

openshift-ci-robot · 2023-04-20T21:34:42Z

@rfredette: This pull request references Jira Issue OCPBUGS-6661, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/jira refresh

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.

rfredette · 2023-04-20T21:47:24Z

/retest-required

candita · 2023-05-10T15:54:24Z

Assigning Miciah and Andy due to their current level of feedback. Feel free to remove, and add to PR Scrub instead.
/assign @Miciah
/assign @frobware

openshift-ci · 2023-05-10T21:58:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from frobware. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

openshift-ci · 2023-05-11T01:03:28Z

@rfredette: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-upgrade	`8c320e2`	link	true	`/test e2e-upgrade`
ci/prow/e2e-agnostic	`8c320e2`	link	true	`/test e2e-agnostic`

Full PR test history. Your PR dashboard.

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.

Miciah · 2023-05-16T17:44:55Z

images/router/haproxy/conf/haproxy-config.template

+	{{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{ else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}
+	{{- if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{ end }}


You replaced spaces with tabs. Please use spaces for consistency with the rest of the template.

I'm wary of breaking OKD users, or people using custom router deployments (unsupported, but better not to break them if we don't need to). If ROUTER_MUTUAL_TLS_AUTH_CA or ROUTER_MUTUAL_TLS_AUTH_CRL is set, we should still respect it. Likewise for ROUTER_MUTUAL_TLS_AUTH_CRL below.

Suggested change

{{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{ else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}

{{- if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{ end }}

{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CA") }} ca-file {{ . }} {{- else }}{{ if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{- else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{- end }}{{ end }}

{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CRL") }} crl-file {{ . }} {{- else }}{{ if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{- end }}{{ end }}

Likewise for ROUTER_MUTUAL_TLS_AUTH_CRL below.

My router changes still use ROUTER_MUTUAL_TLS_AUTH_CA since the operator still needs to supply the CA bundle, so using with (env "ROUTER_MUTUAL_TRLS_AUTH_CA") by itself to select the old behavior isn't specific enough.

What do you think of something like this?

Suggested change

{{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{ else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}

{{- if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{ end }}

{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CRL") }} crl-file {{ . }}

{{- with (env "ROUTER_MUTUAL_TLS_AUTH_CA") }} ca-file {{ . }} {{- else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}

{{- else }}

{{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{- else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}

{{- if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{ end }}

{{- end}}

That could work (though I think you also need to re-add {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CRL") }} crl-file {{. }} {{ end }}).

The fix I proposed does include crl-file ... in that case, but maybe it's a bit confusing. I'll see if I can make it clearer.

I've included a clearer version of this fix in #472.

rfredette · 2023-05-17T23:38:05Z

Closing in favor of #472, which includes this fix as well as a fix for OCPBUGS-9464.

openshift-ci-robot · 2023-05-17T23:38:07Z

@rfredette: This pull request references Jira Issue OCPBUGS-6661. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

This PR makes the router parse the certificates supplied in ROUTER_MUTUAL_TLS_AUTH_CA, download CRLs from any CRL distribution points that are found, and write them to /crls/crls.pem. That file is supplied to haproxy to use for mTLS client auth.

This PR requires that the router deployment include an EmptyDir mount for the /crls directory

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.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2023

openshift-ci bot requested review from gcs278 and knobunc February 7, 2023 20:29

rfredette force-pushed the ocpbugs-6661-crl-limit branch from 706a67b to e431715 Compare February 15, 2023 18:37

rfredette mentioned this pull request Feb 15, 2023

OCPBUGS-6661: Handle mTLS CRLs in the router openshift/cluster-ingress-operator#891

Closed

rfredette changed the title ~~WIP: Download CRLs as part of the router startup~~ WIP: Manage mTLS CRLs Feb 15, 2023

Miciah reviewed Feb 15, 2023

View reviewed changes

rfredette changed the title ~~WIP: Manage mTLS CRLs~~ WIP: OCPBUGS-6661: Manage mTLS CRLs Feb 17, 2023

openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Feb 17, 2023

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Feb 17, 2023

rfredette force-pushed the ocpbugs-6661-crl-limit branch from 1673c1c to d309252 Compare February 27, 2023 19:37

rfredette commented Feb 27, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

rfredette commented Feb 27, 2023

View reviewed changes

images/router/haproxy/Dockerfile.rhel8 Outdated Show resolved Hide resolved

Miciah reviewed Feb 28, 2023

View reviewed changes

frobware reviewed Feb 28, 2023

View reviewed changes

Miciah reviewed Mar 3, 2023

View reviewed changes

frobware reviewed Mar 3, 2023

View reviewed changes

pkg/router/crl/crl.go Outdated Show resolved Hide resolved

frobware reviewed Mar 7, 2023

View reviewed changes

pkg/router/crl/crl.go Outdated Show resolved Hide resolved

rfredette force-pushed the ocpbugs-6661-crl-limit branch from 7a406f0 to f4898ec Compare March 14, 2023 00:05

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2023

gcs278 reviewed Mar 21, 2023

View reviewed changes

rfredette force-pushed the ocpbugs-6661-crl-limit branch from 990dd53 to 79f25b4 Compare March 27, 2023 22:14

rfredette force-pushed the ocpbugs-6661-crl-limit branch from 79f25b4 to b2e8375 Compare April 4, 2023 22:53

candita mentioned this pull request Apr 5, 2023

Changing to set for subjectKeyIds for uniqueness openshift/cluster-ingress-operator#897

Closed

rfredette changed the title ~~WIP: OCPBUGS-6661: Manage mTLS CRLs~~ OCPBUGS-6661: Manage mTLS CRLs Apr 20, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2023

openshift-ci bot requested a review from lihongan April 20, 2023 21:34

rfredette mentioned this pull request Apr 20, 2023

OCPBUGS-6661, OCPBUGS-9464: Handle mTLS CRLs, and fix accidental CRL duplication #472

Merged

rfredette force-pushed the ocpbugs-6661-crl-limit branch from b2e8375 to fd46043 Compare May 8, 2023 19:32

openshift-ci bot assigned frobware and Miciah May 10, 2023

Download and manage CRLs in openshift-router

8c320e2

rfredette force-pushed the ocpbugs-6661-crl-limit branch from fd46043 to 8c320e2 Compare May 10, 2023 21:58

Miciah reviewed May 16, 2023

View reviewed changes

rfredette closed this May 17, 2023

	nextUpdate = time.Now().Add(5 * time.Minute)
	nextUpdate = time.Now().Add(defaultUpdateInterval)

	log.V(4).Info("no nextUpdate. only watching for CA updates")
	log.V(4).Info("ManageCRLs: no nextUpdate. only watching for CA updates")

		{{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{ else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}
		{{- if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{ end }}

-	{{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{ else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}
-	{{- if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{ end }}
+    {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CRL") }} crl-file {{ . }}
+        {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CA") }} ca-file {{ . }} {{- else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}
+    {{- else }}
+        {{- if $haveClientCA }} ca-file /var/lib/haproxy/mtls/latest/ca-bundle.pem {{- else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}
+        {{- if $haveCRLs }} crl-file /var/lib/haproxy/mtls/latest/crls.pem {{ end }}
+    {{- end}}

OCPBUGS-6661: Manage mTLS CRLs #447

OCPBUGS-6661: Manage mTLS CRLs #447

Uh oh!

Conversation

rfredette commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Miciah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rfredette commented Mar 14, 2023

Uh oh!

frobware commented Mar 14, 2023

Uh oh!

gcs278 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfredette commented Feb 7, 2023 •

edited

Loading

rfredette May 17, 2023 •

edited

Loading