-
Notifications
You must be signed in to change notification settings - Fork 181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-28928: Prevent upgrades for SHA1 default cert and SHA1 route certs #1014
base: release-4.15
Are you sure you want to change the base?
Conversation
@gcs278: This pull request references Jira Issue OCPBUGS-26498, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
@gcs278: This pull request references Jira Issue OCPBUGS-26498, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
4ea39b1
to
a565640
Compare
@gcs278: This pull request references Jira Issue OCPBUGS-26498, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
@gcs278: This pull request references Jira Issue OCPBUGS-26498, which is invalid:
Comment In response to this:
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. |
ad79a4a
to
cd1e2c9
Compare
if cert.Subject.CommonName == domain && !foundSAN { | ||
return fmt.Errorf("certificate in secret %s/%s has legacy Common Name (CN) but has no Subject Alternative Name (SAN) for domain: %s", secret.Namespace, secret.Name, domain) | ||
if cert.SignatureAlgorithm == x509.SHA1WithRSA || cert.SignatureAlgorithm == x509.ECDSAWithSHA1 { | ||
return fmt.Errorf("certificate in secret %s/%s has weak SHA1 signature algorithm: %s", secret.Namespace, secret.Name, domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a link out to https://docs.openshift.com/container-platform/4.16/release_notes/ocp-4-16-release-notes.html#ocp-4-16-networking ? And possibly extending that section to fix "HaProxy" -> "HAProxy" and link https://docs.openshift.com/container-platform/4.16/networking/routes/secured-routes.html ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a link out to https://docs.openshift.com/container-platform/4.16/release_notes/ocp-4-16-release-notes.html#ocp-4-16-networking ?
Sure, added.
And possibly extending that section to fix "HaProxy" -> "HAProxy" and link https://docs.openshift.com/container-platform/4.16/networking/routes/secured-routes.html ?
Yea seems like a reasonable update, let me follow up with docs to see if they are interested.
/retest-required |
/jira refresh |
@ShudiLi: This pull request references Jira Issue OCPBUGS-28928, which is invalid:
Comment In response to this:
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. |
foundSAN = true | ||
} | ||
} | ||
if cert.Subject.CommonName == domain && !foundSAN { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this SAN upgrade logic seems problematic as we are backporting this in 4.15, so then 4.15 will not have this logic, but 4.16+ will have it.
I think the logic needs to be removed in the master branch instead of removed in a backport like this. The 4.16 bug https://issues.redhat.com/browse/OCPBUGS-26498 is already ON_QA and we've branched to 4.17, so it's probably too late to merge this in master and try to backport it.
I think I should just handle this later when appropriate, keep this backport as "additive" only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to not remove the SAN logic.
If the default certificate is using SHA1, then set Upgradeable to be False. With OpenSSL3.0 provided by RHEL9, the router will fail to start if any cert provided is SHA1.
@gcs278: This pull request references Jira Issue OCPBUGS-28928, which is invalid:
Comment In response to this:
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. |
Bump openshift/api to get new route UnservableInFutureVersions status condition type. go mod edit -replace=github.com/openshift/api=github.com/openshift/api@93d6bda14341f1d18319774367829c3278c171e7 go mod tidy go mod vendor
The route upgradeable control loop determines upgradeablity by searching for the UnservableInFutureVersions condition in the routes. It creates an admin-gate if the condition is found. This implemenentation targets 4.15 upgrades to 4.16 specifically and assumes any UnservableInFutureVersions route status is an upgrade blocker. Add E2E test to validate the functionality of the admin-gate as well as validating that the router adds the UnservableInFutureVersions condition for routes with SHA1 certificates.
@gcs278: This pull request references Jira Issue OCPBUGS-28928, which is valid. 7 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
@gcs278: The following tests failed, say
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. |
This PR handles SHA1 certificate upgradeable scenarios for 4.15. With OpenSSL3.0 provided by RHEL9 in 4.16, the router will fail to start if any cert provided is SHA1.
First, if the default certificate on the Ingress Controller object is using SHA1, then set
Upgradeable
to be False.Secondly, add a new control loop, route deprecated, that determines upgradeablity by searching for the
UnservableInFutureVersions
condition in the routes. TheUnservableInFutureVersions
will be added to the route by openshift/router#555 when a SHA1 certificate exists on the route. It creates an admin-gate if aUnservableInFutureVersions
condition is found. This logic assumes anyUnservableInFutureVersions
route status condition is an upgrade blocker.This implementation targets 4.15 to 4.16 updates specifically, so it is only targeting the
release-4.15
branch and we will be merged in the backport of https://issues.redhat.com/browse/OCPBUGS-26498. The code should not be merged into 4.16.