From e150fff57ef947998ce8f45da112ed45ae01bacb Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 23 Nov 2016 14:47:11 -0800 Subject: [PATCH] Fix some auth filtering bugs --- .../security/transport/client_auth_filter.c | 8 ++++- .../security/transport/server_auth_filter.c | 23 +++++++------- src/core/lib/transport/metadata_batch.c | 30 +++++++++++++++++++ src/core/lib/transport/metadata_batch.h | 18 +++++++++++ 4 files changed, 66 insertions(+), 13 deletions(-) diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c index 43054bcfb4b..80fdbe68eb3 100644 --- a/src/core/lib/security/transport/client_auth_filter.c +++ b/src/core/lib/security/transport/client_auth_filter.c @@ -102,7 +102,13 @@ static void bubble_up_error(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_call_next_op(exec_ctx, elem, &calld->op); } -static void add_error(grpc_error **combined, grpc_error *error) { abort(); } +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 on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *user_data, grpc_credentials_md *md_elems, diff --git a/src/core/lib/security/transport/server_auth_filter.c b/src/core/lib/security/transport/server_auth_filter.c index ed9d92b74e6..34f5bbf8f47 100644 --- a/src/core/lib/security/transport/server_auth_filter.c +++ b/src/core/lib/security/transport/server_auth_filter.c @@ -83,9 +83,9 @@ static grpc_metadata_array metadata_batch_to_md_array( return result; } -#if 0 -static grpc_mdelem remove_consumed_md(grpc_exec_ctx *exec_ctx, void *user_data, - grpc_mdelem md) { +static grpc_filtered_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; @@ -93,11 +93,10 @@ static grpc_mdelem remove_consumed_md(grpc_exec_ctx *exec_ctx, void *user_data, 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_MDNULL; + return GRPC_FILTERED_REMOVE(); } - return md; + return GRPC_FILTERED_MDELEM(md); } -#endif static void destroy_op(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { gpr_free(arg); @@ -122,12 +121,12 @@ static void on_md_processing_done( if (status == GRPC_STATUS_OK) { calld->consumed_md = consumed_md; calld->num_consumed_md = num_consumed_md; -#if 0 - grpc_metadata_batch_filter(&exec_ctx, calld->recv_initial_metadata, - remove_consumed_md, elem); -#else - if (num_consumed_md) abort(); -#endif + /* 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")); grpc_metadata_array_destroy(&calld->md); grpc_exec_ctx_sched(&exec_ctx, calld->on_done_recv, GRPC_ERROR_NONE, NULL); } else { diff --git a/src/core/lib/transport/metadata_batch.c b/src/core/lib/transport/metadata_batch.c index a83f0e0f34e..07b0dd37256 100644 --- a/src/core/lib/transport/metadata_batch.c +++ b/src/core/lib/transport/metadata_batch.c @@ -285,3 +285,33 @@ size_t grpc_metadata_batch_size(grpc_metadata_batch *batch) { } return size; } + +static void add_error(grpc_error **composite, grpc_error *error, + const char *composite_error_string) { + if (error == GRPC_ERROR_NONE) return; + if (*composite == GRPC_ERROR_NONE) { + *composite = GRPC_ERROR_CREATE(composite_error_string); + } + *composite = grpc_error_add_child(*composite, error); +} + +grpc_error *grpc_metadata_batch_filter(grpc_exec_ctx *exec_ctx, + grpc_metadata_batch *batch, + grpc_metadata_batch_filter_func func, + void *user_data, + const char *composite_error_string) { + grpc_linked_mdelem *l = batch->list.head; + grpc_error *error = GRPC_ERROR_NONE; + while (l) { + grpc_linked_mdelem *next = l->next; + grpc_filtered_mdelem new = func(exec_ctx, user_data, l->md); + add_error(&error, new.error, composite_error_string); + if (GRPC_MDISNULL(new.md)) { + grpc_metadata_batch_remove(exec_ctx, batch, l); + } else if (new.md.payload != l->md.payload) { + grpc_metadata_batch_substitute(exec_ctx, batch, l, new.md); + } + l = next; + } + return error; +} diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index d7f6485565b..894a927d3e9 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -133,6 +133,24 @@ grpc_error *grpc_metadata_batch_add_tail( grpc_error *grpc_attach_md_to_error(grpc_error *src, grpc_mdelem md); +typedef struct { + grpc_error *error; + grpc_mdelem md; +} grpc_filtered_mdelem; + +#define GRPC_FILTERED_ERROR(error) \ + ((grpc_filtered_mdelem){(error), GRPC_MDNULL}) +#define GRPC_FILTERED_MDELEM(md) ((grpc_filtered_mdelem){GRPC_ERROR_NONE, (md)}) +#define GRPC_FILTERED_REMOVE() \ + ((grpc_filtered_mdelem){GRPC_ERROR_NONE, GRPC_MDNULL}) + +typedef grpc_filtered_mdelem (*grpc_metadata_batch_filter_func)( + grpc_exec_ctx *exec_ctx, void *user_data, grpc_mdelem elem); +grpc_error *grpc_metadata_batch_filter( + grpc_exec_ctx *exec_ctx, grpc_metadata_batch *batch, + grpc_metadata_batch_filter_func func, void *user_data, + const char *composite_error_string) GRPC_MUST_USE_RESULT; + #ifndef NDEBUG void grpc_metadata_batch_assert_ok(grpc_metadata_batch *comd); #else