Bug 2052467: Custom route HTTPS certificate SAN validation #545

pierreprinetti · 2022-02-22T10:04:26Z

This patch adds a new validation check for custom route HTTPS
certificates: certificates without SAN fields will prevent upgrades.

This change puts in force the long-standing deprecation of the CN field
as a provider for names.

After this patch, custom routes secured with legacy HTTPS certificates
will prevent the upgrade to OCP v4.10. OCP v4.10 is compiled with Go
v1.17 and is incompatible with such legacy certificates; this
deprecation prevents unexpected failures to occur on upgrade.

Addresses: Bug 2052467

openshift-ci · 2022-02-22T10:06:39Z

@pierreprinetti: This pull request references Bugzilla bug 2052467, which is invalid:

expected dependent Bugzilla bug 2037274 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is POST instead
expected dependent Bugzilla bug 2037274 to target a release in 4.10.0, but it targets "4.9.z" instead

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

In response to this:

Bug 2052467: HTTPS certificate validation to check for SAN

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.

s-urbaniak · 2022-02-22T10:52:42Z

@pierreprinetti thank you for the PR 👍 did you test this on a cluster-bot cluster? i.e. by configuring a faulty cert and then resetting it to a valid one?

openshift-ci · 2022-02-23T08:15:14Z

@pierreprinetti: This pull request references Bugzilla bug 2052467, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 2031839 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 2031839 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0
bug has dependents

Requesting review from QA contact:
/cc @xingxingxia

In response to this:

WIP: Bug 2052467: HTTPS certificate validation to check for SAN

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 · 2022-02-23T08:38:02Z

@pierreprinetti: This pull request references Bugzilla bug 2052467, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 2031839 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 2031839 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0
bug has dependents

Requesting review from QA contact:
/cc @xingxingxia

In response to this:

WIP: Bug 2052467: HTTPS certificate validation to check for SAN

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.

stlaz · 2022-02-23T09:44:08Z

pkg/controllers/routercerts/controller.go

@@ -207,6 +207,13 @@ func (c *routerCertsDomainValidationController) validateRouterCertificates() ope
 		return newRouterCertsDegradedf("InvalidServerCertRouterCerts", "secret/%v.spec.data[%v] -n %v: certificate could not validate route hostname %v: %v", c.defaultSecretName, ingressDomain, c.secretNamespace, verifyOptions.DNSName, err)
 	}

+	// check the server certificates for reliance on the deprecated CN field


This is a redundant change, verifyWithAnyCertificate should already return errors.

You should be looking at https://github.com/openshift/cluster-authentication-operator/blob/55b274c54c4a404799dea0e9e41f1b62e9f84c55/pkg/controllers/customroute/custom_route_controller.go

Until Go v1.17, the GODEBUG=x509ignoreCN=0 flag (which is embedded in all base images) will prevent verifyWithAnyCertificate, or any other certificate parsing, from failing due to missing SAN fields. This patch is targeted against OCP v4.9 which is compiled with Go v1.16.

That being said, custom_route_controller seems like a good tip. Do you think that the check should be implemented in both places? @stlaz

Oh and thank you for the review!

I have added the check in ValidateServerCert, which is the validation function called in the custom route controller loop. Thank you!

@stlaz PTAL

I suppose we could have this check even here, although the description should say: "open a bug to the Routing component". cc @openshift/openshift-team-network-edge

@stlaz i believe we had a discussion round with the router team. their assertion is that they don't provision the cert per se, so it is an exercise on the corresponding operators to do the validation.

Thank you. I understand that this means that we keep both checks in place.

we keep both checks in place.

agreed, hence SAN-less certs can happen both in the central route cert and in custom route certs and for both cases we should verify.

@stlaz i believe we had a discussion round with the router team. their assertion is that they don't provision the cert per se, so it is an exercise on the corresponding operators to do the validation.

I believe they eventually decided to introduce the checks. If they did not check their inputs, our operators would go degraded.

pierreprinetti · 2022-02-23T18:24:23Z

@pierreprinetti thank you for the PR +1 did you test this on a cluster-bot cluster? i.e. by configuring a faulty cert and then resetting it to a valid one?

Tested on AWS with cluster-bot, following the BZ steps. The certificate is rejected:

$ oc get ingress.config cluster -o yaml
[...]
status:
  componentRoutes:
  - conditions:
    - lastTransitionTime: "2022-02-23T18:22:02Z"
      message: "Error Configuring custom route: [error validating secret openshift-config/custom-auth-component:
        [certificate relies on legacy Common Name field, use SANs instead:\n\tsub=CN=auth-openshift-custom.jkfd.de;\n\tiss=CN=xxia_test_ca]]"
      reason: CustomRouteError
      status: "True"
      type: Degraded
    - lastTransitionTime: "2022-02-23T18:22:02Z"
      message: "Error Configuring custom route: [error validating secret openshift-config/custom-auth-component:
        [certificate relies on legacy Common Name field, use SANs instead:\n\tsub=CN=auth-openshift-custom.jkfd.de;\n\tiss=CN=xxia_test_ca]]"
      reason: CustomRouteError
      status: "False"
      type: Progressing
[...]

pierreprinetti · 2022-02-23T21:03:46Z

/retest

openshift-ci · 2022-05-04T13:20:12Z

@pierreprinetti: This pull request references Bugzilla bug 2052467, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 2031839 is in the state CLOSED (ERRATA), which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 2031839 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0, 4.10.z
bug has dependents

Requesting review from QA contact:
/cc @xingxingxia

In response to this:

Bug 2052467: HTTPS certificate validation to check for SAN

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.

pierreprinetti · 2022-05-04T14:47:58Z

/test e2e-agnostic e2e-console-login

openshift-ci · 2022-05-05T08:20:44Z

@pierreprinetti: This pull request references Bugzilla bug 2052467, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.z) matches configured target release for branch (4.9.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
dependent bug Bugzilla bug 2031839 is in the state CLOSED (ERRATA), which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
dependent Bugzilla bug 2031839 targets the "4.10.0" release, which is one of the valid target releases: 4.10.0, 4.10.z
bug has dependents

Requesting review from QA contact:
/cc @xingxingxia

In response to this:

Bug 2052467: Custom route HTTPS certificate SAN validation

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.

pierreprinetti · 2022-05-05T08:21:15Z

Changed the scope of this PR to only validate custom route server certificates.

@stlaz PTAL

pierreprinetti · 2022-05-05T12:02:23Z

/retest-required

pierreprinetti · 2022-06-10T14:45:10Z

@stlaz ?

stlaz · 2022-06-13T12:25:26Z

Makefile

@@ -1,6 +1,8 @@
 all: build
 .PHONY: all

+export GODEBUG := x509ignoreCN=0


The art-team sets it in their builds.

pkg/controllers/customroute/custom_route_conditions.go

This patch adds a new validation check for custom route HTTPS certificates: certificates without SAN fields will prevent upgrades. This change puts in force the long-standing deprecation of the CN field as a provider for names. After this patch, custom routes secured with legacy HTTPS certificates will prevent the upgrade to OCP v4.10. OCP v4.10 is compiled with Go v1.17 and is incompatible with such legacy certificates; this deprecation prevents unexpected failures to occur on upgrade. Addresses: Bug 2052467

pierreprinetti · 2022-06-15T10:37:48Z

The art-team sets it in their builds.

The flag is embedded in OCP base images since v4.6. As a consequence, our code in this repository expects to run under it. Local runs (triggered with make) should run with the same flag.

If we don't add the flag, the unit tests will fail. And this is a good thing.

So either we add the flag in the Makefile, or at runtime in the test files. I think that here in the Makefile is better because it sets it for every test, thus making the tests behave a little more closely to what they'd do in production.

pierreprinetti · 2022-06-15T16:22:14Z

/retest

pierreprinetti · 2022-06-16T08:59:44Z

/retest

stlaz · 2022-06-16T11:08:12Z

/lgtm

openshift-ci · 2022-06-16T11:09:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierreprinetti, stlaz

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

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

openshift-ci · 2022-06-16T11:30:41Z

@pierreprinetti: The following test 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-agnostic-ipv6	`10037e4`	link	false	`/test e2e-agnostic-ipv6`

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.

xingxingxia · 2022-06-17T01:33:25Z

/label cherry-pick-approved

pierreprinetti · 2022-06-20T11:52:52Z

@stlaz Are you the one who can put the backport-risk-assessed label?

pierreprinetti · 2022-06-23T08:32:48Z

If the Bugzilla associated with the PR has the "FastFix" keyword, the subjective assessment on the issue has already been done and a customer is impacted. These PRs should be prioritized for merge.
- verified
- does not apply
The bug has significant impact either through severity, reduction in supportability, or number of users affected.
- verified
- does not apply
For branches that are in the Maintenance lifecycle phase:
- The bug is a critical fix, no reasonable workaround exists, and a recommendation for upgrade has been ruled out, or
- The bug is a security related bug
The severity field of the bug must be set to accurately reflect criticality.
- verified
The PR is merged in the next newer release branch and the bug for that newer release is VERIFIED by QE. The PR’s description is well formed with user-focused release notes that state the bug number, impact, cause, and resolution. Where appropriate, it should also contain information about how a user can identify whether a particular cluster is affected.
- verified
- does not apply

stlaz · 2022-06-24T06:19:57Z

/label backport-risk-assessed

openshift-ci · 2022-06-24T06:22:57Z

@pierreprinetti: All pull requests linked via external trackers have merged:

openshift/cluster-authentication-operator#545

Bugzilla bug 2052467 has been moved to the MODIFIED state.

In response to this:

Bug 2052467: Custom route HTTPS certificate SAN validation

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 a review from s-urbaniak February 22, 2022 10:04

openshift-ci bot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Feb 22, 2022

pierreprinetti changed the title ~~Bug 2052467: HTTPS certificate validation to check for SAN~~ WIP: Bug 2052467: HTTPS certificate validation to check for SAN Feb 22, 2022

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 22, 2022

pierreprinetti changed the title ~~WIP: Bug 2052467: HTTPS certificate validation to check for SAN~~ Bug 2052467: HTTPS certificate validation to check for SAN Feb 22, 2022

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 Feb 22, 2022

pierreprinetti changed the title ~~Bug 2052467: HTTPS certificate validation to check for SAN~~ WIP: Bug 2052467: HTTPS certificate validation to check for SAN Feb 22, 2022

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 22, 2022

pierreprinetti force-pushed the bug2052467 branch 2 times, most recently from 63355fd to de2ed0e Compare February 23, 2022 08:13

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 23, 2022

openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 23, 2022

openshift-ci bot requested a review from xingxingxia February 23, 2022 08:15

pierreprinetti force-pushed the bug2052467 branch from de2ed0e to 0293f49 Compare February 23, 2022 08:18

pierreprinetti changed the title ~~WIP: Bug 2052467: HTTPS certificate validation to check for SAN~~ Bug 2052467: HTTPS certificate validation to check for SAN Feb 23, 2022

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 Feb 23, 2022

pierreprinetti changed the title ~~Bug 2052467: HTTPS certificate validation to check for SAN~~ WIP: Bug 2052467: HTTPS certificate validation to check for SAN Feb 23, 2022

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 23, 2022

pierreprinetti force-pushed the bug2052467 branch from 0293f49 to 1a91831 Compare February 23, 2022 08:37

stlaz suggested changes Feb 23, 2022

View reviewed changes

pierreprinetti force-pushed the bug2052467 branch from 1a91831 to 9c9ee6e Compare February 23, 2022 17:11

pierreprinetti changed the title ~~WIP: Bug 2052467: HTTPS certificate validation to check for SAN~~ Bug 2052467: HTTPS certificate validation to check for SAN Feb 23, 2022

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 Feb 23, 2022

pierreprinetti force-pushed the bug2052467 branch from bd4b982 to 3b539f3 Compare May 5, 2022 08:19

pierreprinetti changed the title ~~Bug 2052467: HTTPS certificate validation to check for SAN~~ Bug 2052467: Custom route HTTPS certificate SAN validation May 5, 2022

stlaz suggested changes Jun 13, 2022

View reviewed changes

pierreprinetti force-pushed the bug2052467 branch from 3b539f3 to 10037e4 Compare June 15, 2022 10:27

openshift-ci bot assigned stlaz Jun 16, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2022

openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jun 17, 2022

openshift-ci bot added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Jun 24, 2022

openshift-ci bot merged commit 48a20f5 into openshift:release-4.9 Jun 24, 2022

pierreprinetti deleted the bug2052467 branch June 24, 2022 07:24

vinzent mentioned this pull request Oct 5, 2022

SAN validation fails on intermediate/issuing cert #585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2052467: Custom route HTTPS certificate SAN validation #545

Bug 2052467: Custom route HTTPS certificate SAN validation #545

pierreprinetti commented Feb 22, 2022 •

edited

Loading

openshift-ci bot commented Feb 22, 2022

s-urbaniak commented Feb 22, 2022

openshift-ci bot commented Feb 23, 2022

openshift-ci bot commented Feb 23, 2022

stlaz Feb 23, 2022

pierreprinetti Feb 23, 2022

pierreprinetti Feb 23, 2022

pierreprinetti Feb 23, 2022

pierreprinetti Feb 23, 2022

stlaz Feb 24, 2022

s-urbaniak Feb 24, 2022

pierreprinetti Feb 24, 2022

s-urbaniak Feb 24, 2022

stlaz Feb 28, 2022

pierreprinetti commented Feb 23, 2022

pierreprinetti commented Feb 23, 2022

openshift-ci bot commented May 4, 2022

pierreprinetti commented May 4, 2022

openshift-ci bot commented May 5, 2022

pierreprinetti commented May 5, 2022

pierreprinetti commented May 5, 2022

pierreprinetti commented Jun 10, 2022

stlaz Jun 13, 2022

pierreprinetti commented Jun 15, 2022

pierreprinetti commented Jun 15, 2022

pierreprinetti commented Jun 16, 2022

stlaz commented Jun 16, 2022

openshift-ci bot commented Jun 16, 2022

openshift-ci bot commented Jun 16, 2022

xingxingxia commented Jun 17, 2022

pierreprinetti commented Jun 20, 2022

pierreprinetti commented Jun 23, 2022

stlaz commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

Bug 2052467: Custom route HTTPS certificate SAN validation #545

Bug 2052467: Custom route HTTPS certificate SAN validation #545

Conversation

pierreprinetti commented Feb 22, 2022 • edited Loading

openshift-ci bot commented Feb 22, 2022

s-urbaniak commented Feb 22, 2022

openshift-ci bot commented Feb 23, 2022

openshift-ci bot commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti commented Feb 23, 2022

pierreprinetti commented Feb 23, 2022

openshift-ci bot commented May 4, 2022

pierreprinetti commented May 4, 2022

openshift-ci bot commented May 5, 2022

pierreprinetti commented May 5, 2022

pierreprinetti commented May 5, 2022

pierreprinetti commented Jun 10, 2022

Choose a reason for hiding this comment

pierreprinetti commented Jun 15, 2022

pierreprinetti commented Jun 15, 2022

pierreprinetti commented Jun 16, 2022

stlaz commented Jun 16, 2022

openshift-ci bot commented Jun 16, 2022

openshift-ci bot commented Jun 16, 2022

xingxingxia commented Jun 17, 2022

pierreprinetti commented Jun 20, 2022

pierreprinetti commented Jun 23, 2022

stlaz commented Jun 24, 2022

openshift-ci bot commented Jun 24, 2022

pierreprinetti commented Feb 22, 2022 •

edited

Loading