cmd/age: use CONIN$ instead of /dev/tty on Windows

[codesoap]

I'm intending to solve #128 with this pull request.

The solution for Windows was kindly provided to me by @alexbrainman at https://github.com/golang/go/issues/46164#issuecomment-851329885.

I have to admit, though, that I do not fully understand what is going on, because I am unfamiliar with Windows and it's APIs. As far as I understood, using CONIN$ is also what is recommended by Microsoft:

The CreateFile function enables a process to get a handle to its console's input buffer and active screen buffer, even if STDIN and STDOUT have been redirected. To open a handle to a console's input buffer, specify the CONIN$ value in a call to CreateFile. Specify the CONOUT$ value in a call to CreateFile to open a handle to a console's active screen buffer. CreateFile enables you to specify the read/write access of the handle that it returns.

[alexbrainman]

Given that this command is security sensitive, you should have real Windows developer, like @zx2c4, not me review this change.

Alex

[FiloSottile]

Awesome, thank you both, this is something that kept biting Windows users! It's kind of unfortunate cmd/age needs to know so much about open terminals. Maybe there's space in x/term for an OpenTerminal function.

If @zx2c4 has time to take a look that'd be great, but otherwise I am fairly comfortable with this, the failure mode is not particularly security-sensitive: we don't use this to store a secret, only to read one interactively, so if it doesn't work at most we'll fail to get terminal input. (I guess if permissions are wrong some other user on the system might be able to intercept the input?)

I am getting a decent Windows VM set up so I can test these things properly.

[zx2c4]

CONIN$ is fine and is probably what you want here indeed. What exactly is the security concern here? That it'd read from the wrong console or something like that?

[FiloSottile]

If that’s the case we can drop the build tagged files and just check runtime.GOOS to pick the filename.

On Jun 1, 2021, at 21:14, Andrew Ekstedt @.***> wrote:

 @magical commented on this pull request.

In cmd/age/read_password_windows.go:

+package main + +import (

• "fmt"
• "os"
• "golang.org/x/sys/windows"
• "golang.org/x/term" +)

• +func readPassphraseFromTerminal() ([]byte, error) {

• conin, err := windows.UTF16PtrFromString("CONIN$")
• if err != nil {
• return nil, err
• }
• tty, err := windows.CreateFile(conin, windows.GENERIC_READ|windows.GENERIC_WRITE, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, 0, 0) I don't think you need x/sys/windows. os.Open should work too.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[magical]

Oh my bad, it actually needs to be OpenFile to get r/w permission.

https://play.golang.org/p/FTJwXE0o6AP

Seems to work. I don't know if any of the extra stuff os.OpenFile does could come back to bite us, but the simplification seems nice.

On Tue, Jun 1, 2021, 12:26 Filippo Valsorda @.***> wrote:

If that’s the case we can drop the build tagged files and just check runtime.GOOS to pick the filename.

On Jun 1, 2021, at 21:14, Andrew Ekstedt @.***> wrote:

 @magical commented on this pull request.

In cmd/age/read_password_windows.go:

+package main + +import (

• "fmt"
• "os"
• "golang.org/x/sys/windows"
• "golang.org/x/term" +)

• +func readPassphraseFromTerminal() ([]byte, error) {

• conin, err := windows.UTF16PtrFromString("CONIN$")
• if err != nil {
• return nil, err
• }
• tty, err := windows.CreateFile(conin, windows.GENERIC_READ|windows.GENERIC_WRITE, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, 0, 0) I don't think you need x/sys/windows. os.Open should work too.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/FiloSottile/age/pull/274#issuecomment-852387883, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABK3MYMVGZH73LHBFKAKL3TQUX7FANCNFSM453NFTSA .

[FiloSottile]

The linked issue mentions Open, which is read only. If you didn't try OpenFile I will this evening.

On Jun 1, 2021, at 21:45, Richard Ulmer @.***> wrote:

 @codesoap commented on this pull request.

In cmd/age/read_password_windows.go:

+package main + +import (

• "fmt"
• "os"
• "golang.org/x/sys/windows"
• "golang.org/x/term" +)

• +func readPassphraseFromTerminal() ([]byte, error) {

• conin, err := windows.UTF16PtrFromString("CONIN$")
• if err != nil {
• return nil, err
• }
• tty, err := windows.CreateFile(conin, windows.GENERIC_READ|windows.GENERIC_WRITE, windows.FILE_SHARE_READ, nil, windows.OPEN_EXISTING, 0, 0) I haven't tried it in age, but in another program and it didn't work. See golang/go#46164 (comment) for more info.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[codesoap]

Sounds good. I haven't tried OpenFile yet.