Skip to content

Conversation

@benjaminapetersen
Copy link
Contributor

@benjaminapetersen benjaminapetersen commented Feb 21, 2020

Follow-on to #3389

Full list of related PRs

Will need a cherry-pick including both commits for backport.

TODO:

  • update test-ciphers.sh to validate that this does fix the problem.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 21, 2020
@benjaminapetersen
Copy link
Contributor Author

/assign @spadgett

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2020
@benjaminapetersen benjaminapetersen force-pushed the bug/1777129/birthday-attack-64-bit-ciphers-part-2 branch from aedd7ca to 86f8df1 Compare February 24, 2020 21:30
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 26, 2020
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are correctly denied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, these are also denied :)
Working on figuring out what is up with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett curious if you have a thought.

@benjaminapetersen benjaminapetersen changed the title Update additional server configs with tls config containing restricted set of cipher suites Birthday attack follow: Update additional server configs with tls config containing restricted set of cipher suites Feb 26, 2020
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't really want to create my own test image, but couldn't find the correct mix of tools otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

 docker build -f Dockerfile.ciphertest  -t quay.io/benjaminapetersen/console-ciphertest:latest .

and exists here https://quay.io/repository/benjaminapetersen/console-ciphertest

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to host this on your personal quay account. We could potentially use OpenShift builds to build the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

@benjaminapetersen benjaminapetersen force-pushed the bug/1777129/birthday-attack-64-bit-ciphers-part-2 branch from 996bba7 to c9e7246 Compare February 26, 2020 22:09
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2020
test-ciphers.sh Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Final output for this test looks like:

invalid cipher was correctly denied (DES-CBC3-SHA)
invalid cipher was correctly denied (RSA-AES-128-CBC-SHA256)
invalid cipher was correctly denied (ECDHE-RSA-3DES-EDE-CBC-SHA)
invalid cipher was correctly denied (RSA-3DES-EDE-CBC-SHA)
valid cipher was correctly accepted (ECDHE-RSA-AES128-GCM-SHA256)
valid cipher was correctly accepted (ECDHE-RSA-AES256-GCM-SHA384)
valid cipher was correctly accepted (ECDHE-ECDSA-CHACHA20-POLY1305)
valid cipher was correctly accepted (ECDHE-RSA-CHACHA20-POLY1305)
success
server ciphers correctly handled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't need the RootCAs: x509.NewCertPool(), here, removing.

@benjaminapetersen benjaminapetersen force-pushed the bug/1777129/birthday-attack-64-bit-ciphers-part-2 branch from c9e7246 to 63233ea Compare February 27, 2020 02:41
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@benjaminapetersen I'd suggest we work on the test as a follow on so we don't hold up the basic fix, which is simple. Changes to main.go LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to host this on your personal quay account. We could potentially use OpenShift builds to build the image.

@benjaminapetersen
Copy link
Contributor Author

@spadgett agree about hosting (I don't want it on my personal quay either), not sure how much work it is to setup OpenShift builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't use

set -o errexit

As some of the test requires an error. I'm not sure if there is a better approach.

@benjaminapetersen benjaminapetersen force-pushed the bug/1777129/birthday-attack-64-bit-ciphers-part-2 branch from ed2a33f to 6e42f08 Compare April 17, 2020 14:41
@benjaminapetersen
Copy link
Contributor Author

Updated, eliminated the cipher tests as we've decided it's not really worth the effort.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 17, 2020

@benjaminapetersen: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/verify 86f8df16ba1011afdf847def23670c95977a7430 link /test verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, spadgett

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:
  • OWNERS [benjaminapetersen,spadgett]

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

@spadgett
Copy link
Member

/hold
for #5100

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit 37ebaf8 into openshift:master Apr 18, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 20, 2020
@benjaminapetersen benjaminapetersen deleted the bug/1777129/birthday-attack-64-bit-ciphers-part-2 branch April 27, 2020 17:31
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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants