Skip to content
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-45290: Reject All CA-Signed Certs Using SHA1 #642

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gcs278
Copy link
Contributor

@gcs278 gcs278 commented Dec 4, 2024

Previously, only SHA1 leaf certs were rejected. However, in 4.16, any SHA1 cert that is CA-signed (not self-signed) is unsupported. This led to cases were routes with SHA1 intermediate CA certs were accepted, but HAProxy rejects them. Self-signed SHA1 certificates (i.e. root CA) remain supported since they are not subject to verification.

This update ensures all route certs, including the server, CA, and destination CA certs, are inspected, and any SHA1 cert that is not self-signed is rejected.

@openshift-ci-robot openshift-ci-robot added the jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. label Dec 4, 2024
Copy link
Contributor

openshift-ci bot commented Dec 4, 2024

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 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 Dec 4, 2024
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Dec 4, 2024
@openshift-ci-robot
Copy link
Contributor

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

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

Previously, we rejected leaf certs using SHA1, but we also need to reject intermediate CA certs using SHA1 as HAProxy fails to start.

WIP:
Needs clean up and unit tests.

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 gcs278 force-pushed the reject-intermediate-sha1 branch from d2d10e7 to 36105f1 Compare December 5, 2024 20:17
@gcs278 gcs278 changed the title [WIP] OCPBUGS-45290: Reject Intermediate Certs using SHA1 [WIP] OCPBUGS-45290: Reject All CA-Signed Certs Using SHA1 Dec 5, 2024
@gcs278 gcs278 force-pushed the reject-intermediate-sha1 branch 3 times, most recently from 6b47be2 to 7bb99ea Compare December 5, 2024 20:30
@gcs278 gcs278 changed the title [WIP] OCPBUGS-45290: Reject All CA-Signed Certs Using SHA1 OCPBUGS-45290: Reject All CA-Signed Certs Using SHA1 Dec 5, 2024
@gcs278 gcs278 marked this pull request as ready for review December 5, 2024 20:41
@openshift-ci 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 Dec 5, 2024
@openshift-ci openshift-ci bot requested review from alebedev87 and frobware December 5, 2024 20:41
@gcs278
Copy link
Contributor Author

gcs278 commented Dec 6, 2024

/retest

@gcs278
Copy link
Contributor Author

gcs278 commented Dec 6, 2024

/jira refresh

@openshift-ci-robot
Copy link
Contributor

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

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

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

@gcs278
Copy link
Contributor Author

gcs278 commented Dec 6, 2024

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Dec 6, 2024
@openshift-ci-robot
Copy link
Contributor

@gcs278: This pull request references Jira Issue OCPBUGS-45290, 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.19.0) matches configured target version for branch (4.19.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

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

@openshift-ci openshift-ci bot requested a review from ShudiLi December 6, 2024 02:28
@ShudiLi
Copy link
Member

ShudiLi commented Dec 6, 2024

tested it with 4.18.0-0.ci.test-2024-12-06-060222-ci-ln-4l8tlmt-latest, same steps as the bug

% oc -n test get route                          
NAME           HOST/PORT                  PATH   SERVICES      PORT   TERMINATION   WILDCARD
test-sha1-ca   ExtendedValidationFailed          unsec-apach   8080   edge          None
% oc -n test get route -oyaml | grep -A5 status:
  status:
    ingress:
    - conditions:
      - lastTransitionTime: "2024-12-06T06:54:50Z"
        message: 'spec.tls.caCertificate: Invalid value: "redacted ca certificate
          data": router does not support CA-signed certs using SHA1'
--
        status: "False"
        type: Admitted
      host: test-sha1-ca.apps.example.com
      routerCanonicalHostname: router-default.apps.ci-ln-4l8tlmt-72292.gcp-2.ci.openshift.org
      routerName: default
      wildcardPolicy: None

/label qe-approved
thanks

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 6, 2024
@openshift-ci-robot
Copy link
Contributor

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

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.19.0) matches configured target version for branch (4.19.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

Previously, only SHA1 leaf certs were rejected. However, in 4.16, any SHA1 cert that is CA-signed (not self-signed) is unsupported. This led to cases were routes with SHA1 intermediate CA certs were accepted, but HAProxy rejects them. Self-signed SHA1 certificates (i.e. root CA) remain supported since they are not subject to verification.

This update ensures all route certs, including the server, CA, and destination CA certs, are inspected, and any SHA1 cert that is not self-signed is rejected.

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.

@ShudiLi
Copy link
Member

ShudiLi commented Dec 6, 2024

/retest-required

@gcs278
Copy link
Contributor Author

gcs278 commented Dec 10, 2024

Andy offered to help review (thanks!)
/assign @frobware

@candita
Copy link
Contributor

candita commented Dec 11, 2024

/assign
/assign @Thealisyed

// HAProxy will fail to start if intermediate CA certs use unsupported signature algorithms.
// However, root CAs can still use unsupported algorithms since they are self-signed.
if err := validateCertSignatureAlgorithms(certs); err != nil {
result = append(result, field.Invalid(tlsFieldPath.Child("caCertificate"), "redacted ca certificate data", err.Error()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this log message identify the route, so that the invalid certificate can be found/fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message gets written to the route's Admitted condition type with the reason ExtendedValidationFailed. It is directly written on the affected route, which makes it clear of which route is impacted.

// isSelfSignedCert determines if a provided certificate is
// self-signed by checking if the issuer is equal to the subject.
func isSelfSignedCert(cert *x509.Certificate) bool {
return bytes.Equal(cert.RawIssuer, cert.RawSubject)
Copy link
Contributor

@candita candita Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a discussion of how to tell when a cert is self-signed in https://security.stackexchange.com/questions/93162/how-to-know-if-certificate-is-self-signed, and one example shows "Just because the fields (Issuer and Subject) have the same value that does not mean the certificate is self-signed". Do you think that could apply here, and you also might need to check if there's an Authority present?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea great point. I did find that same comment, but I ended up proceeding with this logic because HAProxy also considers Issuer == Subject as self-signed and allows SHA1 ca-signed leaf certs that have the same Subject & Issuer.

But you made me think about testing Intermediate certs. Just now, I tested a SHA1 Issuer == Subject Intermediate CA cert, and that caused HAProxy to fail. That's not good.

You are right, we need to be more elegant here and handle when the Issuer == Subject edge case. However, I'm struggling to find a perfect solution. I see Authority Key Identifier (AKI) is marked not-critical in the RFC and I see discussion of how it's only recommended, not mandated. I also found an interesting bug in which even the x509 Go library sets AKI to Subject Key Id (SKI) if Subject == Issuer (basically it also assumes if Subj == Issuer, that it's self signed). So I think AKI is not really that reliable to use.

So, I have two options right now:

  1. Check Subject == Issuer && (AKI == nil OR AKI == SKI)
    • This probably covers 99% of cases, but one could still make a cert with Subject == Issuer and AKI == nil while it NOT being self-signed, causing HAProxy to crash
  2. Call CheckSignature and validate if cert is signed by itself
    • I think this is pretty flawless, except:
      1. It's going to be a bit more computationally expensive
      2. There's a risk that the x509 GO library could just flat out reject verification of SHA1 certs in the future. It already flat out rejects doing verifications on MD5 certs. We have to vendor updates to it though, and our unit tests would catch them.

The more I think about this, I wish I could do Option 1, and we had NE-557 to catch the odd cases where Subject == Issuer and AKI isn't added.

But I think we should do option 2, I'll provide benchmarking results. And I tested with the 143 public root CAs on my laptop (some are SHA1) and the function works correctly for them.

@@ -205,6 +205,11 @@ func ExtendedValidateRoute(route *routev1.Route) field.ErrorList {
} else {
tlsConfig.CACertificate = string(data)
}
// HAProxy will fail to start if intermediate CA certs use unsupported signature algorithms.
// However, root CAs can still use unsupported algorithms since they are self-signed.
Copy link
Contributor

@candita candita Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you only check the CACertificate for whether it's self-signed, instead of within validateCertSignatureAlgorithms? In other words, is a self-signed cert okay to be ignored for a DestinationCACertificate or Certificate, which are checked by validateCertSignatureAlgorithm or validateCertificatePEM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, is a self-signed cert okay to be ignored for a DestinationCACertificate or Certificate

DestinationCACertificate needs to allow SHA1 self-signed certs (a lot of root CAs use SHA1). So it is required for us to ignore self-signed certs in our SHA1 validation here.

As for Certificate, there isn't a critical business need to allow SHA1 self-signed leaf certs unlike DestinationCACertificate and CACertificate. But two thoughts:

  1. I think the router should be accurate in what it's rejecting, and shouldn't reject things that HAProxy actually supports.
  2. 4.15 dev clusters (I hope not prod...) may be using self-signed SHA1 certs for testing. When they upgrade, we are currently forcing them to recreate all of them, even though 4.16 HAProxy supports them. Unnecessary upgrade pains for customers.

Open to other opinions.

return certs, fmt.Errorf("router does not support certs using MD5")
default:
// Acceptable algorithm
if err := validateCertSignatureAlgorithms(certs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, this allows the cert to be self-signed now - introducing new behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Our validation was invalid previously, rejecting valid scenarios, so this fixes that.

Do you think that will be a problem?

// openssl req -newkey rsa:1024 -nodes -keyout testCertificateRsaSha1.key -out testCertificateRsaSha1.csr -subj '/CN=www.example.com/ST=SC/C=US/[email protected]/O=Example/OU=Example'
// openssl x509 -req -days 3650 -sha1 -in testCertificateRsaSha1.csr -CA exampleca.crt -CAcreateserial -CAkey exampleca.key -extensions ext -extfile <(echo $'[ext]\nbasicConstraints = CA:FALSE') -out testCertificateRsaSha1.crt
// openssl x509 -req -days 3650 -sha1 -in testCertificateRsaSha1.csr -CA testCertificateRsaSha1CA.crt -CAcreateserial -CAkey testCertificateRsaSha1CA.key -extensions ext -extfile <(echo $'[ext]\nbasicConstraints = CA:FALSE') -out testCertificateRsaSha1.crt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
// openssl x509 -req -days 3650 -sha1 -in testCertificateRsaSha1.csr -CA testCertificateRsaSha1CA.crt -CAcreateserial -CAkey testCertificateRsaSha1CA.key -extensions ext -extfile <(echo $'[ext]\nbasicConstraints = CA:FALSE') -out testCertificateRsaSha1.crt
// openssl x509 -req -days 3650 -sha1 -in testCertificateRsaSha1.csr -CA testCertificateRsaSha1CA.crt -CA createserial -CAkey testCertificateRsaSha1CA.key -extensions ext -extfile <(echo $'[ext]\nbasicConstraints = CA:FALSE') -out testCertificateRsaSha1.crt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@candita candita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, mostly about why it is okay for any certificate to be self-signed now.

@gcs278 gcs278 force-pushed the reject-intermediate-sha1 branch from 7bb99ea to e68af03 Compare December 12, 2024 02:06
Copy link
Contributor

openshift-ci bot commented Dec 12, 2024

[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 candita. For more information see the 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:

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

Copy link
Contributor Author

@gcs278 gcs278 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @candita. I have pushed some fixes and should be ready for review.

// isSelfSignedCert determines if a provided certificate is
// self-signed by checking if the issuer is equal to the subject.
func isSelfSignedCert(cert *x509.Certificate) bool {
return bytes.Equal(cert.RawIssuer, cert.RawSubject)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea great point. I did find that same comment, but I ended up proceeding with this logic because HAProxy also considers Issuer == Subject as self-signed and allows SHA1 ca-signed leaf certs that have the same Subject & Issuer.

But you made me think about testing Intermediate certs. Just now, I tested a SHA1 Issuer == Subject Intermediate CA cert, and that caused HAProxy to fail. That's not good.

You are right, we need to be more elegant here and handle when the Issuer == Subject edge case. However, I'm struggling to find a perfect solution. I see Authority Key Identifier (AKI) is marked not-critical in the RFC and I see discussion of how it's only recommended, not mandated. I also found an interesting bug in which even the x509 Go library sets AKI to Subject Key Id (SKI) if Subject == Issuer (basically it also assumes if Subj == Issuer, that it's self signed). So I think AKI is not really that reliable to use.

So, I have two options right now:

  1. Check Subject == Issuer && (AKI == nil OR AKI == SKI)
    • This probably covers 99% of cases, but one could still make a cert with Subject == Issuer and AKI == nil while it NOT being self-signed, causing HAProxy to crash
  2. Call CheckSignature and validate if cert is signed by itself
    • I think this is pretty flawless, except:
      1. It's going to be a bit more computationally expensive
      2. There's a risk that the x509 GO library could just flat out reject verification of SHA1 certs in the future. It already flat out rejects doing verifications on MD5 certs. We have to vendor updates to it though, and our unit tests would catch them.

The more I think about this, I wish I could do Option 1, and we had NE-557 to catch the odd cases where Subject == Issuer and AKI isn't added.

But I think we should do option 2, I'll provide benchmarking results. And I tested with the 143 public root CAs on my laptop (some are SHA1) and the function works correctly for them.

// openssl req -newkey rsa:1024 -nodes -keyout testCertificateRsaSha1.key -out testCertificateRsaSha1.csr -subj '/CN=www.example.com/ST=SC/C=US/[email protected]/O=Example/OU=Example'
// openssl x509 -req -days 3650 -sha1 -in testCertificateRsaSha1.csr -CA exampleca.crt -CAcreateserial -CAkey exampleca.key -extensions ext -extfile <(echo $'[ext]\nbasicConstraints = CA:FALSE') -out testCertificateRsaSha1.crt
// openssl x509 -req -days 3650 -sha1 -in testCertificateRsaSha1.csr -CA testCertificateRsaSha1CA.crt -CAcreateserial -CAkey testCertificateRsaSha1CA.key -extensions ext -extfile <(echo $'[ext]\nbasicConstraints = CA:FALSE') -out testCertificateRsaSha1.crt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg/router/routeapihelpers/validation_test.go Show resolved Hide resolved
return certs, fmt.Errorf("router does not support certs using MD5")
default:
// Acceptable algorithm
if err := validateCertSignatureAlgorithms(certs); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Our validation was invalid previously, rejecting valid scenarios, so this fixes that.

Do you think that will be a problem?

pkg/router/routeapihelpers/validation_test.go Show resolved Hide resolved
@@ -205,6 +205,11 @@ func ExtendedValidateRoute(route *routev1.Route) field.ErrorList {
} else {
tlsConfig.CACertificate = string(data)
}
// HAProxy will fail to start if intermediate CA certs use unsupported signature algorithms.
// However, root CAs can still use unsupported algorithms since they are self-signed.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, is a self-signed cert okay to be ignored for a DestinationCACertificate or Certificate

DestinationCACertificate needs to allow SHA1 self-signed certs (a lot of root CAs use SHA1). So it is required for us to ignore self-signed certs in our SHA1 validation here.

As for Certificate, there isn't a critical business need to allow SHA1 self-signed leaf certs unlike DestinationCACertificate and CACertificate. But two thoughts:

  1. I think the router should be accurate in what it's rejecting, and shouldn't reject things that HAProxy actually supports.
  2. 4.15 dev clusters (I hope not prod...) may be using self-signed SHA1 certs for testing. When they upgrade, we are currently forcing them to recreate all of them, even though 4.16 HAProxy supports them. Unnecessary upgrade pains for customers.

Open to other opinions.

Copy link
Contributor

openshift-ci bot commented Dec 12, 2024

@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/okd-scos-e2e-aws-ovn e68af03 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-agnostic e68af03 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-sigs/prow repository. I understand the commands that are listed here.

Previously, only SHA1 leaf certs were rejected. However, in 4.16, any
SHA1 cert that is CA-signed (not self-signed) is unsupported. This led
to cases were routes with SHA1 intermediate CA certs were accepted, but
HAProxy rejects them. Self-signed SHA1 certificates (i.e. root CA)
remain supported since they are not subject to verification.

This update ensures all route certs, including the server, CA, and
destination CA certs, are inspected, and any SHA1 cert that is not
self-signed is rejected.
@gcs278 gcs278 force-pushed the reject-intermediate-sha1 branch from e68af03 to 660cacd Compare December 24, 2024 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants