Unix domain socket binding on WCF Client #5172

birojnayak · 2023-06-02T21:07:54Z

Phase 1 ensuring no auth and posix identity works

mconnew · 2023-09-12T21:19:16Z

/azp run

azure-pipelines · 2023-09-12T21:19:26Z

Azure Pipelines successfully started running 1 pipeline(s).

mconnew · 2023-09-12T23:21:22Z

src/System.ServiceModel.UnixDomainSocket/src/Resources/strings.resx

+    <value>The specified channel type {0} is not supported by this channel manager.</value>
+  </data>
+  <data name="TcpConnectError" xml:space="preserve">
+    <value>Could not connect to {0}. TCP error code {1}: {2}. </value>


Are the string resources that start with Tcp used? If so, they should probably be renamed and the message modified.

mconnew · 2023-09-12T23:22:36Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketBinding.cs

+    public class UnixDomainSocketBinding : Binding
+    {
+        //private OptionalReliableSession _reliableSession;
+        // private BindingElements


This binding is to provide similar capabilities to NetNamedPipe so won't ever support reliable sessions. Remove the commented out reliable sessions fields.

mconnew · 2023-09-12T23:23:17Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketBinding.cs

+        }
+
+        public UnixDomainSocketBinding(string configurationName)
+            : this()


Remove this method. It's only in the other bindings for legacy reasons. This is a new type so no need to start with this unsupported constructor.

mconnew · 2023-09-12T23:23:42Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketBinding.cs

+            _transport = new UnixDomainSocketTransportBindingElement();
+            _encoding = new BinaryMessageEncodingBindingElement();
+            //_session = new ReliableSessionBindingElement();
+           // _reliableSession = new OptionalReliableSession(_session);


Cleanup reliable sessions commented code

mconnew · 2023-09-12T23:24:11Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketBinding.cs

+
+            // add security (*optional)
+            //SecurityBindingElement wsSecurity = CreateMessageSecurity();
+            //if (wsSecurity != null)


We likely won't ever want to support message security as NetNamedPipe doesn't.

mconnew · 2023-09-12T23:24:19Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketBinding.cs

+
+        /*
+        private SecurityBindingElement CreateMessageSecurity()
+        {


mconnew · 2023-09-12T23:24:31Z

...erviceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketClientCredentialType.cs

+    {
+        None,
+        Default ,
+        Certificate ,


Fix formatting

mconnew · 2023-09-12T23:29:03Z

...m.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketTransportSecurity.cs

+        }
+
+        internal BindingElement CreateTransportProtectionOnly()
+        {


This method should be used when SecurityMode is Transport and ClientCredentialType is None. We still need to secure the connection use TLS, we just don't provide a client certificate.

CoreWCF/CoreWCF#1249

mconnew · 2023-09-12T23:29:21Z

...m.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketTransportSecurity.cs

+                    return CreatePosixIdentityOnlyBinding();
+                }
+            }else if (_clientCredentialType == UnixDomainSocketClientCredentialType.Certificate )
+            {


mconnew · 2023-09-12T23:30:17Z

...m.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketTransportSecurity.cs

+        }
+
+        internal bool InternalShouldSerialize()
+        {


Remove, this was used when writing out the binding to config and would be used to know if it was using defaults or not. If using defaults, no need to write it out to the xml. This is left over dead code.

mconnew · 2023-09-12T23:32:02Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/Channels/SslProtocolsHelper.cs

+            if (!IsDefined(value))
+            {
+                throw DiagnosticUtility.ExceptionUtility.ThrowHelperError(new InvalidEnumArgumentException("value", (int)value,
+                    typeof(SslProtocols)));


nameof(value)

mconnew · 2023-09-12T23:32:29Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/Channels/TransportDefaults.cs

+    internal static class ConnectionOrientedTransportDefaults
+    {
+        //public const bool AllowNtlm = SspiSecurityTokenProvider.DefaultAllowNtlm;
+        //public const int ConnectionBufferSize = 8192;


Remove unused commented members

mconnew · 2023-09-12T23:33:41Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/Channels/TypeFowards.cs

+using System.Runtime.CompilerServices;
+
+[assembly:TypeForwardedTo(typeof(System.ServiceModel.Channels.ConnectionOrientedTransportBindingElement))]
+[assembly:TypeForwardedTo(typeof(System.ServiceModel.Channels.SslStreamSecurityBindingElement))]


You don't need these. These were in nettcp to tell existing code that these types had been moved from nettcp to NetFramingBase, which is not for case for UDS

mconnew · 2023-09-12T23:36:38Z

...ceModel.UnixDomainSocket/src/System/ServiceModel/Channels/UnixPosixIdentityBindingElement.cs

+
+        [DefaultValue(Net.Security.ProtectionLevel.EncryptAndSign)]
+        public ProtectionLevel ProtectionLevel
+        {


I thought we removed this from the service side? This binding element doesn't do any encryption or signing of the data transmitted so it doesn't make sense to have it. If it's still there on the service side, please open an issue to remove it there too.

tracking here on the server side CoreWCF/CoreWCF#1214 (comment) and removing client

mconnew · 2023-09-12T23:36:54Z

...nixDomainSocket/src/System/ServiceModel/Channels/UnixPosixIdentitySecurityUpgradeProvider.cs

+        internal IdentityVerifier IdentityVerifier { get; private set; }
+
+        public ProtectionLevel ProtectionLevel { get; }
+


mconnew · 2023-09-12T23:37:33Z

...nixDomainSocket/src/System/ServiceModel/Channels/UnixPosixIdentitySecurityUpgradeProvider.cs

+                return null;;
+            }
+            public override string GetNextUpgrade()
+            {


Formatting. Need a blank line between methods.

mconnew · 2023-09-12T23:38:29Z

...nixDomainSocket/src/System/ServiceModel/Channels/UnixPosixIdentitySecurityUpgradeProvider.cs

+        private class UnixPosixIdentitySecurityUpgradeInitiator : StreamSecurityUpgradeInitiator
+        {
+            string upgradeString = UnixPosixUpgradeString;
+            public UnixPosixIdentitySecurityUpgradeInitiator()


Missing acessor modifier (private). Naming convention should begin with _

mconnew · 2023-09-12T23:39:00Z

...System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/Channels/UnsafeNativeMethods.cs

+    internal static class UnsafeNativeMethods
+    {
+        //public const int ERROR_SUCCESS = 0;
+        //public const int ERROR_FILE_NOT_FOUND = 2;


Remove unused commented values

mconnew · 2023-09-12T23:40:44Z

Now that CoreWCF UDS has been released, can you add a scenario test. The folder for them is under the System.Private.ServiceModel\tests\Scenario folder

mconnew · 2023-10-05T19:26:18Z

Feedback from our call, we need to introduce a new security mode similar to BasicHttpSecurityMode.TransportCredentialOnly to use the Posix identity client credential type. This is because security mode Transport is intended to have the connection have integrity and confidentiality which the Posix credential type doesn't provide. We could potentially add support for using the posix identity with Ssl (service side certificate only) later if there's demand.

mconnew · 2023-10-10T20:14:45Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketBinding.cs

+    {
+        private UnixDomainSocketTransportBindingElement _transport;
+        private BinaryMessageEncodingBindingElement _encoding;
+       // private ReliableSessionBindingElement _session;


Remove commented line

mconnew · 2023-10-10T20:20:12Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketSecurity.cs

+    {
+        internal const SecurityMode DefaultMode = SecurityMode.Transport;
+
+        private SecurityMode _mode;


Need to create a UDS specific enum for this as we aren't supporting Message or TransportWithMessageCredentials. See NetNamedPipeSecurityMode Enum docs.

This also goes with my comment here where we need TransportCredentialOnly for IdentityOnly

mconnew · 2023-10-10T20:30:24Z

...erviceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketClientCredentialType.cs

+        Certificate,
+        Windows,
+        IdentityOnly,
+    }


As we're making a change to add a TransportCredentialOnly security mode, I don't think Only needs to be on here now. Also as it's now clearly only for *nix based systems like mac and Linux, I think PosixIdentity would be clearer, or maybe just Posix as we have just "Windows"

mconnew · 2023-10-19T18:26:22Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketSecurity.cs

+        private UnixDomainSocketSecurityMode _mode;
+
+        public UnixDomainSocketSecurity()
+        {


Instead of doing this inline, make DefaultMode a private static field and use the original constructor. Then you can use the default mode in the exception message of the private constructor

mconnew · 2023-10-19T18:29:37Z

src/System.ServiceModel.UnixDomainSocket/src/System/ServiceModel/UnixDomainSocketSecurity.cs

+            if (_mode == UnixDomainSocketSecurityMode.Transport || _mode == UnixDomainSocketSecurityMode.TransportCredentialOnly)
+            {
+                if(_mode == UnixDomainSocketSecurityMode.TransportCredentialOnly && Transport.ClientCredentialType != UnixDomainSocketClientCredentialType.PosixIdentity)
+                {


Also throw when _mode it Transport and credential type is PosixIdentity as it should only be used for TransportCredentialOnly

birojnayak marked this pull request as ready for review June 26, 2023 19:17

mconnew reviewed Sep 12, 2023

View reviewed changes

This was referenced Oct 4, 2023

Formatting fix for file UnixDomainSocketTransportSecurity CoreWCF/CoreWCF#1214

Closed

Feature : - Fix TransportSecurity with clientcredential as none CoreWCF/CoreWCF#1249

Open

birojnayak force-pushed the main branch from ab7da84 to d3ba5c9 Compare October 9, 2023 17:28

mconnew reviewed Oct 10, 2023

View reviewed changes

Unix domain socket binding on WCF Client

1678344

birojnayak force-pushed the main branch from c10d404 to 1678344 Compare October 14, 2023 00:06

review comments

a21cad9

mconnew reviewed Oct 19, 2023

View reviewed changes

birojnayak added 2 commits October 20, 2023 00:21

incorporated review comments

b660de3

Incorporated the feedback for socket connection

95ef200

mconnew approved these changes Oct 24, 2023

View reviewed changes

mconnew merged commit 6a652f6 into dotnet:main Oct 24, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unix domain socket binding on WCF Client #5172

Unix domain socket binding on WCF Client #5172

birojnayak commented Jun 2, 2023

mconnew commented Sep 12, 2023

azure-pipelines bot commented Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

birojnayak Oct 5, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

birojnayak Oct 4, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew Sep 12, 2023

mconnew commented Sep 12, 2023

mconnew commented Oct 5, 2023

mconnew Oct 10, 2023

mconnew Oct 10, 2023

mconnew Oct 10, 2023

mconnew Oct 10, 2023

mconnew Oct 19, 2023

mconnew Oct 19, 2023

		internal IdentityVerifier IdentityVerifier { get; private set; }

		public ProtectionLevel ProtectionLevel { get; }

Unix domain socket binding on WCF Client #5172

Unix domain socket binding on WCF Client #5172

Conversation

birojnayak commented Jun 2, 2023

mconnew commented Sep 12, 2023

azure-pipelines bot commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mconnew commented Sep 12, 2023

mconnew commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment