From 9ffc2c6447f2177ff406a9f4d17d8413967ab7ad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina@redhat.com>
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 <krb5/krb5.h>
+#include <dhash.h>
#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 <dhash.h>
#include <sys/types.h>
#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