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

Moving Away from SHA1 to SHA512 (Security) #527

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

shantanubansal
Copy link

No description provided.

@shantanubansal
Copy link
Author

@stakater-user @faizanahmad055 Can you please review this?

@github-actions
Copy link

@shantanubansal Yikes! You better fix it before anyone else finds out! Build has Failed!

@shantanubansal shantanubansal changed the title Moving Away from SHA1 to SHA512 (Security) Moving Away from SHA1 to SHA512 (Security) and Other Dependency Updates Sep 14, 2023
@github-actions
Copy link

@shantanubansal Yikes! You better fix it before anyone else finds out! Build has Failed!

@shantanubansal shantanubansal changed the title Moving Away from SHA1 to SHA512 (Security) and Other Dependency Updates Moving Away from SHA1 to SHA512 (Security) Sep 14, 2023
@github-actions
Copy link

@shantanubansal Image is available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-91e5ad5a

@github-actions
Copy link

@shantanubansal Image is available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-f40824d9

@shantanubansal
Copy link
Author

@karl-johan-grahn Can you please approve this PR?

@shantanubansal
Copy link
Author

@waseem-h @ahmedwaleedmalik @stakater-user @faizanahmad055 @karl-johan-grahn Can anyone of you please approve or review the PR?

@ravi-k8
Copy link

ravi-k8 commented Sep 15, 2023

/lgtm

@ahmedwaleedmalik
Copy link
Contributor

@shantanubansal please refrain from tagging individuals directly. I'm no longer working on this project.

@faizanahmad055
Copy link
Contributor

faizanahmad055 commented Sep 20, 2023

@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 ?

@shantanubansal
Copy link
Author

shantanubansal commented Sep 20, 2023

@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?
If comparison is an issue can we use subtle.ConstantTimeCompare for string match? It will highly optimize the string comparison.

@github-actions
Copy link

@shantanubansal Image is available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-2ec1fafa

@github-actions
Copy link

@shantanubansal Image is available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-79eb1871

@faizanahmad055
Copy link
Contributor

@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?

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.

Copy link

@karl-johan-grahn Images are available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-e8b409b2\ndocker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-UBI-e8b409b2

Copy link

github-actions bot commented Dec 6, 2023

@karl-johan-grahn Images are available for testing. docker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-70b58a43\ndocker pull ghcr.io/stakater/reloader:SNAPSHOT-PR-527-UBI-70b58a43

@MuneebAijaz
Copy link
Contributor

@shantanubansal can we cater the feedback @faizanahmad055 has suggested above so this can be merged?

@IdanAdar
Copy link
Contributor

Any chance this PR can be looked at by a dev team member?

@halkeye
Copy link
Contributor

halkeye commented Sep 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants