grpc-dotnet Make ChannelCredentials.Create work with allowing insecure credentials

Fixes https://github.com/grpc/grpc-dotnet/issues/1800

Note that this change means Grpc.Core using a newer version of Grpc.Core.Api no longer throws an error when calling ChannelCredentials.Create(ChannelCredentials.Insecure, callCredentials). That could be remedied by updating Grpc.Core to throw an error inside the configurator like Grpc.Net.Client does, but Grpc.Core is in maintenance. IMO not worth it.

I think this is safe to change because new behavior goes from error to non-error, and I don't see a situation where anyone would ever depend on the error.

@jtattermusch Thoughts?

Jul 01 '22 08:07 JamesNK

@apolcyn Thoughts?

Jul 12 '22 05:07 JamesNK

@apolcyn ping. What do you think about this?

Jul 14 '22 23:07 JamesNK

In the core library, when someone composes a call credentials with an insecure channel creds, the call creds are just silently dropped.

The motivation for that was security: call creds typically add sensitive headers. So they wanted to make it hard for people to accidentally send them on unencrypted connections.

Personally, I think throwing an error achieves the same original goal in a more user friendly way. So the original behavior actually seems right to me.

But I'm saying all this without full background here - have we considered that security motivation here?

Jul 15 '22 00:07 apolcyn

The goal is to not throw an error if the unsecured channel is configured to allow sending call credentials.

GrpcChannel has an UnsafeUseInsecureChannelCallCredentials setting (false by default). Validation of whether a call credential is allowed is moved into the channel so it can decide whether to throw an error. Is that ok with the current Grpc.Core code?

Jul 15 '22 00:07 JamesNK

Even though it's not really breaking anything for Grpc.Core (since the headers still won't go out), it's a regression to go from obvious error to silent error (not sending headers).

Also, I'm kind of assuming that this will simply cause headers to be silently dropped in Grpc.Core - I think it wouldn't be out of the question if this caused Grpc.Core to crash when trying to compose insecure with call creds (Grpc.Core has special handling for insecure channel creds by representing them with null in C#).

If we were to go with this, I'd probably be on the side of changing the configurator in Grpc.Core, too.

Jul 19 '22 22:07 apolcyn

As far as I know Grpc.Core isn't being updated except for security issues.

I don't think we should block improving grpc-dotnet for Grpc.Core in this case as the only change is not throwing an exception. And there is already the silent behavior if credentials are specified for a gRPC call using CallOptions.CallCredentials.

Jul 21 '22 08:07 JamesNK

This fix need for workaround for macos. Please merge!

Jul 28 '22 08:07 vahpetr

I think this PR has potential to break things for the Grpc.Core when the new modified Grpc.Core.Api is used (and that's something we really don't want), so I think we'll need to do some extra verification to avoid potential problems. What I think we can do:

mirror all the changes to Grpc.Core.Api from this PR and rebase them on top of the Grpc.Core implementation in the 1.46.x branch of grpc/grpc (https://github.com/grpc/grpc/tree/v1.46.x/src/csharp).
add extra tests that verify that Grpc.Core still behaves ok with the Grpc.Core.Api changes in (without testing the changes out against Grpc.Core, we're just theorizing about the impact here).
we can run the tests as a draft PR against the v1.46.x branch of grpc/grpc.

If things are still behaving fine when tested against Grpc.Core, we can consider this change (even though I'd really like to see a solution that doesn't involve modifying Grpc.Core.Api behavior - I'll try to think of one).

Also, just FTR, C core has special kind of credentials that are "Insecure", but allow the metadata call credentials to be used with them. We call them "local" credentials: https://github.com/grpc/grpc/blob/2badafbc4d246d5165546a61eeb36aa017987d30/include/grpc/grpc_security.h#L689 Unfortunately they were never added to Grpc.Core: https://github.com/grpc/grpc/pull/19287/files

Aug 18 '22 13:08 jtattermusch

@jtattermusch When is Grpc.Core going to get a new release with the changes there? We should make sure the change in Grpc.Core.Api isn't publically available before Grpc.Core is.

I don't think there is urgency to merge this PR. No need to create a Grpc.Core release just for this.

Oct 25 '22 07:10 JamesNK

@jtattermusch When is Grpc.Core going to get a new release with the changes there? We should make sure the change in Grpc.Core.Api isn't publically available before Grpc.Core is.

I don't think there is urgency to merge this PR. No need to create a Grpc.Core release just for this.

We are not planning a Grpc.Core patch release from the 1.46.x branch at the moment, but if we don't happen to need one within the next month or so, I can make one.

Oct 25 '22 12:10 jtattermusch

@JamesNK Not applied in version 2.51.0?