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

First pass at Windows support #29

Closed
wants to merge 1 commit into from

Conversation

neersighted
Copy link

Hey, this is a quick-and-dirty windows PoC. I'm not sure how you want to go about structuring the program in the long run, so for now everything still lives in main.

I have plans to add a single-binary (but dual process) WSL proxy component -- some guidance on how you want to proceed there layout/abstraction-wise would be welcome (my thoughts are a cmd/yubikey-agent and a cmd/yubikey-agent-wsl that only builds on Windows).

Big thanks to @tobiaskohlbau for getting a hard-to-track-down buffer bug fixed in upstream gopass.

@tobiaskohlbau
Copy link

Hi @neersighted great work.

Service

Your implementation leaves some open questions which I'm still digging around with in my own implementation. How to support demonized running within windows. During my research I've found two possible options:

  1. Running through Windows Task Schedule and render the terminal on offscreen. (That's what I'm currently using for testing.)
  2. Write a Integration for Windows Services and a "client" service which connects to the service and spawns the "pinentry" on request. This is needed by the rule that a windows service is not allowed to spawn a UI process (at least as far as I found out).

I tend towards the first option as this avoids having a service and a "client" component and the service handling on windows is also quiet different to linux, whereas the program needs to handle some specific interface/api to be a service instead of just a program which is handled from the outside. Also a service runs in the context of a windows specific user by default which enables other users on the same system to connect to the service. Therefore there must exist a system which avoids access for the wrong person.

WSL

For the part of WSL support IMHO I would leave this to external tooling instead of supporting it directly within yubikey-agent. At least for now it's always required to have another component running/launched to access from within WSL. I've merged support for this kind of scenario within https://github.com/BlackReloaded/wsl2-ssh-pageant where it's able to talk to the windows named pipe.

By the way which pinentry utility are you using? I'm not comfortable with relying to have gpg installed on the machine in order to have a pinentry.exe. This weekend I tried to compile the pinentry from gpg without the gpg stuff and bundle it for distribution. But I did not succeed for now. I love the idea of having a lightweight pinentry utility on windows similar to mac and linux which is distributed via chocolatey or the new winget package manager. I considered writing a pinentry UI from scratch but I'm not too familiar with UI handling on Windows.

flag.Usage()
os.Exit(1)
if runtime.GOOS == "windows" {
*socketPath = "\\\\.\\\\pipe\\\\openssh-ssh-agent"

Choose a reason for hiding this comment

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

I would avoid hardcoding the path where the pipe is implemented. If someone wants to customize it, there should exist a way to do it. Similar to Mac and Linux.

Copy link
Author

Choose a reason for hiding this comment

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

I think you might be misreading this line -- this is meant to set a default on Windows only (the flag is required on Unix systems). Maybe it would be better to set a default up above in the flag section instead.

Choose a reason for hiding this comment

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

You’re right I screwed this up. Sorry for the hassle.

@neersighted
Copy link
Author

Thanks for the feedback, @tobiaskohlbau! I'm not sure on the daemonization part yet -- I was planning on leaving that up to the user, for the most part. I'm reasonably sure a text mode program can be run through task scheduler with no UI -- otherwise, I'll have to do some more digging. I was figuring that would be an exercise left to the reader for now.

WRT WSL, I am quite familiar with wsl2-ssh-pageant as I currently use it for SSH and GPG. I would myself prefer to have something in this repo, as part of the goal of this project is for it to be turnkey, and WSL represents a major platform these days. Having an integrated, seamless solution seems quite advantageous for adoption. Also, existing solutions that use socat and make a mess of forking themselves to the background are not very attractive to me.

I'm currently using pinentry-basic.exe from Gpg4win -- haven't found a better solution yet, but I am looking.

@FiloSottile
Copy link
Owner

Thank you for doing this! I can't really test the Windows code myself, but if it's reasonably self-contained I'd be very happy to merge it. Here are some guidelines:

  • at least for the benefit of code review, don't split code out of main.go for now unless necessary
  • add a windows.md file with instructions on how to set this up including how to get a pinentry (doesn't have to be perfect, it can have TODOs and WIPs, but it helps with knowing what to improve)
  • if you add another package main, put it in cmd/$NAME and leave the current main package in the root
  • every Go file needs the license header

Ping me once you've rebased and it's ready for review and I'll get to it quickly (this time ^^).

@carstenblt
Copy link

Hi, what's the status on this? I got it running, except that the first connection attempt always fails with
agent 13: verify pin: transmitting request: the smart card has been reset, so any shared state information is invalid
A simple pinentry GUI would be nice and should be pretty straightforward. Let me know if I can help with that.

Base automatically changed from master to main February 11, 2021 12:14
@neersighted
Copy link
Author

Closing in favor of #69.

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.

4 participants