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

2nd attempt for Windows support #70

Open
axxelG opened this issue Dec 22, 2020 · 5 comments
Open

2nd attempt for Windows support #70

axxelG opened this issue Dec 22, 2020 · 5 comments

Comments

@axxelG
Copy link

axxelG commented Dec 22, 2020

Situation

I picked up #29 rebased it on master, took care of the comments in the MR and fixed some minor issues. See #69
I kept small commits for now, so it's easier to follow my changes.

I reached a state where the agent works but got stuck with a problem that blocks me from using it for me.

Problem

Windows has a timeout for smartcard transactions (default: 5 seconds)

If a transaction is held on the card for more than five seconds with no operations happening on that card, then the card is reset.
Source: https://docs.microsoft.com/en-us/windows/win32/api/winscard/nf-winscard-scardbegintransaction?redirectedfrom=MSDN

This does not play well with yubikey-agent:

  1. If you were asked to enter the new PIN during setup. You need to be quick. If you need more than 5 seconds to enter the PIN twice, you will fail. This I fixed with 4f046ef
  2. You have only 5 seconds to enter the PIN for normal agent usage, too
  3. If you entered the pin, you can start as many ssh sessions as you want within 5 seconds and than you have to enter the PIN again.
    There is a registry setting to increase the timeout, but I'm not sure about the side effects and this would only help to mitigate the problem but not solve it.

Idea

Actually I don't like that the card connections are open forever. I can imagine that this might cause other problems, too. I had for example weird things happening when I had the YubiKey Manager open and worked on yubikey-agent. But I was not aware of the timeout at that time, so that might have caused most of the weirdness.

My idea to approach this problem:

  1. Cache the pin in yubikey-agent and define an own timeout.
  2. Open a new connection (transaction?) to the YubiKey for every request.

Unfortunately I struggle to find a starting point here.

  • Is yubikey-agent even the right place to deal with that or should this go to one of the libraries used? Which one?
  • Can this cause side effects on other operating systems? I don't think so, but ...
  • Would this have a bigger impact on the general structure of yubikey-agent?
  • Is it weakening the security if the PIN is cached?

I would be able to spend some time on this but I will need someone to confirm that this is wanted to be implemented and support. At least for general design questions and testing.

@andreasbrett
Copy link

  • Is it weakening the security if the PIN is cached?

Yes and no not really. Yes when comparing it to other operating systems where this would not be needed. Not really when comparing it to the way Windows handles the timeout (open for X seconds but no restriction on how many sessions can be opened with it). An attacker being able to run as many sessions as it wants within 5 seconds after pin entry is not good and (to me) worse than having a FOSS tool cache the PIN. To me it is critical to unlock my Yubikey only for 1 connection/transaction and if that would mean yubikey-agent caches my PIN until that transaction is done, I'm fine with that.

@axxelG
Copy link
Author

axxelG commented Dec 23, 2020

I took a deeper look today and caching the PIN is easier than I thought. I added a minimal solution to my branch. See 41efb9a

ToDo:

  • Add flag to enable PIN caching
  • Make PIN caching default for Windows
  • Expire cache. What would be a sane default? 2 hours?
  • Add flag for PIN cache expiration duration

Optional:

  • Move PIN handling to a separate file/package. I like to have smaller files but I'll go with whatever is preferred by the maintainers.
  • Enable single use PIN cache (Require PIN for every use like andreasbrett proposed
  • Have the cache expire n seconds after the last usage

I would implement it with a new class PINCache or pinCache if we keep it in main with a set and get function that are handling the cache expiration.

Any opinion/input is highly appreciated. Especially regarding the security implication of storing the PIN in memory.

@axxelG
Copy link
Author

axxelG commented Jan 24, 2021

If you use the Windows Crypto API to access the YubiKey PIV section the PIN is asked only once and the timeout ist quite long, if there is one at all.

The Windows code is not accessible for me but I think we can safely assume that this can only be done by somehow storing the PIN. Seeing no other opinion here and Microsoft doing a similar approach is good enough for me to move on.

During my research I found https://github.com/buptczq/WinCryptSSHAgent which I will use now. I will happily finish the work on this PR but will not contribute long term for a tool I don't use by myself.

@FiloSottile I guess it's up to you to decide if you want to have and maintain Windows compatibility for yubikey-agent or not. Feel free to give feedback or just close this issue + MR

@naikrovek
Copy link

so, reading this, has Windows support been added? README.md says "WIP".

@xskrasek
Copy link

I would also like to know about this

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

No branches or pull requests

4 participants