Drop Mono.Security #127

Closed
roji opened this Issue Dec 12, 2013 · 8 comments

3 participants

@roji
Npgsql member

We already use the .NET framework SSL implementation, but Mono.Security has been kept for backwards compatibility.

@roji
Npgsql member

@franciscojunior said in npgsql-devel:

Regarding the Mono.Security support, I don't have much information about that yet. Sebastien Pouliot is the guy responsible for the security part of Mono. I don't know if he is still playing with this today. I did some tests (which culminated in my suggestion to remove the Mono.Security assembly) where I could get SSLStream to work where with Mono.Security I couldn't: http://fxjr.blogspot.com.br/2013/07/sslstream-support-added-to-npgsql.html
Also in the past we were getting reports of errors when trying to connect to Postgresql through SSL, although I didn't receive more information why the errors were happening.

I'm open to check which library would work better for Npgsql. Also, after Shay's work on legacy entity framework reflection, I think that if we decide to keep Mono.Security, we could make the assembly required only when user need to connect through SSL. If not, the user can go only with Npgsql.dll assembly. This would help to fix an old request of having a single Npgsql assembly to be deployed.

@roji
Npgsql member

IMHO, it isn't really a problem to keep packaging Mono.Security with Npgsql. I know that some other projects do so, since .NET doesn't provide some needed security features which Mono.Security does.

The question is rather, does Npgsql require Mono.Security in order to provide all SSL features provided by the Postgresql backend? If not, then it should probably be removed as a needless dependency. If Npgsql can't provide all backend-supported SSL features with Mono.Security, then we should probably keep it...

But we need someone who actually knows something about all this to help :)

@franciscojunior franciscojunior modified the milestone: 2.3, 2.2 Apr 8, 2014
@roji
Npgsql member

Try to cover as much of the Postgresql backend SSL capabilities in tests (#291), and based on that get rid of Mono.Security...

@jochenwezel

Based on npgsql_v2.2.0, I've created a (SVN) patch file to drop all Mono.Security stuff - no problem on running with conventional connection, but not tested with an SSL connection (I've got no server for testing).
Maybe someone else can test and prepare a github push request - at least for a branch "No-Mono.Security-Support" (or similar)?

Index: Npgsql/Npgsql/NpgsqlClosedState.cs
===================================================================
--- Npgsql/Npgsql/NpgsqlClosedState.cs  (Revision 1360)
+++ Npgsql/Npgsql/NpgsqlClosedState.cs  (Arbeitskopie)
@@ -32,8 +32,6 @@
 using System.Net.Sockets;
 using System.Reflection;
 using System.Threading;
-using Mono.Security.Protocol.Tls;
-using SecurityProtocolType=Mono.Security.Protocol.Tls.SecurityProtocolType;
 using System.Security.Cryptography.X509Certificates;

 namespace Npgsql
@@ -181,24 +179,9 @@
                         context.DefaultProvideClientCertificatesCallback(clientCertificates);

                         //if (context.UseMonoSsl)
-                        if (!NpgsqlConnector.UseSslStream)
+                        if (!true)
                         {
-                            SslClientStream sslStreamPriv;
-
-                            sslStreamPriv = new SslClientStream(
-                                    baseStream,
-                                    context.Host,
-                                    true,
-                                    SecurityProtocolType.Default,
-                                    clientCertificates);
-
-                            sslStreamPriv.ClientCertSelectionDelegate =
-                                    new CertificateSelectionCallback(context.DefaultCertificateSelectionCallback);
-                            sslStreamPriv.ServerCertValidationDelegate =
-                                    new CertificateValidationCallback(context.DefaultCertificateValidationCallback);
-                            sslStreamPriv.PrivateKeyCertSelectionDelegate =
-                                    new PrivateKeySelectionCallback(context.DefaultPrivateKeySelectionCallback);
-                            sslStream = sslStreamPriv;
+                            //deactivated Mono.Security.dll code
                         }
                         else
                         {
Index: Npgsql/Npgsql/NpgsqlConnection.cs
===================================================================
--- Npgsql/Npgsql/NpgsqlConnection.cs   (Revision 1360)
+++ Npgsql/Npgsql/NpgsqlConnection.cs   (Arbeitskopie)
@@ -36,7 +36,6 @@
 using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
 using System.Transactions;
-using Mono.Security.Protocol.Tls;
 using IsolationLevel = System.Data.IsolationLevel;

 #if WITHDESIGN
@@ -98,30 +97,6 @@
         internal ProvideClientCertificatesCallback ProvideClientCertificatesCallbackDelegate;

         /// <summary>
-        /// Mono.Security.Protocol.Tls.CertificateSelectionCallback delegate.
-        /// </summary>
-        [Obsolete("CertificateSelectionCallback, CertificateValidationCallback and PrivateKeySelectionCallback have been replaced with ValidateRemoteCertificateCallback.")]
-        public event CertificateSelectionCallback CertificateSelectionCallback;
-
-        internal CertificateSelectionCallback CertificateSelectionCallbackDelegate;
-
-        /// <summary>
-        /// Mono.Security.Protocol.Tls.CertificateValidationCallback delegate.
-        /// </summary>
-        [Obsolete("CertificateSelectionCallback, CertificateValidationCallback and PrivateKeySelectionCallback have been replaced with ValidateRemoteCertificateCallback.")]
-        public event CertificateValidationCallback CertificateValidationCallback;
-
-        internal CertificateValidationCallback CertificateValidationCallbackDelegate;
-
-        /// <summary>
-        /// Mono.Security.Protocol.Tls.PrivateKeySelectionCallback delegate.
-        /// </summary>
-        [Obsolete("CertificateSelectionCallback, CertificateValidationCallback and PrivateKeySelectionCallback have been replaced with ValidateRemoteCertificateCallback.")]
-        public event PrivateKeySelectionCallback PrivateKeySelectionCallback;
-
-        internal PrivateKeySelectionCallback PrivateKeySelectionCallbackDelegate;
-
-        /// <summary>
         /// Called to validate server's certificate during SSL handshake
         /// </summary>
         public event ValidateRemoteCertificateCallback ValidateRemoteCertificateCallback;
@@ -164,9 +139,6 @@
             NotificationDelegate = new NotificationEventHandler(OnNotification);

             ProvideClientCertificatesCallbackDelegate = new ProvideClientCertificatesCallback(DefaultProvideClientCertificatesCallback);
-            CertificateValidationCallbackDelegate = new CertificateValidationCallback(DefaultCertificateValidationCallback);
-            CertificateSelectionCallbackDelegate = new CertificateSelectionCallback(DefaultCertificateSelectionCallback);
-            PrivateKeySelectionCallbackDelegate = new PrivateKeySelectionCallback(DefaultPrivateKeySelectionCallback);
             ValidateRemoteCertificateCallbackDelegate = new ValidateRemoteCertificateCallback(DefaultValidateRemoteCertificateCallback);

             // Fix authentication problems. See https://bugzilla.novell.com/show_bug.cgi?id=MONO77559 and
@@ -643,9 +615,6 @@
                 connector = new NpgsqlConnector(this);

                 connector.ProvideClientCertificatesCallback += ProvideClientCertificatesCallbackDelegate;
-                connector.CertificateSelectionCallback += CertificateSelectionCallbackDelegate;
-                connector.CertificateValidationCallback += CertificateValidationCallbackDelegate;
-                connector.PrivateKeySelectionCallback += PrivateKeySelectionCallbackDelegate;
                 connector.ValidateRemoteCertificateCallback += ValidateRemoteCertificateCallbackDelegate;

                 connector.Open();
@@ -749,9 +718,6 @@
             else
             {
                 Connector.ProvideClientCertificatesCallback -= ProvideClientCertificatesCallbackDelegate;
-                Connector.CertificateSelectionCallback -= CertificateSelectionCallbackDelegate;
-                Connector.CertificateValidationCallback -= CertificateValidationCallbackDelegate;
-                Connector.PrivateKeySelectionCallback -= PrivateKeySelectionCallbackDelegate;
                 Connector.ValidateRemoteCertificateCallback -= ValidateRemoteCertificateCallbackDelegate;

                 if (Connector.Transaction != null)
@@ -963,54 +929,8 @@
         // Event handlers
         //

+      
         /// <summary>
-        /// Default SSL CertificateSelectionCallback implementation.
-        /// </summary>
-        internal X509Certificate DefaultCertificateSelectionCallback(X509CertificateCollection clientCertificates,
-                                                                     X509Certificate serverCertificate, string targetHost,
-                                                                     X509CertificateCollection serverRequestedCertificates)
-        {
-            if (CertificateSelectionCallback != null)
-            {
-                return CertificateSelectionCallback(clientCertificates, serverCertificate, targetHost, serverRequestedCertificates);
-            }
-            else
-            {
-                return null;
-            }
-        }
-
-        /// <summary>
-        /// Default SSL CertificateValidationCallback implementation.
-        /// </summary>
-        internal bool DefaultCertificateValidationCallback(X509Certificate certificate, int[] certificateErrors)
-        {
-            if (CertificateValidationCallback != null)
-            {
-                return CertificateValidationCallback(certificate, certificateErrors);
-            }
-            else
-            {
-                return true;
-            }
-        }
-
-        /// <summary>
-        /// Default SSL PrivateKeySelectionCallback implementation.
-        /// </summary>
-        internal AsymmetricAlgorithm DefaultPrivateKeySelectionCallback(X509Certificate certificate, string targetHost)
-        {
-            if (PrivateKeySelectionCallback != null)
-            {
-                return PrivateKeySelectionCallback(certificate, targetHost);
-            }
-            else
-            {
-                return null;
-            }
-        }
-
-        /// <summary>
         /// Default SSL ProvideClientCertificatesCallback implementation.
         /// </summary>
         internal void DefaultProvideClientCertificatesCallback(X509CertificateCollection certificates)
Index: Npgsql/Npgsql/NpgsqlConnector.cs
===================================================================
--- Npgsql/Npgsql/NpgsqlConnector.cs    (Revision 1360)
+++ Npgsql/Npgsql/NpgsqlConnector.cs    (Arbeitskopie)
@@ -36,7 +36,6 @@
 using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
 using System.Threading;
-using Mono.Security.Protocol.Tls;
 using NpgsqlTypes;
 using System.Text;

@@ -83,21 +82,6 @@
         internal event ProvideClientCertificatesCallback ProvideClientCertificatesCallback;

         /// <summary>
-        /// Mono.Security.Protocol.Tls.CertificateSelectionCallback delegate.
-        /// </summary>
-        internal event CertificateSelectionCallback CertificateSelectionCallback;
-
-        /// <summary>
-        /// Mono.Security.Protocol.Tls.CertificateValidationCallback delegate.
-        /// </summary>
-        internal event CertificateValidationCallback CertificateValidationCallback;
-
-        /// <summary>
-        /// Mono.Security.Protocol.Tls.PrivateKeySelectionCallback delegate.
-        /// </summary>
-        internal event PrivateKeySelectionCallback PrivateKeySelectionCallback;
-
-        /// <summary>
         /// Called to validate server's certificate during SSL handshake
         /// </summary>
         internal event ValidateRemoteCertificateCallback ValidateRemoteCertificateCallback;
@@ -505,52 +489,11 @@
             }
         }

-        /// <summary>
-        /// Default SSL CertificateSelectionCallback implementation.
-        /// </summary>
-        internal X509Certificate DefaultCertificateSelectionCallback(X509CertificateCollection clientCertificates,
-                                                                     X509Certificate serverCertificate, string targetHost,
-                                                                     X509CertificateCollection serverRequestedCertificates)
-        {
-            if (CertificateSelectionCallback != null)
-            {
-                return CertificateSelectionCallback(clientCertificates, serverCertificate, targetHost, serverRequestedCertificates);
-            }
-            else
-            {
-                return null;
-            }
-        }
+       

-        /// <summary>
-        /// Default SSL CertificateValidationCallback implementation.
-        /// </summary>
-        internal bool DefaultCertificateValidationCallback(X509Certificate certificate, int[] certificateErrors)
-        {
-            if (CertificateValidationCallback != null)
-            {
-                return CertificateValidationCallback(certificate, certificateErrors);
-            }
-            else
-            {
-                return true;
-            }
-        }
+     

-        /// <summary>
-        /// Default SSL PrivateKeySelectionCallback implementation.
-        /// </summary>
-        internal AsymmetricAlgorithm DefaultPrivateKeySelectionCallback(X509Certificate certificate, string targetHost)
-        {
-            if (PrivateKeySelectionCallback != null)
-            {
-                return PrivateKeySelectionCallback(certificate, targetHost);
-            }
-            else
-            {
-                return null;
-            }
-        }
+      

         /// <summary>
         /// Default SSL ProvideClientCertificatesCallback implementation.
Index: Npgsql/Npgsql/NpgsqlConnectorPool.cs
===================================================================
--- Npgsql/Npgsql/NpgsqlConnectorPool.cs    (Revision 1360)
+++ Npgsql/Npgsql/NpgsqlConnectorPool.cs    (Arbeitskopie)
@@ -372,9 +372,6 @@
             {

                 Connector.ProvideClientCertificatesCallback += Connection.ProvideClientCertificatesCallbackDelegate;
-                Connector.CertificateSelectionCallback += Connection.CertificateSelectionCallbackDelegate;
-                Connector.CertificateValidationCallback += Connection.CertificateValidationCallbackDelegate;
-                Connector.PrivateKeySelectionCallback += Connection.PrivateKeySelectionCallbackDelegate;
                 Connector.ValidateRemoteCertificateCallback += Connection.ValidateRemoteCertificateCallbackDelegate;

                 try
@@ -405,17 +402,11 @@
                             NpgsqlConnector Spare = new NpgsqlConnector(Connection);

                             Spare.ProvideClientCertificatesCallback += Connection.ProvideClientCertificatesCallbackDelegate;
-                            Spare.CertificateSelectionCallback += Connection.CertificateSelectionCallbackDelegate;
-                            Spare.CertificateValidationCallback += Connection.CertificateValidationCallbackDelegate;
-                            Spare.PrivateKeySelectionCallback += Connection.PrivateKeySelectionCallbackDelegate;
                             Spare.ValidateRemoteCertificateCallback += Connection.ValidateRemoteCertificateCallbackDelegate;

                             Spare.Open();

                             Spare.ProvideClientCertificatesCallback -= Connection.ProvideClientCertificatesCallbackDelegate;
-                            Spare.CertificateSelectionCallback -= Connection.CertificateSelectionCallbackDelegate;
-                            Spare.CertificateValidationCallback -= Connection.CertificateValidationCallbackDelegate;
-                            Spare.PrivateKeySelectionCallback -= Connection.PrivateKeySelectionCallbackDelegate;
                             Spare.ValidateRemoteCertificateCallback -= Connection.ValidateRemoteCertificateCallbackDelegate;

                             Queue.Available.Enqueue(Spare);
@@ -463,9 +454,6 @@
             }

             Connector.ProvideClientCertificatesCallback -= Connection.ProvideClientCertificatesCallbackDelegate;
-            Connector.CertificateSelectionCallback -= Connection.CertificateSelectionCallbackDelegate;
-            Connector.CertificateValidationCallback -= Connection.CertificateValidationCallbackDelegate;
-            Connector.PrivateKeySelectionCallback -= Connection.PrivateKeySelectionCallbackDelegate;
             Connector.ValidateRemoteCertificateCallback -= Connection.ValidateRemoteCertificateCallbackDelegate;

             /*bool inQueue = false;
@roji
Npgsql member

@jochenwezel, thanks for the contribution.

Npgsql 2.2 (just released) uses SSLStream rather than Mono.Security by default, so it is in effect a way of testing whether Mono.Security is required. We hope that we get no negative feedback about it, which would mean it's safe to remove Mono.Security altogether in 3.0.

In the meantime it's also important to start writing tests for SSL (#291), which currently aren't covered at all in our suite.

In any case, any chance you can submit your modification as a github pull request for review/testing?

@jochenwezel

I've created the provided patch to drop the Mono.Security.dll from my projects. Especially when distributing those projects which reference a GAC lib like Mono.Security, it is always a problem if you haven't manually inserted it into the own project. Setup (e.g. setup of Click Once applications) would search for it, don't find it and stop installing.

Currently, I'm not familiar with github, branches, etc. Maybe it's pretty simple but I haven't got the time to
get me into a souverain position.

@roji
Npgsql member

We have a report that Mono.Security does not run well under medium trust: https://groups.google.com/forum/#!msg/npgsql-help/6DwOZDR7Jag/_rI7Jh9uj4UJ

@roji roji added refactor and removed research labels Mar 12, 2015
@roji roji changed the title from Consider getting rid of Mono.Security to Drop Mono.Security Mar 12, 2015
@roji
Npgsql member

Done by @Emill as part of #532

@roji roji closed this Mar 23, 2015
@roji roji modified the milestone: 3.0, 3.0-beta1 Mar 30, 2015
@roji roji added a commit that referenced this issue Apr 5, 2015
@roji roji Get rid of Mono.Security
Relates to #127
3d85c58
@roji roji modified the milestone: 3.0-beta1, 3.0 Aug 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment