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.

Sign up for GitHub

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

Jump to bottom

Add: password entry verification to prevent password mistype #662

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add: password entry verification to prevent password mistype #662

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f10b71
Add: password entry verification to prevent password mistype
abastola0 Nov 19, 2023
067fda9
fix: non-zero exit status, PEP 8 compliant variables, removed unneces…
abastola0 Nov 19, 2023
e277eb3
fix: applied black code formatting
abastola0 Nov 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions keyring/cli.py
View file
Open in desktop
Expand Up @@ -86,10 +86,24 @@ def do_get(self):
print(password)

def do_set(self):
password = self.input_password(
f"Password for '{self.username}' in '{self.service}': "
)
set_password(self.service, self.username, password)
trials = 3
for i in range(trials):
password = self.input_password(
f"Password for '{self.username}' in '{self.service}': ")

reentered_password = self.input_password(
Copy link
Owner

Choose a reason for hiding this comment

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

I've started work on tests and run into an issue. The input_password method is coded to accept the input from a pipe (if present). This change of expecting two passwords when it's piped in is unacceptable. Probably instead we'll want to hook this logic in at getpass.getpass instead of wrapping input_password.

Copy link
Owner

Choose a reason for hiding this comment

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

In #663, I'm working on adding some tests to capture at least the current expectation and hopefully serve as a foundation for testing this more sophisticated behavior. Interestingly, those tests didn't seem to capture the concern I mentioned above, but I expected them to, so I'll need to think about it a bit more before proceeding.

I've also worked on some refactorings in the feature/password-verification branch. I'd pushed them to this PR, but decided to leave them in their own branch for now while working out this issue.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Please let me know if you need help with any other issues as well. I'll be more than happy to contribute.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should ask to re-enter password only if sys.stdin.isatty()?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I think that works. I think it would be a good idea to skip verification for if not typed in explicitly. I can make this change if it's fine with @jaraco. I would imagine the piped password would already be visual to the user in some form so the verification would not make much sense in that case.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jaraco. Any update on this?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the delay. Yes, please proceed with drafting an implementation that avoids the re-prompt for non-interactive cases.

f"Re-enter password for '{self.username}' in '{self.service}': ")

if password == reentered_password:
# If passwords match, set the password and break the loop
set_password(self.service, self.username, password)
break
else:
print('Password verification failed! Try again!\n')

else: # This else is associated with the for-loop, not the if-statement
sys.stderr.write(f"Password verification failed for {trials} times. Aborting!!\n")
sys.exit(1)

def do_del(self):
delete_password(self.service, self.username)
Expand Down