From a274f341d545c88c68178227b3be40ac4c03e77e Mon Sep 17 00:00:00 2001 From: Julien Boeuf Date: Wed, 1 Nov 2017 20:18:02 -0700 Subject: [PATCH] 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; }