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

Support Named Pipes in gRPC target strings #198

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

amartinezfayo
Copy link
Member

This PR adds support to Named Pipes in gRPC target strings.
The npipe URI scheme is now supported to specify a named pipe target address. An opaque URI is used for this:

  • npipe:<pipeName>: where pipeName is the named pipe name in the local host.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
err: "",
},
{
addr: "npipe:pipe\\name",
Copy link
Member

Choose a reason for hiding this comment

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

Should the backslash be allowed? From CreateNamedPipe:

The pipename part of the name can include any character other than a backslash, including numbers and special characters. The entire pipe name string can be up to 256 characters long. Pipe names are not case sensitive.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's interesting. We have been using backslashes in the pipe name in SPIRE (we have set the default named pipe name for the Workload API named pipe as \spire-agent\public\api).

Looking at the code in the go-winio library, I see that instead of the CreateNamedPipe function, the (undocumented?) NtCreateNamedPipeFile function is used: https://github.com/microsoft/go-winio/blob/master/pipe.go#L338

I'm thinking that's why we didn't run into this limitation when creating the pipes in SPIRE. It seems that the NtCreateNamedPipeFile function allows the use of backslashes (and also the pipe name string can be larger than 256 long).

We can probably remove this test case, but I'm thinking that we may not perform additional validations to check that the pipe name conforms with the CreateNamedPipe function. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting. Hmm. Meh, in that case I don't care if we have this test case or not. But your right, I don't think we need to do extra validations.

@amartinezfayo amartinezfayo merged commit fcf03d7 into spiffe:main Jun 9, 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.

None yet

2 participants