-
Notifications
You must be signed in to change notification settings - Fork 519
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
Moving Away from SHA1 to SHA512 (Security) #527
base: master
Are you sure you want to change the base?
Conversation
@stakater-user @faizanahmad055 Can you please review this? |
@shantanubansal Yikes! You better fix it before anyone else finds out! Build has Failed! |
@shantanubansal Yikes! You better fix it before anyone else finds out! Build has Failed! |
This reverts commit c9a25c9.
@shantanubansal Image is available for testing. |
@shantanubansal Image is available for testing. |
@karl-johan-grahn Can you please approve this PR? |
@waseem-h @ahmedwaleedmalik @stakater-user @faizanahmad055 @karl-johan-grahn Can anyone of you please approve or review the PR? |
/lgtm |
@shantanubansal please refrain from tagging individuals directly. I'm no longer working on this project. |
@shantanubansal Thank you for the PR, Can you please pull the upstream changes from the master and I can then review it? Also, at this point, I am unsure of the performance impact. We used SHA1 because it was efficient as the only purpose was to identify the objects. I am leaning towards making it configurable from a list of hashing algorithms. What do you think @MuneebAijaz ? |
@faizanahmad055 I feel we can make it configurable, if we can have a go-boring compatibility that will be awesome. But Can you please elaborate whats the performance impact? |
@shantanubansal Image is available for testing. |
@shantanubansal Image is available for testing. |
It can hit computationally harder to calculate and compare the hash specially when there is going to be too much load. I think making it an optional and choosing from the list of algorithms with default of SHA1 would be the way to go. |
@karl-johan-grahn Images are available for testing. |
@karl-johan-grahn Images are available for testing. |
@shantanubansal can we cater the feedback @faizanahmad055 has suggested above so this can be merged? |
Any chance this PR can be looked at by a dev team member? |
I would recommend fixing the issue you have in the build, and to actually give details in the description as to what problem it solves, and such. It helps reviewers prioritize when they have time. Like why is switching from sha1 to sha512 a security thing? are you worried about collisions? what are the drawbacks to this solution? what are the benifits over the current implementation. |
No description provided.