Moving Away from SHA1 to SHA512 (Security)

[shantanubansal]

No description provided.

[shantanubansal]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[shantanubansal]

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

[shantanubansal]

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

[ravi-k8]

/lgtm

[ahmedwaleedmalik]

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

[faizanahmad055]

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

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

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

[github-actions]

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

[faizanahmad055]

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

[github-actions]

@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

[github-actions]

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

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

[IdanAdar]

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

[halkeye]

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.

[shantanubansal]

@CodiumAI-Agent /improve

[CodiumAI-Agent]

PR Code Suggestions ✨

Category	Suggestion	Impact
Possible issue	Return early on write error After logging a write error, return immediately to avoid producing an invalid or partial hash. This ensures that on failure, the function does not continue and generate an incorrect value. internal/pkg/crypto/sha.go [14-17] _, err := io.WriteString(hasher, data) if err != nil { logrus.Errorf("Unable to write data in hash writer %v", err) + return "" } Suggestion importance[1-10]: 7 __ Why: After logging the write error, without a return the function continues and may produce an invalid hash; returning early prevents this.	Medium
General	Show expected versus actual Include the expected and actual hash values in the error message to make test failures easier to diagnose. This provides clear context when the assertion fails. internal/pkg/crypto/sha_test.go [12-14] if result != sha { - t.Errorf("Failed to generate SHA") + t.Errorf("GenerateSHA(%q) = %s; want %s", data, result, sha) } Suggestion importance[1-10]: 5 __ Why: Including the actual and expected hash in the error message greatly improves test diagnosability, though it's a non-critical enhancement.	Low