diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index fcc5d4f4ff6..0edcea8d7ad 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -83,6 +83,7 @@ #include "src/core/lib/load_balancing/subchannel_interface.h" #include "src/core/lib/resolver/resolver_registry.h" #include "src/core/lib/resolver/server_address.h" +#include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/service_config/service_config_call_data.h" #include "src/core/lib/service_config/service_config_impl.h" #include "src/core/lib/slice/slice.h" @@ -976,6 +977,16 @@ class ClientChannel::ClientChannelControlHelper return chand_->default_authority_; } + RefCountedPtr GetChannelCredentials() override { + return chand_->channel_args_.GetObject() + ->duplicate_without_call_credentials(); + } + + RefCountedPtr GetUnsafeChannelCredentials() + override { + return chand_->channel_args_.GetObject()->Ref(); + } + grpc_event_engine::experimental::EventEngine* GetEventEngine() override { return chand_->owning_stack_->EventEngine(); } diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index feb78ed34fa..06b5823307f 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1409,21 +1409,11 @@ ChannelArgs BuildBalancerChannelArgs( // credentials. .Remove(GRPC_ARG_CHANNEL_CREDENTIALS); } - // Create channel args for channel credentials that does not contain bearer - // token credentials. - auto* channel_credentials = args.GetObject(); - GPR_ASSERT(channel_credentials != nullptr); - RefCountedPtr creds_sans_call_creds = - channel_credentials->duplicate_without_call_credentials(); - GPR_ASSERT(creds_sans_call_creds != nullptr); return grpclb_channel_args // A channel arg indicating the target is a grpclb load balancer. .Set(GRPC_ARG_ADDRESS_IS_GRPCLB_LOAD_BALANCER, 1) // Tells channelz that this is an internal channel. .Set(GRPC_ARG_CHANNELZ_IS_INTERNAL_CHANNEL, 1) - // A channel args for new channel credentials that does not contain bearer - // tokens. - .SetObject(creds_sans_call_creds) // The fake resolver response generator, which we use to inject // address updates into the LB channel. .SetObject(response_generator->Ref()); @@ -1597,17 +1587,17 @@ absl::Status GrpcLb::UpdateBalancerChannelLocked(const ChannelArgs& args) { if (balancer_addresses.empty()) { status = absl::UnavailableError("balancer address list must be non-empty"); } + // Create channel credentials that do not contain call credentials. + auto channel_credentials = channel_control_helper()->GetChannelCredentials(); // Construct args for balancer channel. ChannelArgs lb_channel_args = BuildBalancerChannelArgs(response_generator_.get(), args); // Create balancer channel if needed. if (lb_channel_ == nullptr) { std::string uri_str = absl::StrCat("fake:///", server_name_); - auto* creds = lb_channel_args.GetObject(); - GPR_ASSERT(creds != nullptr); - lb_channel_ = grpc_channel_create( - uri_str.c_str(), creds, - lb_channel_args.Remove(GRPC_ARG_CHANNEL_CREDENTIALS).ToC().get()); + lb_channel_ = + grpc_channel_create(uri_str.c_str(), channel_credentials.get(), + lb_channel_args.ToC().get()); GPR_ASSERT(lb_channel_ != nullptr); // Set up channelz linkage. channelz::ChannelNode* child_channelz_node = @@ -1623,7 +1613,9 @@ absl::Status GrpcLb::UpdateBalancerChannelLocked(const ChannelArgs& args) { // resolver. Resolver::Result result; result.addresses = std::move(balancer_addresses); - result.args = lb_channel_args; + // Pass channel creds via channel args, since the fake resolver won't + // do this automatically. + result.args = lb_channel_args.SetObject(std::move(channel_credentials)); response_generator_->SetResponse(std::move(result)); // Return status. return status; diff --git a/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc b/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc index 03146e7da6f..3b1927d6d0c 100644 --- a/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc +++ b/src/core/ext/filters/client_channel/lb_policy/rls/rls.cc @@ -1516,11 +1516,13 @@ RlsLb::RlsChannel::RlsChannel(RefCountedPtr lb_policy) GRPC_TRACE_FLAG_ENABLED(grpc_lb_rls_trace) ? "RlsChannel" : nullptr), lb_policy_(std::move(lb_policy)) { // Get channel creds from parent channel. - // TODO(roth): Once we eliminate insecure builds, get this via a - // method on the helper instead of digging through channel args. - auto* creds = lb_policy_->channel_args_.GetObject(); + // Note that we are using the "unsafe" channel creds here, which do + // include any associated call creds. This is safe in this case, + // because we are using the parent channel's authority on the RLS channel. + auto creds = + lb_policy_->channel_control_helper()->GetUnsafeChannelCredentials(); // Use the parent channel's authority. - std::string authority(lb_policy_->channel_control_helper()->GetAuthority()); + auto authority = lb_policy_->channel_control_helper()->GetAuthority(); ChannelArgs args = ChannelArgs() .Set(GRPC_ARG_DEFAULT_AUTHORITY, authority) .Set(GRPC_ARG_CHANNELZ_IS_INTERNAL_CHANNEL, 1); @@ -1543,7 +1545,7 @@ RlsLb::RlsChannel::RlsChannel(RefCountedPtr lb_policy) .Set(GRPC_ARG_SERVICE_CONFIG_DISABLE_RESOLUTION, 1); } channel_ = grpc_channel_create(lb_policy_->config_->lookup_service().c_str(), - creds, args.ToC().get()); + creds.get(), args.ToC().get()); if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_rls_trace)) { gpr_log(GPR_INFO, "[rlslb %p] RlsChannel=%p: created channel %p for %s", lb_policy_.get(), this, channel_, diff --git a/src/core/lib/load_balancing/delegating_helper.h b/src/core/lib/load_balancing/delegating_helper.h index 9fbabc03c86..072a4ddda7d 100644 --- a/src/core/lib/load_balancing/delegating_helper.h +++ b/src/core/lib/load_balancing/delegating_helper.h @@ -25,6 +25,7 @@ #include "absl/strings/string_view.h" #include +#include #include #include "src/core/lib/channel/channel_args.h" @@ -33,6 +34,7 @@ #include "src/core/lib/load_balancing/lb_policy.h" #include "src/core/lib/load_balancing/subchannel_interface.h" #include "src/core/lib/resolver/server_address.h" +#include "src/core/lib/security/credentials/credentials.h" namespace grpc_core { @@ -59,6 +61,15 @@ class LoadBalancingPolicy::DelegatingChannelControlHelper return parent_helper()->GetAuthority(); } + RefCountedPtr GetChannelCredentials() override { + return parent_helper()->GetChannelCredentials(); + } + + RefCountedPtr GetUnsafeChannelCredentials() + override { + return parent_helper()->GetUnsafeChannelCredentials(); + } + grpc_event_engine::experimental::EventEngine* GetEventEngine() override { return parent_helper()->GetEventEngine(); } diff --git a/src/core/lib/load_balancing/lb_policy.h b/src/core/lib/load_balancing/lb_policy.h index 90e326e208e..e7c43a7bc7b 100644 --- a/src/core/lib/load_balancing/lb_policy.h +++ b/src/core/lib/load_balancing/lb_policy.h @@ -35,6 +35,7 @@ #include "absl/types/variant.h" #include +#include #include #include "src/core/ext/filters/client_channel/lb_policy/backend_metric_data.h" @@ -297,6 +298,20 @@ class LoadBalancingPolicy : public InternallyRefCounted { /// Returns the channel authority. virtual absl::string_view GetAuthority() = 0; + /// Returns the channel credentials from the parent channel. This can + /// be used to create a control-plane channel inside an LB policy. + virtual RefCountedPtr GetChannelCredentials() = 0; + + /// Returns the UNSAFE ChannelCredentials used to construct the channel, + /// including bearer tokens. LB policies should generally have no use for + /// these credentials, and use of them is heavily discouraged. These must + /// be used VERY carefully to avoid sending bearer tokens to untrusted + /// servers, as the server could then impersonate the client. Generally, + /// it is safe to use these credentials only when communicating with the + /// backends. + virtual RefCountedPtr + GetUnsafeChannelCredentials() = 0; + /// Returns the EventEngine to use for timers and async work. virtual grpc_event_engine::experimental::EventEngine* GetEventEngine() = 0; diff --git a/test/core/client_channel/lb_policy/lb_policy_test_lib.h b/test/core/client_channel/lb_policy/lb_policy_test_lib.h index eecc3874ccb..95af80f806d 100644 --- a/test/core/client_channel/lb_policy/lb_policy_test_lib.h +++ b/test/core/client_channel/lb_policy/lb_policy_test_lib.h @@ -80,6 +80,7 @@ #include "src/core/lib/load_balancing/lb_policy_registry.h" #include "src/core/lib/load_balancing/subchannel_interface.h" #include "src/core/lib/resolver/server_address.h" +#include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/service_config/service_config_call_data.h" #include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/uri/uri_parser.h" @@ -439,6 +440,15 @@ class LoadBalancingPolicyTest : public ::testing::Test { absl::string_view GetAuthority() override { return "server.example.com"; } + RefCountedPtr GetChannelCredentials() override { + return nullptr; + } + + RefCountedPtr GetUnsafeChannelCredentials() + override { + return nullptr; + } + grpc_event_engine::experimental::EventEngine* GetEventEngine() override { return event_engine_.get(); }