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

Add WSL support via UnixSocket #14

Merged
merged 4 commits into from
Jan 29, 2022
Merged

Add WSL support via UnixSocket #14

merged 4 commits into from
Jan 29, 2022

Conversation

mehrdadn
Copy link

@mehrdadn mehrdadn commented Jul 22, 2021

Handles dlech/KeeAgent#220.

Note: Please perform a security review before using this code. I only made these changes for my own personal use, so I may have paid insufficient attention to certain things (like file/socket permissions).

Copy link
Owner

@dlech dlech left a comment

Choose a reason for hiding this comment

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

Nice! I assume that this is for WSL1 and not WLS2? If so, it would be nice to update the names to reflect that.

SshAgentLib/PageantAgent.cs Show resolved Hide resolved
SshAgentLib/UnixSocket.cs Outdated Show resolved Hide resolved
SshAgentLib/UnixSocket.cs Outdated Show resolved Hide resolved
SshAgentLib/UnixSocket.cs Outdated Show resolved Hide resolved
SshAgentLib/UnixSocket.cs Outdated Show resolved Hide resolved
SshAgentLib/UnixSocket.cs Outdated Show resolved Hide resolved
@mehrdadn
Copy link
Author

mehrdadn commented Jan 6, 2022

Thanks for the review! Unfortunately all of them appear to be regarding the existing coding patterns, not anything done differently in my PR. So you might want to take care of them alongside the others when you get the chance.

@dlech
Copy link
Owner

dlech commented Jan 6, 2022

Did you also see the question about WSL1 vs WSL2?

@mehrdadn
Copy link
Author

mehrdadn commented Jan 6, 2022

Oh sorry, I forgot to reply to that. I guess it depends how you look at it. To my knowledge WSL2 doesn't support AF_UNIX interop at all currently so I don't expect this would help on WSL2. But I also don't expect a Socket or Agent for WSL2 would look any different than this one—that would imply Microsoft would introduce a new AF_UNIXv2 or something which would be bizarre for no gain that I can envision. So I thought just calling it WSL would seem apt, but if you think it's better to call it WSL1 I guess I can do that too.

@dlech
Copy link
Owner

dlech commented Jan 6, 2022

How about just adding a comment at the top of the file saying that it only works for WSL1?

@mehrdadn
Copy link
Author

mehrdadn commented Jan 6, 2022

Ok sure!

@mehrdadn
Copy link
Author

mehrdadn commented Jan 7, 2022

Just pushed the changes.

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2022

Codecov Report

Merging #14 (7c85e3a) into master (ae6f8b9) will increase coverage by 0.50%.
The diff coverage is 64.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   52.02%   52.53%   +0.50%     
==========================================
  Files          40       43       +3     
  Lines        4755     4955     +200     
  Branches      458      481      +23     
==========================================
+ Hits         2474     2603     +129     
- Misses       2120     2182      +62     
- Partials      161      170       +9     
Impacted Files Coverage Δ
SshAgentLib/PageantAgent.cs 0.00% <0.00%> (ø)
SshAgentLib/Microsoft/UnixDomainSocketEndPoint.cs 53.93% <53.93%> (ø)
SshAgentLib/WslSocket.cs 88.50% <88.50%> (ø)
...tLib/Microsoft/UnixDomainSocketEndPoint.Windows.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae6f8b9...7c85e3a. Read the comment docs.

@mehrdadn
Copy link
Author

@dlech Were you intending to merge this?

@dlech
Copy link
Owner

dlech commented Jan 29, 2022

I will test it tomorrow.

@mehrdadn
Copy link
Author

Ah awesome, thanks!

dlech added 2 commits January 29, 2022 10:10
- Use backported UnixDomainSocketEndPoint from .NET core.
- Rename UnixSocket to WslSocket since this is Windows-only.
- Remove use of System.Reflection.
- Use async/await instead of blocking threads.
- Add unit tests.
- Fix bugs discoverd by unit tests.
- Add changelog entry.
@dlech
Copy link
Owner

dlech commented Jan 29, 2022

I reworked this a bit to simplify the code and fix some issues.

@dlech dlech merged commit 07c4d17 into dlech:master Jan 29, 2022
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