From d043fa06bcc896a12b4563bbf61fe99fdd0bb9d4 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 23 Mar 2018 13:58:27 -0700 Subject: [PATCH] PR comments --- .../security_connector/security_connector.cc | 19 +++++++++----- src/core/lib/surface/channel.cc | 5 ++-- test/core/end2end/fixtures/h2_fakesec.cc | 1 - test/core/end2end/tests/default_host.cc | 26 +++++++++---------- test/cpp/end2end/end2end_test.cc | 2 +- test/cpp/end2end/shutdown_test.cc | 1 - 6 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/core/lib/security/security_connector/security_connector.cc b/src/core/lib/security/security_connector/security_connector.cc index 8040ec99c44..a0e05a3fbd6 100644 --- a/src/core/lib/security/security_connector/security_connector.cc +++ b/src/core/lib/security/security_connector/security_connector.cc @@ -474,14 +474,21 @@ static bool fake_channel_check_call_host(grpc_channel_security_connector* sc, gpr_split_host_port(host, &authority_hostname, &authority_ignored_port); gpr_split_host_port(c->target, &target_hostname, &target_ignored_port); if (c->ssl_target_name_override != nullptr) { - abort(); - if (strcmp(authority_hostname, target_hostname) != 0) { + char* ssl_target_name_override_hostname = nullptr; + char* ssl_target_name_override_ignored_port = nullptr; + gpr_split_host_port(c->ssl_target_name_override, + &ssl_target_name_override_hostname, + &ssl_target_name_override_ignored_port); + if (strcmp(authority_hostname, ssl_target_name_override_hostname) != 0) { gpr_log(GPR_ERROR, "Authority (host) '%s' != SSL Target override '%s'", - host, c->ssl_target_name_override); + host, ssl_target_name_override_hostname); + abort(); } - } - if (strcmp(authority_hostname, target_hostname) != 0) { - gpr_log(GPR_ERROR, "Authority (host) '%s' != Target '%s'", host, c->target); + gpr_free(ssl_target_name_override_hostname); + gpr_free(ssl_target_name_override_ignored_port); + } else if (strcmp(authority_hostname, target_hostname) != 0) { + gpr_log(GPR_ERROR, "Authority (host) '%s' != Target '%s'", + authority_hostname, target_hostname); abort(); } gpr_free(authority_hostname); diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 1eb5ef0c0c8..807e28eef1b 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -157,8 +157,7 @@ grpc_channel* grpc_channel_create_with_builder( } static grpc_core::UniquePtr get_default_authority( - const char* target, const grpc_channel_args* input_args, - grpc_channel_stack_type channel_stack_type) { + const grpc_channel_args* input_args) { bool has_default_authority = false; char* ssl_override = nullptr; grpc_core::UniquePtr default_authority; @@ -202,7 +201,7 @@ grpc_channel* grpc_channel_create(const char* target, grpc_transport* optional_transport) { grpc_channel_stack_builder* builder = grpc_channel_stack_builder_create(); const grpc_core::UniquePtr default_authority = - get_default_authority(target, input_args, channel_stack_type); + get_default_authority(input_args); grpc_channel_args* args = build_channel_args(input_args, default_authority.get()); grpc_channel_stack_builder_set_channel_arguments(builder, args); diff --git a/test/core/end2end/fixtures/h2_fakesec.cc b/test/core/end2end/fixtures/h2_fakesec.cc index bbf65fcd245..8b326befd33 100644 --- a/test/core/end2end/fixtures/h2_fakesec.cc +++ b/test/core/end2end/fixtures/h2_fakesec.cc @@ -132,7 +132,6 @@ static void chttp2_init_server_fake_secure_fullstack( static grpc_end2end_test_config configs[] = { {"chttp2/fake_secure_fullstack", FEATURE_MASK_SUPPORTS_DELAYED_CONNECTION | - FEATURE_MASK_SUPPORTS_PER_CALL_CREDENTIALS | FEATURE_MASK_SUPPORTS_CLIENT_CHANNEL | FEATURE_MASK_SUPPORTS_AUTHORITY_HEADER, chttp2_create_fixture_secure_fullstack, diff --git a/test/core/end2end/tests/default_host.cc b/test/core/end2end/tests/default_host.cc index cda716a259d..5984e522f64 100644 --- a/test/core/end2end/tests/default_host.cc +++ b/test/core/end2end/tests/default_host.cc @@ -85,7 +85,13 @@ static void end_test(grpc_end2end_test_fixture* f) { grpc_completion_queue_destroy(f->shutdown_cq); } -static void simple_request_body(grpc_end2end_test_fixture f) { +static void test_invoke_simple_request(grpc_end2end_test_config config) { + grpc_end2end_test_fixture f = + begin_test(config, "test_invoke_simple_request", nullptr, nullptr); + const bool override_host_for_call_creds_use = + (config.feature_mask & FEATURE_MASK_SUPPORTS_PER_CALL_CREDENTIALS) != 0; + const char* expected_host = + override_host_for_call_creds_use ? "foo.test.google.fr" : "localhost"; grpc_call* c; grpc_call* s; cq_verifier* cqv = cq_verifier_create(f.cq); @@ -191,9 +197,12 @@ static void simple_request_body(grpc_end2end_test_fixture f) { GPR_ASSERT(status == GRPC_STATUS_UNIMPLEMENTED); GPR_ASSERT(0 == grpc_slice_str_cmp(details, "xyz")); GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo")); - char* target = grpc_channel_get_target(f.client); - GPR_ASSERT(grpc_slice_buf_start_eq(call_details.host, target, 9)); - gpr_free(target); + if (override_host_for_call_creds_use) { + validate_host_override_string("foo.test.google.fr", call_details.host, + config); + } + GPR_ASSERT(grpc_slice_buf_start_eq(call_details.host, expected_host, + strlen(expected_host))); GPR_ASSERT(was_cancelled == 1); grpc_slice_unref(details); @@ -206,21 +215,12 @@ static void simple_request_body(grpc_end2end_test_fixture f) { grpc_call_unref(s); cq_verifier_destroy(cqv); -} -static void test_invoke_simple_request(grpc_end2end_test_config config) { - grpc_end2end_test_fixture f; - - f = begin_test(config, "test_invoke_simple_request", nullptr, nullptr); - simple_request_body(f); end_test(&f); config.tear_down_data(&f); } void default_host(grpc_end2end_test_config config) { - if ((config.feature_mask & FEATURE_MASK_SUPPORTS_PER_CALL_CREDENTIALS) != 0) { - return; - } test_invoke_simple_request(config); } diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 5efea4a58dc..60238e930d7 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -290,7 +290,7 @@ class End2endTest : public ::testing::TestWithParam { args.SetUserAgentPrefix(user_agent_prefix_); } args.SetString(GRPC_ARG_SECONDARY_USER_AGENT_STRING, "end2end_test"); - args.SetString(GRPC_ARG_DEFAULT_AUTHORITY, "test-authority"); + if (!GetParam().inproc) { channel_ = CreateCustomChannel(server_address_.str(), channel_creds, args); diff --git a/test/cpp/end2end/shutdown_test.cc b/test/cpp/end2end/shutdown_test.cc index dd916da5c4e..a53de691bcd 100644 --- a/test/cpp/end2end/shutdown_test.cc +++ b/test/cpp/end2end/shutdown_test.cc @@ -86,7 +86,6 @@ class ShutdownTest : public ::testing::TestWithParam { ChannelArguments args; auto channel_creds = GetCredentialsProvider()->GetChannelCredentials(GetParam(), &args); - args.SetString(GRPC_ARG_DEFAULT_AUTHORITY, "test-authority"); channel_ = CreateCustomChannel(target, channel_creds, args); stub_ = grpc::testing::EchoTestService::NewStub(channel_); }