Remove grpc.internal. channel args at API sites (#27536)

* Remove grpc.internal. channel args at API sites

gRPC uses channel args both as an API surface and as an internal
implementation detail. The merits of this are debatable, but it's
probably the best mechanism we have right now, and changing it would be
an effort best not undertaken today.

In order to focus hardening efforts to the highest payoff, this change
introduces a filter to remove any internal channel args received from
outside our public API, effectively guaranteeing that any usage of these
arguments comes from within code that we maintain.

There will likely be a whack-a-mole game over the following weeks to
mark more channel arguments as internal - I have not done a thorough
audit!

* get api usage right

* fix

* fixes

* rename internal -> test_only so it passes through

src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h

@@ -24,6 +24,6 @@
 // For testing purpose, this channel arg indicating xds_cluster_resolver LB
 // policy should use the fake DNS resolver to resolve logical dns cluster.
 #define GRPC_ARG_XDS_LOGICAL_DNS_CLUSTER_FAKE_RESOLVER_RESPONSE_GENERATOR \
-  "grpc.internal.xds_logical_dns_cluster_fake_resolver_response_generator"
+  "grpc.TEST_ONLY.xds_logical_dns_cluster_fake_resolver_response_generator"
 
 #endif  // GRPC_CORE_EXT_FILTERS_CLIENT_CHANNEL_LB_POLICY_XDS_XDS_CHANNEL_ARGS_H

src/core/ext/filters/client_channel/subchannel_pool_interface.cc

@@ -24,7 +24,7 @@
 #include "src/core/lib/gpr/useful.h"
 
 // The subchannel pool to reuse subchannels.
-#define GRPC_ARG_SUBCHANNEL_POOL "grpc.subchannel_pool"
+#define GRPC_ARG_SUBCHANNEL_POOL "grpc.internal.subchannel_pool"
 // The subchannel key ID that is only used in test to make each key unique.
 #define GRPC_ARG_SUBCHANNEL_KEY_TEST_ONLY_ID "grpc.subchannel_key_test_only_id"
 

src/core/ext/transport/chttp2/client/insecure/channel_create.cc

@@ -88,6 +88,7 @@ grpc_channel* grpc_insecure_channel_create(const char* target,
                                            const grpc_channel_args* args,
                                            void* reserved) {
   grpc_core::ExecCtx exec_ctx;
+  args = grpc_channel_args_remove_grpc_internal(args);
   GRPC_API_TRACE(
       "grpc_insecure_channel_create(target=%s, args=%p, reserved=%p)", 3,
       (target, args, reserved));
@@ -103,6 +104,7 @@ grpc_channel* grpc_insecure_channel_create(const char* target,
   grpc_channel* channel = grpc_core::CreateChannel(target, new_args, &error);
   // Clean up.
   grpc_channel_args_destroy(new_args);
+  grpc_channel_args_destroy(args);
   if (channel == nullptr) {
     intptr_t integer;
     grpc_status_code status = GRPC_STATUS_INTERNAL;

src/core/ext/transport/chttp2/client/secure/secure_channel_create.cc

@@ -154,6 +154,7 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds,
       "reserved=%p)",
       4, ((void*)creds, target, (void*)args, (void*)reserved));
   GPR_ASSERT(reserved == nullptr);
+  args = grpc_channel_args_remove_grpc_internal(args);
   grpc_channel* channel = nullptr;
   grpc_error_handle error = GRPC_ERROR_NONE;
   if (creds != nullptr) {
@@ -173,6 +174,7 @@ grpc_channel* grpc_secure_channel_create(grpc_channel_credentials* creds,
     // Clean up.
     grpc_channel_args_destroy(new_args);
   }
+  grpc_channel_args_destroy(args);
   if (channel == nullptr) {
     intptr_t integer;
     grpc_status_code status = GRPC_STATUS_INTERNAL;

src/core/ext/xds/xds_server_config_fetcher.cc

@@ -524,10 +524,12 @@ grpc_server_config_fetcher* grpc_server_config_fetcher_xds_create(
     grpc_server_xds_status_notifier notifier, const grpc_channel_args* args) {
   grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
   grpc_core::ExecCtx exec_ctx;
+  args = grpc_channel_args_remove_grpc_internal(args);
   GRPC_API_TRACE("grpc_server_config_fetcher_xds_create()", 0, ());
   grpc_error_handle error = GRPC_ERROR_NONE;
   grpc_core::RefCountedPtr<grpc_core::XdsClient> xds_client =
       grpc_core::XdsClient::GetOrCreate(args, &error);
+  grpc_channel_args_destroy(args);
   if (error != GRPC_ERROR_NONE) {
     gpr_log(GPR_ERROR, "Failed to create xds client: %s",
             grpc_error_std_string(error).c_str());

src/core/lib/channel/channel_args.cc

@@ -25,6 +25,7 @@
 
 #include <vector>
 
+#include "absl/strings/match.h"
 #include "absl/strings/str_format.h"
 #include "absl/strings/str_join.h"
 
@@ -72,6 +73,22 @@ grpc_channel_args* grpc_channel_args_copy_and_remove(
                                                    num_to_remove, nullptr, 0);
 }
 
+grpc_channel_args* grpc_channel_args_remove_grpc_internal(
+    const grpc_channel_args* src) {
+  if (src == nullptr) return nullptr;
+  // Create result.
+  grpc_channel_args* dst =
+      static_cast<grpc_channel_args*>(gpr_malloc(sizeof(grpc_channel_args)));
+  dst->args =
+      static_cast<grpc_arg*>(gpr_malloc(sizeof(grpc_arg) * src->num_args));
+  dst->num_args = 0;
+  for (size_t i = 0; i < src->num_args; ++i) {
+    if (absl::StartsWith(src->args[i].key, "grpc.internal.")) continue;
+    dst->args[dst->num_args++] = copy_arg(&src->args[i]);
+  }
+  return dst;
+}
+
 static bool should_remove_arg(const grpc_arg* arg, const char** to_remove,
                               size_t num_to_remove) {
   for (size_t i = 0; i < num_to_remove; ++i) {

src/core/lib/channel/channel_args.h

@@ -41,6 +41,15 @@ grpc_channel_args* grpc_channel_args_copy_and_add(const grpc_channel_args* src,
                                                   const grpc_arg* to_add,
                                                   size_t num_to_add);
 
+/** Remove any channel args prefixed with 'grpc.internal.'
+ *  These are used for internal implementation details and are not intended to
+ *  be exposed to users.
+ *  Returns a new channel args instance.
+ *  Does not take ownership of \a src.
+ *  Should be called by any public API that receives channel args. */
+grpc_channel_args* grpc_channel_args_remove_grpc_internal(
+    const grpc_channel_args* src);
+
 /** Copies the arguments in \a src except for those whose keys are in
     \a to_remove. */
 grpc_channel_args* grpc_channel_args_copy_and_remove(

src/core/lib/channel/channelz.h

@@ -41,7 +41,7 @@
 #include "src/core/lib/json/json.h"
 
 // Channel arg key for channelz node.
-#define GRPC_ARG_CHANNELZ_CHANNEL_NODE "grpc.channelz_channel_node"
+#define GRPC_ARG_CHANNELZ_CHANNEL_NODE "grpc.internal.channelz_channel_node"
 
 // Channel arg key for indicating an internal channel.
 #define GRPC_ARG_CHANNELZ_IS_INTERNAL_CHANNEL \

src/core/lib/security/security_connector/security_connector.cc

@@ -88,11 +88,13 @@ int grpc_server_security_connector::server_security_connector_cmp(
 }
 
 static void connector_arg_destroy(void* p) {
+  if (p == nullptr) return;
   static_cast<grpc_security_connector*>(p)->Unref(DEBUG_LOCATION,
                                                   "connector_arg_destroy");
 }
 
 static void* connector_arg_copy(void* p) {
+  if (p == nullptr) return nullptr;
   return static_cast<grpc_security_connector*>(p)
       ->Ref(DEBUG_LOCATION, "connector_arg_copy")
       .release();

src/core/lib/surface/server.cc

@@ -1469,9 +1469,11 @@ void Server::CallData::StartTransportStreamOpBatch(
 
 grpc_server* grpc_server_create(const grpc_channel_args* args, void* reserved) {
   grpc_core::ExecCtx exec_ctx;
+  args = grpc_channel_args_remove_grpc_internal(args);
   GRPC_API_TRACE("grpc_server_create(%p, %p)", 2, (args, reserved));
   grpc_server* c_server = new grpc_server;
   c_server->core_server = grpc_core::MakeOrphanable<grpc_core::Server>(args);
+  grpc_channel_args_destroy(args);
   return c_server;
 }
 

test/core/end2end/fuzzers/api_fuzzer_corpus/channelz-crash

@@ -0,0 +1,19 @@
+actions {
+  create_channel {
+    target: "\001\000\000\000\323\273`*"
+    channel_args {
+      key: "grpc.channelz_channel_node"
+      resource_quota {
+      }
+    }
+    channel_args {
+      resource_quota {
+      }
+    }
+    channel_args {
+      resource_quota {
+      }
+    }
+  }
+}
+

test/core/surface/secure_channel_create_test.cc

@@ -44,10 +44,7 @@ void test_unknown_scheme_target(void) {
 }
 
 void test_security_connector_already_in_arg(void) {
-  grpc_arg arg;
-  arg.type = GRPC_ARG_POINTER;
-  arg.value.pointer.p = nullptr;
-  arg.key = const_cast<char*>(GRPC_ARG_SECURITY_CONNECTOR);
+  grpc_arg arg = grpc_security_connector_to_arg(nullptr);
   grpc_channel_args args;
   args.num_args = 1;
   args.args = &arg;