Make ChannelCredentials.Create work with allowing insecure credentials

[JamesNK]

Fixes #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?

[JamesNK]

@apolcyn Thoughts?

[JamesNK]

@apolcyn ping. What do you think about this?

[apolcyn]

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?

[JamesNK]

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?

[apolcyn]

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.

[JamesNK]

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.

[vahpetr]

This fix need for workaround for macos. Please merge!

[jtattermusch]

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

[jtattermusch]

LGTM, but let's wait until test failures in grpc/grpc#31376 are resolved and the PR is merged.

I'd also like @captainsafia to approve.

[captainsafia]

Is it meaningful to add an assertion that uses the constructed invoker to issue a call and checks the token?

[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.

[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.

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.

[MeysamMoghaddam]

@JamesNK Not applied in version 2.51.0?

[bugproof]

💀 can't test my app on local network without messing with certificates

[danielwinkler]

@JamesNK sorry to bug you, but is there a chance to create a release with this feature soonish?

[JamesNK]

This is in 2.52.0-pre1, which is on NuGet now. Non-preview release will be in a week, or maybe two.

[danielwinkler]

Thanks for the info @JamesNK !