[release-4.15] OCPBUGS-49392: Block Upgrades for CA-Signed Certs Using SHA1

[gcs278]

Previously, upgrades from 4.15 to 4.16 were only blocked for default leaf certs using SHA1. However, in 4.16, any SHA1 cert that is CA-signed (not self-signed) is unsupported. As a result, we were incorrectly allowing upgrades for SHA1 intermediate certificates, while blocking upgrades for self-signed SHA1 leaf certificates.

This fix blocks upgrades to 4.16 for IngressControllers with default certificates containing CA-signed SHA1 certs. Root CA certs using SHA1 are still supported.

Additional Fix For Blocking 4.15-->4.16 Upgrades for DSA SHA1 Default Certs:

Previously, upgrades were not being blocked when default certificates used DSA SHA1. Although DSA certificates are rejected at the router level by our validation logic, they can still be used in a default certificate. In 4.15, DSA SHA1 certificates are allowed, but in 4.16, they are rejected by OpenSSL. This fix adds an upgrade blocker in the rare case that a user was using a DSA SHA1 default certificate.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-45290, which is invalid:

• expected the bug to target the "4.15.z" version, but no target version was set
• release note text must be set and not match the template OR release note type must be set to "Release Note Not Required". For more information you can reference the OpenShift Bug Process.
• expected Jira Issue OCPBUGS-45290 to depend on a bug targeting a version in 4.16.0, 4.16.z and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA), but no dependents were found

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:

Previously, we blocked upgrades for SHA1 leaf certs as they are rejected in 4.16. However, intermediate CA certs using SHA1 are also rejected. This update blocks updates for IngressControllers with defaultCertificates that contain an SHA1 intermediate cert.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-45290, which is invalid:

• expected the bug to target either version "4.15." or "openshift-4.15.", but it targets "4.19.0" instead
• release note text must be set and not match the template OR release note type must be set to "Release Note Not Required". For more information you can reference the OpenShift Bug Process.
• expected Jira Issue OCPBUGS-45290 to depend on a bug targeting a version in 4.16.0, 4.16.z and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA), but no dependents were found

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:

Previously, upgrades were only blocked for default leaf certs using SHA1. However, in 4.16, any SHA1 cert that is CA-signed (not self-signed) is unsupported (e.g. an intermediate CA).

This fix blocks upgrades to 4.16 for IngressControllers with default certificates containing CA-signed SHA1 certs. Root CA certs using SHA1are still supported.

WIP:
Need unit testing

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@gcs278: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

In response to this:

Previously, upgrades were only blocked for default leaf certs using SHA1. However, in 4.16, any SHA1 cert that is CA-signed (not self-signed) is unsupported (e.g. an intermediate CA).

This fix blocks upgrades to 4.16 for IngressControllers with default certificates containing CA-signed SHA1 certs. Root CA certs using SHA1are still supported.

WIP:
Need unit testing

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@gcs278: This pull request references Jira Issue OCPBUGS-49392, which is invalid:

• expected dependent Jira Issue OCPBUGS-49391 to be in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA), but it is New instead

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:

Previously, upgrades were only blocked for default leaf certs using SHA1. However, in 4.16, any SHA1 cert that is CA-signed (not self-signed) is unsupported (e.g. an intermediate CA).

This fix blocks upgrades to 4.16 for IngressControllers with default certificates containing CA-signed SHA1 certs. Root CA certs using SHA1are still supported.

WIP:
Need unit testing

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 openshift-eng/jira-lifecycle-plugin repository.

[Thealisyed]

/assign

[Miciah]

The commit message doesn't say anything about adding x509.DSAWithSHA1. Is this PR actually fixing two separate defects (first, the use of cert.IsCA instead of checking the signer; and second, the missing x509.DSAWithSHA1 case expression)?

[gcs278]

EDIT: Sorry, I thought I was in the router repo not CIO....let me rethink/adjust my answer. Keeping the one below because it actually applies to openshift/router#641

I added per this comment thread for the 4.19 fix. It wasn't really fixing anything new, as DSA was already rejected by the router. I decided to add it to provide a more precise error message about SHA1 not being supported rather than a more obscure Invalid value: "redacted key data": block PRIVATE KEY is not valid.

For this 4.15 upgrade blocker, I carried it over for completeness/consistency, but I suppose that since it's already rejected, it's a bit pointless to block upgrades for it and may cause an unnecessary upgrade blocker for someone that already has a DSA SHA1 cert that is rejected. I don't think anyone should be using DSA SHA1 at this point, but I suppose its better to be safe.

I'll remove it.

[gcs278]

Update: Sorry for the confusion here. My answer above actually applies to openshift/router#641 (I'll follow up over there). Here's what I should have said...

Is this PR actually fixing two separate defects (first, the use of cert.IsCA instead of checking the signer; and second, the missing x509.DSAWithSHA1 case expression)?

Short Answer: Yes, upon further reflection, it is fixing two things, which I should mention in the commit message(s).

Long Answer: Originally, for this PR, I added DSA SHA1 without thinking much about it. However, I did some testing for DSA SHA1, and it's nuanced, but we DO actually need to block upgrades with DSA SHA1 for CIO. Here's why:

• While openshift-router (not HAProxy) rejects DSA certificates because it doesn't have logic to parse them, the CIO allows cluster admins to create DSA SHA1 default certificates
• However, you can't use the K8S TLS secret object because it can't parse the DSA key and you get an error (I guess K8S doesn't support it?) but you can still use an opaque/generic secret:
  • oc create secret generic router-cert -n openshift-ingress --from-file=tls.key=dsa.key --from-file=tls.crt=dsa-combo.crt
• I tested a 4.15 router image with a DSA-SHA1 default cert, and it worked
• I tested a 4.16 router image with a DSA-SHA1 default cert, and it failed

In summary, I was unaware there was a path for users to use DSA certs in the default certificate on the IngressController. I do think we need to block upgrades in CIO with DSA-SHA1 in this PR, but not in the router PR because the router has always rejected DSA certs. It's an odd situation.

[gcs278]

I split the fixes two commit so I can clearly document each.

I'm not against splitting it out into another PR and/or bug if recommended, but it feels closely related enough I thought to keep it together for now.

[Miciah]

Just to recap,

• [release-4.15] OCPBUGS-28928: Prevent upgrades for SHA1 default cert and SHA1 route certs  #1014 added logic to check the signing algorithm on every certificate in the chain;
• OCPBUGS-42480: Limit SHA-1 check to the leaf cert in default cert chain #1154 changed that logic to check only the leaf certificate; and
• this PR changes that logic to checking every CA-signed (not self-signed) certificate in the chain (including any root, intermediates, and leaf, but not root because a root certificate is necessarily self-signed).

Right?

[gcs278]

Just to recap,

• [release-4.15] OCPBUGS-28928: Prevent upgrades for SHA1 default cert and SHA1 route certs  #1014 added logic to check the signing algorithm on every certificate in the chain;
• OCPBUGS-42480: Limit SHA-1 check to the leaf cert in default cert chain #1154 changed that logic to check only the leaf certificate; and
• this PR changes that logic to checking every CA-signed (not self-signed) certificate in the chain (including any root, intermediates, and leaf, but not root because a root certificate is necessarily self-signed).

Right?

Yup exactly. An unfortunate failure(s) on my part, which convicted me to do better and actually dig in and understand what/how/why OpenSSL is doing the rejections rather than just trying to observe them from the outside. I found the HAProxy code that throws the md too weak error and traced it into the OpenSSL library skipping checks for self-signed certs. And the fix here and in the router reflect how OpenSSL determines what constitutes a self-signed certificate.

[openshift-ci]

@gcs278: 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-aws-ovn	b457c4a	link	true	/test e2e-aws-ovn
ci/prow/e2e-hypershift	b457c4a	link	true	/test e2e-hypershift
ci/prow/e2e-azure-operator	b457c4a	link	true	/test e2e-azure-operator
ci/prow/e2e-gcp-operator	b457c4a	link	true	/test e2e-gcp-operator
ci/prow/e2e-aws-operator	b457c4a	link	true	/test e2e-aws-operator
ci/prow/e2e-aws-ovn-serial	b457c4a	link	true	/test e2e-aws-ovn-serial
ci/prow/e2e-aws-ovn-single-node	b457c4a	link	false	/test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-upgrade	b457c4a	link	true	/test e2e-aws-ovn-upgrade

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-sigs/prow repository. I understand the commands that are listed here.

[Miciah]

Thanks!
/approve
/lgtm

[openshift-ci]

[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

[Thealisyed]

How do these changes impact the performance of the certificate validation process, given if it actually has any performance benchmarks?

[gcs278]

How do these changes impact the performance of the certificate validation process, given if it actually has any performance benchmarks?

Great question. We don't maintain a baseline of performance for really anything in the router, but what I have done before is create a benchmark GO test, and compared the old logic to the new logic.

I think performance is more of a concern with the router admitting routes vs. the CIO, but either way we still need to be cautious.

At one point, I did benchmarking for an iteration of this change (mainly benchmarking the isSelfSigned function) and the impact was minimal. The code is a couple of byte comparisons and an object comparison, all of which aren't heavy operations, so I feel pretty confident about the performance impact being negligible.

Now, at one point I was actually using the X509 validation function to validation each certificate against itself to detect if it was self-signed, and that was not negligible. If that was my current implementation, I would be more cautious and provide some hard benchmarking numbers.

[Miciah]

I think performance is more of a concern with the router admitting routes vs. the CIO, but either way we still need to be cautious.

At one point, I did benchmarking for an iteration of this change (mainly benchmarking the isSelfSigned function) and the impact was minimal. The code is a couple of byte comparisons and an object comparison, all of which aren't heavy operations, so I feel pretty confident about the performance impact being negligible.

Now, at one point I was actually using the X509 validation function to validation each certificate against itself to detect if it was self-signed, and that was not negligible. If that was my current implementation, I would be more cautious and provide some hard benchmarking numbers.

+1, we already parse the certificate, which is the costly part. Agreed, this isn't a major concern for the default certificate chain. It's more of a concern for the router, but we're already doing a lot of parsing, validation, and reserialization there. The main thing to look at is the added cost from the changes.