Skip to content

Bug 2047913: Fix HAProxy tests on FIPS properly#26803

Merged
openshift-merge-robot merged 4 commits intoopenshift:masterfrom
Miciah:BZ2047913-fix-HAProxy-tests-on-FIPS-properly
Apr 1, 2022
Merged

Bug 2047913: Fix HAProxy tests on FIPS properly#26803
openshift-merge-robot merged 4 commits intoopenshift:masterfrom
Miciah:BZ2047913-fix-HAProxy-tests-on-FIPS-properly

Conversation

@Miciah
Copy link
Contributor

@Miciah Miciah commented Jan 29, 2022

Revert "Skip some HAProxy tests on FIPS"

This reverts #26800.

test/extended/util/url: Use curl --connect-to

  • test/extended/util/url/url.go (CURL): Add a new ProxyHost field.
    (Through): Assign the given address to ProxyHost instead of changing the request host or headers.
    (ToShell): Use Curl's --connect-to flag to specify the proxy host.

Fix HAProxy tests on FIPS properly

Change existing HAProxy tests to use a FIPS-compatible, 2048-bit RSA key. Add a new test to verify that using a FIPS-incompatible, 1024-bit RSA key for the router's default certificate fails on FIPS clusters and succeeds on non-FIPS clusters.

The certificate used in this new test comes from the default certificate that is currently built into the router image (which is why the tests were failing on FIPS). The certificate used for the already existing tests was generated using the following Shell commands on a RHEL 8.4 system with OpenSSL 1.1.1g:

openssl req -x509 -newkey rsa:2048 -days 3650 -keyout ca.key -out ca.crt -nodes -subj '/C=US/ST=SC/L=Default City/O=Default Company Ltd/OU=Test CA/CN=www.exampleca.com/emailAddress=example@example.com'
openssl req -newkey rsa:2048 -nodes -keyout tls.key -out router.csr -subj '/CN=www.example.com/ST=SC/C=US/emailAddress=example@example.com/O=Example/OU=Example'
printf 'basicConstraints=CA:FALSE\n' > router.cnf
openssl x509 -req -days 3650 -in router.csr -signkey tls.key -CA ca.crt -CAcreateserial -CAkey ca.key -out tls.crt -clrext -extfile router.cnf
cat tls.{crt,key} > default_pub_keys.pem

These commands generate a certificate that has the same parameters as the old one except for the serial number, signature algorithm, validity period, and key size.

  • test/extended/router/certs.go: New file with the new tests.
  • test/extended/testdata/router/router-common.yaml: Change route-1 and route-2 from non-TLS routes to edge-terminated TLS routes with insecureEdgeTerminationPolicy: Allow.
  • test/extended/testdata/router/router-override-domains.yaml:
  • test/extended/testdata/router/router-override.yaml:
  • test/extended/testdata/router/router-scoped.yaml:
  • test/extended/testdata/router/weighted-router.yaml: Add a parameter for the default certificate with the default value being a certificate with a FIPS-compatible, 2048-bit RSA key.
  • test/extended/testdata/bindata.go:
  • test/extended/util/annotate/generated/zz_generated.annotations.go: Regenerate.

test/extended/router: Dump crypto-policy for FIPS

Update the new tests that use a FIPS-incompatible key to gather and print out the content of /etc/crypto-policies/back-ends/opensslcnf.config so that the configured crypto-policy can be examined in case of test failures.

  • test/extended/router/certs.go: cat /etc/crypto-policies/back-ends/opensslcnf.config after each test.

@openshift-ci openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high 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 Jan 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 29, 2022

@Miciah: This pull request references Bugzilla bug 2047913, which is invalid:

  • expected the bug to target the "4.11.0" release, but it targets "---" 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.

Details

In response to this:

Bug 2047913: Fix HAProxy tests on FIPS properly

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.

@Miciah
Copy link
Contributor Author

Miciah commented Jan 29, 2022

/bugzilla refresh

@openshift-ci openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 29, 2022

@Miciah: This pull request references Bugzilla bug 2047913, 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.

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

Requesting review from QA contact:
/cc @lihongan

Details

In response to this:

/bugzilla 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.

@openshift-ci openshift-ci bot requested a review from lihongan January 29, 2022 00:40
port = "443"
}
}
resolve = fmt.Sprintf("--resolve %s:%s:%s", host, port, ut.ProxyHost)
Copy link
Contributor

Choose a reason for hiding this comment

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

will this function on ipv6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, if the address is bracketed. I've added a comment to Through to mention that IPv6 addresses should be bracketed. I've also replaced --resolve with --connect-to, which handles IPv4 addresses, bracketed IPv6 addresses, and host names (the last would have failed with --resolve, which would cause problems on platforms such as AWS where the LB status has a host name rather than an IP address).

@deads2k
Copy link
Contributor

deads2k commented Jan 29, 2022

/approve

The overall approach looks reasonable. I'll leave lgtm with someone on network-edge. @frobware perhaps?

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2022
* test/extended/util/url/url.go (CURL): Add a new ProxyHost field.
(Through): Assign the given address to ProxyHost instead of changing the
request host or headers.
(ToShell): Use Curl's --connect-to flag to specify the proxy host.
Change existing HAProxy tests to use a FIPS-compatible, 2048-bit RSA key.
Add a new test to verify that using a FIPS-incompatible, 1024-bit RSA key
for the router's default certificate fails on FIPS clusters and succeeds on
non-FIPS clusters.

The certificate used in this new test comes from the default certificate
that is currently built into the router image (which is why the tests were
failing on FIPS).  The certificate used for the already existing tests was
generated using the following Shell commands on a RHEL 8.4 system with
OpenSSL 1.1.1g:

    openssl req -x509 -newkey rsa:2048 -days 3650 -keyout ca.key -out ca.crt -nodes -subj '/C=US/ST=SC/L=Default City/O=Default Company Ltd/OU=Test CA/CN=www.exampleca.com/emailAddress=example@example.com'
    openssl req -newkey rsa:2048 -nodes -keyout tls.key -out router.csr -subj '/CN=www.example.com/ST=SC/C=US/emailAddress=example@example.com/O=Example/OU=Example'
    printf 'basicConstraints=CA:FALSE\n' > router.cnf
    openssl x509 -req -days 3650 -in router.csr -signkey tls.key -CA ca.crt -CAcreateserial -CAkey ca.key -out tls.crt -clrext -extfile router.cnf
    cat tls.{crt,key} > default_pub_keys.pem

These commands generate a certificate that has the same parameters as the
old one except for the serial number, signature algorithm, validity period,
and key size.

This commit fixes bug 2047913.

https://bugzilla.redhat.com/show_bug.cgi?id=2047913

* test/extended/router/certs.go: New file with the new tests.
* test/extended/testdata/router/router-common.yaml: Change route-1 and
route-2 from non-TLS routes to edge-terminated TLS routes with
insecureEdgeTerminationPolicy: Allow.
* test/extended/testdata/router/router-override-domains.yaml:
* test/extended/testdata/router/router-override.yaml:
* test/extended/testdata/router/router-scoped.yaml:
* test/extended/testdata/router/weighted-router.yaml: Add a parameter for
the default certificate with the default value being a certificate with a
FIPS-compatible, 2048-bit RSA key.
* test/extended/testdata/bindata.go:
* test/extended/util/annotate/generated/zz_generated.annotations.go:
Regenerate.
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 29, 2022

@Miciah: This pull request references Bugzilla bug 2047913, which is valid.

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

Requesting review from QA contact:
/cc @lihongan

Details

In response to this:

Bug 2047913: Fix HAProxy tests on FIPS properly

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.

@Miciah Miciah force-pushed the BZ2047913-fix-HAProxy-tests-on-FIPS-properly branch from d0cab54 to 646a575 Compare January 29, 2022 03:01
@Miciah
Copy link
Contributor Author

Miciah commented Jan 29, 2022

Latest push updates some additional manifests with the FIPS-compatible certificate and replaces --resolve with --connect-to.

@Miciah
Copy link
Contributor Author

Miciah commented Jan 29, 2022

The new test failed because the router started even with a FIPS-incompatible, 1024-bit key on a FIPS-enabled cluster. I'm not sure how that could happen. The test passed for me when I tested with cluster-bot; in manual testing, the router did not start with a FIPS-incompatible key on a FIPS-enabled GCP cluster.
/test e2e-aws-fips

@Miciah
Copy link
Contributor Author

Miciah commented Jan 29, 2022

/test e2e-gcp-fips-serial

@frobware
Copy link
Contributor

Many failures.

/retest

@Miciah
Copy link
Contributor Author

Miciah commented Jan 31, 2022

The tests work on cluster-bot fips clusters. In addition to my earlier manual testing on a FIPS-enabled GCP cluster, I used /msg @cluster-bot test e2e openshift/origin#26803 aws,fips and /msg @cluster-bot test e2e openshift/origin#26803 gcp,fips to verify that the tests work properly on FIPS clusters. Linked below are the passing cluster-bot jobs, which show that [sig-network][Feature:Router] when FIPS is enabled the HAProxy router should not work when configured with a 1024-bit RSA key passes and [sig-network][Feature:Router] when FIPS is disabled the HAProxy router should serve routes when configured with a 1024-bit RSA key is skipped on FIPS clusters:

https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-aws-modern/1488206794919514112

https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-gcp-modern/1488178502883610624

I'm increasingly thinking that our e2e-aws-fips and e2e-gcp-fips-serial tests are broken.

/test e2e-aws-fips
/test e2e-gcp-fips-serial

Update the new tests that use a FIPS-incompatible key to gather and print
out the content of /etc/crypto-policies/back-ends/opensslcnf.config so that
the configured crypto-policy can be examined in case of test failures.

* test/extended/router/certs.go: cat
/etc/crypto-policies/back-ends/opensslcnf.config after each test.
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 31, 2022

@Miciah: An error was encountered querying GitHub for users with public email (hongli@redhat.com) for bug 2047913 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message. non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

Details

In response to this:

Bug 2047913: Fix HAProxy tests on FIPS properly

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.

@Miciah
Copy link
Contributor Author

Miciah commented Feb 1, 2022

The e2e-gcp-fips-serial job has gone missing...
/refresh

@Miciah
Copy link
Contributor Author

Miciah commented Feb 1, 2022

/test e2e-gcp-fips-serial

@Miciah
Copy link
Contributor Author

Miciah commented Feb 1, 2022

/test images

@Miciah
Copy link
Contributor Author

Miciah commented Feb 2, 2022

/test e2e-aws-fips

@Miciah
Copy link
Contributor Author

Miciah commented Mar 9, 2022

/test e2e-aws-fips
/test e2e-gcp-fips-serial

@Miciah
Copy link
Contributor Author

Miciah commented Mar 9, 2022

   * could not run steps: step src failed: could not get build src: builds.build.openshift.io "src" not found 

/test e2e-aws-fips

@Miciah
Copy link
Contributor Author

Miciah commented Mar 31, 2022

/test e2e-aws-fips

@Miciah
Copy link
Contributor Author

Miciah commented Mar 31, 2022

/retest

1 similar comment
@Miciah
Copy link
Contributor Author

Miciah commented Apr 1, 2022

/retest

@frobware
Copy link
Contributor

frobware commented Apr 1, 2022

/lgtm

@frobware
Copy link
Contributor

frobware commented Apr 1, 2022

/skip

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, frobware, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 1, 2022

@Miciah: 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-gcp-ovn-rt-upgrade 11f619c link false /test e2e-gcp-ovn-rt-upgrade

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit f5aaf27 into openshift:master Apr 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 1, 2022

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

Bugzilla bug 2047913 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2047913: Fix HAProxy tests on FIPS properly

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants