Add ability to set password length#29
Conversation
ignatk
left a comment
ignatk
left a comment
There was a problem hiding this comment.
We actually have a TODO in the code to add length (to both passwords and raw outputs): https://github.com/cloudflare/gokey/blob/master/cmd/gokey/main.go#L82-L83
Can you make it just length and apply to both passwords and raw symmetric keys?
cmd/gokey/main.go
Outdated
| flag.StringVar(&realm, "r", "", "password/key realm (most probably purpose of the password/key)") | ||
| flag.StringVar(&output, "o", "", "output path to store generated key/password (default stdout)") | ||
| flag.BoolVar(&unsafe, "u", false, "UNSAFE: allow key generation without a seed") | ||
| flag.IntVar(&passwordLength, "l", 10, `password length (only used for "pass" type)`) |
There was a problem hiding this comment.
this probably needs some input validation - what if the user puts 0 or -1 or 1234567890?
There was a problem hiding this comment.
Just pushed some changes. Length now works on both passwords and raw symmetric keys.
I wanted to keep the defaults for each output type for backward compatibility. So, genRaw checks if length was passed, if it wasn't then it sets length to 32. Passwords still use the default of 10 characters.
It also does some input validation, but it may need to be tweaked.
cmd/gokey/main.go
Outdated
|
|
||
| func genPass(seed []byte, w io.Writer) { | ||
| password, err := gokey.GetPass(pass, realm, seed, &gokey.PasswordSpec{10, 3, 3, 1, 1, ""}) | ||
| upperLower := math.Ceil(float64(passwordLength) * 3.0 / 10.0) |
There was a problem hiding this comment.
I suggest not to hardcode this logic - what if there would be a weird website which will require "all uppercase"? It is not supported now, but I think all of them should be cmd line parameters.
There was a problem hiding this comment.
Excellent point - I'll work on that
There was a problem hiding this comment.
I wasn't sure how to do this with only the standard library since these cmd line parameters wouldn't always be used.
Check for invalid lengths [cloudflare#29]
ignatk
left a comment
ignatk
left a comment
There was a problem hiding this comment.
As stated inline, I think we should do most parameter validation inline inside the main function before calling the helpers-generators. The reason is - we want to have slightly different validation logic (like not enforcing 1024 limit on the raw type) as well as additional logic: we should probably throw an error if the "l" flag is explicitly defined for key types, which do not support it, like ec256, rsa2048 etc
|
I believe I resolved all the issues you mentioned. :) |
ignatk
left a comment
ignatk
left a comment
There was a problem hiding this comment.
Looks good. Beside the one last inline comment can you squash the work now and update the documentation in a separate commit?
Thanks
levidurfee
force-pushed
the
password-length
branch
from
c9c1ff3 to
a581bb1
Compare
|
I've squashed the commits and added one commit for docs. |
|
Sorry not to mention it: there are two doc files - the one you modified and https://github.com/cloudflare/gokey/blob/master/gokey.1.md (for Debian packaging). Can you modify it as well and squash with your doc commit? |
levidurfee
force-pushed
the
password-length
branch
from
07bd14c to
fe6ffa0
Compare
|
@ignatk I appreciate your help with this pull request. Please let me know if there is anything else I need to do before it is accepted. |
|
This is great. Thank you for your time and contribution. |
2 participants
Having the ability to determine the length of the password generated is something I would find helpful.
Thanks :)