From 5e01e2ac977655aa074faf7fde0a74298f5e4c55 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 20 Jan 2017 18:11:52 -0800 Subject: Revert "Metadata handling rewrite" --- .../lib/security/transport/client_auth_filter.c | 115 ++++++++------------- .../lib/security/transport/security_connector.c | 2 +- .../lib/security/transport/security_handshaker.c | 2 +- .../lib/security/transport/server_auth_filter.c | 69 +++++++------ 4 files changed, 86 insertions(+), 102 deletions(-) (limited to 'src/core/lib/security/transport') diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c index cf056e8008..b7f6fd23e3 100644 --- a/src/core/lib/security/transport/client_auth_filter.c +++ b/src/core/lib/security/transport/client_auth_filter.c @@ -45,7 +45,6 @@ #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/security/transport/security_connector.h" #include "src/core/lib/slice/slice_internal.h" -#include "src/core/lib/slice/slice_string_helpers.h" #include "src/core/lib/support/string.h" #include "src/core/lib/surface/call.h" #include "src/core/lib/transport/static_metadata.h" @@ -55,10 +54,8 @@ /* We can have a per-call credentials. */ typedef struct { grpc_call_credentials *creds; - bool have_host; - bool have_method; - grpc_slice host; - grpc_slice method; + grpc_mdstr *host; + grpc_mdstr *method; /* pollset{_set} bound to this call; if we need to make external network requests, they should be done under a pollset added to this pollset_set so that work can progress when this call wants work to progress @@ -92,12 +89,14 @@ static void reset_auth_metadata_context( auth_md_context->channel_auth_context = NULL; } -static void add_error(grpc_error **combined, grpc_error *error) { - if (error == GRPC_ERROR_NONE) return; - if (*combined == GRPC_ERROR_NONE) { - *combined = GRPC_ERROR_CREATE("Client auth metadata plugin error"); - } - *combined = grpc_error_add_child(*combined, error); +static void bubble_up_error(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, + grpc_status_code status, const char *error_msg) { + call_data *calld = elem->call_data; + gpr_log(GPR_ERROR, "Client side authentication failure: %s", error_msg); + grpc_slice error_slice = grpc_slice_from_copied_string(error_msg); + grpc_transport_stream_op_add_close(exec_ctx, &calld->op, status, + &error_slice); + grpc_call_next_op(exec_ctx, elem, &calld->op); } static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *user_data, @@ -111,37 +110,30 @@ static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *user_data, grpc_metadata_batch *mdb; size_t i; reset_auth_metadata_context(&calld->auth_md_context); - grpc_error *error = GRPC_ERROR_NONE; if (status != GRPC_CREDENTIALS_OK) { - error = grpc_error_set_int( - GRPC_ERROR_CREATE(error_details != NULL && strlen(error_details) > 0 - ? error_details - : "Credentials failed to get metadata."), - GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAUTHENTICATED); - } else { - GPR_ASSERT(num_md <= MAX_CREDENTIALS_METADATA_COUNT); - GPR_ASSERT(op->send_initial_metadata != NULL); - mdb = op->send_initial_metadata; - for (i = 0; i < num_md; i++) { - add_error(&error, - grpc_metadata_batch_add_tail( - exec_ctx, mdb, &calld->md_links[i], - grpc_mdelem_from_slices( - exec_ctx, grpc_slice_ref_internal(md_elems[i].key), - grpc_slice_ref_internal(md_elems[i].value)))); - } + bubble_up_error(exec_ctx, elem, GRPC_STATUS_UNAUTHENTICATED, + (error_details != NULL && strlen(error_details) > 0) + ? error_details + : "Credentials failed to get metadata."); + return; } - if (error == GRPC_ERROR_NONE) { - grpc_call_next_op(exec_ctx, elem, op); - } else { - grpc_transport_stream_op_finish_with_failure(exec_ctx, op, error); + GPR_ASSERT(num_md <= MAX_CREDENTIALS_METADATA_COUNT); + GPR_ASSERT(op->send_initial_metadata != NULL); + mdb = op->send_initial_metadata; + for (i = 0; i < num_md; i++) { + grpc_metadata_batch_add_tail( + mdb, &calld->md_links[i], + grpc_mdelem_from_slices(exec_ctx, + grpc_slice_ref_internal(md_elems[i].key), + grpc_slice_ref_internal(md_elems[i].value))); } + grpc_call_next_op(exec_ctx, elem, op); } 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); + char *service = gpr_strdup(grpc_mdstr_as_c_string(calld->method)); char *last_slash = strrchr(service, '/'); char *method_name = NULL; char *service_url = NULL; @@ -157,15 +149,14 @@ void build_auth_metadata_context(grpc_security_connector *sc, 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); + sc->url_scheme == NULL ? "" : sc->url_scheme, + grpc_mdstr_as_c_string(calld->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 = GRPC_AUTH_CONTEXT_REF(auth_context, "grpc_auth_metadata_context"); gpr_free(service); - gpr_free(host); } static void send_security_metadata(grpc_exec_ctx *exec_ctx, @@ -189,12 +180,8 @@ static void send_security_metadata(grpc_exec_ctx *exec_ctx, calld->creds = grpc_composite_call_credentials_create(channel_call_creds, ctx->creds, NULL); if (calld->creds == NULL) { - grpc_transport_stream_op_finish_with_failure( - exec_ctx, op, - grpc_error_set_int( - GRPC_ERROR_CREATE( - "Incompatible credentials set on channel and call."), - GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAUTHENTICATED)); + bubble_up_error(exec_ctx, elem, GRPC_STATUS_UNAUTHENTICATED, + "Incompatible credentials set on channel and call."); return; } } else { @@ -220,14 +207,9 @@ static void on_host_checked(grpc_exec_ctx *exec_ctx, void *user_data, send_security_metadata(exec_ctx, elem, &calld->op); } else { char *error_msg; - char *host = grpc_slice_to_c_string(calld->host); gpr_asprintf(&error_msg, "Invalid host %s set in :authority metadata.", - host); - gpr_free(host); - grpc_call_element_signal_error( - exec_ctx, elem, grpc_error_set_int(GRPC_ERROR_CREATE(error_msg), - GRPC_ERROR_INT_GRPC_STATUS, - GRPC_STATUS_UNAUTHENTICATED)); + grpc_mdstr_as_c_string(calld->host)); + bubble_up_error(exec_ctx, elem, GRPC_STATUS_UNAUTHENTICATED, error_msg); gpr_free(error_msg); } } @@ -265,30 +247,23 @@ static void auth_start_transport_op(grpc_exec_ctx *exec_ctx, if (op->send_initial_metadata != NULL) { for (l = op->send_initial_metadata->list.head; l != NULL; l = l->next) { - grpc_mdelem md = l->md; + grpc_mdelem *md = l->md; /* Pointer comparison is OK for md_elems created from the same context. */ - if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_AUTHORITY)) { - if (calld->have_host) { - grpc_slice_unref_internal(exec_ctx, calld->host); - } - calld->host = grpc_slice_ref_internal(GRPC_MDVALUE(md)); - calld->have_host = true; - } else if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_PATH)) { - if (calld->have_method) { - grpc_slice_unref_internal(exec_ctx, calld->method); - } - calld->method = grpc_slice_ref_internal(GRPC_MDVALUE(md)); - calld->have_method = true; + if (md->key == GRPC_MDSTR_AUTHORITY) { + if (calld->host != NULL) GRPC_MDSTR_UNREF(exec_ctx, calld->host); + calld->host = GRPC_MDSTR_REF(md->value); + } else if (md->key == GRPC_MDSTR_PATH) { + if (calld->method != NULL) GRPC_MDSTR_UNREF(exec_ctx, calld->method); + calld->method = GRPC_MDSTR_REF(md->value); } } - if (calld->have_host) { - char *call_host = grpc_slice_to_c_string(calld->host); + if (calld->host != NULL) { + const char *call_host = grpc_mdstr_as_c_string(calld->host); calld->op = *op; /* Copy op (originates from the caller's stack). */ grpc_channel_security_connector_check_call_host( exec_ctx, chand->security_connector, call_host, chand->auth_context, on_host_checked, elem); - gpr_free(call_host); GPR_TIMER_END("auth_start_transport_op", 0); return; /* early exit */ } @@ -321,11 +296,11 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, void *ignored) { call_data *calld = elem->call_data; grpc_call_credentials_unref(exec_ctx, calld->creds); - if (calld->have_host) { - grpc_slice_unref_internal(exec_ctx, calld->host); + if (calld->host != NULL) { + GRPC_MDSTR_UNREF(exec_ctx, calld->host); } - if (calld->have_method) { - grpc_slice_unref_internal(exec_ctx, calld->method); + if (calld->method != NULL) { + GRPC_MDSTR_UNREF(exec_ctx, calld->method); } reset_auth_metadata_context(&calld->auth_md_context); } diff --git a/src/core/lib/security/transport/security_connector.c b/src/core/lib/security/transport/security_connector.c index b09127811b..5aa26e0577 100644 --- a/src/core/lib/security/transport/security_connector.c +++ b/src/core/lib/security/transport/security_connector.c @@ -601,7 +601,7 @@ static grpc_security_connector_vtable ssl_server_vtable = { ssl_server_destroy, ssl_server_check_peer}; static grpc_slice compute_default_pem_root_certs_once(void) { - grpc_slice result = grpc_empty_slice(); + grpc_slice result = gpr_empty_slice(); /* First try to load the roots from the environment. */ char *default_root_certs_path = diff --git a/src/core/lib/security/transport/security_handshaker.c b/src/core/lib/security/transport/security_handshaker.c index 37d57d759b..5e75856c7a 100644 --- a/src/core/lib/security/transport/security_handshaker.c +++ b/src/core/lib/security/transport/security_handshaker.c @@ -124,7 +124,7 @@ static void security_handshake_failed_locked(grpc_exec_ctx *exec_ctx, } const char *msg = grpc_error_string(error); gpr_log(GPR_DEBUG, "Security handshake failed: %s", msg); - + grpc_error_free_string(msg); if (!h->shutdown) { // TODO(ctiller): It is currently necessary to shutdown endpoints // before destroying them, even if we know that there are no diff --git a/src/core/lib/security/transport/server_auth_filter.c b/src/core/lib/security/transport/server_auth_filter.c index 36e81d6501..5e98ba895d 100644 --- a/src/core/lib/security/transport/server_auth_filter.c +++ b/src/core/lib/security/transport/server_auth_filter.c @@ -33,13 +33,12 @@ #include -#include -#include - #include "src/core/lib/security/context/security_context.h" #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/security/transport/auth_filters.h" -#include "src/core/lib/slice/slice_internal.h" + +#include +#include typedef struct call_data { grpc_metadata_batch *recv_initial_metadata; @@ -68,34 +67,48 @@ static grpc_metadata_array metadata_batch_to_md_array( grpc_metadata_array_init(&result); for (l = batch->list.head; l != NULL; l = l->next) { grpc_metadata *usr_md = NULL; - grpc_mdelem md = l->md; - grpc_slice key = GRPC_MDKEY(md); - grpc_slice value = GRPC_MDVALUE(md); + grpc_mdelem *md = l->md; + grpc_mdstr *key = md->key; + grpc_mdstr *value = md->value; if (result.count == result.capacity) { result.capacity = GPR_MAX(result.capacity + 8, result.capacity * 2); result.metadata = gpr_realloc(result.metadata, result.capacity * sizeof(grpc_metadata)); } usr_md = &result.metadata[result.count++]; - usr_md->key = grpc_slice_ref_internal(key); - usr_md->value = grpc_slice_ref_internal(value); + usr_md->key = grpc_mdstr_as_c_string(key); + usr_md->value = grpc_mdstr_as_c_string(value); + usr_md->value_length = GRPC_SLICE_LENGTH(value->slice); } return result; } -static grpc_filtered_mdelem remove_consumed_md(grpc_exec_ctx *exec_ctx, - void *user_data, - grpc_mdelem md) { +static grpc_mdelem *remove_consumed_md(grpc_exec_ctx *exec_ctx, void *user_data, + grpc_mdelem *md) { grpc_call_element *elem = user_data; call_data *calld = elem->call_data; size_t i; for (i = 0; i < calld->num_consumed_md; i++) { const grpc_metadata *consumed_md = &calld->consumed_md[i]; - if (grpc_slice_eq(GRPC_MDKEY(md), consumed_md->key) && - grpc_slice_eq(GRPC_MDVALUE(md), consumed_md->value)) - return GRPC_FILTERED_REMOVE(); + /* Maybe we could do a pointer comparison but we do not have any guarantee + that the metadata processor used the same pointers for consumed_md in the + callback. */ + if (GRPC_SLICE_LENGTH(md->key->slice) != strlen(consumed_md->key) || + GRPC_SLICE_LENGTH(md->value->slice) != consumed_md->value_length) { + continue; + } + if (memcmp(GRPC_SLICE_START_PTR(md->key->slice), consumed_md->key, + GRPC_SLICE_LENGTH(md->key->slice)) == 0 && + memcmp(GRPC_SLICE_START_PTR(md->value->slice), consumed_md->value, + GRPC_SLICE_LENGTH(md->value->slice)) == 0) { + return NULL; /* Delete. */ + } } - return GRPC_FILTERED_MDELEM(md); + return md; +} + +static void destroy_op(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { + gpr_free(arg); } /* called from application code */ @@ -117,33 +130,29 @@ static void on_md_processing_done( if (status == GRPC_STATUS_OK) { calld->consumed_md = consumed_md; calld->num_consumed_md = num_consumed_md; - /* TODO(ctiller): propagate error */ - GRPC_LOG_IF_ERROR( - "grpc_metadata_batch_filter", - grpc_metadata_batch_filter(&exec_ctx, calld->recv_initial_metadata, - remove_consumed_md, elem, - "Response metadata filtering error")); - for (size_t i = 0; i < calld->md.count; i++) { - grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].key); - grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].value); - } + grpc_metadata_batch_filter(&exec_ctx, calld->recv_initial_metadata, + remove_consumed_md, elem); grpc_metadata_array_destroy(&calld->md); grpc_closure_sched(&exec_ctx, calld->on_done_recv, GRPC_ERROR_NONE); } else { - for (size_t i = 0; i < calld->md.count; i++) { - grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].key); - grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].value); - } + grpc_slice message; + grpc_transport_stream_op *close_op = gpr_malloc(sizeof(*close_op)); + memset(close_op, 0, sizeof(*close_op)); grpc_metadata_array_destroy(&calld->md); error_details = error_details != NULL ? error_details : "Authentication metadata processing failed."; + message = grpc_slice_from_copied_string(error_details); calld->transport_op->send_initial_metadata = NULL; if (calld->transport_op->send_message != NULL) { grpc_byte_stream_destroy(&exec_ctx, calld->transport_op->send_message); calld->transport_op->send_message = NULL; } calld->transport_op->send_trailing_metadata = NULL; + close_op->on_complete = + grpc_closure_create(destroy_op, close_op, grpc_schedule_on_exec_ctx); + grpc_transport_stream_op_add_close(&exec_ctx, close_op, status, &message); + grpc_call_next_op(&exec_ctx, elem, close_op); grpc_closure_sched(&exec_ctx, calld->on_done_recv, grpc_error_set_int(GRPC_ERROR_CREATE(error_details), GRPC_ERROR_INT_GRPC_STATUS, status)); -- cgit v1.2.3