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
Added gopass audit
command
#228
Conversation
Could you please elaborate on your use case for this change? |
The usecase is rather simple: Finding weak passwords that you've used in the past (and should probably change). |
Ok! The only use case I can think of, for which merging this would make sense: recursively iterate over all passwords and then check if they are weak. Sorry if this seems pessimistic to you, just thinking out loud. |
If you look closely, this is iterating over all your passwords. Also, cracklib, while pretty simplistic in its function, still is rather horrendous C code. The Golang reimplementation is a lot shorter, concise code and even easier to test. Adding "all of this" comes down to one vendored go file. Personally I wouldn't touch piping with a ten foot pole. Before you know it you're trying to parse the different output formats from three slightly different, yet commonly deployed versions of a single tool. Not to mention translations and everything else in-between. I agree, you could solve all of this with a little bash script... but in an ideal world I'd rather have gopass directly check new & existing passwords for common flaws, instead of relying on a third party script that users have to actively seek out and run. |
Nothing specifically to this implementation but in general I think it's a great idea. Other password managers already have this feature and some more checks. A (Watchtower in this case contains passwords from website with issues like Heartbleet etc) As written in the project description:
I'd think integrating these kind of features would make more sense than making people write shell scripts :) |
@dewey Detecting re-used passwords and tracking the last-changed timestamp is also something on my mind. Some nice ideas here, which software is that a screenshot of? |
Screenshot is from 1Password. Forgot to mention that! |
Thanks for elaborating on this more. So @muesli do you want to continue to work on your lib and keep us posted about the current state of it? |
@metalmatze Sure, I'll finish the wordlist & dictionary detection tonight and post an update here. The other features are out-of-scope for crunchy, though, and need to be implemented in gopass directly (like the dupe & age checks). I'm happy to tackle those as well, but I'd consider them separate PRs. |
Yes. A separate PR would be nice, although I could work on that too once the checking of the passwords themselves are functional. |
Dictionary lookups are implemented now, too. I've tested this on macOS and Linux and it uses the default system dictionaries installed to /usr/share/dict. If no dictionaries were found, it only relies on the regular sanity checks. We could ship a common wordlist for Windows installs, but this still needs to be implemented. It's still an improvement over no checks whatsoever, even on Windows. On Ubuntu you probably want to install the wordlists coming with |
By now I also have some dupe checks ready to push, but I don't want to pile up more commits in this PR. I'll create a new one once this PR got merged into master. See: muesli@20a7e68 |
Very nice addition, I'm still a little concerned about the UX (there is already a |
@dominikschulz If you have an idea for a better name, I'm all ears. Other command names I could think of: validate, verify, hedge, ... |
May sound a bit opinionated, but how about |
What about |
What about |
Perfect, I like that even better. No idea how I couldn't come up with that name myself. Will make the required changes, so hopefully we can merge this soon. Got more changes / branches lined up now, all derived from this one ;-) |
I think I'll go ahead and squash that into one commit for neatness sake. Will also update to latest crunchy. |
`gopass audit` validates known passwords against common flaws, like being too short or systematic. This uses my own Golang implementation of cracklib: https://github.com/muesli/crunchy
Audit is very nice, but I dislike the idea of having a separate top level command for every operation, as it's already overloaded by the root entries of the store. We can merge this as is if you like, but we may need to reorganize our top level commands soon. |
Sure, would be absolutely happy with that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
|
||
validator := crunchy.NewValidator() | ||
var out io.Writer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you define out
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit it doesn't really serve any purpose here, but I copied the basic skeleton from action/list.go. At this point I wasn't quite sure if you intend to handle output in other ways in the future, so I decided to stick to the basic pattern I found there. By now I think I realized you're just doing that for multi-page outputs action/list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing that right now.
`gopass audit` validates known passwords against common flaws, like being too short or systematic. This uses my own Golang implementation of cracklib: https://github.com/muesli/crunchy
gopass audit
validates known passwords against common flaws,like being too short or systematic.
This uses my own Golang implementation of cracklib: https://github.com/muesli/crunchy
Wordlist / dictionary checking is still on my TODO-list, but the API should remain
stable for that. A simple vendor-bump would be enough in that case.
EDIT: renamed to
gopass audit
after lengthy discussion below, to avoid confusion.