Skip to content
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

Passphrase fixups #906

Merged
merged 3 commits into from
Aug 11, 2016
Merged

Passphrase fixups #906

merged 3 commits into from
Aug 11, 2016

Conversation

riyazdf
Copy link
Contributor

@riyazdf riyazdf commented Aug 10, 2016

Fixes some logic in the keystore when counting number of passphrase attempts, especially when short passphrases are provided.

🐳 $ bin/notary init testGun
Root key found, using: 4327a8deba2cae19a4b393d2c6cf3a2f349652c84bc41bc1e09ade182d86b19a
Enter passphrase for root key with ID 4327a8d:
Enter passphrase for new targets key with ID 454c204 (testGun):
Passphrase is too short. Please use a password manager to generate and store a good random passphrase.
Enter passphrase for new targets key with ID 454c204 (testGun):
Passphrase is too short. Please use a password manager to generate and store a good random passphrase.
...
Enter passphrase for new targets key with ID 454c204 (testGun):
Passphrase is too short. Please use a password manager to generate and store a good random passphrase.

* fatal: failed to add key to filestore: maximum number of passphrase attempts exceeded

Also, ensures that PromptRetriever() has an actual terminal when requesting passphrases and will error out with ErrNoInput if that is not the case instead of trying to read input over and over.

Closes #899
Closes #812

Signed-off-by: Riyaz Faizullabhoy <[email protected]>
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
@riyazdf riyazdf force-pushed the passphrase-retrieval-fixups branch from 5c19422 to c7f0bab Compare August 10, 2016 21:02
@riyazdf riyazdf self-assigned this Aug 10, 2016
@riyazdf riyazdf modified the milestone: Notary 0.4 Aug 10, 2016
if err != nil {
continue
if err == nil {
break
}
if giveup || attempts > 10 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: wondering if we should decrease this number to 3 or some such? That would give the user 5 tries. (currently it fails out after 12 tries - I was wondering whether that's too many)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. 10 is feeling like too many. Also would be good to move the exact number to a const.

@cyli
Copy link
Contributor

cyli commented Aug 11, 2016

Thank you for fixing this! LGTM - took it for a spin and it correctly terminates, and in a script without stdin it immediately fails.

if err != nil {
return "", false, err
}
term.DisableEcho(0, state)
defer term.RestoreTerminal(0, state)
term.DisableEcho(os.Stdin.Fd(), state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a good reason for us to switch to crypto/terminal in the future. Better API for retrieving passwords from the terminal.

@endophage
Copy link
Contributor

LGTM. We can modify the number of retries separately

@endophage endophage merged commit 09d6eeb into master Aug 11, 2016
@cyli cyli deleted the passphrase-retrieval-fixups branch August 11, 2016 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants