Fix crypto configuration for golang 1.14 by marun · Pull Request #875 · openshift/library

Copy link

Contributor

commented Sep 2, 2020 •

edited

The 1.19.0 bump tried to ensure library-go compatibility with the addition of a _SHA256 suffix to the following cipher suites:

• TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
• TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256

The naive approach taken by the bump's test fixes was to include both the new and old names in the cipher map, which satisfied the check in TestConstantMaps that all golang cipher suites were mapped to a name.

However, CipherSuitesToNamesOrDie was not tested, and when called by kube-apiserver would panic due to the suite ids of the ciphers in question mapping to both the old and new cipher names.

This commit ensures CipherSuitesToNamesOrDie is tested to not panic when called with the default set of ciphers, and ensures that attempting to convert the suite ids in question always returns the new name.

The 1.19.0 bump tried to ensure library-go compatibility with the
addition of a _SHA256 suffix to the following cipher suites:

 - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
 - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256

The naive approach taken by the bump's test fixes was to include both
the new and old names in the cipher map, which satisfied the check in
TestConstantMaps that all golang cipher suites were mapped to a name.

However, CipherSuitesToNamesOrDie was not tested, and when called by
kube-apiserver would panic due to the suite ids of the ciphers in
question mapping to both the old and new cipher names.

This commit ensures CipherSuitesToNamesOrDie is tested to not panic
when called with the default set of ciphers, and ensures that
attempting to conver the suite ids in question always returns the new
name.

Copy link

Contributor Author

commented Sep 2, 2020

/cc @soltysh @sttts @deads2k

openshift-ci-robot requested review from deads2k, soltysh, sttts and smarterclayton September 2, 2020 05:35

marun mentioned this pull request Sep 2, 2020

Merged

approved these changes

Copy link

Member

left a comment

Choose a reason for hiding this comment

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

/lgtm

openshift-ci-robot assigned soltysh Sep 2, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020

reviewed

"ECDHE-ECDSA-AES256-GCM-SHA384": "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", // 0xC0,0x2C

"ECDHE-RSA-AES256-GCM-SHA384": "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", // 0xC0,0x30

"ECDHE-ECDSA-CHACHA20-POLY1305": "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256", // 0xCC,0xA9

"ECDHE-RSA-CHACHA20-POLY1305": "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256", // 0xCC,0xA8

Copy link

Contributor

Choose a reason for hiding this comment

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

are these the same?

Copy link

Contributor

Choose a reason for hiding this comment

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

ic, they are

Copy link

Contributor

commented Sep 2, 2020

/approve

/approve/approve cancel

a4e32e3