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

datapath: Use SHA256 instead of SHA1 for datapath hash #14279

Merged
merged 2 commits into from
Dec 8, 2020

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Dec 4, 2020

SHA1 is probably fine for hashing the datapath contents, but it will likely raise a flag in any security reviews. In anticipation of this happening in the future, switch to SHA256 instead.

This PR removes the last direct use of SHA1 by Cilium so we can remove the resumable SHA1 package too.

cc @sharlns as raised by @rolinh in the SIG Security meeting today.

Signed-off-by: Tom Payne <tom@isovalent.com>
Signed-off-by: Tom Payne <tom@isovalent.com>
@twpayne twpayne added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels Dec 4, 2020
@twpayne
Copy link
Contributor Author

twpayne commented Dec 4, 2020

test-me-please

@twpayne twpayne requested a review from a team December 4, 2020 17:08
@twpayne twpayne requested review from a team as code owners December 4, 2020 17:08
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Dec 4, 2020
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

New implementation looks functionally identical, we lost a couple of code comments and some unit testing but I don't think it's a loss. Somehow the old unit test code didn't even seem to test the implementation in the package anyway.

Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Thanks! I was a little worried about possible side-effects of changing the hash type (e.g. in the context of an upgrade) but apparently all objects are reloaded anyway in such a scenario.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 7, 2020
@nebril nebril merged commit 312e227 into master Dec 8, 2020
@nebril nebril deleted the pr/twpayne/no-sha1 branch December 8, 2020 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants