Don't loop forever if fscrypt unlock doesn't have a tty
#430
Conversation
crypto/key.go
Outdated
| // We got EOF in the middle of a line, before seeing a newline. | ||
| // If there is at least some input, that's ok, treat it the same as a newline. | ||
| // But if we have read 0 bytes (not even a newline) and hit EOF, something is wrong, so return an error. | ||
| if totalBytesRead == 0 { |
There was a problem hiding this comment.
Shouldn't this just go into the io.EOF case? It shouldn't make a difference whether there is a trailing newline or not, and I don't think empty passphrases are meant to be supported.
If doing it that way, then there is no need for the new io.ErrUnexpectedEOF case (which is also misleadingly named because it is sometimes expected).
There was a problem hiding this comment.
oh, nice! if empty passwords aren't supported that makes this much easier, yes.
actions/callback.go
Outdated
| func unwrapProtectorKey(info ProtectorInfo, keyFn KeyFunc) (*crypto.Key, error) { | ||
| retry := false | ||
| for { | ||
| for i := 0; i < 3; i++ { |
There was a problem hiding this comment.
It is already supposed to not retry for non-interactive sessions. Either the program is running interactively, in which case there is no need for a limit, or it's running non-interactively in which case it should only try once. So there should be no need for an arbitrary limit such as 3. Perhaps you found a case in which it allows retries when it shouldn't?
There was a problem hiding this comment.
I think by "non-interactive" you mean --quiet:
Lines 150 to 151 in 15f711c
If you like I can change quietFlag to default to true if stdin is not a tty? but that might be confusing for existing users; I don't know enough about how fscrypt is used.
There was a problem hiding this comment.
Programs typically check whether stdin is a tty or not to decide whether they're running interactively or not. That should be the case here too. It sounds like the issue is that the retry behavior is currently determined by --quiet instead. It should instead be based on whether stdin is a tty.
There was a problem hiding this comment.
ok, done :) i also fixed a bug where an empty password wouldn't retry when interactive, but i don't have a way to automatically test it because of the infinite retry behavior. i did test manually that the new code works.
jyn514
force-pushed
the
no-tty
branch
2 times, most recently
from
45f7ee2 to
fcd80e4
Compare
|
Sorry, I've been busy. I'm taking a look at this again. If I understand correctly, removing support for empty passwords is actually unnecessary to fix the issue. Would you mind splitting that change into a separate commit, or even a separate pull request? Also, I tested Empty passwords aren't actually necessary to support that use case, as people can just choose any other default password, like "default" or whatever. Empty passwords should not have been allowed. But since they were allowed, I'm not sure we can rule out that someone is using them. |
i have removed all the stuff about empty passwords. i don't feel strongly about disallowing empty passwords so i'm not going to open a separate PR. i do want to note that this is now more prone to the original bug than my original PR—if stdin is a tty, but no input is available (maybe because we're running under |
|
👋 i'd appreciate a review when you have a chance :) |
2 participants
This fixes the bug in two places:
This still retries empty passwords when interactive.
Fixes #429