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

Remove DEL characters from password input #5837

Merged
merged 3 commits into from
Dec 12, 2018

Conversation

thorhs
Copy link
Contributor

@thorhs thorhs commented Nov 23, 2018

iTerm password manager sends \x03\x7f before sending a password
from its password manager to make sure the password is not being
echoed to the screen. Unfortunately, vault login does not handle
the Space DEL sequence, causing the login to fail when using the
password manager. This patch uses a simple method to delete the
sequence if present anywhere in the string, although it is strictly
only needed at the start of input.

This can be tested by using the iTerm built-in password manager, the
passwords entered from the password manager are not accepted by
vault. The first two characters are \x03\x7f.

After the patch, the \x7f character is removed as well as the preceding
character. If the \x7f character is the first on the line, only that
character is deleted.

iTerm password manager sends \x03\0x7f before sending a password
from its password manager to make sure the password is not being
echoed to the screen.  Unfortunately, vault login does not handle
the Space DEL sequence, causing the login to fail when using the
password manager.  This patch uses a simple method to delete the
sequence if present anywhere in the string, although it is strictly
only needed at the start of input.
@jefferai
Copy link
Member

I don't follow the logic here. Why aren't you just doing a trimprefix with that exact substring?

@thorhs
Copy link
Contributor Author

thorhs commented Nov 23, 2018

The logic is to search for the first instance of the \x7f character, and remove it and the preceding character, then loop to see if there are any other instances. If there are no instances found, return the current string. There may be more elegant ways to acomplish the task, but this is the way I came up with.

While I could just TrimPrefix, this felt like a more complete solution which would handle DEL in other areas of the password, or if iTerm would change the preceding space (\x03) to some other character in the future.

@jefferai
Copy link
Member

Considering what iterm is doing here is special purpose and rather bogus, I think we should just handle it explicitly. That way if other cases come up we can handle them knowingly.

The logic now only removes the two byte prefix sent in by iTerm
instead of trying to remove all deletes in the string.

This has been tested to work with the iTerm password manager.

As a small correction, the byte sequence is \x20\x7f.  The
earlier commit message incorrectly stated it was \x03\x7f.
@thorhs
Copy link
Contributor Author

thorhs commented Nov 23, 2018

@jefferai: I have simplified the code to only handle the iTerm sequence by using TrimPrefix. This has been tested using the iTerm Password Manager.

@jefferai jefferai added this to the 1.0.1 milestone Dec 12, 2018
@jefferai jefferai merged commit 8bdd74c into hashicorp:master Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants