Skip to content

Conversation

Vindaar
Copy link
Contributor

@Vindaar Vindaar commented Feb 23, 2018

Adds a getPassword proc to the terminal module, which allows to read passwords from stdin, i.e. read input without showing any characters. Characters are entered as hidden text, which is removed once user input is finished.
It handles backspaces as well as Unicode characters in the input.

I hope I didn't break too many conventions in this. :-)

Adds a `getPassword` proc to terminal, which allows to read passwords
from stdin, i.e. without showing any characters. Characters are
entered as hidden text, which is removed upon finished user input.

Handles backspace as well as unicode characters in passwords.
Changes the return type of getPassword from string to TaintedString to
conform with taint mode.
Wraps the unsetControlCHook proc in compile time checks for
noSignalHandler and useNimRtl to fix compilation with
-d:useNimRtl (otherwise complains about missing implementation of the
proc)
@Vindaar
Copy link
Contributor Author

Vindaar commented Feb 24, 2018

Applied a few fixes for the failed tests.

I changed the return value of getPassword to TaintedString instead of string to comply with taint mode. I'm not exactly sure if that's a better solution than simply converting the user input to string. I suppose if one wants to check the user password for certain character classes etc. it might be useful as a form of input validation?

Also wraps the unsetControlCHook in the same compile time checks as the signalHandler proc. I'm not exactly happy with opening a second when block, which is the same as the one above setControlCHook, but implementing the unset proc before the set proc seems weird.
I'm also not sure whether there should be a check for noSignalHandler in system.nim as well.

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Just one thing, otherwise looks good.

stdout.showCursor()
stdout.resetAttributes()
stdout.write("\n")
if stackTraceAvailable() == true:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a function that does this already which could be called? Presumably there is already a hook somewhere which does this.

P.S. == true is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell there's no proc to do that. resetAttributes is the closest, but it only affects the text style and leaves the cursor as it is. One could of course define a resetTerminal proc, which combines the two.

The newline I insert should not be part of that proc though, since I only need it due to moving the cursor one line up to get back down.

Regarding the if statement. Oh, good point, thanks. I'll change that.

@Vindaar
Copy link
Contributor Author

Vindaar commented Feb 26, 2018

I've since given the way I handle the input some more thought. Depending on how paranoid one is, using readLine is probably not the greatest idea. It's all fine, as long as the user finishes the input or sends a SIGINT, but in the unlikely case someone sends some other signal like SIGSTP or SIGTERM (or simply never presses return), the text will still be written in the terminal, albeit invisible.

A malicious actor could probably use that easily enough to steal the password, especially because the average user of a program making use of this proc will not be aware of the fact that the text is only invisible.

I'll look into replacing the readLine with readBuffer to read one character at a time to mitigate that. Plus, as a bonus, one might be able to include an option for an "asterisk styled" password input that way.

@dom96
Copy link
Contributor

dom96 commented Feb 26, 2018

Perhaps it would make sense to look into using the native operating system functions for reading passwords?

@Araq
Copy link
Member

Araq commented Feb 26, 2018

@dom96 There aren't any. The other implementations all use hacks like these afaict.

@Vindaar
Copy link
Contributor Author

Vindaar commented Feb 26, 2018

To be fair, I haven't spend too much time researching it, but what Araq says seems to be the case.
For example in case of GNU:

https://www.gnu.org/software/libc/manual/html_node/getpass.html

@dom96
Copy link
Contributor

dom96 commented Feb 26, 2018

@Araq @Vindaar There are implementations in POSIX (and I heard somebody mention a WinAPI function too, possibly in IRC can't remember) https://stackoverflow.com/a/6869218/492186

@Araq
Copy link
Member

Araq commented Feb 26, 2018

@Vindaar
Copy link
Contributor Author

Vindaar commented Feb 26, 2018

Thanks @Araq, will do!

@dom96
Copy link
Contributor

dom96 commented Feb 26, 2018

Why not use the POSIX implementation on POSIX systems?

@dom96
Copy link
Contributor

dom96 commented Feb 26, 2018

IMO you should use getpass on POSIX and some other implementation on Windows (if the win API has no equivalent functions).

@Vindaar
Copy link
Contributor Author

Vindaar commented Feb 26, 2018

I was about to say mainly because you stumble upon things like (http://man7.org/linux/man-pages/man3/getpass.3.html):

This function is obsolete. Do not use it. If you want to read input without terminal echoing enabled, see the description of the ECHO flag in termios(3).

But awkward as it is, while looking through Nim's source, I realized that there is indeed already a read password proc in the stdlib. So you were right after all @dom96. It's simply in impure/rdstdin.nim:
https://nim-lang.org/docs/rdstdin.html#stdin_1

So close this PR? rstdin seems to suffer from visibility issues, haha.

@Araq
Copy link
Member

Araq commented Feb 26, 2018

Is listed in https://nim-lang.org/docs/theindex.html

Closing.

@Araq Araq closed this Feb 26, 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.

3 participants