From 89f86c08e20afb830807d7a2930c7374fbdf6aee Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Wed, 1 Nov 2017 18:20:40 -0700 Subject: [PATCH 1/4] Lower backup poll interval in async_end2end_test --- test/cpp/end2end/async_end2end_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/cpp/end2end/async_end2end_test.cc b/test/cpp/end2end/async_end2end_test.cc index cf1cc7e486d..5b6e9aa3121 100644 --- a/test/cpp/end2end/async_end2end_test.cc +++ b/test/cpp/end2end/async_end2end_test.cc @@ -458,6 +458,7 @@ TEST_P(AsyncEnd2endTest, SequentialRpcs) { } TEST_P(AsyncEnd2endTest, ReconnectChannel) { + // GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS is set to 100ms in main() if (GetParam().inproc) { return; } @@ -2012,9 +2013,9 @@ INSTANTIATE_TEST_CASE_P(AsyncEnd2endServerTryCancel, } // namespace grpc int main(int argc, char** argv) { - // Change the backup poll interval from 5s to 200ms to speed up the + // Change the backup poll interval from 5s to 100ms to speed up the // ReconnectChannel test - gpr_setenv("GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS", "200"); + gpr_setenv("GRPC_CLIENT_CHANNEL_BACKUP_POLL_INTERVAL_MS", "100"); grpc_test_init(argc, argv); gpr_tls_init(&g_is_async_end2end_test); ::testing::InitGoogleTest(&argc, argv); From dc7c50e6a442d3877d649b9277a56761a86f6b77 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Fri, 3 Nov 2017 14:46:42 -0700 Subject: [PATCH 2/4] Switch PB dependence to PB headers dependence --- BUILD | 2 +- WORKSPACE | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/BUILD b/BUILD index f2848fdfc8b..bcb75af0321 100644 --- a/BUILD +++ b/BUILD @@ -1545,7 +1545,7 @@ grpc_cc_library( grpc_cc_library( name = "grpc++_config_proto", external_deps = [ - "protobuf", + "protobuf_headers", ], language = "c++", public_hdrs = [ diff --git a/WORKSPACE b/WORKSPACE index 907cef1fcae..5f87d68a2fb 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -23,6 +23,11 @@ bind( actual = "@com_google_protobuf//:protoc_lib", ) +bind( + name = "protobuf_headers", + actual = "@com_google_protobuf//:protobuf_headers", +) + bind( name = "protocol_compiler", actual = "@com_google_protobuf//:protoc", From 5e7ceebf6b7e36ecfd11f02c6accdc5d9a71c644 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Fri, 3 Nov 2017 22:37:03 +0000 Subject: [PATCH 3/4] Need explicit "protobuf" dependence on proto_utils_test --- test/cpp/codegen/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/test/cpp/codegen/BUILD b/test/cpp/codegen/BUILD index 8de46be8162..6cc81e33983 100644 --- a/test/cpp/codegen/BUILD +++ b/test/cpp/codegen/BUILD @@ -51,6 +51,7 @@ grpc_cc_test( ], external_deps = [ "gtest", + "protobuf", ], ) From a274f341d545c88c68178227b3be40ac4c03e77e Mon Sep 17 00:00:00 2001 From: Julien Boeuf Date: Wed, 1 Nov 2017 20:18:02 -0700 Subject: [PATCH 4/4] Removing port 443 for the service name (used as audience) when the url is https. --- .../lib/security/transport/auth_filters.h | 8 ++ .../security/transport/client_auth_filter.cc | 48 +++++++----- test/core/security/credentials_test.c | 75 +++++++++++++++++++ 3 files changed, 111 insertions(+), 20 deletions(-) diff --git a/src/core/lib/security/transport/auth_filters.h b/src/core/lib/security/transport/auth_filters.h index ba5df7fe70f..b49bd554de9 100644 --- a/src/core/lib/security/transport/auth_filters.h +++ b/src/core/lib/security/transport/auth_filters.h @@ -19,6 +19,7 @@ #ifndef GRPC_CORE_LIB_SECURITY_TRANSPORT_AUTH_FILTERS_H #define GRPC_CORE_LIB_SECURITY_TRANSPORT_AUTH_FILTERS_H +#include #include "src/core/lib/channel/channel_stack.h" #ifdef __cplusplus @@ -28,6 +29,13 @@ extern "C" { extern const grpc_channel_filter grpc_client_auth_filter; extern const grpc_channel_filter grpc_server_auth_filter; +void grpc_auth_metadata_context_build( + const char *url_scheme, grpc_slice call_host, grpc_slice call_method, + grpc_auth_context *auth_context, + grpc_auth_metadata_context *auth_md_context); + +void grpc_auth_metadata_context_reset(grpc_auth_metadata_context *context); + #ifdef __cplusplus } #endif diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index a8464dbf9e7..0bbfa471d21 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -65,7 +65,7 @@ typedef struct { grpc_auth_context *auth_context; } channel_data; -static void reset_auth_metadata_context( +void grpc_auth_metadata_context_reset( grpc_auth_metadata_context *auth_md_context) { if (auth_md_context->service_url != NULL) { gpr_free((char *)auth_md_context->service_url); @@ -96,7 +96,7 @@ static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *arg, grpc_call_element *elem = (grpc_call_element *)batch->handler_private.extra_arg; call_data *calld = (call_data *)elem->call_data; - reset_auth_metadata_context(&calld->auth_md_context); + grpc_auth_metadata_context_reset(&calld->auth_md_context); grpc_error *error = GRPC_ERROR_REF(input_error); if (error == GRPC_ERROR_NONE) { GPR_ASSERT(calld->md_array.size <= MAX_CREDENTIALS_METADATA_COUNT); @@ -119,34 +119,41 @@ static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *arg, } } -void build_auth_metadata_context(grpc_security_connector *sc, - grpc_auth_context *auth_context, - call_data *calld) { - char *service = grpc_slice_to_c_string(calld->method); +void grpc_auth_metadata_context_build( + const char *url_scheme, grpc_slice call_host, grpc_slice call_method, + grpc_auth_context *auth_context, + grpc_auth_metadata_context *auth_md_context) { + char *service = grpc_slice_to_c_string(call_method); char *last_slash = strrchr(service, '/'); char *method_name = NULL; char *service_url = NULL; - reset_auth_metadata_context(&calld->auth_md_context); + grpc_auth_metadata_context_reset(auth_md_context); if (last_slash == NULL) { gpr_log(GPR_ERROR, "No '/' found in fully qualified method name"); service[0] = '\0'; + method_name = gpr_strdup(""); } else if (last_slash == service) { - /* No service part in fully qualified method name: will just be "/". */ - service[1] = '\0'; + method_name = gpr_strdup(""); } else { *last_slash = '\0'; method_name = gpr_strdup(last_slash + 1); } - if (method_name == NULL) method_name = gpr_strdup(""); - char *host = grpc_slice_to_c_string(calld->host); - gpr_asprintf(&service_url, "%s://%s%s", - sc->url_scheme == NULL ? "" : sc->url_scheme, host, service); - calld->auth_md_context.service_url = service_url; - calld->auth_md_context.method_name = method_name; - calld->auth_md_context.channel_auth_context = + char *host_and_port = grpc_slice_to_c_string(call_host); + if (strcmp(url_scheme, GRPC_SSL_URL_SCHEME) == 0) { + /* Remove the port if it is 443. */ + char *port_delimiter = strrchr(host_and_port, ':'); + if (port_delimiter != NULL && strcmp(port_delimiter + 1, "443") == 0) { + *port_delimiter = '\0'; + } + } + gpr_asprintf(&service_url, "%s://%s%s", url_scheme == NULL ? "" : url_scheme, + host_and_port, service); + auth_md_context->service_url = service_url; + auth_md_context->method_name = method_name; + auth_md_context->channel_auth_context = GRPC_AUTH_CONTEXT_REF(auth_context, "grpc_auth_metadata_context"); gpr_free(service); - gpr_free(host); + gpr_free(host_and_port); } static void cancel_get_request_metadata(grpc_exec_ctx *exec_ctx, void *arg, @@ -198,8 +205,9 @@ static void send_security_metadata(grpc_exec_ctx *exec_ctx, call_creds_has_md ? ctx->creds : channel_call_creds); } - build_auth_metadata_context(&chand->security_connector->base, - chand->auth_context, calld); + grpc_auth_metadata_context_build( + chand->security_connector->base.url_scheme, calld->host, calld->method, + chand->auth_context, &calld->auth_md_context); GPR_ASSERT(calld->pollent != NULL); @@ -369,7 +377,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, if (calld->have_method) { grpc_slice_unref_internal(exec_ctx, calld->method); } - reset_auth_metadata_context(&calld->auth_md_context); + grpc_auth_metadata_context_reset(&calld->auth_md_context); } /* Constructor for channel_data */ diff --git a/test/core/security/credentials_test.c b/test/core/security/credentials_test.c index 34f310142c8..72618602e94 100644 --- a/test/core/security/credentials_test.c +++ b/test/core/security/credentials_test.c @@ -24,6 +24,8 @@ #include #include +#include + #include #include #include @@ -35,6 +37,7 @@ #include "src/core/lib/security/credentials/google_default/google_default_credentials.h" #include "src/core/lib/security/credentials/jwt/jwt_credentials.h" #include "src/core/lib/security/credentials/oauth2/oauth2_credentials.h" +#include "src/core/lib/security/transport/auth_filters.h" #include "src/core/lib/support/env.h" #include "src/core/lib/support/string.h" #include "src/core/lib/support/tmpfile.h" @@ -1178,6 +1181,77 @@ static void test_channel_creds_duplicate_without_call_creds(void) { grpc_exec_ctx_finish(&exec_ctx); } +typedef struct { + const char *url_scheme; + const char *call_host; + const char *call_method; + const char *desired_service_url; + const char *desired_method_name; +} auth_metadata_context_test_case; + +static void test_auth_metadata_context(void) { + auth_metadata_context_test_case test_cases[] = { + // No service nor method. + {"https", "www.foo.com", "", "https://www.foo.com", ""}, + // No method. + {"https", "www.foo.com", "/Service", "https://www.foo.com/Service", ""}, + // Empty service and method. + {"https", "www.foo.com", "//", "https://www.foo.com/", ""}, + // Empty method. + {"https", "www.foo.com", "/Service/", "https://www.foo.com/Service", ""}, + // Malformed url. + {"https", "www.foo.com:", "/Service/", "https://www.foo.com:/Service", + ""}, + // https, default explicit port. + {"https", "www.foo.com:443", "/Service/FooMethod", + "https://www.foo.com/Service", "FooMethod"}, + // https, default implicit port. + {"https", "www.foo.com", "/Service/FooMethod", + "https://www.foo.com/Service", "FooMethod"}, + // https with ipv6 literal, default explicit port. + {"https", "[1080:0:0:0:8:800:200C:417A]:443", "/Service/FooMethod", + "https://[1080:0:0:0:8:800:200C:417A]/Service", "FooMethod"}, + // https with ipv6 literal, default implicit port. + {"https", "[1080:0:0:0:8:800:200C:443]", "/Service/FooMethod", + "https://[1080:0:0:0:8:800:200C:443]/Service", "FooMethod"}, + // https, custom port. + {"https", "www.foo.com:8888", "/Service/FooMethod", + "https://www.foo.com:8888/Service", "FooMethod"}, + // https with ipv6 literal, custom port. + {"https", "[1080:0:0:0:8:800:200C:417A]:8888", "/Service/FooMethod", + "https://[1080:0:0:0:8:800:200C:417A]:8888/Service", "FooMethod"}, + // custom url scheme, https default port. + {"blah", "www.foo.com:443", "/Service/FooMethod", + "blah://www.foo.com:443/Service", "FooMethod"}}; + for (uint32_t i = 0; i < GPR_ARRAY_SIZE(test_cases); i++) { + const char *url_scheme = test_cases[i].url_scheme; + grpc_slice call_host = + grpc_slice_from_copied_string(test_cases[i].call_host); + grpc_slice call_method = + grpc_slice_from_copied_string(test_cases[i].call_method); + grpc_auth_metadata_context auth_md_context; + memset(&auth_md_context, 0, sizeof(auth_md_context)); + grpc_auth_metadata_context_build(url_scheme, call_host, call_method, NULL, + &auth_md_context); + if (strcmp(auth_md_context.service_url, + test_cases[i].desired_service_url) != 0) { + gpr_log(GPR_ERROR, "Invalid service url, want: %s, got %s.", + test_cases[i].desired_service_url, auth_md_context.service_url); + GPR_ASSERT(false); + } + if (strcmp(auth_md_context.method_name, + test_cases[i].desired_method_name) != 0) { + gpr_log(GPR_ERROR, "Invalid method name, want: %s, got %s.", + test_cases[i].desired_method_name, auth_md_context.method_name); + GPR_ASSERT(false); + } + GPR_ASSERT(auth_md_context.channel_auth_context == NULL); + grpc_slice_unref(call_host); + grpc_slice_unref(call_method); + grpc_auth_metadata_context_reset(&auth_md_context); + } +} + int main(int argc, char **argv) { grpc_test_init(argc, argv); grpc_init(); @@ -1211,6 +1285,7 @@ int main(int argc, char **argv) { test_metadata_plugin_failure(); test_get_well_known_google_credentials_file_path(); test_channel_creds_duplicate_without_call_creds(); + test_auth_metadata_context(); grpc_shutdown(); return 0; }