The committers listed above are authorized under a signed CLA.

• ✅ login: Kimahriman / name: Adam Binford (a7e84c4, 363e21b, 0bf8e76, 3643efa)

@gtcooke94 , Please take a look at this PR

Deferring the review to @gtcooke94.

We need to think about this more. This represents a significant behavior change to the C code that I don't think we can introduce without breaking semantic versioning. I also don't think we should introduce this change at all (it's intentional that a user setting certificate overrides takes precedence over the default system root cert).

I believe the issue here lies in how the python layering configures certificates, not in the C-Core behavior. Thus, this needs to be fixed in the Python layering. To be specific to your above comments, Python converts (5), the gRPC roots to (3), the override callback.

My very rough understanding, and let me know if any of this is incorrect, is that a user setting a certificate would be through (1) or (2). Someone writing custom C could theoretically provide (3) themselves but is that a real use case? That would be the main breaking change that would require someone to explicitly disable system certs (unless they really want the bundled ones for some reason). I'm assuming the main use case for (3) is to provide the bundled certs in a language package specific way. But users writing C code would actually get the system certs by default because (3) wouldn't be set, which is actually a disconnect between C and other languages

But users writing C code would actually get the system certs by default because (3) wouldn't be set, which is actually a disconnect between C and other languages

Can you expand on what you mean here?

As for whether or not (3) is a real use case, we can't really know - such is a challenge of working on OSS. Given that this ordering change is specifically for the callback which is exposed in C-Core APIs, I think we could technically change this without breaking commitments because C-Core APIs are not considered stable. On the other hand, changing the ordering of certificate preferences, even if only accessible via a C-Core API isn't so much a breaking compiler change, but rather a significant security difference.

Adding @sergiitk to this discussion as the Python lead, and I think the bug here lies in Python's integration with C-Core, not C-Core itself.

It might be worth considering adding a new API/parameter to the existing core API to express intent - "I'm setting an environment level preference" vs "I want to override the defaults for my application".

Such a thing would likely need updating the Python sources, and Ruby/PHP/etc.

It will be a significant behavior change for Python users however, and it's unclear to me how to safely do that.

But users writing C code would actually get the system certs by default because (3) wouldn't be set, which is actually a disconnect between C and other languages

Can you expand on what you mean here?

The easiest way to explain is by using the C++ example examples/cpp/auth. I'm having a little trouble getting it to work, as it currently gives me

root@dbcaf7edf57c:/data/examples/cpp/auth# ./ssl_client 
16: Invalid signature
Greeter received: RPC failed

during a normal run.

But if I do the same thing as the Python example and remove specifying a CA in the channel credentials on the client side:

  grpc::SslCredentialsOptions ssl_options;
  // ssl_options.pem_root_certs = LoadStringFromFile(kRootCertificate); <- commented this out
  // Create a channel with SSL credentials
  GreeterClient greeter(
      grpc::CreateChannel(target_str, grpc::SslCredentials(ssl_options)));

And then run as is I would get a certificate verification error as expected:

root@dbcaf7edf57c:/data/examples/cpp/auth# ./ssl_client 
14: failed to connect to all addresses; last error: UNKNOWN: ipv4:127.0.0.1:50051: Ssl handshake failed (TSI_PROTOCOL_FAILURE): SSL_ERROR_SSL: error:1000007d:SSL routines:OPENSSL_internal:CERTIFICATE_VERIFY_FAILED: unable to get local issuer certificate
Greeter received: RPC failed

But if I then load the root.ca into the system certs, it is able to successfully verify the cert (but I still get the Invalid signature error, not sure if that's an issue with my setup or something wrong with that example now)

root@dbcaf7edf57c:/data/examples/cpp/auth# cp credentials/root.crt /usr/local/share/ca-certificates/
root@dbcaf7edf57c:/data/examples/cpp/auth# update-ca-certificates 
Updating certificates in /etc/ssl/certs...
rehash: warning: skipping ca-certificates.crt,it does not contain exactly one certificate or CRL
rehash: warning: skipping duplicate certificate in root.pem
1 added, 0 removed; done.
Running hooks in /etc/ca-certificates/update.d...
Processing triggers for ca-certificates-java (20240118) ...
Replacing debian:root.pem
done.
done.
root@dbcaf7edf57c:/data/examples/cpp/auth# ./ssl_client 
16: Invalid signature
Greeter received: RPC failed

So for some client languages, the system CAs are used by default, and for some the gRPC bundled CAs are used by default. Based on https://github.com/search?q=repo%3Agrpc%2Fgrpc%20grpc_set_ssl_roots_override_callback&type=code it looks like Python, Ruby, and PHP will use the gRPC bundled CAs, where clients like C++, Objective C, and maybe Node would use the system CAs by default.

I think we are on the same page - the existing ordering behavior (that we must keep) is that system roots are preferred over gRPC roots, and both are below the callback in preference order. It is a bug in the Python, Ruby, and PHP wrappers that the gRPC roots are preferred over system roots by setting the callback as the gRPC roots.

I think we are on the same page - the existing ordering behavior (that we must keep) is that system roots are preferred over gRPC roots, and both are below the callback in preference order. It is a bug in the Python, Ruby, and PHP wrappers that the gRPC roots are preferred over system roots by setting the callback as the gRPC roots.

Are you saying it's up to Python/Ruby/PHP to supply the system roots in the callback instead? Or that they should be using a different mechanism than the callback to supply the gRPC roots as a fallback? It seems that the callback is the method of supplying a language-specific location for the gRPC roots, so this PR would exactly fix the bug as you described it? With the only caveat being manual use of grpc_set_ssl_roots_override_callback by users in C code would break without disabling system roots.

This PR "fixes" the Python behavior by changing the Core C behavior which is what we cannot do.
The wrapping languages are incorrectly using this override by using it to provide the gRPC repo roots. While this callback is currently used to provide wrapped language roots, that is not it's only use.

I do wonder what would happen if we simply didn't override anything at all - would the C Core behavior properly fall back to the system then repo roots since Python is still just wrapping C-Core?
The python roots aren't special, they seem to be just the gRPC repo roots ref.

This PR "fixes" the Python behavior by changing the Core C behavior which is what we cannot do. The wrapping languages are incorrectly using this override by using it to provide the gRPC repo roots. While this callback is currently used to provide wrapped language roots, that is not it's only use.

I'm saying I'm guessing/assuming the whole point of the Core C behavior in question is to provide methods for wrapping languages to provide the same CAs in a package-dependent manner, so it seems odd to say that can't be changed if that is the bug. Direct use of the callback is possible and I'm kinda asking if that is a real use case anyone would actually use, and if the answer is yes there is a simple workaround when upgrading, just disable the system roots via GRPC_NOT_USE_SYSTEM_SSL_ROOTS=true, then the existing behavior would remain.

I do wonder what would happen if we simply didn't override anything at all - would the C Core behavior properly fall back to the system then repo roots since Python is still just wrapping C-Core? The python roots aren't special, they seem to be just the gRPC repo roots ref.

The problem is default C-Core system roots will likely only exist if you install grpc via a system package manager. If you simple pip install grpcio, then /usr/share/grpc/roots.pem isn't going to exist. That's why the callback exists, because you need to call Python code in order to extract the roots.pem from the installed Python package

Relevant issue with linked PRs indicating the callback was created exactly for this purpose: #4834

I do see what you're getting at and thanks for linking the original design, but I'm still very hesitant to change a behavior in a breaking way (even if it isn't intended to be used in that way). This is definitely in a murky place stability-wise since it is accessible via C-Core APIs. Pulling in @markdroth for his thoughts.

If we do decide that changing the ordering behavior is the route we want to take, in a best effort to not break anything, we'll need to put it behind a flag such that the default behavior does not change, and you can opt-in to the new ordering.

I'd still rather see this fixed at the wrapping layer if we can do that without changing the ordering in ssl_utils.cc.

If we do decide that changing the ordering behavior is the route we want to take, in a best effort to not break anything, we'll need to put it behind a flag such that the default behavior does not change, and you can opt-in to the new ordering.

The main awkward thing is there is already a flag to opt out of using system CAs. So also adding a flag to opt in to using them would be pretty confusing to a user. But it is an option.

I'd still rather see this fixed at the wrapping layer if we can do that without changing the ordering in ssl_utils.cc.

I just don't really see how this could be fixed at the wrapping layer, unless you "break" the behavior by respecting GRPC_NOT_USE_SYSTEM_SSL_ROOTS in each wrapping function and forcing Python/PHP/Ruby to provide their own way of finding the correct system CAs, and switching those languages to use the system CAs by default.

It is awkward that there is already a flag, but the new flag wouldn't specifically be opting into using system root certs at all, but rather opting into using system root certs above the certs in the callback in preference ordering. This would open up a way forward without causing a default behavior change.

To be clear, I agree with you, and I think this PR is what was intended when this override function was added, but somewhere along the way the intention got mixed up. However, I generally want to avoid a breaking security behavior change for anyone who happened to use this.

It is awkward that there is already a flag, but the new flag wouldn't specifically be opting into using system root certs at all, but rather opting into using system root certs above the certs in the callback in preference ordering. This would open up a way forward without causing a default behavior change.

Something like GRPC_PREFER_USE_SYSTEM_SSL_ROOTS? I can start looking into that unless you are waiting for other opinions.

How about GRPC_USE_SYSTEM_ROOTS_OVER_LANGUAGE_CALLBACK

I've discussed some with other contributors and we think this is the best approach to take - we can also mark the existing C API with more documentation saying what it is and basically not to use it and migrate to something else (if someone is manually setting roots via this callback, they should be able to use TlsCredentials instead as the intended way for a user to provide certificates).

Then, with enough time and notice, we can swap the ordering to what was intended (this PRs ordering).

Ok did my best to add a new config and test it