remove bcrypt code from code-base (#4844) #4845
Conversation
what is a |
Codecov Report
@@ Coverage Diff @@
## master #4845 +/- ##
==========================================
- Coverage 63.02% 63.02% -0.01%
==========================================
Files 191 191
Lines 27452 27440 -12
==========================================
- Hits 17302 17294 -8
+ Misses 8975 8972 -3
+ Partials 1175 1174 -1
Continue to review full report at Codecov.
|
|
Ahm, yes xD - I fix it! |
|
Yes, that's exactly the function used in bcrypt for comparing the computed hash values. It compares two byte slices in constant time. This prevents timing attacks. |
|
Closed until we decide what to do... |
|
This PR should fulfill the requirements - reopened |
cmd/credential.go
Outdated
|
|
||
| return (cred.AccessKey == ccred.AccessKey && | ||
| bcrypt.CompareHashAndPassword(cred.secretKeyHash, []byte(ccred.SecretKey)) == nil) | ||
| return subtle.ConstantTimeCompare([]byte(cred.SecretKey), []byte(ccred.SecretKey)) == 1 |
There was a problem hiding this comment.
How about doing below?
return (cred.AccessKey == ccred.AccessKey && subtle.ConstantTimeCompare([]byte(cred.SecretKey), []byte(ccred.SecretKey)) == 1)There was a problem hiding this comment.
The access key is public information so this is fine. However for me it looks a lite bit wired when credentials are compared and some parts are constant time and some aren't. It's just easier to miss something in the future - but the decision is up to you...
There was a problem hiding this comment.
I think you can do the change.
cmd/credential.go
Outdated
| _, err = rand.Read(keyBytes) | ||
| if err != nil { | ||
| return cred, err | ||
| // Generate access key - the access key consits only of alpha-numeric digits. |
There was a problem hiding this comment.
I think you can keep function unchanged to minimize this PR
There was a problem hiding this comment.
This PR also cleans up the code a bit - like unnecessary if-else. aso.
There was a problem hiding this comment.
Yep. Its better to send different PR doing cleanup.
|
Can you squash your commits? @aead |
|
Squashed |
Bcrypt is not neccessary and not used properly. This change replace the whole bcrypt hash computation through a constant time compare and removes bcrypt from the code base.
Bcrypt is not neccessary and not used properly. This change
replace the whole bcrypt hash computation through a constant time
compare and removes bcrypt from the code base.
Fixes #4844
How Has This Been Tested?
Manually
Types of changes
Checklist: