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

Update SHA1 to SHA256 #819

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

myersjac
Copy link

Updating the Data Cache to utilize a more up-to-date with security standards hashing algorithm.

Tests have also been updated to utilize and confirm the functionality. All Tests passing

As an employee of a large corporation, I would love to utilize this library to load images as it is very performant. Unfortunately, based on our corporate security standards, we are unable to use this framework as it stands because it contains usage of an insecure hash function. This PR's goal is to update that hash function from SHA1 to SHA256, allowing it to adhere to current security standards and will make it so that it does not get flagged as a vulnerability within static scans of the framework

Updating the Data Cache to utilize a more up-to-date with security standards hashing algorithm.

Tests have also been updated to utilize and confirm the functionality. All Tests passing
@myersjac
Copy link
Author

Screenshot 2024-10-17 at 3 48 11 PM

///
/// ```swift
/// print("http://test.com".sha1)
/// // prints "50334ee0b51600df6397ce93ceed4728c37fee4e"
/// print("http://test.com".256)

Choose a reason for hiding this comment

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

should it be /// print("http://test.com".sha256)?

Copy link
Author

Choose a reason for hiding this comment

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

oof good catch. let me revise

Copy link
Author

Choose a reason for hiding this comment

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

@duguyihou fixed, thank you

@fabianfabian
Copy link

Is there a performance difference when switching to sha256? How big?

Is the hash even used for securing anything here? Those types of corporate policies are usually for password hashing.

@kean
Copy link
Owner

kean commented Oct 30, 2024

Hey, I just wanted to say that I saw the PR, and I agree that it's fair concern as .sha1 is now considered "Insecure". But I do share the same concerns as @fabianfabian. I begrudgingly switched from md5 to (slower) sha1 in one of the previous releases, and sha256 will likely be slower still.

Is the hash even used for securing anything here?

No, but this type of tooling typically doesn't care.

I've been thinking about switching to an SQLite-based cache that wouldn't require hashing.

@myersjac
Copy link
Author

Yeah, from what I understood from the code itself there aren't any actual security concerns with this.

It's only an issue for me as because it's included within the framework, and that gets picked up in the static scan that we run for security vulnerabilities.

I believe our use case doesn't utilize this part of the code even, but our company's security policy is really strict that the inclusion of using SHA1 for something is not acceptable.

We would love to use this framework as it's been extremely performant, but as it stands we can't because of the use of SHA1.

Regardless, do whatever you think is best for it. 👍

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.

4 participants