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

NamedPipeTransportOptions should support multiple PipeSecurity (DACL) #53306

Closed
1 task done
kenans opened this issue Jan 11, 2024 · 6 comments · Fixed by #56567
Closed
1 task done

NamedPipeTransportOptions should support multiple PipeSecurity (DACL) #53306

kenans opened this issue Jan 11, 2024 · 6 comments · Fixed by #56567
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@kenans
Copy link

kenans commented Jan 11, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Today, Kestrel is capable of binding to multiple NamedPipeEndpoint

// Kestrel will listen on both "pipe1" and "pipe2"
builder.WebHost.ConfigureKestrel(options =>
{
    options.ListenNamedPipe("pipe1");
    options.ListenNamedPipe("pipe2");
});

However, DACL for each Named Pipe is set up using the identical NamedPipeTransportOptions.PipeSecurity found in NamedPipeConnectionListener

if (_options.PipeSecurity != null)
{
    stream = NamedPipeServerStreamAcl.Create(
        _endpoint.PipeName,
        PipeDirection.InOut,
        NamedPipeServerStream.MaxAllowedServerInstances,
        PipeTransmissionMode.Byte,
        pipeOptions,
        inBufferSize: 0, // Buffer in System.IO.Pipelines
        outBufferSize: 0, // Buffer in System.IO.Pipelines
        _options.PipeSecurity);    // <-- NamedPipeTransportOptions.PipeSecurity is used to create NamedPipeServerStream
}
else
{
    // ...
}

The underlying issue for this is that NamedPipeTransportFactory.BindAsync(EndPoint endpoint, CancellationToken cancellationToken) is called multiple times with each NamedPipeEndpoint, but they all share the same NamedPipeTransportOptions.

public ValueTask<IConnectionListener> BindAsync(EndPoint endpoint, CancellationToken cancellationToken = default)
{
    // ...
    
    // The same _options instance is always used here for each endpoint
    var listener = new NamedPipeConnectionListener(namedPipeEndPoint, _options, _loggerFactory, _objectPoolProvider); 

    // ...
}

Describe the solution you'd like

It would be nice to be able to assign unique DACL (PipeSecurity) to different NamedPipeEndpoint.

One possible approach is to map PipeSecurity with the pipe name in NamedPipeTransportOptions,

public sealed class NamedPipeTransportOptions
{
    // ...

    IDictionary<string, PipeSecurity> PipeSecurities { get; set; }

    // ...
}

When NamedPipeServerStream is created, do something like below,

var pipeSecurity = _optionsPipeSecuriteies[endpoint.PipeName];
var stream = NamedPipeServerStreamAcl.Create(
    _endpoint.PipeName,
    PipeDirection.InOut,
    NamedPipeServerStream.MaxAllowedServerInstances,
    PipeTransmissionMode.Byte,
    pipeOptions,
    inBufferSize: 0, // Buffer in System.IO.Pipelines
    outBufferSize: 0, // Buffer in System.IO.Pipelines
    pipeSecurityy); 

Additional context

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jan 11, 2024
@amcasey
Copy link
Member

amcasey commented Jan 17, 2024

@kenans Thanks for the suggestion! Can you tell us a bit more about what you'd do with this functionality? What's an example of where pipes would benefit from having different PipeSecuritys?

@amcasey
Copy link
Member

amcasey commented Jan 17, 2024

@JamesNK May recall our reasoning for not doing this in the first place (possibly, it was just that no one had asked).

@JamesNK
Copy link
Member

JamesNK commented Jan 17, 2024

No one had asked.

I think if we do this then we would have a func similar to SocketTransportOptions.CreateBoundListenSocket.

public sealed class NamedPipeTransportOptions
{
    // ...

    Func<EndPoint, PipeSecurity> CreatePipeSecurity { get; set; }

    // ...
}

Although I think we should look to see if there are any other settings that should be configurable. Maybe we should allow someone to override creating the whole pipe, e.g. Func<EndPoint, NamedPipeServerStream> CreatePipe { get; set; }

@JamesNK
Copy link
Member

JamesNK commented Jan 17, 2024

No one had asked.

I think if we do this then we would have a func similar to SocketTransportOptions.CreateBoundListenSocket with a callback.

public sealed class NamedPipeTransportOptions
{
    // ...

    Func<EndPoint, PipeSecurity> CreatePipeSecurity { get; set; }

    // ...
}

The callback could replace the current property (we deprecate it) or we have them live side-by-side and throw an error if both are set.

Although I think we should look to see if there are any other settings that should be configurable on a per-pipe basis.

Maybe we should allow someone to override creating the whole pipe, e.g. Func<EndPoint, NamedPipeServerStream> CreateBoundServerPipe { get; set; }. The security could be set there.

@kenans
Copy link
Author

kenans commented Jan 18, 2024

@kenans Thanks for the suggestion! Can you tell us a bit more about what you'd do with this functionality? What's an example of where pipes would benefit from having different PipeSecuritys?

The idea is to have more granular control over the named pipe permissions (DACL) via PipeSecurity. E.g.

  • "./pipe/user" allows access from both Users and Administrators
  • "./pipe/admin" only allows access from Administrators

@kenans
Copy link
Author

kenans commented Jan 18, 2024

@JamesNK Thank you for the reply!

Both NamedPipeTransportOptions.CreatePipeSecurity and NamedPipeTransportOptions.CreateBoundServerPipe look good to me.

Regarding the BYO pipe proposal, it would be nice to clarify that existing settings in NamedPipeTransportOptions would not apply if CreateBoundServerPipe is set, as NamedPIpeServerStream would be fully controlled by the factory method.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants