Skip to content

Implement SHA-256 and SHA-512 hashed passwords#75

Open
pitpalme wants to merge 7 commits intoabbot:masterfrom
pitpalme:feature/sha-crypt
Open

Implement SHA-256 and SHA-512 hashed passwords#75
pitpalme wants to merge 7 commits intoabbot:masterfrom
pitpalme:feature/sha-crypt

Conversation

@pitpalme
Copy link

@pitpalme pitpalme commented May 9, 2021

Fix #72

Peter Palmreuther added 2 commits May 9, 2021 23:37
Implement SHA-256 and SHA-512 hashed passwords
c49a14f 
Fix abbot#72
Fix integer literal
9902bda 
travis checks using go version 1.10.x and 1.11.x revealed integer
literal obviously only usable at newer go versions.
@abbot
Copy link
Owner

abbot commented May 9, 2021

This reimplements sha256/sha512 algorithm, which is available in the standard library (crypto/sha256 and crypto/sha512 respectively). I'm not sure I understand why this path was chosen instead of using the standard implementations?

@pitpalme
Copy link
Author

pitpalme commented May 10, 2021

Actually it's usage of standard library for sha* hashing, as far as I am aware.
Couldn't find sha-crypt with it's 21 steps in my sdk.
To be honest I couldn't even find sha-crypt compatible base64 encoding in standard lib.
I'd be happy to get a hint, where to look for it.

@pitpalme
Copy link
Author

pitpalme commented May 10, 2021

Nevermind, please hold back this PR. I'll rearrange code.
Just noticed "MD5Crypt" in md5crypt.go and will try to move implementation into a separate file and in a similar simplicity.

Move SHA-crypt implementation
51a74bb 
SHA-crypt implementation moved to a separate file.
Copying already existing MD5Crypt implementation base64 encoding could be
realized much shorter and more elegant.
@pitpalme
Copy link
Author

pitpalme commented May 10, 2021

Here I am - again.
I've moved implementation, so "basic.go" is kept slim.

Open to suggestions for improvement - and open for pointers to SHA crypt implementation in standard library. :)

Peter Palmreuther added 4 commits May 11, 2021 00:48
Move SHA-crypt test to individual test file
0a24f63
Add some sanity check and cleanup code
1d895e3 
Extract digest length calculation and add sanity check.

Wrap hash algorithm information in helper functions to centralize
access and simplify code.
Reduce memory footprint
8dec016 
SHA digest byte mapping as []uint consumed 8 times the memory it really
needs. It's now []uint8, sufficient to store indexes in digest []byte.
Reduce number of memory allocations
5c84622 
By carefully pre-allocating and reusing already allocated but no more
needed structures and slices one can significantly reduce number of
allocation calls.
Within SHA-crypt "X rounds hashing" impact can be significant.
@pitpalme
Copy link
Author

pitpalme commented May 11, 2021

OK, I guess I'm done.
Sorry for not submitting one commit at one time. But I wanted to not miss the chance for general feedback, before tweaking the code.

Would be happy to hear about chances, this PR being merged - or reasons it can't.

@andig
Copy link

andig commented Oct 4, 2021

@abbot newer Shelly devices require SHA256 according to RFC7617: https://shelly-api-docs.shelly.cloud/gen2/Overview/CommonDeviceTraits/#authentication

Is there any chance to look at the open PRs and see what can be merged?

@abbot
Copy link
Owner

abbot commented Oct 6, 2021

Thanks for additional context. I have some reservations about adding crypto code directly into this package, so instead I started work on moving crypto-related code to x/crypto (in fact there was an open proposal golang/go#14274 about that).

@abbot abbot mentioned this pull request Oct 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

More crypters [Patch included]

3 participants

@pitpalme @abbot @andig