dotnet / wcf

This repo contains the client-oriented WCF libraries that enable applications built on .NET Core to communicate with WCF services.
MIT License
1.72k stars 558 forks source link

UnixDomainSocketBinding default security on Windows doesn't work #5621

Open mconnew opened 3 months ago

mconnew commented 3 months ago

On Windows, it uses NegotiateStream to establish the connection. As part of that, we pass a target name to NegotiateStream.AuthenticateAsClientAsync which is used to get a Kerberos ticket or decide to use NTLM. There's shared code which implicitly uses the hostname from the endpoint address Uri, but with UDS there is no hostname. This results in a bad target name being used (it ends up using host/) and authentication failing.

Workaround: Construct you EndpointAddress like this:

var endpointAddress = new EndpointAddress(new Uri("net.uds://" + servicePath), new SpnEndpointIdentity("host/localhost"));

This will override the implicit target name to be host/localhost and the NegotiateStream authentication will succeed.

A few options to fix this.

  1. On the CreateChannel code path, we could create a new Uri from the passed in Uri which has the hostname portion populated with localhost if it's currently empty.
  2. On the CreateChannel code path, we could add an SpnEndpointIdentity("host/localhost") to the EndpointAddress if there isn't currently an identity.
  3. In the code which calculates the target name, treat an empty hostname as the empty string target name. So if the hostname is non-empty, generate the target name host/localhost, but if the hostname is empty, generate the target name String.Empty. I have verified String.Empty successfully authenticates.
mconnew commented 3 months ago

On working out why the tests weren't failing, I found where the fix needs to be. In UnixDomainSocketChannelFactory<TChannel>.OnCreateChannel there's this code:

            if (address.Identity == null)
            {
                var hostIdentity = new DnsEndpointIdentity(address.Uri.Host ?? "localhost");
                var uriBuilder = new UriBuilder(address.Uri);
                uriBuilder.Host = null;
                address = new EndpointAddress(uriBuilder.Uri, hostIdentity,address.Headers.ToArray());
            }

The null check on address.Uri.Host needs to be a string.IsNullOrEmpty call instead. Also the tests (at least for CoreWCF, haven't checked WCF yet) had an extra catch block in the try/finally used for test cleanup which swallowed the failure, but this isn't what made it pass. A UriBuilder is used which provides a hostname of localhost by default in the Uri. So basically the code should be this:

            if (address.Identity == null)
            {
                var hostIdentity = new DnsEndpointIdentity(string.IsNullOrEmpty(address.Uri.Host) ? "localhost" : address.Uri.Host);
                var uriBuilder = new UriBuilder(address.Uri);
                uriBuilder.Host = null;
                address = new EndpointAddress(uriBuilder.Uri, hostIdentity,address.Headers.ToArray());
            }
mconnew commented 3 months ago

@imcarolwang, can you create a PR to fix this issue. Take a look at the CoreWCF issue for a comment with a description of why the tests didn't fail in the unit tests there and make sure any of those issues are fixed in the WCF tests too. Basically don't use UriBuilder and make sure there isn't an extra catch block.

imcarolwang commented 3 months ago

@imcarolwang, can you create a PR to fix this issue. Take a look at the CoreWCF issue for a comment with a description of why the tests didn't fail in the unit tests there and make sure any of those issues are fixed in the WCF tests too. Basically don't use UriBuilder and make sure there isn't an extra catch block.

Hi @mconnew , I checked the WCF test, it doesn’t include a catch block. And in CoreWCF, the LinuxSocketFilepath is an empty string, whereas in WCF, the UriBuilderis initialized with a non-null temporary path. Given these differences, it seems the fix applied in CoreWCF may not be necessary for WCF. Could you please advise if this approach is suitable?

Test in WCF: link

Update: Please disregard my previous comments. I didn’t have the full picture at the time. I now understand the issue and will try work on fixing the bug in WCF.