This repository was archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed #26395
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
20e7102
Expose PipeOptions.CurrentUserOnly and add windows implementation whe…
safern 47bf74e
PR Feedback
safern 7d9ab1a
Add missing if statement
safern c827c7f
Move PipeSecurity types in implementation to System.IO.Pipes
safern b53a112
PR Feedback and move tests to netcoreapp file
safern 7410628
Add CurrentUserOnly implementation for NamedPipeClient in Unix
safern 528b493
PR Feedback
safern 18d6521
Refactor to not have duplicated code
safern b3f14ca
Implement server side current option only in named pipes for unix
safern 2b537e6
More PR Feedback
safern eeb1e39
PR Feedback round 3
safern cea4c1e
Add more tests
safern 57f674d
Fix build and add using to WindowsIdentity objects
71d475e
Fix packaging issues
c282a2b
netstandard-Windows_NT needs to be built from sources
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we going to do on unix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still reading about Unix security. Not yet what dependencies we're going to need. If any of the ones imported for windows is needed I will move it to the common ItemGroup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Unix we will probably want to create a directory that's chmod'd to only be accessible by the current user, and then create the domain socket in that directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the main thing still missing: right now there is enforcement on the client but nothing on the server side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm working on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you're saying. What if for the UserOnly pipes instead of adding the corefx prefix in the pipe name we use that space that we're already taking, for the directory name?
corefx/src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs
Line 29 in 40cbd56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could create a directory that contains the userid so that it is unique and related to this user and we don't generate random names, so maybe something like:
tempdir/{euid}_fx/{pipename}where an euid is using the 16 bits. As the docs say:Which at the end will be using less space than what
CoreFxPipe_prefix is already using on a regular named pipe.@stephentoub thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub noting for posterity that on Roslyn they currently don't use this approach, they connect blindly then check GetPeerID() against GetEUid().
https://github.com/dotnet/roslyn/blob/c7a465fabaf688affbd5897d7348431eeb2059b5/src/Compilers/Shared/BuildServerConnection.cs#L490
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub any reason to prefer one over the other? It seems like getpeerid() is for this exact purpose. https://linux.die.net/man/3/getpeereid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danmosemsft I am starting to look back at this to add the tests this week, and I think your point about
getpeeridis valid. The only thing that cross my mind is that the existing API on the server side currently has to either throw an exception for each mismatched peer (after the socket connection) or simply ignore them - neither seems good (perhaps a new API in the future?). In this sense the directory is helpful because prevents connections before it reaches the server socket. OTOH it will introduce some small difference between Unix and Windows in at least some (mis)configurations:On Windows: client will connect fine;
On Unix: client, depending on API, hangs or throws timeout exception (since the path for the named pipes is different between server and client)
All of this to say that let's keep the directory, but also add the
getpeeridcheck also on the server side (extra precaution), it is already done on the client side.