From 9ffc2c6447f2177ff406a9f4d17d8413967ab7ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= Date: Mon, 19 Oct 2020 12:40:07 +0200 Subject: [PATCH 15/19] kcm: store credentials list in hash table to avoid cache lookups Iteration over ccache requires CRED_UUID_LIST and then calling CRED_BY_UUID for each uuid in the obtained list. Each CRED_BY_UUID operation invoked ldb_search and decryption. This was a substantional bottle neck. Resolves: https://github.com/SSSD/sssd/issues/5349 :fixes: KCM performance has improved dramatically for cases where large amount of credentials are stored in the ccache. --- src/responder/kcm/kcmsrv_ccache.c | 46 +++++ src/responder/kcm/kcmsrv_ccache.h | 7 + src/responder/kcm/kcmsrv_ccache_mem.c | 30 ++-- src/responder/kcm/kcmsrv_ops.c | 245 +++++++++++++++++++------- src/responder/kcm/kcmsrv_ops.h | 5 +- 5 files changed, 249 insertions(+), 84 deletions(-) diff --git a/src/responder/kcm/kcmsrv_ccache.c b/src/responder/kcm/kcmsrv_ccache.c index 59f8a7293fa7422c199ca2916c8e6ae6039d9312..60eacd4516b1269168caea744d91377686ab03f6 100644 --- a/src/responder/kcm/kcmsrv_ccache.c +++ b/src/responder/kcm/kcmsrv_ccache.c @@ -28,6 +28,9 @@ #include "responder/kcm/kcmsrv_ccache_pvt.h" #include "responder/kcm/kcmsrv_ccache_be.h" +static struct kcm_cred *kcm_cred_dup(TALLOC_CTX *mem_ctx, + struct kcm_cred *crd); + static int kcm_cc_destructor(struct kcm_ccache *cc) { if (cc == NULL) { @@ -94,6 +97,33 @@ done: return ret; } +struct kcm_ccache *kcm_cc_dup(TALLOC_CTX *mem_ctx, + const struct kcm_ccache *cc) +{ + struct kcm_ccache *dup; + struct kcm_cred *crd_dup; + struct kcm_cred *crd; + + dup = talloc_zero(mem_ctx, struct kcm_ccache); + if (dup == NULL) { + return NULL; + } + memcpy(dup, cc, sizeof(struct kcm_ccache)); + + dup->creds = NULL; + DLIST_FOR_EACH(crd, cc->creds) { + crd_dup = kcm_cred_dup(dup, crd); + if (crd_dup == NULL) { + talloc_free(dup); + return NULL; + } + + DLIST_ADD(dup->creds, crd_dup); + } + + return dup; +} + const char *kcm_cc_get_name(struct kcm_ccache *cc) { return cc ? cc->name : NULL; @@ -204,6 +234,22 @@ struct kcm_cred *kcm_cred_new(TALLOC_CTX *mem_ctx, return kcreds; } +static struct kcm_cred *kcm_cred_dup(TALLOC_CTX *mem_ctx, + struct kcm_cred *crd) +{ + struct kcm_cred *dup; + + dup = talloc_zero(mem_ctx, struct kcm_cred); + if (dup == NULL) { + return NULL; + } + + uuid_copy(dup->uuid, crd->uuid); + dup->cred_blob = crd->cred_blob; + + return dup; +} + /* Add a cred to ccache */ errno_t kcm_cc_store_creds(struct kcm_ccache *cc, struct kcm_cred *crd) diff --git a/src/responder/kcm/kcmsrv_ccache.h b/src/responder/kcm/kcmsrv_ccache.h index b0a7acb9fed8a8f89a3d0e2239ab28c7ce80fa23..77cf8f61d563d29afe00d8a04e8053b24547746d 100644 --- a/src/responder/kcm/kcmsrv_ccache.h +++ b/src/responder/kcm/kcmsrv_ccache.h @@ -72,6 +72,13 @@ errno_t kcm_cc_new(TALLOC_CTX *mem_ctx, krb5_principal princ, struct kcm_ccache **_cc); +/* + * Duplicate the ccache. Only ccache and credentials are duplicated, + * but their data are a shallow copy. + */ +struct kcm_ccache *kcm_cc_dup(TALLOC_CTX *mem_ctx, + const struct kcm_ccache *cc); + /* * Returns true if a client can access a ccache. * diff --git a/src/responder/kcm/kcmsrv_ccache_mem.c b/src/responder/kcm/kcmsrv_ccache_mem.c index baa698054fa4c6952b41b0f25dfdfa825f8e675b..0e3a7b239eda83c9fdec3b116231d4ec1444ef10 100644 --- a/src/responder/kcm/kcmsrv_ccache_mem.c +++ b/src/responder/kcm/kcmsrv_ccache_mem.c @@ -49,24 +49,6 @@ struct ccdb_mem { unsigned int nextid; }; -/* In order to provide a consistent interface, we need to let the caller - * of getbyXXX own the ccache, therefore the memory back end returns a shallow - * copy of the ccache - */ -static struct kcm_ccache *kcm_ccache_dup(TALLOC_CTX *mem_ctx, - struct kcm_ccache *in) -{ - struct kcm_ccache *out; - - out = talloc_zero(mem_ctx, struct kcm_ccache); - if (out == NULL) { - return NULL; - } - memcpy(out, in, sizeof(struct kcm_ccache)); - - return out; -} - static struct ccache_mem_wrap *memdb_get_by_uuid(struct ccdb_mem *memdb, struct cli_creds *client, uuid_t uuid) @@ -417,7 +399,11 @@ static struct tevent_req *ccdb_mem_getbyuuid_send(TALLOC_CTX *mem_ctx, ccwrap = memdb_get_by_uuid(memdb, client, uuid); if (ccwrap != NULL) { - state->cc = kcm_ccache_dup(state, ccwrap->cc); + /* In order to provide a consistent interface, we need to let the caller + * of getbyXXX own the ccache, therefore the memory back end returns a shallow + * copy of the ccache + */ + state->cc = kcm_cc_dup(state, ccwrap->cc); if (state->cc == NULL) { ret = ENOMEM; goto immediate; @@ -470,7 +456,11 @@ static struct tevent_req *ccdb_mem_getbyname_send(TALLOC_CTX *mem_ctx, ccwrap = memdb_get_by_name(memdb, client, name); if (ccwrap != NULL) { - state->cc = kcm_ccache_dup(state, ccwrap->cc); + /* In order to provide a consistent interface, we need to let the caller + * of getbyXXX own the ccache, therefore the memory back end returns a shallow + * copy of the ccache + */ + state->cc = kcm_cc_dup(state, ccwrap->cc); if (state->cc == NULL) { ret = ENOMEM; goto immediate; diff --git a/src/responder/kcm/kcmsrv_ops.c b/src/responder/kcm/kcmsrv_ops.c index 6ae1f0c647f4d385477ddeadbad93287cba05c55..f458c724b0eaa3d43df4ad30baa3f896b8d87965 100644 --- a/src/responder/kcm/kcmsrv_ops.c +++ b/src/responder/kcm/kcmsrv_ops.c @@ -22,9 +22,11 @@ #include "config.h" #include +#include #include "util/sss_iobuf.h" #include "util/sss_krb5.h" +#include "util/sss_ptr_hash.h" #include "util/util_creds.h" #include "responder/kcm/kcm.h" #include "responder/kcm/kcmsrv_pvt.h" @@ -1074,6 +1076,73 @@ static void kcm_op_get_principal_getbyname_done(struct tevent_req *subreq) tevent_req_done(req); } +static void +kcm_creds_table_delete_cb(hash_entry_t *item, + hash_destroy_enum deltype, + void *pvt) +{ + /* Delete the old credential if it is being overwritten. */ + talloc_free(item->value.ptr); +} + +/* Store credentials in a hash table. + * + * If the table already exist we add the new credentials to the table and + * overwrite the ones that already exist. This allows us to correctly serve + * also parallel GET_CRED_UUID_LIST requests from the same connection since + * it will have its own uuid list and cursor on the client side and we make + * all uuid (old, updated and newly added) available. + */ +static errno_t +kcm_creds_to_table(TALLOC_CTX *mem_ctx, + struct kcm_cred *creds, + hash_table_t **_table) +{ + char str[UUID_STR_SIZE]; + uuid_t uuid; + errno_t ret; + + if (*_table == NULL) { + *_table = sss_ptr_hash_create(mem_ctx, kcm_creds_table_delete_cb, NULL); + if (*_table == NULL) { + return ENOMEM; + } + } + + for (struct kcm_cred *crd = creds; + crd != NULL; + crd = kcm_cc_next_cred(crd)) { + ret = kcm_cred_get_uuid(crd, uuid); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, "Credential has no UUID, skipping\n"); + continue; + } + uuid_unparse(uuid, str); + + ret = sss_ptr_hash_add_or_override(*_table, str, crd, struct kcm_cred); + if (ret != EOK) { + return ret; + } + + talloc_steal(*_table, crd); + } + + return EOK; +} + +static struct kcm_cred * +kcm_creds_lookup(hash_table_t *table, uuid_t uuid) +{ + char str[UUID_STR_SIZE]; + + if (uuid == NULL) { + return NULL; + } + + uuid_unparse(uuid, str); + return sss_ptr_hash_lookup(table, str, struct kcm_cred); +} + /* (name) -> (uuid, ...) */ static void kcm_op_get_cred_uuid_list_getbyname_done(struct tevent_req *subreq); @@ -1123,12 +1192,15 @@ static void kcm_op_get_cred_uuid_list_getbyname_done(struct tevent_req *subreq) errno_t ret; struct kcm_ccache *cc; struct kcm_cred *crd; + struct kcm_conn_data *conn_data; uuid_t uuid; struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req); struct kcm_op_common_state *state = tevent_req_data(req, struct kcm_op_common_state); + conn_data = state->op_ctx->conn_data; + ret = kcm_ccdb_getbyname_recv(subreq, state, &cc); talloc_zfree(subreq); if (ret != EOK) { @@ -1140,12 +1212,20 @@ static void kcm_op_get_cred_uuid_list_getbyname_done(struct tevent_req *subreq) } if (cc == NULL) { - DEBUG(SSSDBG_MINOR_FAILURE, "No credentials by that UUID\n"); + DEBUG(SSSDBG_MINOR_FAILURE, "No ccache by that name\n"); state->op_ret = ERR_NO_CREDS; tevent_req_done(req); return; } + ret = kcm_creds_to_table(conn_data, kcm_cc_get_cred(cc), &conn_data->creds); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "Unable to build credentials hash table " + "[%d]: %s\n", ret, sss_strerror(ret)); + tevent_req_error(req, ret); + return; + } + for (crd = kcm_cc_get_cred(cc); crd != NULL; crd = kcm_cc_next_cred(crd)) { @@ -1172,6 +1252,34 @@ static void kcm_op_get_cred_uuid_list_getbyname_done(struct tevent_req *subreq) tevent_req_done(req); } +static errno_t +kcm_op_get_cred_by_uuid_reply(struct kcm_cred *crd, + struct sss_iobuf *reply) +{ + struct sss_iobuf *cred_blob; + errno_t ret; + + cred_blob = kcm_cred_get_creds(crd); + if (cred_blob == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Credentials lack the creds blob\n"); + return ERR_NO_CREDS; + } + + ret = sss_iobuf_write_len(reply, sss_iobuf_get_data(cred_blob), + sss_iobuf_get_size(cred_blob)); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "Cannot write ccache blob [%d]: %s\n", + ret, sss_strerror(ret)); + } + + return ret; +} + +struct kcm_op_get_cred_by_uuid_state { + struct kcm_op_common_state common; + uuid_t uuid; +}; + /* (name, uuid) -> (cred) */ static void kcm_op_get_cred_by_uuid_getbyname_done(struct tevent_req *subreq); @@ -1182,20 +1290,51 @@ kcm_op_get_cred_by_uuid_send(TALLOC_CTX *mem_ctx, { struct tevent_req *req = NULL; struct tevent_req *subreq = NULL; - struct kcm_op_common_state *state = NULL; + struct kcm_op_get_cred_by_uuid_state *state; + struct kcm_cred *crd; errno_t ret; const char *name; - req = tevent_req_create(mem_ctx, &state, struct kcm_op_common_state); + req = tevent_req_create(mem_ctx, &state, + struct kcm_op_get_cred_by_uuid_state); if (req == NULL) { return NULL; } - state->op_ctx = op_ctx; + state->common.op_ctx = op_ctx; ret = sss_iobuf_read_stringz(op_ctx->input, &name); if (ret != EOK) { goto immediate; } + + ret = sss_iobuf_read_len(state->common.op_ctx->input, UUID_BYTES, + state->uuid); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "Cannot read input UUID [%d]: %s\n", + ret, sss_strerror(ret)); + goto immediate; + } + + if (op_ctx->conn_data->creds != NULL) { + crd = kcm_creds_lookup(op_ctx->conn_data->creds, state->uuid); + if (crd == NULL) { + /* This should not happen, it can only happen if wrong UUID was + * requested which suggests bug in the caller application. */ + DEBUG(SSSDBG_MINOR_FAILURE, "No credentials by that UUID\n"); + kcm_debug_uuid(state->uuid); + state->common.op_ret = ERR_KCM_CC_END; + ret = EOK; + goto immediate; + } else { + ret = kcm_op_get_cred_by_uuid_reply(crd, op_ctx->reply); + if (ret == ERR_NO_CREDS) { + state->common.op_ret = ret; + ret = EOK; + } + goto immediate; + } + } + DEBUG(SSSDBG_TRACE_LIBS, "Returning creds by UUID for %s\n", name); subreq = kcm_ccdb_getbyname_send(state, ev, @@ -1210,7 +1349,11 @@ kcm_op_get_cred_by_uuid_send(TALLOC_CTX *mem_ctx, return req; immediate: - tevent_req_error(req, ret); + if (ret == EOK) { + tevent_req_done(req); + } else { + tevent_req_error(req, ret); + } tevent_req_post(req, ev); return req; } @@ -1219,14 +1362,14 @@ static void kcm_op_get_cred_by_uuid_getbyname_done(struct tevent_req *subreq) { struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req); - struct kcm_op_common_state *state = tevent_req_data(req, - struct kcm_op_common_state); + struct kcm_op_get_cred_by_uuid_state *state = tevent_req_data(req, + struct kcm_op_get_cred_by_uuid_state); errno_t ret; struct kcm_ccache *cc; struct kcm_cred *crd; - uuid_t uuid_in; - uuid_t uuid; - struct sss_iobuf *cred_blob; + struct kcm_conn_data *conn_data; + + conn_data = state->common.op_ctx->conn_data; ret = kcm_ccdb_getbyname_recv(subreq, state, &cc); talloc_zfree(subreq); @@ -1238,69 +1381,45 @@ static void kcm_op_get_cred_by_uuid_getbyname_done(struct tevent_req *subreq) return; } - if (cc == NULL) { - DEBUG(SSSDBG_MINOR_FAILURE, "No credentials by that name\n"); - state->op_ret = ERR_NO_MATCHING_CREDS; - tevent_req_done(req); - return; - } - - ret = sss_iobuf_read_len(state->op_ctx->input, - UUID_BYTES, uuid_in); + ret = kcm_creds_to_table(conn_data, kcm_cc_get_cred(cc), &conn_data->creds); if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "Cannot read input UUID [%d]: %s\n", - ret, sss_strerror(ret)); + DEBUG(SSSDBG_OP_FAILURE, "Unable to build credentials hash table " + "[%d]: %s\n", ret, sss_strerror(ret)); tevent_req_error(req, ret); return; } - for (crd = kcm_cc_get_cred(cc); - crd != NULL; - crd = kcm_cc_next_cred(crd)) { - ret = kcm_cred_get_uuid(crd, uuid); - if (ret != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, - "Cannot get UUID from creds, skipping\n"); - continue; - } - - if (uuid_compare(uuid, uuid_in) == 0) { - break; + if (conn_data->creds != NULL) { + crd = kcm_creds_lookup(conn_data->creds, state->uuid); + if (crd == NULL) { + DEBUG(SSSDBG_MINOR_FAILURE, "No credentials by that UUID\n"); + kcm_debug_uuid(state->uuid); + state->common.op_ret = ERR_KCM_CC_END; + } else { + ret = kcm_op_get_cred_by_uuid_reply(crd, state->common.op_ctx->reply); + if (ret != EOK && ret != ERR_NO_CREDS) { + tevent_req_error(req, ret); + return; + } + state->common.op_ret = ret; } - kcm_debug_uuid(uuid); - } - - if (crd == NULL) { - state->op_ret = ERR_KCM_CC_END; - DEBUG(SSSDBG_MINOR_FAILURE, "No credentials by that UUID\n"); - tevent_req_done(req); - return; - } - - cred_blob = kcm_cred_get_creds(crd); - if (cred_blob == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "Credentials lack the creds blob\n"); - state->op_ret = ERR_NO_CREDS; - tevent_req_done(req); - return; - } - - ret = sss_iobuf_write_len(state->op_ctx->reply, - sss_iobuf_get_data(cred_blob), - sss_iobuf_get_size(cred_blob)); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "Cannot write ccache blob [%d]: %s\n", - ret, sss_strerror(ret)); - tevent_req_error(req, ret); - return; } - state->op_ret = EOK; tevent_req_done(req); } +static errno_t kcm_op_get_cred_by_uuid_recv(struct tevent_req *req, + uint32_t *_op_ret) +{ + struct kcm_op_get_cred_by_uuid_state *state; + + state = tevent_req_data(req, struct kcm_op_get_cred_by_uuid_state); + + TEVENT_REQ_RETURN_ON_ERROR(req); + *_op_ret = state->common.op_ret; + return EOK; +} + /* (name, flags, credtag) -> () */ /* FIXME */ static struct tevent_req * @@ -2156,7 +2275,7 @@ static struct kcm_op kcm_optable[] = { { "RETRIEVE", NULL, NULL }, { "GET_PRINCIPAL", kcm_op_get_principal_send, NULL }, { "GET_CRED_UUID_LIST", kcm_op_get_cred_uuid_list_send, NULL }, - { "GET_CRED_BY_UUID", kcm_op_get_cred_by_uuid_send, NULL }, + { "GET_CRED_BY_UUID", kcm_op_get_cred_by_uuid_send, kcm_op_get_cred_by_uuid_recv }, { "REMOVE_CRED", kcm_op_remove_cred_send, NULL }, { "SET_FLAGS", NULL, NULL }, { "CHOWN", NULL, NULL }, diff --git a/src/responder/kcm/kcmsrv_ops.h b/src/responder/kcm/kcmsrv_ops.h index fd2dd03c9da3660e0c1346752e4db59c7cbe2c41..ab6c13791baa43837cf84ebd523735b622a24020 100644 --- a/src/responder/kcm/kcmsrv_ops.h +++ b/src/responder/kcm/kcmsrv_ops.h @@ -24,6 +24,7 @@ #include "config.h" +#include #include #include "util/sss_iobuf.h" #include "responder/kcm/kcmsrv_pvt.h" @@ -33,7 +34,9 @@ struct kcm_op *kcm_get_opt(uint16_t opcode); const char *kcm_opt_name(struct kcm_op *op); struct kcm_conn_data { - void *data; + /* Credentials obtained by GET_CRED_UUID_LIST. We use to improve performance + * by avoiding ccache lookups in GET_CRED_BY_UUID. */ + hash_table_t *creds; }; struct tevent_req *kcm_cmd_send(TALLOC_CTX *mem_ctx, -- 2.25.4