Golang Proxy Scrypt module to be compatible with the Perl Crypt::ScryptKDF lib #1560
Conversation
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
|
||
| const KEY_DELIM = ":" | ||
|
|
||
| var DefaultParams = SCRYPTComponents{ |
There was a problem hiding this comment.
Minor suggestion: a comment here linking https://godoc.org/golang.org/x/crypto/scrypt or https://www.tarsnap.com/scrypt/scrypt.pdf or both would be good, since these numbers are magical, and anyone trying to understand them will be confused without those explanations.
| func DerivePassword(password string) (string, error) { | ||
| var salt []byte | ||
| var err error | ||
| salt, err = generateSalt(64) |
There was a problem hiding this comment.
A GUID, 16 bytes, should be sufficient. Though I'm not strongly opposed to more, if you feel strongly about it. Also, what about putting GUIDLen in a constant? Maybe even in lib/go-tc? Seems generally useful.
There was a problem hiding this comment.
I got the parameters to that from https://github.com/apache/incubator-trafficcontrol/blob/master/traffic_ops/app/lib/Utils/Helper.pm#L148. The reason I chose 64 because I thought keys need the same scrypt parameters to make the passwords format and length compatible
I could add that to the DefaultsParams and use from their to make the parameter more prominent.
| } | ||
|
|
||
| keylenBytes := len(scryptPassword) - DefaultParams.DKLen | ||
| if keylenBytes < 1 { |
There was a problem hiding this comment.
Nitpick: could be if keylenBytes := len(scryptPassword) - DefaultParams.DKLen; keylenBytes < 1 {
There was a problem hiding this comment.
-1 on that one.. I find it less readable when the assignment is this long
| return nil | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
This needs to construct an error. It's returning the err we just verified is nil, and thus verifying invalid passwords.
Verified with a negative test:
derivedPassword, err := DerivePassword(pass)
err = VerifyPassword("notpassword", derivedPassword)
if err == nil {
t.Errorf(" expected error, actual success")
}
$ go test
--- FAIL: TestDerivePassword (0.20s)
authenticate_test.go:39: expected error, actual success
|
|
||
| var err error | ||
| var scomp SCRYPTComponents | ||
| if scryptPassword == "" { |
There was a problem hiding this comment.
Should this be at the top of the func, to avoid calling Split if unnecessary?
There was a problem hiding this comment.
I'm not quite sure what you mean here
There was a problem hiding this comment.
If scryptPassword is empty, we return, but we don't need sh for that. The Split function is moderately expensive. So, it's slightly more efficient to check and return before calling Split.
+func parseScrypt(scryptPassword string) (SCRYPTComponents, error) {
+ if scryptPassword == "" {
+ return SCRYPTComponents{}, errors.New("scrypt password is required")
+ }
+ sh := strings.Split(scryptPassword, ":")
⋮
But Split isn't that expensive, so it's not a big deal to leave it; just a minor improvement.
| pass := "password" | ||
| derivedPassword, err := DerivePassword(pass) | ||
| err = VerifyPassword(pass, derivedPassword) | ||
| if err != nil { |
There was a problem hiding this comment.
A negative test would be really good, too. Making sure a bad password doesn't successfully verify. I'd vote for some edge cases too, e.g. "", "0", ".", "\0", "\n" "somethingextremelylong".
There was a problem hiding this comment.
I agree with that, I'll add that
| return scomp, errors.New("salt cannot be decoded") | ||
| } | ||
| scomp.SaltLen = len(scomp.Salt) | ||
| if len(scomp.Salt) < 0 { |
There was a problem hiding this comment.
len will never return less than zero. Should this be == 0? Or should it be removed?
|
|
||
| scomp.R, err = strconv.Atoi(r) | ||
| if err != nil { | ||
| return scomp, errors.New(fmt.Sprintf("i=%d, type: %T\n", scomp.R, scomp.R)) |
There was a problem hiding this comment.
Minor suggestion: could be fmt.Errorf("i=%d, type: %T", scomp.R, scomp.R). Suggest removing newline, they're usually put in the print rather than the error
There was a problem hiding this comment.
good to know, wasn't aware of fmt.Errorf
| var err error | ||
| salt, err = generateSalt(64) | ||
| if err != nil { | ||
| return "", err |
There was a problem hiding this comment.
Minor suggestion: context on these errors would help debugging, e.g. return "", errors.New("generating salt: " + err.Error())
|
|
||
| // Algorithm | ||
| scomp.Algorithm = sh[0] | ||
| if scomp.Algorithm == "" { |
There was a problem hiding this comment.
Should we also verify Algorithm == "SCRYPT"?
|
I agree with @dangogh though, this is great, the login endpoint is one of the "hard" things, great to check off the list for traffic_ops_golang. |
| @@ -0,0 +1,10 @@ | |||
| # Treat all files in this repo as binary, with no git magic updating | |||
There was a problem hiding this comment.
I'd vote removing this file, too. I'm not enough of a Git Wizard to know if it's going to mess with our repo, but .git* files in subdirectories make me anxious it's going to mess with something. I don't think it'll hurt to delete it.
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
|
|
||
| // VerifyPassword parses the original Derived Key (DK) from the SCRYPT password | ||
| // so that it can compare that with the password/scriptPassword param | ||
| func VerifyPassword(password string, scryptPassword string) (bool, error) { |
There was a problem hiding this comment.
I had to read the whole function to figure out what cases return true/false, and what return error/nil, and when (false,nil) and (true,error) were returned. As-is, adding the specifics about what's returned in what case to the function-level comment would be really helpful.
But, I like the original better, just returning an error, where nil means success. Since (true,error) never makes sense, we can return nil on success. Just change the last line to return errors.New("invalid password").
But, it works, so I'll pass the review, if you disagree.
There was a problem hiding this comment.
@rob05c I went back and forth on the (bool, error) return. I decided to go back to the original and added the return error that you mentioned.
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
…k and added more test cases
|
FAILURE |
|
Refer to this link for build results (access rights to CI server needed): |
|
SUCCESS |
|
Refer to this link for build results (access rights to CI server needed): |
Wrote this module to be used for creating user passwords in the tm_user table that are backward compatible with the Perl http://search.cpan.org/~mik/Crypt-ScryptKDF-0.010/lib/Crypt/ScryptKDF.pm library being used by Traffic Ops.
The goal is to have this function be useful for Golang TO API testing, TO /login and /users functionality as well.