From 1d9ac6612d3a64f6598e69a4d41bdba6c01b6b2f Mon Sep 17 00:00:00 2001 From: Julien Boeuf Date: Thu, 17 Dec 2015 21:35:47 -0800 Subject: [PATCH] Removing the peer from the SSL security connector. - Missing unit tests. --- src/core/httpcli/httpcli_security_connector.c | 5 +- src/core/security/client_auth_filter.c | 22 +-- src/core/security/credentials.c | 2 +- src/core/security/security_connector.c | 137 +++++++++++------- src/core/security/security_connector.h | 33 ++--- test/core/security/security_connector_test.c | 2 + 6 files changed, 109 insertions(+), 92 deletions(-) diff --git a/src/core/httpcli/httpcli_security_connector.c b/src/core/httpcli/httpcli_security_connector.c index 60c6cf6ac2c..ba7cba25f90 100644 --- a/src/core/httpcli/httpcli_security_connector.c +++ b/src/core/httpcli/httpcli_security_connector.c @@ -84,7 +84,8 @@ static void httpcli_ssl_do_handshake(grpc_exec_ctx *exec_ctx, } static void httpcli_ssl_check_peer(grpc_exec_ctx *exec_ctx, - grpc_security_connector *sc, tsi_peer peer, + grpc_security_connector *sc, + tsi_peer peer, grpc_security_peer_check_cb cb, void *user_data) { grpc_httpcli_ssl_channel_security_connector *c = @@ -98,8 +99,8 @@ static void httpcli_ssl_check_peer(grpc_exec_ctx *exec_ctx, c->secure_peer_name); status = GRPC_SECURITY_ERROR; } - tsi_peer_destruct(&peer); cb(exec_ctx, user_data, status, NULL); + tsi_peer_destruct(&peer); } static grpc_security_connector_vtable httpcli_ssl_vtable = { diff --git a/src/core/security/client_auth_filter.c b/src/core/security/client_auth_filter.c index c55990025bc..1cb247d874a 100644 --- a/src/core/security/client_auth_filter.c +++ b/src/core/security/client_auth_filter.c @@ -250,27 +250,13 @@ static void auth_start_transport_op(grpc_exec_ctx *exec_ctx, } } if (calld->host != NULL) { - grpc_security_status status; const char *call_host = grpc_mdstr_as_c_string(calld->host); calld->op = *op; /* Copy op (originates from the caller's stack). */ - status = grpc_channel_security_connector_check_call_host( - exec_ctx, chand->security_connector, call_host, on_host_checked, - elem); - if (status != GRPC_SECURITY_OK) { - if (status == GRPC_SECURITY_ERROR) { - char *error_msg; - gpr_asprintf(&error_msg, - "Invalid host %s set in :authority metadata.", - call_host); - bubble_up_error(exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT, - error_msg); - gpr_free(error_msg); - } - return; /* early exit */ - } + grpc_channel_security_connector_check_call_host( + exec_ctx, chand->security_connector, call_host, chand->auth_context, + on_host_checked, elem); + return; /* early exit */ } - send_security_metadata(exec_ctx, elem, op); - return; /* early exit */ } /* pass control down the stack */ diff --git a/src/core/security/credentials.c b/src/core/security/credentials.c index a0054741ad6..866c4b792ff 100644 --- a/src/core/security/credentials.c +++ b/src/core/security/credentials.c @@ -881,7 +881,7 @@ static grpc_security_status fake_transport_security_create_security_connector( grpc_channel_credentials *c, grpc_call_credentials *call_creds, const char *target, const grpc_channel_args *args, grpc_channel_security_connector **sc, grpc_channel_args **new_args) { - *sc = grpc_fake_channel_security_connector_create(call_creds, 1); + *sc = grpc_fake_channel_security_connector_create(call_creds); return GRPC_SECURITY_OK; } diff --git a/src/core/security/security_connector.c b/src/core/security/security_connector.c index 3828451795a..204cd324f6a 100644 --- a/src/core/security/security_connector.c +++ b/src/core/security/security_connector.c @@ -130,22 +130,28 @@ void grpc_security_connector_do_handshake(grpc_exec_ctx *exec_ctx, } } -void grpc_security_connector_check_peer( - grpc_exec_ctx *exec_ctx, grpc_security_connector *sc, tsi_peer peer, - grpc_security_peer_check_cb cb, void *user_data) { +void grpc_security_connector_check_peer(grpc_exec_ctx *exec_ctx, + grpc_security_connector *sc, + tsi_peer peer, + grpc_security_peer_check_cb cb, + void *user_data) { if (sc == NULL) { - tsi_peer_destruct(&peer); cb(exec_ctx, user_data, GRPC_SECURITY_ERROR, NULL); + tsi_peer_destruct(&peer); } else { sc->vtable->check_peer(exec_ctx, sc, peer, cb, user_data); } } -grpc_security_status grpc_channel_security_connector_check_call_host( +void grpc_channel_security_connector_check_call_host( grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc, - const char *host, grpc_security_call_host_check_cb cb, void *user_data) { - if (sc == NULL || sc->check_call_host == NULL) return GRPC_SECURITY_ERROR; - return sc->check_call_host(exec_ctx, sc, host, cb, user_data); + const char *host, grpc_auth_context *auth_context, + grpc_security_call_host_check_cb cb, void *user_data) { + if (sc == NULL || sc->check_call_host == NULL) { + cb(exec_ctx, user_data, GRPC_SECURITY_ERROR); + } else { + sc->check_call_host(exec_ctx, sc, host, auth_context, cb, user_data); + } } #ifdef GRPC_SECURITY_CONNECTOR_REFCOUNT_DEBUG @@ -222,11 +228,6 @@ grpc_security_connector *grpc_find_security_connector_in_args( /* -- Fake implementation. -- */ -typedef struct { - grpc_channel_security_connector base; - int call_host_check_is_async; -} grpc_fake_channel_security_connector; - static void fake_channel_destroy(grpc_security_connector *sc) { grpc_channel_security_connector *c = (grpc_channel_security_connector *)sc; grpc_call_credentials_unref(c->request_metadata_creds); @@ -270,21 +271,17 @@ static void fake_check_peer(grpc_exec_ctx *exec_ctx, end: cb(exec_ctx, user_data, status, auth_context); - tsi_peer_destruct(&peer); grpc_auth_context_unref(auth_context); + tsi_peer_destruct(&peer); } -static grpc_security_status fake_channel_check_call_host( - grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc, - const char *host, grpc_security_call_host_check_cb cb, void *user_data) { - grpc_fake_channel_security_connector *c = - (grpc_fake_channel_security_connector *)sc; - if (c->call_host_check_is_async) { - cb(exec_ctx, user_data, GRPC_SECURITY_OK); - return GRPC_SECURITY_PENDING; - } else { - return GRPC_SECURITY_OK; - } +static void fake_channel_check_call_host(grpc_exec_ctx *exec_ctx, + grpc_channel_security_connector *sc, + const char *host, + grpc_auth_context *auth_context, + grpc_security_call_host_check_cb cb, + void *user_data) { + cb(exec_ctx, user_data, GRPC_SECURITY_OK); } static void fake_channel_do_handshake(grpc_exec_ctx *exec_ctx, @@ -312,20 +309,17 @@ static grpc_security_connector_vtable fake_server_vtable = { fake_server_destroy, fake_server_do_handshake, fake_check_peer}; grpc_channel_security_connector *grpc_fake_channel_security_connector_create( - grpc_call_credentials *request_metadata_creds, - int call_host_check_is_async) { - grpc_fake_channel_security_connector *c = - gpr_malloc(sizeof(grpc_fake_channel_security_connector)); - memset(c, 0, sizeof(grpc_fake_channel_security_connector)); - gpr_ref_init(&c->base.base.refcount, 1); - c->base.base.is_client_side = 1; - c->base.base.url_scheme = GRPC_FAKE_SECURITY_URL_SCHEME; - c->base.base.vtable = &fake_channel_vtable; - c->base.request_metadata_creds = + grpc_call_credentials *request_metadata_creds) { + grpc_channel_security_connector *c = gpr_malloc(sizeof(*c)); + memset(c, 0, sizeof(*c)); + gpr_ref_init(&c->base.refcount, 1); + c->base.is_client_side = 1; + c->base.url_scheme = GRPC_FAKE_SECURITY_URL_SCHEME; + c->base.vtable = &fake_channel_vtable; + c->request_metadata_creds = grpc_call_credentials_ref(request_metadata_creds); - c->base.check_call_host = fake_channel_check_call_host; - c->call_host_check_is_async = call_host_check_is_async; - return &c->base; + c->check_call_host = fake_channel_check_call_host; + return c; } grpc_security_connector *grpc_fake_server_security_connector_create(void) { @@ -346,9 +340,6 @@ typedef struct { tsi_ssl_handshaker_factory *handshaker_factory; char *target_name; char *overridden_target_name; - /* TODO(jboeuf): Remove this: the security connector is channel-wide construct - as opposed to the peer which is for one transport (or sub-channel). */ - tsi_peer peer; } grpc_ssl_channel_security_connector; typedef struct { @@ -365,7 +356,6 @@ static void ssl_channel_destroy(grpc_security_connector *sc) { } if (c->target_name != NULL) gpr_free(c->target_name); if (c->overridden_target_name != NULL) gpr_free(c->overridden_target_name); - tsi_peer_destruct(&c->peer); gpr_free(sc); } @@ -517,14 +507,13 @@ static void ssl_channel_check_peer( (grpc_ssl_channel_security_connector *)sc; grpc_security_status status; grpc_auth_context *auth_context = NULL; - tsi_peer_destruct(&c->peer); - c->peer = peer; status = ssl_check_peer(sc, c->overridden_target_name != NULL ? c->overridden_target_name : c->target_name, &peer, &auth_context); cb(exec_ctx, user_data, status, auth_context); grpc_auth_context_unref(auth_context); + tsi_peer_destruct(&peer); } static void ssl_server_check_peer( @@ -537,22 +526,66 @@ static void ssl_server_check_peer( grpc_auth_context_unref(auth_context); } -static grpc_security_status ssl_channel_check_call_host( - grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc, - const char *host, grpc_security_call_host_check_cb cb, void *user_data) { +static void add_shalow_auth_property_to_peer(tsi_peer *peer, + const grpc_auth_property *prop, + const char *tsi_prop_name) { + tsi_peer_property *tsi_prop = &peer->properties[peer->property_count++]; + tsi_prop->name = (char *)tsi_prop_name; + tsi_prop->value.data = prop->value; + tsi_prop->value.length = prop->value_length; +} + +tsi_peer tsi_shallow_peer_from_ssl_auth_context( + const grpc_auth_context *auth_context) { + size_t max_num_props = 0; + grpc_auth_property_iterator it; + const grpc_auth_property *prop; + tsi_peer peer; + memset(&peer, 0, sizeof(peer)); + + it = grpc_auth_context_property_iterator(auth_context); + while (grpc_auth_property_iterator_next(&it) != NULL) max_num_props++; + + if (max_num_props > 0) { + peer.properties = gpr_malloc(max_num_props * sizeof(tsi_peer_property)); + it = grpc_auth_context_property_iterator(auth_context); + while ((prop = grpc_auth_property_iterator_next(&it)) != NULL) { + if (strcmp(prop->name, GRPC_X509_SAN_PROPERTY_NAME) == 0) { + add_shalow_auth_property_to_peer( + &peer, prop, TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY); + } else if (strcmp(prop->name, GRPC_X509_CN_PROPERTY_NAME) == 0) { + add_shalow_auth_property_to_peer( + &peer, prop, TSI_X509_SUBJECT_COMMON_NAME_PEER_PROPERTY); + } + } + } + return peer; +} + +void tsi_shallow_peer_destruct(tsi_peer *peer) { + if (peer->properties != NULL) gpr_free(peer->properties); +} + +static void ssl_channel_check_call_host(grpc_exec_ctx *exec_ctx, + grpc_channel_security_connector *sc, + const char *host, + grpc_auth_context *auth_context, + grpc_security_call_host_check_cb cb, + void *user_data) { grpc_ssl_channel_security_connector *c = (grpc_ssl_channel_security_connector *)sc; - - if (ssl_host_matches_name(&c->peer, host)) return GRPC_SECURITY_OK; + grpc_security_status status = GRPC_SECURITY_ERROR; + tsi_peer peer = tsi_shallow_peer_from_ssl_auth_context(auth_context); + if (ssl_host_matches_name(&peer, host)) status = GRPC_SECURITY_OK; /* If the target name was overridden, then the original target_name was 'checked' transitively during the previous peer check at the end of the handshake. */ if (c->overridden_target_name != NULL && strcmp(host, c->target_name) == 0) { - return GRPC_SECURITY_OK; - } else { - return GRPC_SECURITY_ERROR; + status = GRPC_SECURITY_OK; } + cb(exec_ctx, user_data, status); + tsi_shallow_peer_destruct(&peer); } static grpc_security_connector_vtable ssl_channel_vtable = { diff --git a/src/core/security/security_connector.h b/src/core/security/security_connector.h index 3f7103699e2..b9f31b60749 100644 --- a/src/core/security/security_connector.h +++ b/src/core/security/security_connector.h @@ -42,7 +42,6 @@ typedef enum { GRPC_SECURITY_OK = 0, - GRPC_SECURITY_PENDING, GRPC_SECURITY_ERROR } grpc_security_status; @@ -124,9 +123,8 @@ void grpc_security_connector_do_handshake(grpc_exec_ctx *exec_ctx, grpc_security_handshake_done_cb cb, void *user_data); -/* Check the peer. - Ownership of the peer is transfered. - TODO(jboeuf): Pass the peer by const pointer and do not pass ownership. */ +/* Check the peer. Callee takes ownership of the peer object. + The callback will include the resulting auth_context. */ void grpc_security_connector_check_peer(grpc_exec_ctx *exec_ctx, grpc_security_connector *sc, tsi_peer peer, @@ -160,30 +158,24 @@ typedef void (*grpc_security_call_host_check_cb)(grpc_exec_ctx *exec_ctx, struct grpc_channel_security_connector { grpc_security_connector base; /* requires is_client_side to be non 0. */ grpc_call_credentials *request_metadata_creds; - grpc_security_status (*check_call_host)(grpc_exec_ctx *exec_ctx, - grpc_channel_security_connector *sc, - const char *host, - grpc_security_call_host_check_cb cb, - void *user_data); + void (*check_call_host)(grpc_exec_ctx *exec_ctx, + grpc_channel_security_connector *sc, const char *host, + grpc_auth_context *auth_context, + grpc_security_call_host_check_cb cb, void *user_data); }; -/* Checks that the host that will be set for a call is acceptable. - Implementations can choose do the check either synchronously or - asynchronously. In the first case, a successful call will return - GRPC_SECURITY_OK. In the asynchronous case, the call will return - GRPC_SECURITY_PENDING unless an error is detected early on. - TODO(jboeuf): add a grpc_auth_context param to test against. */ -grpc_security_status grpc_channel_security_connector_check_call_host( +/* Checks that the host that will be set for a call is acceptable. */ +void grpc_channel_security_connector_check_call_host( grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc, - const char *host, grpc_security_call_host_check_cb cb, void *user_data); + const char *host, grpc_auth_context *auth_context, + grpc_security_call_host_check_cb cb, void *user_data); /* --- Creation security connectors. --- */ /* For TESTING ONLY! Creates a fake connector that emulates real channel security. */ grpc_channel_security_connector *grpc_fake_channel_security_connector_create( - grpc_call_credentials *request_metadata_creds, - int call_host_check_is_async); + grpc_call_credentials *request_metadata_creds); /* For TESTING ONLY! Creates a fake connector that emulates real server security. */ @@ -247,5 +239,8 @@ const tsi_peer_property *tsi_peer_get_property_by_name(const tsi_peer *peer, /* Exposed for testing only. */ grpc_auth_context *tsi_ssl_peer_to_auth_context(const tsi_peer *peer); +tsi_peer tsi_shallow_peer_from_auth_context( + const grpc_auth_context *auth_context); +void tsi_shallow_peer_destruct(tsi_peer *peer); #endif /* GRPC_INTERNAL_CORE_SECURITY_SECURITY_CONNECTOR_H */ diff --git a/test/core/security/security_connector_test.c b/test/core/security/security_connector_test.c index 3f6c592b0bd..2f417f891e7 100644 --- a/test/core/security/security_connector_test.c +++ b/test/core/security/security_connector_test.c @@ -242,6 +242,8 @@ static void test_cn_and_multiple_sans_and_others_ssl_peer_to_auth_context( GRPC_AUTH_CONTEXT_UNREF(ctx, "test"); } +/* TODO(jboeuf): Unit-test tsi_shallow_peer_from_auth_context. */ + int main(int argc, char **argv) { grpc_test_init(argc, argv); grpc_init();