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.

Sign up for GitHub

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

Jump to bottom

[feat] - Add Generic Hasher Interface with Blake2b Implementation #3337

Merged
merged 8 commits into from
Sep 27, 2024
Merged

[feat] - Add Generic Hasher Interface with Blake2b Implementation #3337

merged 8 commits into from
Sep 27, 2024

Conversation

ahrav
Copy link
Collaborator

@ahrav ahrav commented Sep 26, 2024 •
edited
Loading

Description:

This PR includes the following:

  • Introduced a Hasher interface to abstract hashing functionality.
  • Implemented FNVHasher using the FNV-64a algorithm.
  • Implemented SHA256Hasher using the SHA-256 algorithm.

Bechmarks

Screenshot 2024-09-26 at 8 16 35 AM

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

@ahrav ahrav requested a review from a team September 26, 2024 02:29
@ahrav ahrav marked this pull request as ready for review September 26, 2024 02:29
@ahrav ahrav requested a review from a team as a code owner September 26, 2024 02:29
@ahrav ahrav marked this pull request as draft September 26, 2024 03:06
@dustin-decker
Copy link
Contributor

dustin-decker commented Sep 26, 2024 •
edited
Loading

May I suggest using blake2b? I've added it to this PR. It's a fast cryptographic hash function that is only ~4x slower than FNV when using the default 256 bit settings.

@ahrav
Copy link
Collaborator Author

ahrav commented Sep 26, 2024

May I suggest using blake2b? I've added it to this PR. It's a fast cryptographic hash function that is only ~4x slower than FNV when using the default 256 bit settings.

Oh whoops, I might've forced pushed over your change 🤦

@ahrav ahrav changed the title [feat] - Add Generic Hasher Interface with FNV-64a and SHA-256 Implementations [feat] - Add Generic Hasher Interface with FNV-64a, SHA-256, and Blake2b Implementations Sep 26, 2024
@ahrav ahrav marked this pull request as ready for review September 26, 2024 17:56
mcastorina
mcastorina reviewed Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@mcastorina mcastorina left a comment

Choose a reason for hiding this comment

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

overall this lgtm; love the extensive testing and benchmarks! just had one question about thread safety.

pkg/hasher/hasher.go Show resolved Hide resolved
pkg/hasher/hasher.go Outdated Show resolved Hide resolved
pkg/hasher/mutex.go Outdated Show resolved Hide resolved
@ahrav ahrav requested review from mcastorina and a team September 26, 2024 18:59
mcastorina
mcastorina approved these changes Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@mcastorina mcastorina left a comment

Choose a reason for hiding this comment

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

Just an out of date comment and possibly useless MutexHasher struct, but lgtm!

dustin-decker
dustin-decker approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@dustin-decker dustin-decker left a comment

Choose a reason for hiding this comment

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

LGTM. Should we have anything other than Blake2b for what we'll be using this for though?

@ahrav ahrav changed the title [feat] - Add Generic Hasher Interface with FNV-64a, SHA-256, and Blake2b Implementations [feat] - Add Generic Hasher Interface with SHA-256, and Blake2b Implementations Sep 27, 2024
@ahrav
Copy link
Collaborator Author

ahrav commented Sep 27, 2024

LGTM. Should we have anything other than Blake2b for what we'll be using this for though?

I was thinking we'd use the SHA256 hasher for enterprise, but I supposed we could implement the interface outside of OSS.

@dustin-decker
Copy link
Contributor

LGTM. Should we have anything other than Blake2b for what we'll be using this for though?

I was thinking we'd use the SHA256 hasher for enterprise, but I supposed we could implement the interface outside of OSS.

No, it's a different alg that we'd implement in enterprise.

@dustin-decker dustin-decker changed the title [feat] - Add Generic Hasher Interface with SHA-256, and Blake2b Implementations [feat] - Add Generic Hasher Interface with Blake2b Implementation Sep 27, 2024
@ahrav ahrav merged commit ee51fc5 into main Sep 27, 2024
12 checks passed
@ahrav ahrav deleted the feat-hasher branch September 27, 2024 03:11
kashifkhan0771 pushed a commit to kashifkhan0771/trufflehog that referenced this pull request Oct 1, 2024
@ahrav @kashifkhan0771
[feat] - Add Generic Hasher Interface with Blake2b Implementation (tr…
f885825 
…ufflesecurity#3337)

* Add hasher interface and fnv + sha256 implemenations

* update

* remove

* fix test

* update

* remove

* remove

* fix spelling
@blsaccess blsaccess mentioned this pull request Oct 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dustin-decker dustin-decker dustin-decker approved these changes

@mcastorina mcastorina mcastorina approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahrav @dustin-decker @mcastorina