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

feat: sha1.Sum() #570

Closed
wants to merge 5 commits into from
Closed

feat: sha1.Sum() #570

wants to merge 5 commits into from

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Mar 7, 2023

Description

calculate sha1 hash of given data

How has this been tested?

tests by gnodev

@r3v4s r3v4s marked this pull request as ready for review March 7, 2023 09:59
@r3v4s r3v4s requested a review from a team as a code owner March 7, 2023 09:59
@thehowl
Copy link
Member

thehowl commented Mar 8, 2023

Mmh. I have a couple of questions:

  • Do you have a specific use case where you want to use sha1 hashing for backwards-compatibility reasons? If not, shouldn't we probably avoid including it in the standard library for known reasons - and also because copying the code from the Go standard library crypto/sha1 is actually pretty straightforward?
    (The suggestion here is that if we should include hashing functions, the first one to be included should be sha256).
  • Should we not include this using the go standard library crypto/sha1 go files instead of just having it as a native binding?

@@ -0,0 +1,18 @@
package msha1
Copy link
Member

Choose a reason for hiding this comment

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

You can move this test to the stdlibs folder, or you can keep it here and create an official example folder to test the stdlibs. If you choose to keep it here, I suggest moving it outside of the r/ folder. What about gno.land/p/demo/stdlibs_tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving test to stdlibs folder looks more straightforward to me. I'll move it

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Given the security concerns, it would be best to choose a more secure cryptographic primitive than SHA1. Do you have a specific use case in mind for which you need to use SHA1?

@r3v4s
Copy link
Contributor Author

r3v4s commented Mar 9, 2023

@thehowl @moul
Hello guys, I really don't have specific use case using sha1 instead of sha3.
I saw HashBytes() calls sha2.Sum256() => so implement sha1.sum(), sha3.sum256(), sha3.sum512() using injections and made pr sha1 first to review.

I'll move the test case to stdlibs, however I knew about the security concerns(such as collision) so I think it's ok to block this pr

@r3v4s
Copy link
Contributor Author

r3v4s commented Mar 9, 2023

  • Should we not include this using the go standard library crypto/sha1 go files instead of just having it as a native binding?

Hey @thehowl this is thing I wanted to discuss with someone else. Before we continue I just want make sure that terms of stdlib injection and native binding is same, right?

If we just take sha1 as example I do agree that it is more straightforward to copy code from Go standlibrary(crypto/sha1)

But I'm sure there will be more function that we need in Gno that works same as in Go, and if certain package has complex structure copying code seems lots of work to do.

There will be pros and cons to use injection and I think it is good topics to discuss.

@moul
Copy link
Member

moul commented Mar 9, 2023

On the selection of algorithms, it deserves a more open discussion, and it would be nice to have experts helping us.

@r3v4s
Copy link
Contributor Author

r3v4s commented Mar 9, 2023

Totally Agree.

FYI, currently NIST's standard is SHA3 which is family of FIPS 180-4 and FIPS 202 (ref).

If you(or someone with expertise) think SHA3 is good for now, pls take a look this branch.

if arg0.V != nil {
slice := arg0.V.(*gno.SliceValue)
array := slice.GetBase(m.Store)
bz = array.GetReadonlyBytes()
Copy link
Member

Choose a reason for hiding this comment

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

Please consider using

bz = array.GetReadonlyBytes()[:slice.Length]

The function GetReadonlyBytes() could return leading 0 in the bytes array.

@moul
Copy link
Member

moul commented Mar 10, 2023

We will not be adding the deprecated SHA1 and instead opt to add other versions, as suggested in #580.
Thanks for your contribution. Closing this PR.

@moul moul closed this Mar 10, 2023
@r3v4s r3v4s deleted the feature/sha1 branch July 20, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants