[C#] Move insecure compose validation into Channel (#31376)

* Move insecure compose validation into Channel

* try fix compilation problem

* Fix tests?

* Revert "Fix tests?"

This reverts commit 7852e996b9.

* cleanup in Channel constructor if credential creation fails

* fix throw

Co-authored-by: James Newton-King <james@newtonking.com>

* review feedback

Co-authored-by: James Newton-King <james@newtonking.com>

Co-authored-by: Jan Tattermusch <jtattermusch@google.com>

src/csharp/Grpc.Core.Api/ChannelCredentials.cs

@@ -89,6 +89,11 @@ namespace Grpc.Core
         /// <summary>
         /// Returns <c>true</c> if this credential type allows being composed by <c>CompositeCredentials</c>.
         /// </summary>
+        /// <remark>
+        /// Note: No longer used. Decision on whether composition is allowed now happens in
+        /// <see cref="ChannelCredentialsConfiguratorBase.SetCompositeCredentials(object, ChannelCredentials, CallCredentials)"/>.
+        /// Internal property left for safety because Grpc.Core has internal access to Grpc.Core.Api.
+        /// </remark>
         internal virtual bool IsComposable => false;
 
         private sealed class InsecureCredentials : ChannelCredentials
@@ -118,11 +123,6 @@ namespace Grpc.Core
             {
                 this.channelCredentials = GrpcPreconditions.CheckNotNull(channelCredentials);
                 this.callCredentials = GrpcPreconditions.CheckNotNull(callCredentials);
-
-                if (!channelCredentials.IsComposable)
-                {
-                    throw new ArgumentException(string.Format("CallCredentials can't be composed with {0}. CallCredentials must be used with secure channel credentials like SslCredentials.", channelCredentials.GetType().Name));
-                }
             }
 
             public override void InternalPopulateConfiguration(ChannelCredentialsConfiguratorBase configurator, object state)

src/csharp/Grpc.Core.Tests/ChannelCredentialsTest.cs

@@ -24,18 +24,6 @@ namespace Grpc.Core.Tests
 {
     public class ChannelCredentialsTest
     {
-        [Test]
-        public void InsecureCredentials_IsNonComposable()
-        {
-            Assert.IsFalse(ChannelCredentials.Insecure.IsComposable);
-        }
-
-        [Test]
-        public void SecureCredentials_IsComposable()
-        {
-            Assert.IsTrue(ChannelCredentials.SecureSsl.IsComposable);
-        }
-
         [Test]
         public void ChannelCredentials_CreateComposite()
         {
@@ -44,10 +32,6 @@ namespace Grpc.Core.Tests
 
             Assert.Throws(typeof(ArgumentNullException), () => ChannelCredentials.Create(null, new FakeCallCredentials()));
             Assert.Throws(typeof(ArgumentNullException), () => ChannelCredentials.Create(new FakeChannelCredentials(true), null));
-
-            // forbid composing non-composable
-            var ex = Assert.Throws(typeof(ArgumentException), () => ChannelCredentials.Create(new FakeChannelCredentials(false), new FakeCallCredentials()));
-            Assert.AreEqual("CallCredentials can't be composed with FakeChannelCredentials. CallCredentials must be used with secure channel credentials like SslCredentials.", ex.Message);
         }
 
         [Test]
@@ -63,5 +47,16 @@ namespace Grpc.Core.Tests
             var nativeCreds4 = ChannelCredentials.SecureSsl.ToNativeCredentials();
             Assert.AreSame(nativeCreds3, nativeCreds4);
         }
+
+        [Test]
+        public void ChannelCredentials_MalformedSslCredentialsCanStillCreateNativeCredentials()
+        {
+            // pass malformed root pem certs, but creation of native credentials still passes,
+            // since the credentials are parsed lazily by the C core.
+            using (var nativeCreds = new SslCredentials("MALFORMED_ROOT_CERTS_THAT_WILL_THROW_WHEN_CREATING_NATIVE_CREDENTIALS").ToNativeCredentials())
+            {
+                Assert.IsFalse(nativeCreds.IsInvalid);
+            }
+        }
     }
 }

src/csharp/Grpc.Core.Tests/ChannelTest.cs

@@ -123,5 +123,16 @@ namespace Grpc.Core.Tests
             // check that Channel.ShutdownAsync has run
             Assert.AreEqual(ChannelState.Shutdown, channel.State);
         }
+
+        [Test]
+        public void CompositeCredentialsWithInsecureThrow()
+        {
+            var compositeCredentials = ChannelCredentials.Create(
+                ChannelCredentials.Insecure,
+                CallCredentials.FromInterceptor((context, metadata) => TaskUtils.CompletedTask));
+            var ex = Assert.Throws(typeof(InvalidOperationException), () => new Channel("localhost", compositeCredentials));
+            Assert.AreEqual("CallCredentials can't be composed with InsecureCredentials. " +
+                "CallCredentials must be used with secure channel credentials like SslCredentials.", ex.Message);
+        }
     }
 }

src/csharp/Grpc.Core/Channel.cs

@@ -69,20 +69,29 @@ namespace Grpc.Core
             EnsureUserAgentChannelOption(this.options);
             this.environment = GrpcEnvironment.AddRef();
 
-            this.completionQueue = this.environment.PickCompletionQueue();
-            using (var nativeChannelArgs = ChannelOptions.CreateChannelArgs(this.options.Values))
+            try
             {
-                var nativeCredentials = credentials.ToNativeCredentials();
-                if (nativeCredentials != null)
+                this.completionQueue = this.environment.PickCompletionQueue();
+                using (var nativeChannelArgs = ChannelOptions.CreateChannelArgs(this.options.Values))
                 {
-                    this.handle = ChannelSafeHandle.CreateSecure(nativeCredentials, target, nativeChannelArgs);
-                }
-                else
-                {
-                    this.handle = ChannelSafeHandle.CreateInsecure(target, nativeChannelArgs);
+                    var nativeCredentials = credentials.ToNativeCredentials();
+                    if (nativeCredentials != null)
+                    {
+                        this.handle = ChannelSafeHandle.CreateSecure(nativeCredentials, target, nativeChannelArgs);
+                    }
+                    else
+                    {
+                        this.handle = ChannelSafeHandle.CreateInsecure(target, nativeChannelArgs);
+                    }
                 }
+                GrpcEnvironment.RegisterChannel(this);
+            }
+            catch (Exception)
+            {
+                // Constructor can't be async.
+                _ = Task.Run(async () => await GrpcEnvironment.ReleaseAsync().ConfigureAwait(false));
+                throw;
             }
-            GrpcEnvironment.RegisterChannel(this);
         }
 
         /// <summary>

src/csharp/Grpc.Core/Internal/DefaultChannelCredentialsConfigurator.cs

@@ -82,7 +82,15 @@ namespace Grpc.Core.Internal
         {
             using (var callCreds = callCredentials.ToNativeCredentials())
             {
-                var nativeComposite = ChannelCredentialsSafeHandle.CreateComposite(channelCredentials.ToNativeCredentials(), callCreds);
+                var nativeChannelCredentials = channelCredentials.ToNativeCredentials();
+                // nativeChannelCredentials == null means insecure channel credentials were used.
+                if (nativeChannelCredentials == null)
+                {
+                    throw new InvalidOperationException($"CallCredentials can't be composed with {channelCredentials.GetType().Name}. " +
+                        $"CallCredentials must be used with secure channel credentials like SslCredentials.");
+                }
+                
+                var nativeComposite = ChannelCredentialsSafeHandle.CreateComposite(nativeChannelCredentials, callCreds);
                 if (nativeComposite.IsInvalid)
                 {
                     throw new ArgumentException("Error creating native composite credentials. Likely, this is because you are trying to compose incompatible credentials.");