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

rfredette · 2023-04-20T22:05:45Z

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 a file that is supplied to HAProxy for mTLS certificate validation.

It also includes a fix for an issue where, when a CRL is distributed in a certificate that is not its issuer, the CRL would sometimes be missing or two copies of the CRL would be present.

openshift-ci-robot · 2023-04-20T22:05:51Z

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

expected the bug to target the "4.14.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:

The CRLs on disk don't include enough information to accurately determine which CA certificate they were downloaded for. Instead of re-parsing the CRLs on disk, keep them in a map indexed by the CA's
subject key, so we can avoid unnecessary downloads and incorrect attribution.

This change requires #447 , so until that merges, this includes the changes from that PR as well. As of this writing, only the commit "Cache current CRLs in memory instead of re-parsing from disk" (36ebc38) is new.

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-20T22:06:47Z

/jira refresh

openshift-ci-robot · 2023-04-20T22:06:58Z

@rfredette: This pull request references Jira Issue OCPBUGS-9464, which is valid. The bug has been moved to the POST state.

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 ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

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.

frobware · 2023-05-17T14:48:42Z

/assign

openshift-ci-robot · 2023-05-17T23:36:16Z

@rfredette: This pull request references Jira Issue OCPBUGS-9464, 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:

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 a file that is supplied to HAProxy for mTLS certificate validation.

It also includes a fix for an issue where, when a CRL is distributed in a certificate that is not its issuer, the CRL would sometimes be missing or two copies of the CRL would be present.

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.

frobware · 2023-05-19T12:08:21Z

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

-    {{- 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 (ne (env "ROUTER_MUTUAL_TLS_AUTH_CRL") "") }}
+        {{- with (env "ROUTER_MUTUAL_TLS_AUTH_CA") }} ca-file {{ . }} {{- else }} ca-file /etc/ssl/certs/ca-bundle.trust.crt {{ end }}


There are some changes here:

previously: {{. }}, now: {{ . }} -- innocuous

previously: {{ else }}, now {{- else }} -- but now we trim all the whitespace before the action

Is the last change intentional? If so, for what reason?
My comment holds true for the other template change below too.

It's not an intentional change; I will update this to get rid of that drift.

frobware · 2023-05-19T12:55:36Z

pkg/router/crl/crl.go

+		return nil, fmt.Errorf("http.Get failed: %w", err)
+	}
+	defer resp.Body.Close()
+	crlBytes, err := io.ReadAll(resp.Body)


Before we ReadAll() we should check the response code:

if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("received non-OK status code: %d", resp.StatusCode) }

The error message may be more helpful when a GET fails (e.g., 404).

~~What happens if the status code is a redirect?~~

frobware · 2023-05-19T13:10:07Z

pkg/router/crl/crl.go

+		if block == nil {
+			break
+		}
+		clientCAData = data


In clientCAData are we only concerned with certificates?

if block.Type != "CERTIFICATE" { return nil, time.Time{}, false, fmt.Errorf("client CA bundle contains a non-certificate PEM block") }

Or log and skip.

We should only care about certificates; any other PEM block type shouldn't matter. I think log & skip any non-certificate content is the way to go.

frobware · 2023-05-19T13:21:40Z

pkg/router/template/router.go

+			caUpdateChannel <- struct{}{}
+		}
+		if err := r.watchVolumeMountDir(filepath.Dir(caPath), caReloadFn); err != nil {
+			log.V(0).Error(err, "failed to establish watch on mTLS certificate directory")
 			return nil


Although this is existing behaviour, why does this not return the error?

@Miciah I noticed that writeDefaultCert doesn't return an error for its calls to watchVolumeMountDir() should the call fail. Do you know of any history for why this returns nil?

If we were to return an error I'd propose we do that change in 4.14+ only for the moment as we want the backports to discretely fix OCPBUGS-6661 and OCPBUGS-9464 only.

The CRLs on disk don't include enough information to accurately determine which CA certificate they were downloaded for. Instead of re-parsing the CRLs on disk, keep them in a map indexed by the CA's subject key, so we can avoid unnecessary downloads and incorrect attribution.

Allows easier backporting to releases reliant on go 1.18 or older

openshift-ci · 2023-05-19T21:45:12Z

@rfredette: all tests passed!

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.

frobware · 2023-05-22T14:36:59Z

/lgtm

Miciah · 2023-05-22T16:34:00Z

/approve

openshift-ci · 2023-05-22T16:36:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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~~ [Miciah]

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

openshift-ci-robot · 2023-05-22T16:40:08Z

@rfredette: Jira Issue OCPBUGS-9464: Some pull requests linked via external trackers have merged:

openshift/router#472

The following pull requests linked via external trackers have not merged:

openshift/cluster-ingress-operator#930 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-9464 has not been moved to the MODIFIED state.

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 a file that is supplied to HAProxy for mTLS certificate validation.

It also includes a fix for an issue where, when a CRL is distributed in a certificate that is not its issuer, the CRL would sometimes be missing or two copies of the CRL would be present.

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-05-23T15:09:36Z

/cherry-pick release-4.13
/cherry-pick release-4.12
/cherry-pick release-4.11

openshift-cherrypick-robot · 2023-05-23T15:10:18Z

@rfredette: new pull request created: #485

In response to this:

/cherry-pick release-4.13
/cherry-pick release-4.12
/cherry-pick release-4.11

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 requested review from gcs278 and Miciah April 20, 2023 22:06

openshift-ci bot requested a review from lihongan April 20, 2023 22:06

rfredette mentioned this pull request May 11, 2023

OCPBUGS-6661, OCPBUGS-9464: Move mTLS CRL handling into the router, and fix accidental duplication of CRLs openshift/cluster-ingress-operator#930

Merged

rfredette force-pushed the ocpbugs-9464-duplicate-crls branch from 36ebc38 to bef0660 Compare May 16, 2023 19:13

openshift-ci bot assigned frobware May 17, 2023

rfredette changed the title ~~OCPBUGS-9464: Cache current CRLs in memory instead of re-parsing from disk~~ OCPBUGS-6661, OCPBUGS-9464: Handle mTLS CRLs, and fix accidental CRL duplication May 17, 2023

rfredette force-pushed the ocpbugs-9464-duplicate-crls branch from bef0660 to e701475 Compare May 17, 2023 23:03

openshift-ci-robot added jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. and removed jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. labels May 17, 2023

rfredette mentioned this pull request May 17, 2023

OCPBUGS-6661: Manage mTLS CRLs #447

Closed

frobware suggested changes May 19, 2023

View reviewed changes

rfredette added 3 commits May 19, 2023 15:11

Download and manage CRLs in openshift-router

901dba0

Revert to pkix.CertificateList instead of x509.RevocationList

6593d76

Allows easier backporting to releases reliant on go 1.18 or older

rfredette force-pushed the ocpbugs-9464-duplicate-crls branch from e701475 to 6593d76 Compare May 19, 2023 19:11

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2023

openshift-merge-robot merged commit 5426a9b into openshift:master May 22, 2023

openshift-cherrypick-robot mentioned this pull request May 23, 2023

[release-4.13] OCPBUGS-13967, OCPBUGS-13964: Handle mTLS CRLs, and fix accidental CRL duplication #485

Merged

openshift-ci-robot mentioned this pull request May 24, 2023

Revert "OCPBUGS-6661, OCPBUGS-9464: Move mTLS CRL handling into the router, and fix accidental duplication of CRLs" openshift/cluster-ingress-operator#938

Merged

openshift-ci-robot mentioned this pull request Jun 2, 2023

OCPBUGS-6661, OCPBUGS-9464: Move mTLS CRL handling into the router, and fix accidental duplication of CRLs openshift/cluster-ingress-operator#939

Merged

Miciah mentioned this pull request Sep 16, 2024

OCPBUGS-29894: Check if CRLs are downloaded when determining ready status #595

Open

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

openshift-ci-robot commented Apr 20, 2023

Uh oh!

rfredette commented Apr 20, 2023

Uh oh!

openshift-ci-robot commented Apr 20, 2023

Uh oh!

frobware commented May 17, 2023

Uh oh!

openshift-ci-robot commented May 17, 2023

Uh oh!

frobware May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfredette May 19, 2023

Choose a reason for hiding this comment

Uh oh!

rfredette May 19, 2023

Choose a reason for hiding this comment

Uh oh!

frobware May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfredette May 19, 2023

Choose a reason for hiding this comment

Uh oh!

frobware May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfredette May 19, 2023

Choose a reason for hiding this comment

Uh oh!

rfredette May 19, 2023

Choose a reason for hiding this comment

Uh oh!

frobware May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frobware May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frobware May 19, 2023

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented May 19, 2023

Uh oh!

frobware commented May 22, 2023

Uh oh!

Miciah commented May 22, 2023

Uh oh!

openshift-ci bot commented May 22, 2023

Uh oh!

openshift-ci-robot commented May 22, 2023

Uh oh!

rfredette commented May 23, 2023

Uh oh!

openshift-cherrypick-robot commented May 23, 2023

Uh oh!

Uh oh!

rfredette commented Apr 20, 2023 •

edited

Loading

frobware May 19, 2023 •

edited

Loading

frobware May 19, 2023 •

edited

Loading

frobware May 19, 2023 •

edited

Loading

frobware May 19, 2023 •

edited

Loading

frobware May 19, 2023 •

edited

Loading