URL: https://github.com/SSSD/sssd/pull/225 Author: jhrozek Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/225/head:pr225 git checkout pr225
From 69cd66daf3478d79fd222c78b5552ba933874d1a Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 30 May 2017 12:19:53 +0200 Subject: [PATCH 01/10] SECRETS: Remove unused declarations --- src/responder/secrets/secsrv.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/responder/secrets/secsrv.h b/src/responder/secrets/secsrv.h index 3d23c405b..0575cbaba 100644 --- a/src/responder/secrets/secsrv.h +++ b/src/responder/secrets/secsrv.h @@ -32,8 +32,6 @@ #define SEC_NET_TIMEOUT 5 -struct resctx; - struct sec_ctx { struct resolv_ctx *resctx; struct resp_ctx *rctx; From f1265ce9b926ab2d7e43b0ff611b817225abf6e7 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Mon, 5 Jun 2017 15:19:13 +0200 Subject: [PATCH 02/10] SECRETS: Do not link with c-ares Since we started using libcurl for the proxy provider, there is no point in initializing or linking against c-ares. If we want to explicitly use a resolver in the future, we should use libcurl callbacks. --- Makefile.am | 1 - src/responder/secrets/proxy.c | 2 -- src/responder/secrets/secsrv.c | 6 ------ src/responder/secrets/secsrv.h | 3 --- 4 files changed, 12 deletions(-) diff --git a/Makefile.am b/Makefile.am index 5635a8c8f..e0f0bff44 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1491,7 +1491,6 @@ sssd_secrets_SOURCES = \ src/util/sss_iobuf.c \ src/util/tev_curl.c \ $(SSSD_RESPONDER_OBJ) \ - $(SSSD_RESOLV_OBJ) \ $(NULL) sssd_secrets_LDADD = \ $(HTTP_PARSER_LIBS) \ diff --git a/src/responder/secrets/proxy.c b/src/responder/secrets/proxy.c index a4e97f83e..a910b3853 100644 --- a/src/responder/secrets/proxy.c +++ b/src/responder/secrets/proxy.c @@ -29,7 +29,6 @@ #define SEC_PROXY_TIMEOUT 5 struct proxy_context { - struct resolv_ctx *resctx; struct confdb_ctx *cdb; struct tcurl_ctx *tcurl; }; @@ -585,7 +584,6 @@ int proxy_secrets_provider_handle(struct sec_ctx *sctx, pctx = talloc(handle, struct proxy_context); if (!pctx) return ENOMEM; - pctx->resctx = sctx->resctx; pctx->cdb = sctx->rctx->cdb; pctx->tcurl = tcurl_init(pctx, sctx->rctx->ev); if (pctx->tcurl == NULL) { diff --git a/src/responder/secrets/secsrv.c b/src/responder/secrets/secsrv.c index b0467e90e..ae2a658ae 100644 --- a/src/responder/secrets/secsrv.c +++ b/src/responder/secrets/secsrv.c @@ -162,12 +162,6 @@ static int sec_process_init(TALLOC_CTX *mem_ctx, goto fail; } - ret = resolv_init(sctx, ev, SEC_NET_TIMEOUT, &sctx->resctx); - if (ret != EOK) { - /* not fatal for now */ - DEBUG(SSSDBG_CRIT_FAILURE, "Failed to initialize resolver library\n"); - } - /* Set up file descriptor limits */ responder_set_fd_limit(sctx->fd_limit); diff --git a/src/responder/secrets/secsrv.h b/src/responder/secrets/secsrv.h index 0575cbaba..1aad272da 100644 --- a/src/responder/secrets/secsrv.h +++ b/src/responder/secrets/secsrv.h @@ -30,10 +30,7 @@ #include <tevent.h> #include <ldb.h> -#define SEC_NET_TIMEOUT 5 - struct sec_ctx { - struct resolv_ctx *resctx; struct resp_ctx *rctx; int fd_limit; int containers_nest_level; From c0e9fe38ab9cdcf9d03cc5db95b7549fd5cadfa0 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 30 May 2017 12:31:57 +0200 Subject: [PATCH 03/10] SECRETS: Store quotas in a per-hive configuration structure Adds two new structures to hold the quotas and associate a quota with a hive. This is just an internal change for now, but will allow us to read quota configuration from per-hive sections later. --- src/responder/secrets/local.c | 21 +++++++++------------ src/responder/secrets/secsrv.c | 6 +++--- src/responder/secrets/secsrv.h | 17 ++++++++++++++--- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c index 66401ef50..0b879939f 100644 --- a/src/responder/secrets/local.c +++ b/src/responder/secrets/local.c @@ -34,9 +34,8 @@ struct local_context { struct ldb_context *ldb; struct sec_data master_key; - int containers_nest_level; - int max_secrets; - int max_payload_size; + + struct sec_quota *quota_secrets; }; static int local_decrypt(struct local_context *lctx, TALLOC_CTX *mem_ctx, @@ -398,11 +397,11 @@ static int local_db_check_containers_nest_level(struct local_context *lctx, /* We need do not care for the synthetic containers that constitute the * base path (cn=<uidnumber>,cn=user,cn=secrets). */ nest_level = ldb_dn_get_comp_num(leaf_dn) - 3; - if (nest_level > lctx->containers_nest_level) { + if (nest_level > lctx->quota_secrets->containers_nest_level) { DEBUG(SSSDBG_OP_FAILURE, "Cannot create a nested container of depth %d as the maximum" "allowed number of nested containers is %d.\n", - nest_level, lctx->containers_nest_level); + nest_level, lctx->quota_secrets->containers_nest_level); return ERR_SEC_INVALID_CONTAINERS_NEST_LEVEL; } @@ -430,10 +429,10 @@ static int local_db_check_number_of_secrets(TALLOC_CTX *mem_ctx, ret = ldb_search(lctx->ldb, tmp_ctx, &res, dn, LDB_SCOPE_SUBTREE, attrs, LOCAL_SIMPLE_FILTER); - if (res->count >= lctx->max_secrets) { + if (res->count >= lctx->quota_secrets->max_secrets) { DEBUG(SSSDBG_OP_FAILURE, "Cannot store any more secrets as the maximum allowed limit (%d) " - "has been reached\n", lctx->max_secrets); + "has been reached\n", lctx->quota_secrets->max_secrets); ret = ERR_SEC_INVALID_TOO_MANY_SECRETS; goto done; @@ -451,14 +450,14 @@ static int local_check_max_payload_size(struct local_context *lctx, { int max_payload_size; - max_payload_size = lctx->max_payload_size * 1024; /* kb */ + max_payload_size = lctx->quota_secrets->max_payload_size * 1024; /* kb */ if (payload_size > max_payload_size) { DEBUG(SSSDBG_OP_FAILURE, "Secrets' payload size [%d kb (%d)] exceeds the maximum allowed " "payload size [%d kb (%d)]\n", payload_size * 1024, /* kb */ payload_size, - lctx->max_payload_size, /* kb */ + lctx->quota_secrets->max_payload_size, /* kb */ max_payload_size); return ERR_SEC_PAYLOAD_SIZE_IS_TOO_LARGE; @@ -1019,9 +1018,7 @@ int local_secrets_provider_handle(struct sec_ctx *sctx, return EIO; } - lctx->containers_nest_level = sctx->containers_nest_level; - lctx->max_secrets = sctx->max_secrets; - lctx->max_payload_size = sctx->max_payload_size; + lctx->quota_secrets = &sctx->sec_config.quota; lctx->master_key.data = talloc_size(lctx, MKEY_SIZE); if (!lctx->master_key.data) return ENOMEM; diff --git a/src/responder/secrets/secsrv.c b/src/responder/secrets/secsrv.c index ae2a658ae..e3a8c1476 100644 --- a/src/responder/secrets/secsrv.c +++ b/src/responder/secrets/secsrv.c @@ -52,7 +52,7 @@ static int sec_get_config(struct sec_ctx *sctx) sctx->rctx->confdb_service_path, CONFDB_SEC_CONTAINERS_NEST_LEVEL, DEFAULT_SEC_CONTAINERS_NEST_LEVEL, - &sctx->containers_nest_level); + &sctx->sec_config.quota.containers_nest_level); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, @@ -64,7 +64,7 @@ static int sec_get_config(struct sec_ctx *sctx) sctx->rctx->confdb_service_path, CONFDB_SEC_MAX_SECRETS, DEFAULT_SEC_MAX_SECRETS, - &sctx->max_secrets); + &sctx->sec_config.quota.max_secrets); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, @@ -76,7 +76,7 @@ static int sec_get_config(struct sec_ctx *sctx) sctx->rctx->confdb_service_path, CONFDB_SEC_MAX_PAYLOAD_SIZE, DEFAULT_SEC_MAX_PAYLOAD_SIZE, - &sctx->max_payload_size); + &sctx->sec_config.quota.max_payload_size); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, diff --git a/src/responder/secrets/secsrv.h b/src/responder/secrets/secsrv.h index 1aad272da..629b027f6 100644 --- a/src/responder/secrets/secsrv.h +++ b/src/responder/secrets/secsrv.h @@ -30,12 +30,23 @@ #include <tevent.h> #include <ldb.h> +struct sec_quota { + int max_secrets; + int max_payload_size; + int containers_nest_level; +}; + +struct sec_hive_config { + const char *confdb_section; + + struct sec_quota quota; +}; + struct sec_ctx { struct resp_ctx *rctx; int fd_limit; - int containers_nest_level; - int max_secrets; - int max_payload_size; + + struct sec_hive_config sec_config; struct provider_handle **providers; }; From 69217b40ff323e3c3fed0b798f99adb67330cfaf Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 30 May 2017 12:51:19 +0200 Subject: [PATCH 04/10] SECRETS: Read the quotas for cn=secrets from [secrets/secrets] configuration subsection This patch makes obsoletes the old way of configuring quotas for the secrets responder. Instead, adds a new way of configuring each hive separately in a configuration subsection, e.g. [secrets/secrets] max_secrets = 123 The old way is still supported as a backwards-compatible method. --- src/config/cfg_rules.ini | 10 ++++ src/man/sssd-secrets.5.xml | 45 +++++++++++++- src/responder/secrets/secsrv.c | 133 ++++++++++++++++++++++++++++++++++------- src/tests/intg/test_secrets.py | 127 +++++++++++++++++++++++++++++---------- 4 files changed, 259 insertions(+), 56 deletions(-) diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 464346771..0a6e28d57 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -11,6 +11,7 @@ section = ifp section = secrets section = kcm section_re = ^secrets/users/[0-9]\+$ +section_re = ^secrets/secrets$ section_re = ^domain/[^/\@]\+$ section_re = ^domain/[^/\@]\+/[^/\@]\+$ section_re = ^application/[^/\@]\+$ @@ -254,6 +255,15 @@ option = max_secrets option = max_payload_size option = responder_idle_timeout +[rule/allowed_sec_hive_options] +validator = ini_allowed_options +section_re = ^secrets/secrets$ + +# Secrets service - per-hive configuration +option = containers_nest_level +option = max_secrets +option = max_payload_size + [rule/allowed_sec_users_options] validator = ini_allowed_options section_re = ^secrets/users/[0-9]\+$ diff --git a/src/man/sssd-secrets.5.xml b/src/man/sssd-secrets.5.xml index 44a86c3fb..d50cb13d8 100644 --- a/src/man/sssd-secrets.5.xml +++ b/src/man/sssd-secrets.5.xml @@ -57,6 +57,32 @@ collide between users. Secrets can be stored inside <quote>containers</quote> which can be nested. </para> + <para> + Since the secrets responder can be used both externally to store + general secrets, as described in the rest of this man page, but + also internally by other SSSD components to store their secret + material, some configuration options, like quotas can be configured + per <quote>hive</quote> in a configuration subsection named after + the hive. The currently supported hives are: + <variablelist> + <varlistentry> + <term>secrets</term> + <listitem><para>secrets for general usage</para></listitem> + </varlistentry> + <varlistentry> + <term>kcm</term> + <listitem> + <para>used by the + <citerefentry> + <refentrytitle>sssd-kcm</refentrytitle> + <manvolnum>8</manvolnum> + </citerefentry> + service. + </para> + </listitem> + </varlistentry> + </variablelist> + </para> </refsect1> <refsect1 id='usage'> @@ -144,6 +170,12 @@ systemctl enable sssd-secrets.service </para> </listitem> </varlistentry> + </variablelist> + <para> + The following options affect only the secrets <quote>hive</quote> + and therefore should be set in a per-hive subsection. + </para> + <variablelist> <varlistentry> <term>containers_nest_level (integer)</term> <listitem> @@ -161,7 +193,7 @@ systemctl enable sssd-secrets.service <listitem> <para> This option specifies the maximum number of secrets that - can be stored. + can be stored in the hive. </para> <para> Default: 1024 @@ -182,6 +214,17 @@ systemctl enable sssd-secrets.service </varlistentry> </variablelist> <para> + For example, to adjust quotas differently for both the <quote>secrets</quote> + and the <quote>kcm</quote> hives, configure the following: + <programlisting> +[secrets/secrets] +max_payload_size = 128 + +[secrets/kcm] +max_payload_size = 256 + </programlisting> + </para> + <para> The following options are only applicable for configurations that use the <quote>proxy</quote> provider. </para> diff --git a/src/responder/secrets/secsrv.c b/src/responder/secrets/secsrv.c index e3a8c1476..db12cbbc3 100644 --- a/src/responder/secrets/secsrv.c +++ b/src/responder/secrets/secsrv.c @@ -33,54 +33,141 @@ #define DEFAULT_SEC_MAX_SECRETS 1024 #define DEFAULT_SEC_MAX_PAYLOAD_SIZE 16 -static int sec_get_config(struct sec_ctx *sctx) +static int sec_get_quota(struct sec_ctx *sctx, + const char *section_config_path, + int default_max_containers_nest_level, + int default_max_num_secrets, + int default_max_payload, + struct sec_quota *quota) { int ret; ret = confdb_get_int(sctx->rctx->cdb, - sctx->rctx->confdb_service_path, - CONFDB_SERVICE_FD_LIMIT, - DEFAULT_SEC_FD_LIMIT, - &sctx->fd_limit); + section_config_path, + CONFDB_SEC_CONTAINERS_NEST_LEVEL, + default_max_containers_nest_level, + "a->containers_nest_level); + if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to get file descriptors limit\n"); - goto fail; + "Failed to get container nesting level for %s\n", + section_config_path); + return ret; } ret = confdb_get_int(sctx->rctx->cdb, - sctx->rctx->confdb_service_path, - CONFDB_SEC_CONTAINERS_NEST_LEVEL, - DEFAULT_SEC_CONTAINERS_NEST_LEVEL, - &sctx->sec_config.quota.containers_nest_level); + section_config_path, + CONFDB_SEC_MAX_SECRETS, + default_max_num_secrets, + "a->max_secrets); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to get containers' maximum depth\n"); - goto fail; + "Failed to get maximum number of entries for %s\n", + section_config_path); + return ret; } ret = confdb_get_int(sctx->rctx->cdb, - sctx->rctx->confdb_service_path, - CONFDB_SEC_MAX_SECRETS, - DEFAULT_SEC_MAX_SECRETS, - &sctx->sec_config.quota.max_secrets); + section_config_path, + CONFDB_SEC_MAX_PAYLOAD_SIZE, + default_max_payload, + "a->max_payload_size); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to get maximum number of entries\n"); - goto fail; + "Failed to get payload's maximum size for an entry in %s\n", + section_config_path); + return ret; + } + + return EOK; +} + +static int sec_get_hive_config(struct sec_ctx *sctx, + const char *hive_name, + struct sec_hive_config *hive_config, + int default_max_containers_nest_level, + int default_max_num_secrets, + int default_max_payload) +{ + int ret; + TALLOC_CTX *tmp_ctx; + + tmp_ctx = talloc_new(sctx); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + hive_config->confdb_section = talloc_asprintf(sctx, + "config/secrets/%s", + hive_name); + if (hive_config->confdb_section == NULL) { + ret = ENOMEM; + goto done; } + ret = sec_get_quota(sctx, + hive_config->confdb_section, + default_max_containers_nest_level, + default_max_num_secrets, + default_max_payload, + &hive_config->quota); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "Cannot read quota settings for %s [%d]: %s\n", + hive_name, ret, sss_strerror(ret)); + goto done; + } + + ret = EOK; + +done: + talloc_free(tmp_ctx); + return ret; +} + +static int sec_get_config(struct sec_ctx *sctx) +{ + int ret; + ret = confdb_get_int(sctx->rctx->cdb, sctx->rctx->confdb_service_path, - CONFDB_SEC_MAX_PAYLOAD_SIZE, - DEFAULT_SEC_MAX_PAYLOAD_SIZE, - &sctx->sec_config.quota.max_payload_size); + CONFDB_SERVICE_FD_LIMIT, + DEFAULT_SEC_FD_LIMIT, + &sctx->fd_limit); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to get file descriptors limit\n"); + goto fail; + } + + /* Read the global quota first -- this should be removed in a future release */ + /* Note that this sets the defaults for the sec_config quota to be used + * in sec_get_hive_config() + */ + ret = sec_get_quota(sctx, + sctx->rctx->confdb_service_path, + DEFAULT_SEC_CONTAINERS_NEST_LEVEL, + DEFAULT_SEC_MAX_SECRETS, + DEFAULT_SEC_MAX_PAYLOAD_SIZE, + &sctx->sec_config.quota); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to get legacy global quotas\n"); + goto fail; + } + /* Read the per-hive configuration */ + ret = sec_get_hive_config(sctx, + "secrets", + &sctx->sec_config, + sctx->sec_config.quota.containers_nest_level, + sctx->sec_config.quota.max_secrets, + sctx->sec_config.quota.max_payload_size); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, - "Failed to get payload's maximum size for an entry\n"); + "Failed to get configuration of the secrets hive\n"); goto fail; } diff --git a/src/tests/intg/test_secrets.py b/src/tests/intg/test_secrets.py index f029e2787..1b36b09b6 100644 --- a/src/tests/intg/test_secrets.py +++ b/src/tests/intg/test_secrets.py @@ -98,8 +98,6 @@ def setup_for_secrets(request): id_provider = local [secrets] - max_secrets = 10 - max_payload_size = 2 """).format(**locals()) create_conf_fixture(request, conf) @@ -166,36 +164,6 @@ def test_crd_ops(setup_for_secrets, secrets_cli): cli.del_secret("foo") assert str(err404.value).startswith("404") - # Don't allow storing more secrets after reaching the max - # number of entries. - MAX_SECRETS = 10 - - sec_value = "value" - for x in range(MAX_SECRETS): - cli.set_secret(str(x), sec_value) - - with pytest.raises(HTTPError) as err507: - cli.set_secret(str(MAX_SECRETS), sec_value) - assert str(err507.value).startswith("507") - - # Delete all stored secrets used for max secrets tests - for x in range(MAX_SECRETS): - cli.del_secret(str(x)) - - # Don't allow storing a secrets which has a payload larger - # than max_payload_size - KILOBYTE = 1024 - MAX_PAYLOAD_SIZE = 2 * KILOBYTE - - sec_value = "x" * MAX_PAYLOAD_SIZE - - cli.set_secret("foo", sec_value) - - sec_value += "x" - with pytest.raises(HTTPError) as err413: - cli.set_secret("bar", sec_value) - assert str(err413.value).startswith("413") - def run_curlwrap_tool(args, exp_http_code): cmd = subprocess.Popen(args, @@ -385,3 +353,98 @@ def test_containers(setup_for_secrets, secrets_cli): with pytest.raises(HTTPError) as err406: cli.create_container(container) assert str(err406.value).startswith("406") + + +def run_quota_test(cli, max_secrets, max_payload_size): + sec_value = "value" + for x in range(max_secrets): + cli.set_secret(str(x), sec_value) + + with pytest.raises(HTTPError) as err507: + cli.set_secret(str(max_secrets), sec_value) + assert str(err507.value).startswith("507") + + # Delete all stored secrets used for max secrets tests + for x in range(max_secrets): + cli.del_secret(str(x)) + + # Don't allow storing a secrets which has a payload larger + # than max_payload_size + KILOBYTE = 1024 + kb_payload_size = max_payload_size * KILOBYTE + + sec_value = "x" * kb_payload_size + + cli.set_secret("foo", sec_value) + + sec_value += "x" + with pytest.raises(HTTPError) as err413: + cli.set_secret("bar", sec_value) + assert str(err413.value).startswith("413") + + +@pytest.fixture +def setup_for_global_quota(request): + conf = unindent("""\ + [sssd] + domains = local + services = nss + + [domain/local] + id_provider = local + + [secrets] + max_secrets = 10 + max_payload_size = 2 + """).format(**locals()) + + create_conf_fixture(request, conf) + create_sssd_secrets_fixture(request) + return None + + +def test_global_quota(setup_for_global_quota, secrets_cli): + """ + Test that the deprecated configuration of quotas in the global + secrets section is still supported + """ + cli = secrets_cli + + # Don't allow storing more secrets after reaching the max + # number of entries. + run_quota_test(cli, 10, 2) + + +@pytest.fixture +def setup_for_secrets_quota(request): + conf = unindent("""\ + [sssd] + domains = local + services = nss + + [domain/local] + id_provider = local + + [secrets] + max_secrets = 5 + max_payload_size = 1 + + [secrets/secrets] + max_secrets = 10 + max_payload_size = 2 + """).format(**locals()) + + create_conf_fixture(request, conf) + create_sssd_secrets_fixture(request) + return None + + +def test_sec_quota(setup_for_secrets_quota, secrets_cli): + """ + Test that the new secrets/secrets section takes precedence. + """ + cli = secrets_cli + + # Don't allow storing more secrets after reaching the max + # number of entries. + run_quota_test(cli, 10, 2) From 62893f148adebd56ae1d298467beae312338cb66 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 4 Apr 2017 14:45:30 +0200 Subject: [PATCH 05/10] SECRETS: Rename local_db_req.basedn to local_db_req.req_dn This will make it possible to reuse the basedn name later for the "hive" base DN in order to differentiate quotas for different hives. There is no functional change in this patch. --- src/responder/secrets/local.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c index 0b879939f..c833f1d27 100644 --- a/src/responder/secrets/local.c +++ b/src/responder/secrets/local.c @@ -206,7 +206,7 @@ static char *local_dn_to_path(TALLOC_CTX *mem_ctx, struct local_db_req { char *path; - struct ldb_dn *basedn; + struct ldb_dn *req_dn; }; #define LOCAL_SIMPLE_FILTER "(type=simple)" @@ -231,9 +231,9 @@ static int local_db_get_simple(TALLOC_CTX *mem_ctx, DEBUG(SSSDBG_TRACE_INTERNAL, "Searching for [%s] at [%s] with scope=base\n", - LOCAL_SIMPLE_FILTER, ldb_dn_get_linearized(lc_req->basedn)); + LOCAL_SIMPLE_FILTER, ldb_dn_get_linearized(lc_req->req_dn)); - ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->basedn, LDB_SCOPE_BASE, + ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->req_dn, LDB_SCOPE_BASE, attrs, "%s", LOCAL_SIMPLE_FILTER); if (ret != EOK) { DEBUG(SSSDBG_TRACE_LIBS, @@ -297,9 +297,9 @@ static int local_db_list_keys(TALLOC_CTX *mem_ctx, DEBUG(SSSDBG_TRACE_INTERNAL, "Searching for [%s] at [%s] with scope=subtree\n", - LOCAL_SIMPLE_FILTER, ldb_dn_get_linearized(lc_req->basedn)); + LOCAL_SIMPLE_FILTER, ldb_dn_get_linearized(lc_req->req_dn)); - ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->basedn, LDB_SCOPE_SUBTREE, + ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->req_dn, LDB_SCOPE_SUBTREE, attrs, "%s", LOCAL_SIMPLE_FILTER); if (ret != EOK) { DEBUG(SSSDBG_TRACE_LIBS, @@ -321,7 +321,7 @@ static int local_db_list_keys(TALLOC_CTX *mem_ctx, } for (unsigned i = 0; i < res->count; i++) { - keys[i] = local_dn_to_path(keys, lc_req->basedn, res->msgs[i]->dn); + keys[i] = local_dn_to_path(keys, lc_req->req_dn, res->msgs[i]->dn); if (!keys[i]) { ret = ENOMEM; goto done; @@ -483,7 +483,7 @@ static int local_db_put_simple(TALLOC_CTX *mem_ctx, ret = ENOMEM; goto done; } - msg->dn = lc_req->basedn; + msg->dn = lc_req->req_dn; /* make sure containers exist */ ret = local_db_check_containers(msg, lctx, msg->dn); @@ -587,9 +587,9 @@ static int local_db_delete(TALLOC_CTX *mem_ctx, DEBUG(SSSDBG_TRACE_INTERNAL, "Searching for [%s] at [%s] with scope=base\n", - LOCAL_CONTAINER_FILTER, ldb_dn_get_linearized(lc_req->basedn)); + LOCAL_CONTAINER_FILTER, ldb_dn_get_linearized(lc_req->req_dn)); - ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->basedn, LDB_SCOPE_BASE, + ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->req_dn, LDB_SCOPE_BASE, attrs, LOCAL_CONTAINER_FILTER); if (ret != EOK) { DEBUG(SSSDBG_TRACE_LIBS, @@ -599,8 +599,8 @@ static int local_db_delete(TALLOC_CTX *mem_ctx, if (res->count == 1) { DEBUG(SSSDBG_TRACE_INTERNAL, - "Searching for children of [%s]\n", ldb_dn_get_linearized(lc_req->basedn)); - ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->basedn, LDB_SCOPE_ONELEVEL, + "Searching for children of [%s]\n", ldb_dn_get_linearized(lc_req->req_dn)); + ret = ldb_search(lctx->ldb, tmp_ctx, &res, lc_req->req_dn, LDB_SCOPE_ONELEVEL, attrs, NULL); if (ret != EOK) { DEBUG(SSSDBG_TRACE_LIBS, @@ -612,13 +612,13 @@ static int local_db_delete(TALLOC_CTX *mem_ctx, ret = EEXIST; DEBUG(SSSDBG_OP_FAILURE, "Failed to remove '%s': Container is not empty\n", - ldb_dn_get_linearized(lc_req->basedn)); + ldb_dn_get_linearized(lc_req->req_dn)); goto done; } } - ret = ldb_delete(lctx->ldb, lc_req->basedn); + ret = ldb_delete(lctx->ldb, lc_req->req_dn); if (ret != EOK) { DEBUG(SSSDBG_TRACE_LIBS, "ldb_delete returned %d: %s\n", ret, ldb_strerror(ret)); @@ -645,7 +645,7 @@ static int local_db_create(TALLOC_CTX *mem_ctx, ret = ENOMEM; goto done; } - msg->dn = lc_req->basedn; + msg->dn = lc_req->req_dn; /* make sure containers exist */ ret = local_db_check_containers(msg, lctx, msg->dn); @@ -760,7 +760,7 @@ static int local_secrets_map_path(TALLOC_CTX *mem_ctx, goto done; } - ret = local_db_dn(mem_ctx, ldb, basedn, lc_req->path, &lc_req->basedn); + ret = local_db_dn(mem_ctx, ldb, basedn, lc_req->path, &lc_req->req_dn); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to map request to local db DN\n"); From 7a7f7bd86fb58d36eab10eb26af49a95d3a1d841 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 4 Apr 2017 15:33:38 +0200 Subject: [PATCH 06/10] SECRETS: Use separate quotas for /kcm and /secrets hives This would differentiate between out-of-capacity errors for secrets and for KCM as they are two independent trees as far as sssd-secrets is concerned. The quotas for /kcm are also different in their defaults. For the /secrets hive, we presume a large amount of small secrets. For the /kcm hive, we presume a small amount of large secrets, because the secret is a ccache which contains multiple credentials. The operations are also passed in a struct quota from the local request context instead of local_context. The quota is assigned to the request context when the hive is selected. --- src/config/cfg_rules.ini | 3 ++- src/man/sssd-secrets.5.xml | 4 ++-- src/responder/secrets/local.c | 46 ++++++++++++++++++++++++------------------ src/responder/secrets/secsrv.c | 20 ++++++++++++++++++ src/responder/secrets/secsrv.h | 1 + 5 files changed, 51 insertions(+), 23 deletions(-) diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 0a6e28d57..8db80c0de 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -12,6 +12,7 @@ section = secrets section = kcm section_re = ^secrets/users/[0-9]\+$ section_re = ^secrets/secrets$ +section_re = ^secrets/kcm$ section_re = ^domain/[^/\@]\+$ section_re = ^domain/[^/\@]\+/[^/\@]\+$ section_re = ^application/[^/\@]\+$ @@ -257,7 +258,7 @@ option = responder_idle_timeout [rule/allowed_sec_hive_options] validator = ini_allowed_options -section_re = ^secrets/secrets$ +section_re = ^secrets/\(secrets\|kcm\)$ # Secrets service - per-hive configuration option = containers_nest_level diff --git a/src/man/sssd-secrets.5.xml b/src/man/sssd-secrets.5.xml index d50cb13d8..ba77d6232 100644 --- a/src/man/sssd-secrets.5.xml +++ b/src/man/sssd-secrets.5.xml @@ -196,7 +196,7 @@ systemctl enable sssd-secrets.service can be stored in the hive. </para> <para> - Default: 1024 + Default: 1024 (secrets hive), 256 (kcm hive) </para> </listitem> </varlistentry> @@ -208,7 +208,7 @@ systemctl enable sssd-secrets.service a secret payload in kilobytes. </para> <para> - Default: 16 + Default: 16 (secrets hive), 65536 (64 MiB) (kcm hive) </para> </listitem> </varlistentry> diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c index c833f1d27..58e70f8b6 100644 --- a/src/responder/secrets/local.c +++ b/src/responder/secrets/local.c @@ -36,6 +36,7 @@ struct local_context { struct sec_data master_key; struct sec_quota *quota_secrets; + struct sec_quota *quota_kcm; }; static int local_decrypt(struct local_context *lctx, TALLOC_CTX *mem_ctx, @@ -206,7 +207,9 @@ static char *local_dn_to_path(TALLOC_CTX *mem_ctx, struct local_db_req { char *path; + const char *basedn; struct ldb_dn *req_dn; + struct sec_quota *quota; }; #define LOCAL_SIMPLE_FILTER "(type=simple)" @@ -389,7 +392,7 @@ static int local_db_check_containers(TALLOC_CTX *mem_ctx, return ret; } -static int local_db_check_containers_nest_level(struct local_context *lctx, +static int local_db_check_containers_nest_level(struct local_db_req *lc_req, struct ldb_dn *leaf_dn) { int nest_level; @@ -397,11 +400,11 @@ static int local_db_check_containers_nest_level(struct local_context *lctx, /* We need do not care for the synthetic containers that constitute the * base path (cn=<uidnumber>,cn=user,cn=secrets). */ nest_level = ldb_dn_get_comp_num(leaf_dn) - 3; - if (nest_level > lctx->quota_secrets->containers_nest_level) { + if (nest_level > lc_req->quota->containers_nest_level) { DEBUG(SSSDBG_OP_FAILURE, "Cannot create a nested container of depth %d as the maximum" "allowed number of nested containers is %d.\n", - nest_level, lctx->quota_secrets->containers_nest_level); + nest_level, lc_req->quota->containers_nest_level); return ERR_SEC_INVALID_CONTAINERS_NEST_LEVEL; } @@ -410,7 +413,8 @@ static int local_db_check_containers_nest_level(struct local_context *lctx, } static int local_db_check_number_of_secrets(TALLOC_CTX *mem_ctx, - struct local_context *lctx) + struct local_context *lctx, + struct local_db_req *lc_req) { TALLOC_CTX *tmp_ctx; static const char *attrs[] = { NULL }; @@ -421,7 +425,7 @@ static int local_db_check_number_of_secrets(TALLOC_CTX *mem_ctx, tmp_ctx = talloc_new(mem_ctx); if (!tmp_ctx) return ENOMEM; - dn = ldb_dn_new(tmp_ctx, lctx->ldb, "cn=secrets"); + dn = ldb_dn_new(tmp_ctx, lctx->ldb, lc_req->basedn); if (!dn) { ret = ENOMEM; goto done; @@ -429,11 +433,10 @@ static int local_db_check_number_of_secrets(TALLOC_CTX *mem_ctx, ret = ldb_search(lctx->ldb, tmp_ctx, &res, dn, LDB_SCOPE_SUBTREE, attrs, LOCAL_SIMPLE_FILTER); - if (res->count >= lctx->quota_secrets->max_secrets) { + if (res->count >= lc_req->quota->max_secrets) { DEBUG(SSSDBG_OP_FAILURE, "Cannot store any more secrets as the maximum allowed limit (%d) " - "has been reached\n", lctx->quota_secrets->max_secrets); - + "has been reached\n", lc_req->quota->max_secrets); ret = ERR_SEC_INVALID_TOO_MANY_SECRETS; goto done; } @@ -445,19 +448,19 @@ static int local_db_check_number_of_secrets(TALLOC_CTX *mem_ctx, return ret; } -static int local_check_max_payload_size(struct local_context *lctx, +static int local_check_max_payload_size(struct local_db_req *lc_req, int payload_size) { int max_payload_size; - max_payload_size = lctx->quota_secrets->max_payload_size * 1024; /* kb */ + max_payload_size = lc_req->quota->max_payload_size * 1024; /* kb */ if (payload_size > max_payload_size) { DEBUG(SSSDBG_OP_FAILURE, "Secrets' payload size [%d kb (%d)] exceeds the maximum allowed " "payload size [%d kb (%d)]\n", payload_size * 1024, /* kb */ payload_size, - lctx->quota_secrets->max_payload_size, /* kb */ + lc_req->quota->max_payload_size, /* kb */ max_payload_size); return ERR_SEC_PAYLOAD_SIZE_IS_TOO_LARGE; @@ -494,7 +497,7 @@ static int local_db_put_simple(TALLOC_CTX *mem_ctx, goto done; } - ret = local_db_check_number_of_secrets(msg, lctx); + ret = local_db_check_number_of_secrets(msg, lctx, lc_req); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "local_db_check_number_of_secrets failed [%d]: %s\n", @@ -502,7 +505,7 @@ static int local_db_put_simple(TALLOC_CTX *mem_ctx, goto done; } - ret = local_check_max_payload_size(lctx, strlen(secret)); + ret = local_check_max_payload_size(lc_req, strlen(secret)); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, "local_check_max_payload_size failed [%d]: %s\n", @@ -656,7 +659,7 @@ static int local_db_create(TALLOC_CTX *mem_ctx, goto done; } - ret = local_db_check_containers_nest_level(lctx, msg->dn); + ret = local_db_check_containers_nest_level(lc_req, msg->dn); if (ret != EOK) goto done; ret = ldb_msg_add_string(msg, "type", "container"); @@ -698,13 +701,13 @@ static int local_db_create(TALLOC_CTX *mem_ctx, } static int local_secrets_map_path(TALLOC_CTX *mem_ctx, - struct ldb_context *ldb, + struct local_context *lctx, struct sec_req_ctx *secreq, struct local_db_req **_lc_req) { int ret; struct local_db_req *lc_req; - const char *basedn; + struct ldb_context *ldb = lctx->ldb; /* be strict for now */ if (secreq->parsed_url.fragment != NULL) { @@ -742,12 +745,14 @@ static int local_secrets_map_path(TALLOC_CTX *mem_ctx, SEC_BASEPATH, sizeof(SEC_BASEPATH) - 1) == 0) { lc_req->path = talloc_strdup(lc_req, secreq->mapped_path + (sizeof(SEC_BASEPATH) - 1)); - basedn = SECRETS_BASEDN; + lc_req->basedn = SECRETS_BASEDN; + lc_req->quota = lctx->quota_secrets; } else if (strncmp(secreq->mapped_path, SEC_KCM_BASEPATH, sizeof(SEC_KCM_BASEPATH) - 1) == 0) { lc_req->path = talloc_strdup(lc_req, secreq->mapped_path + (sizeof(SEC_KCM_BASEPATH) - 1)); - basedn = KCM_BASEDN; + lc_req->basedn = KCM_BASEDN; + lc_req->quota = lctx->quota_kcm; } else { ret = EINVAL; goto done; @@ -760,7 +765,7 @@ static int local_secrets_map_path(TALLOC_CTX *mem_ctx, goto done; } - ret = local_db_dn(mem_ctx, ldb, basedn, lc_req->path, &lc_req->req_dn); + ret = local_db_dn(mem_ctx, ldb, lc_req->basedn, lc_req->path, &lc_req->req_dn); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to map request to local db DN\n"); @@ -829,7 +834,7 @@ static struct tevent_req *local_secret_req(TALLOC_CTX *mem_ctx, } DEBUG(SSSDBG_TRACE_LIBS, "Content-Type: %s\n", content_type); - ret = local_secrets_map_path(state, lctx->ldb, secreq, &lc_req); + ret = local_secrets_map_path(state, lctx, secreq, &lc_req); if (ret) { DEBUG(SSSDBG_OP_FAILURE, "Cannot map request path to local path\n"); goto done; @@ -1019,6 +1024,7 @@ int local_secrets_provider_handle(struct sec_ctx *sctx, } lctx->quota_secrets = &sctx->sec_config.quota; + lctx->quota_kcm = &sctx->kcm_config.quota; lctx->master_key.data = talloc_size(lctx, MKEY_SIZE); if (!lctx->master_key.data) return ENOMEM; diff --git a/src/responder/secrets/secsrv.c b/src/responder/secrets/secsrv.c index db12cbbc3..2fcdf8e6c 100644 --- a/src/responder/secrets/secsrv.c +++ b/src/responder/secrets/secsrv.c @@ -30,9 +30,17 @@ #define DEFAULT_SEC_FD_LIMIT 2048 #define DEFAULT_SEC_CONTAINERS_NEST_LEVEL 4 + #define DEFAULT_SEC_MAX_SECRETS 1024 #define DEFAULT_SEC_MAX_PAYLOAD_SIZE 16 +/* The number of secrets in the /kcm hive should be quite small, + * but the secret size must be large because one secret in the /kcm + * hive holds the whole ccache which consists of several credentials + */ +#define DEFAULT_SEC_KCM_MAX_SECRETS 256 +#define DEFAULT_SEC_KCM_MAX_PAYLOAD_SIZE 65536 + static int sec_get_quota(struct sec_ctx *sctx, const char *section_config_path, int default_max_containers_nest_level, @@ -171,6 +179,18 @@ static int sec_get_config(struct sec_ctx *sctx) goto fail; } + ret = sec_get_hive_config(sctx, + "kcm", + &sctx->kcm_config, + DEFAULT_SEC_CONTAINERS_NEST_LEVEL, + DEFAULT_SEC_KCM_MAX_SECRETS, + DEFAULT_SEC_KCM_MAX_PAYLOAD_SIZE); + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to get configuration of the secrets hive\n"); + goto fail; + } + ret = confdb_get_int(sctx->rctx->cdb, sctx->rctx->confdb_service_path, CONFDB_RESPONDER_CLI_IDLE_TIMEOUT, CONFDB_RESPONDER_CLI_IDLE_DEFAULT_TIMEOUT, diff --git a/src/responder/secrets/secsrv.h b/src/responder/secrets/secsrv.h index 629b027f6..afc092764 100644 --- a/src/responder/secrets/secsrv.h +++ b/src/responder/secrets/secsrv.h @@ -47,6 +47,7 @@ struct sec_ctx { int fd_limit; struct sec_hive_config sec_config; + struct sec_hive_config kcm_config; struct provider_handle **providers; }; From 10911c108231615f411e601188364673a78a7be4 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Tue, 4 Apr 2017 15:34:17 +0200 Subject: [PATCH 07/10] TESTS: Test that ccaches can be stored after max_secrets is reached for regular non-ccache secrets Test that even when we store the maximum number of secrets, we can still store Kerberos credentials, but only until we reach the max_secrets limit as well. --- src/tests/intg/test_kcm.py | 52 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/src/tests/intg/test_kcm.py b/src/tests/intg/test_kcm.py index ae49eca80..fd2b37e1a 100644 --- a/src/tests/intg/test_kcm.py +++ b/src/tests/intg/test_kcm.py @@ -23,12 +23,16 @@ import socket import time import signal +from requests import HTTPError import kdc import krb5utils import config from util import unindent from test_secrets import create_sssd_secrets_fixture +from secrets import SecretsLocalClient + +MAX_SECRETS = 10 class KcmTestEnv(object): @@ -109,7 +113,7 @@ def kcm_teardown(): return kcm_pid -def create_sssd_conf(kcm_path, ccache_storage): +def create_sssd_conf(kcm_path, ccache_storage, max_secrets=MAX_SECRETS): return unindent("""\ [sssd] domains = local @@ -121,6 +125,9 @@ def create_sssd_conf(kcm_path, ccache_storage): [kcm] socket_path = {kcm_path} ccache_storage = {ccache_storage} + + [secrets] + max_secrets = {max_secrets} """).format(**locals()) @@ -464,3 +471,46 @@ def test_kcm_sec_parallel_klist(setup_for_kcm_sec, for p in processes: rc = p.wait() assert rc == 0 + + +def get_secrets_socket(): + return os.path.join(config.RUNSTATEDIR, "secrets.socket") + + +@pytest.fixture +def secrets_cli(request): + sock_path = get_secrets_socket() + cli = SecretsLocalClient(sock_path=sock_path) + return cli + + +def test_kcm_secrets_quota(setup_for_kcm_sec, + setup_secrets, + secrets_cli): + testenv = setup_for_kcm_sec + cli = secrets_cli + + # Make sure the secrets store is depleted first + sec_value = "value" + for x in range(MAX_SECRETS): + cli.set_secret(str(x), sec_value) + + with pytest.raises(HTTPError) as err507: + cli.set_secret(str(MAX_SECRETS), sec_value) + assert str(err507.value).startswith("507") + + # We should still be able to store KCM ccaches, but no more + # than MAX_SECRETS + for x in range(MAX_SECRETS): + princ = "%s%d" % ("kcmtest", x) + testenv.k5kdc.add_principal(princ, princ) + + for x in range(MAX_SECRETS-1): + princ = "%s%d" % ("kcmtest", x) + out, _, _ = testenv.k5util.kinit(princ, princ) + assert out == 0 + + # we stored 0 to MAX_SECRETS-1, storing another one must fail + princ = "%s%d" % ("kcmtest", MAX_SECRETS) + out, _, _ = testenv.k5util.kinit(princ, princ) + assert out != 0 From af2bdb25ce7e20bd499fafc923f39498d011bfe9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Mon, 5 Jun 2017 16:10:55 +0200 Subject: [PATCH 08/10] SECRETS: Add a new option to control per-UID limits Adds a new option max_uid_secrets that allows to set a limit of secrets for this particular client so that the user cannot starve other users. Resolves: https://pagure.io/SSSD/sssd/issue/3363 --- src/confdb/confdb.h | 1 + src/config/SSSDConfig/__init__.py.in | 1 + src/config/cfg_rules.ini | 1 + src/config/etc/sssd.api.conf | 1 + src/man/sssd-secrets.5.xml | 12 ++++++ src/responder/secrets/local.c | 81 ++++++++++++++++++++++++++++++++++++ src/responder/secrets/secsrv.c | 23 +++++++++- src/responder/secrets/secsrv.h | 1 + src/tests/intg/test_secrets.py | 45 ++++++++++++++++++++ 9 files changed, 165 insertions(+), 1 deletion(-) diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 2ba1bc47e..0e641aa38 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -236,6 +236,7 @@ #define CONFDB_SEC_CONF_ENTRY "config/secrets" #define CONFDB_SEC_CONTAINERS_NEST_LEVEL "containers_nest_level" #define CONFDB_SEC_MAX_SECRETS "max_secrets" +#define CONFDB_SEC_MAX_UID_SECRETS "max_uid_secrets" #define CONFDB_SEC_MAX_PAYLOAD_SIZE "max_payload_size" /* KCM Service */ diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index cd844ce2b..34f78385c 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -128,6 +128,7 @@ option_strings = { 'provider': _('The provider where the secrets will be stored in'), 'containers_nest_level': _('The maximum allowed number of nested containers'), 'max_secrets': _('The maximum number of secrets that can be stored'), + 'max_uid_secrets': _('The maximum number of secrets that can be stored per UID'), 'max_payload_size': _('The maximum payload size of a secret in kilobytes'), # secrets - proxy 'proxy_url': _('The URL Custodia server is listening on'), diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 8db80c0de..3eeb58216 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -263,6 +263,7 @@ section_re = ^secrets/\(secrets\|kcm\)$ # Secrets service - per-hive configuration option = containers_nest_level option = max_secrets +option = max_uid_secrets option = max_payload_size [rule/allowed_sec_users_options] diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index 48d3b53f6..86aa99d02 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -105,6 +105,7 @@ user_attributes = str, None, false provider = str, None, false containers_nest_level = int, None, false max_secrets = int, None, false +max_uid_secrets = int, None, false max_payload_size = int, None, false # Secrets service - proxy proxy_url = str, None, false diff --git a/src/man/sssd-secrets.5.xml b/src/man/sssd-secrets.5.xml index ba77d6232..c74894c62 100644 --- a/src/man/sssd-secrets.5.xml +++ b/src/man/sssd-secrets.5.xml @@ -201,6 +201,18 @@ systemctl enable sssd-secrets.service </listitem> </varlistentry> <varlistentry> + <term>max_uid_secrets (integer)</term> + <listitem> + <para> + This option specifies the maximum number of secrets that + can be stored per-UID in the hive. + </para> + <para> + Default: 256 (secrets hive), 64 (kcm hive) + </para> + </listitem> + </varlistentry> + <varlistentry> <term>max_payload_size (integer)</term> <listitem> <para> diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c index 58e70f8b6..fe32007f7 100644 --- a/src/responder/secrets/local.c +++ b/src/responder/secrets/local.c @@ -412,6 +412,79 @@ static int local_db_check_containers_nest_level(struct local_db_req *lc_req, return EOK; } +static struct ldb_dn *per_uid_container(TALLOC_CTX *mem_ctx, + struct ldb_dn *req_dn) +{ + int user_comp; + int num_comp; + struct ldb_dn *uid_base_dn; + + uid_base_dn = ldb_dn_copy(mem_ctx, req_dn); + if (uid_base_dn == NULL) { + return NULL; + } + + /* Remove all the components up to the per-user base path which consists + * of three components: + * cn=<uidnumber>,cn=users,cn=secrets + */ + user_comp = ldb_dn_get_comp_num(uid_base_dn) - 3; + + if (!ldb_dn_remove_child_components(uid_base_dn, user_comp)) { + DEBUG(SSSDBG_OP_FAILURE, "Cannot remove child components\n"); + talloc_free(uid_base_dn); + return NULL; + } + + num_comp = ldb_dn_get_comp_num(uid_base_dn); + if (num_comp != 3) { + DEBUG(SSSDBG_OP_FAILURE, "Expected 3 components got %d\n", num_comp); + talloc_free(uid_base_dn); + return NULL; + } + + return uid_base_dn; +} + +static int local_db_check_peruid_number_of_secrets(TALLOC_CTX *mem_ctx, + struct local_context *lctx, + struct local_db_req *lc_req) +{ + TALLOC_CTX *tmp_ctx; + static const char *attrs[] = { NULL }; + struct ldb_result *res = NULL; + struct ldb_dn *cli_basedn = NULL; + int ret; + + tmp_ctx = talloc_new(mem_ctx); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + cli_basedn = per_uid_container(tmp_ctx, lc_req->req_dn); + if (cli_basedn == NULL) { + ret = ENOMEM; + goto done; + } + + ret = ldb_search(lctx->ldb, tmp_ctx, &res, cli_basedn, LDB_SCOPE_SUBTREE, + attrs, LOCAL_SIMPLE_FILTER); + if (res->count >= lc_req->quota->max_uid_secrets) { + DEBUG(SSSDBG_OP_FAILURE, + "Cannot store any more secrets for this client (basedn %s) " + "as the maximum allowed limit (%d) has been reached\n", + ldb_dn_get_linearized(cli_basedn), + lc_req->quota->max_uid_secrets); + ret = ERR_SEC_INVALID_TOO_MANY_SECRETS; + goto done; + } + + ret = EOK; +done: + talloc_free(tmp_ctx); + return ret; +} + static int local_db_check_number_of_secrets(TALLOC_CTX *mem_ctx, struct local_context *lctx, struct local_db_req *lc_req) @@ -505,6 +578,14 @@ static int local_db_put_simple(TALLOC_CTX *mem_ctx, goto done; } + ret = local_db_check_peruid_number_of_secrets(msg, lctx, lc_req); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "local_db_check_number_of_secrets failed [%d]: %s\n", + ret, sss_strerror(ret)); + goto done; + } + ret = local_check_max_payload_size(lc_req, strlen(secret)); if (ret != EOK) { DEBUG(SSSDBG_OP_FAILURE, diff --git a/src/responder/secrets/secsrv.c b/src/responder/secrets/secsrv.c index 2fcdf8e6c..36b257c46 100644 --- a/src/responder/secrets/secsrv.c +++ b/src/responder/secrets/secsrv.c @@ -31,7 +31,8 @@ #define DEFAULT_SEC_FD_LIMIT 2048 #define DEFAULT_SEC_CONTAINERS_NEST_LEVEL 4 -#define DEFAULT_SEC_MAX_SECRETS 1024 +#define DEFAULT_SEC_MAX_SECRETS 1024 +#define DEFAULT_SEC_MAX_UID_SECRETS 256 #define DEFAULT_SEC_MAX_PAYLOAD_SIZE 16 /* The number of secrets in the /kcm hive should be quite small, @@ -39,12 +40,14 @@ * hive holds the whole ccache which consists of several credentials */ #define DEFAULT_SEC_KCM_MAX_SECRETS 256 +#define DEFAULT_SEC_KCM_MAX_UID_SECRETS 64 #define DEFAULT_SEC_KCM_MAX_PAYLOAD_SIZE 65536 static int sec_get_quota(struct sec_ctx *sctx, const char *section_config_path, int default_max_containers_nest_level, int default_max_num_secrets, + int default_max_num_uid_secrets, int default_max_payload, struct sec_quota *quota) { @@ -78,6 +81,19 @@ static int sec_get_quota(struct sec_ctx *sctx, ret = confdb_get_int(sctx->rctx->cdb, section_config_path, + CONFDB_SEC_MAX_UID_SECRETS, + default_max_num_uid_secrets, + "a->max_uid_secrets); + + if (ret != EOK) { + DEBUG(SSSDBG_FATAL_FAILURE, + "Failed to get maximum number of per-UID entries for %s\n", + section_config_path); + return ret; + } + + ret = confdb_get_int(sctx->rctx->cdb, + section_config_path, CONFDB_SEC_MAX_PAYLOAD_SIZE, default_max_payload, "a->max_payload_size); @@ -97,6 +113,7 @@ static int sec_get_hive_config(struct sec_ctx *sctx, struct sec_hive_config *hive_config, int default_max_containers_nest_level, int default_max_num_secrets, + int default_max_num_uid_secrets, int default_max_payload) { int ret; @@ -119,6 +136,7 @@ static int sec_get_hive_config(struct sec_ctx *sctx, hive_config->confdb_section, default_max_containers_nest_level, default_max_num_secrets, + default_max_num_uid_secrets, default_max_payload, &hive_config->quota); if (ret != EOK) { @@ -158,6 +176,7 @@ static int sec_get_config(struct sec_ctx *sctx) sctx->rctx->confdb_service_path, DEFAULT_SEC_CONTAINERS_NEST_LEVEL, DEFAULT_SEC_MAX_SECRETS, + DEFAULT_SEC_MAX_UID_SECRETS, DEFAULT_SEC_MAX_PAYLOAD_SIZE, &sctx->sec_config.quota); if (ret != EOK) { @@ -172,6 +191,7 @@ static int sec_get_config(struct sec_ctx *sctx) &sctx->sec_config, sctx->sec_config.quota.containers_nest_level, sctx->sec_config.quota.max_secrets, + sctx->sec_config.quota.max_uid_secrets, sctx->sec_config.quota.max_payload_size); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, @@ -184,6 +204,7 @@ static int sec_get_config(struct sec_ctx *sctx) &sctx->kcm_config, DEFAULT_SEC_CONTAINERS_NEST_LEVEL, DEFAULT_SEC_KCM_MAX_SECRETS, + DEFAULT_SEC_KCM_MAX_UID_SECRETS, DEFAULT_SEC_KCM_MAX_PAYLOAD_SIZE); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, diff --git a/src/responder/secrets/secsrv.h b/src/responder/secrets/secsrv.h index afc092764..afdd731fb 100644 --- a/src/responder/secrets/secsrv.h +++ b/src/responder/secrets/secsrv.h @@ -32,6 +32,7 @@ struct sec_quota { int max_secrets; + int max_uid_secrets; int max_payload_size; int containers_nest_level; }; diff --git a/src/tests/intg/test_secrets.py b/src/tests/intg/test_secrets.py index 1b36b09b6..000b85a14 100644 --- a/src/tests/intg/test_secrets.py +++ b/src/tests/intg/test_secrets.py @@ -448,3 +448,48 @@ def test_sec_quota(setup_for_secrets_quota, secrets_cli): # Don't allow storing more secrets after reaching the max # number of entries. run_quota_test(cli, 10, 2) + + +@pytest.fixture +def setup_for_uid_limit(request): + conf = unindent("""\ + [sssd] + domains = local + services = nss + + [domain/local] + id_provider = local + + [secrets] + + [secrets/secrets] + max_secrets = 10 + max_uid_secrets = 5 + """).format(**locals()) + + create_conf_fixture(request, conf) + create_sssd_secrets_fixture(request) + return None + + +def test_per_uid_limit(setup_for_uid_limit, secrets_cli): + """ + Test that per-UID limits are enforced even if the global limit would still + allow to store more secrets + """ + cli = secrets_cli + + # Don't allow storing more secrets after reaching the max + # number of entries. + MAX_UID_SECRETS = 5 + + sec_value = "value" + for x in range(MAX_UID_SECRETS): + cli.set_secret(str(x), sec_value) + + with pytest.raises(HTTPError) as err507: + cli.set_secret(str(MAX_UID_SECRETS), sec_value) + assert str(err507.value).startswith("507") + + # FIXME - at this point, it would be nice to test that another UID can still + # store secrets, but sadly socket_wrapper doesn't allow us to fake UIDs yet From 45a6b796cef9b71f907e3d586b84ed2f41f94050 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Wed, 7 Jun 2017 15:55:39 +0200 Subject: [PATCH 09/10] CRYPTO: Do not call NSS_Shutdown after every operation Calling setup and teardown on every encryption cases issues like the one described in https://bugzilla.redhat.com/show_bug.cgi?id=1456151 eventually. Similarly to other crypto functions, don't tear down NSS by calling NSS_Shutdown. Let the OS reclaim the resources. Resolves: https://pagure.io/SSSD/sssd/issue/3424 --- src/util/crypto/nss/nss_nite.c | 2 -- src/util/crypto/nss/nss_obfuscate.c | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/util/crypto/nss/nss_nite.c b/src/util/crypto/nss/nss_nite.c index 3641e0512..db3cefa95 100644 --- a/src/util/crypto/nss/nss_nite.c +++ b/src/util/crypto/nss/nss_nite.c @@ -167,7 +167,6 @@ int sss_encrypt(TALLOC_CTX *mem_ctx, enum encmethod enctype, done: talloc_free(tmp_ctx); - nspr_nss_cleanup(); return ret; } @@ -300,6 +299,5 @@ int sss_decrypt(TALLOC_CTX *mem_ctx, enum encmethod enctype, done: talloc_free(tmp_ctx); - nspr_nss_cleanup(); return ret; } diff --git a/src/util/crypto/nss/nss_obfuscate.c b/src/util/crypto/nss/nss_obfuscate.c index a55f22b6d..df9c41b3a 100644 --- a/src/util/crypto/nss/nss_obfuscate.c +++ b/src/util/crypto/nss/nss_obfuscate.c @@ -189,7 +189,6 @@ int sss_password_encrypt(TALLOC_CTX *mem_ctx, const char *password, int plen, ret = EOK; done: talloc_free(tmp_ctx); - nspr_nss_cleanup(); return ret; } @@ -325,6 +324,5 @@ int sss_password_decrypt(TALLOC_CTX *mem_ctx, char *b64encoded, ret = EOK; done: talloc_free(tmp_ctx); - nspr_nss_cleanup(); return ret; } From dcb2fb7789b377982cbfb8fa35c5557f79ff102d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Wed, 7 Jun 2017 17:20:43 +0200 Subject: [PATCH 10/10] SECRETS: Support 0 as unlimited for the quotas Add a special value for all the quota-like settings that means 'no limit'. Because the responder also had a global limit on the size of the accepted body (64kB), this patch also removes the hardcoded limit and insteads keep track of the biggest quota value on startup. --- src/man/sssd-secrets.5.xml | 3 +- src/responder/secrets/local.c | 16 ++++++++++ src/responder/secrets/secsrv.c | 15 ++++++++++ src/responder/secrets/secsrv.h | 1 + src/responder/secrets/secsrv_cmd.c | 6 +++- src/responder/secrets/secsrv_private.h | 2 +- src/tests/intg/test_secrets.py | 55 ++++++++++++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 3 deletions(-) diff --git a/src/man/sssd-secrets.5.xml b/src/man/sssd-secrets.5.xml index c74894c62..d43dcf21c 100644 --- a/src/man/sssd-secrets.5.xml +++ b/src/man/sssd-secrets.5.xml @@ -173,7 +173,8 @@ systemctl enable sssd-secrets.service </variablelist> <para> The following options affect only the secrets <quote>hive</quote> - and therefore should be set in a per-hive subsection. + and therefore should be set in a per-hive subsection. Setting the + option to 0 means "unlimited". </para> <variablelist> <varlistentry> diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c index fe32007f7..4855fb808 100644 --- a/src/responder/secrets/local.c +++ b/src/responder/secrets/local.c @@ -397,6 +397,10 @@ static int local_db_check_containers_nest_level(struct local_db_req *lc_req, { int nest_level; + if (lc_req->quota->containers_nest_level == 0) { + return EOK; + } + /* We need do not care for the synthetic containers that constitute the * base path (cn=<uidnumber>,cn=user,cn=secrets). */ nest_level = ldb_dn_get_comp_num(leaf_dn) - 3; @@ -456,6 +460,10 @@ static int local_db_check_peruid_number_of_secrets(TALLOC_CTX *mem_ctx, struct ldb_dn *cli_basedn = NULL; int ret; + if (lc_req->quota->max_uid_secrets == 0) { + return EOK; + } + tmp_ctx = talloc_new(mem_ctx); if (tmp_ctx == NULL) { return ENOMEM; @@ -495,6 +503,10 @@ static int local_db_check_number_of_secrets(TALLOC_CTX *mem_ctx, struct ldb_dn *dn; int ret; + if (lc_req->quota->max_secrets == 0) { + return EOK; + } + tmp_ctx = talloc_new(mem_ctx); if (!tmp_ctx) return ENOMEM; @@ -526,6 +538,10 @@ static int local_check_max_payload_size(struct local_db_req *lc_req, { int max_payload_size; + if (lc_req->quota->max_payload_size == 0) { + return EOK; + } + max_payload_size = lc_req->quota->max_payload_size * 1024; /* kb */ if (payload_size > max_payload_size) { DEBUG(SSSDBG_OP_FAILURE, diff --git a/src/responder/secrets/secsrv.c b/src/responder/secrets/secsrv.c index 36b257c46..2b661b165 100644 --- a/src/responder/secrets/secsrv.c +++ b/src/responder/secrets/secsrv.c @@ -146,6 +146,16 @@ static int sec_get_hive_config(struct sec_ctx *sctx, goto done; } + if (hive_config->quota.max_payload_size == 0 + || (sctx->max_payload_size != 0 + && hive_config->quota.max_payload_size > sctx->max_payload_size)) { + /* If the quota is unlimited or it's larger than what + * we already have, save the total limit so we know how much to + * accept from clients + */ + sctx->max_payload_size = hive_config->quota.max_payload_size; + } + ret = EOK; done: @@ -168,6 +178,11 @@ static int sec_get_config(struct sec_ctx *sctx) goto fail; } + /* Set the global max_payload to ridiculously small value so that either 0 (unlimited) + * or any sensible value overwrite it + */ + sctx->max_payload_size = 1; + /* Read the global quota first -- this should be removed in a future release */ /* Note that this sets the defaults for the sec_config quota to be used * in sec_get_hive_config() diff --git a/src/responder/secrets/secsrv.h b/src/responder/secrets/secsrv.h index afdd731fb..302311640 100644 --- a/src/responder/secrets/secsrv.h +++ b/src/responder/secrets/secsrv.h @@ -49,6 +49,7 @@ struct sec_ctx { struct sec_hive_config sec_config; struct sec_hive_config kcm_config; + int max_payload_size; struct provider_handle **providers; }; diff --git a/src/responder/secrets/secsrv_cmd.c b/src/responder/secrets/secsrv_cmd.c index b88680c3d..fa5970504 100644 --- a/src/responder/secrets/secsrv_cmd.c +++ b/src/responder/secrets/secsrv_cmd.c @@ -178,7 +178,8 @@ static void sec_append_string(TALLOC_CTX *memctx, char **dest, static bool sec_too_much_data(struct sec_req_ctx *req, size_t length) { req->total_size += length; - if (req->total_size > SEC_REQUEST_MAX_SIZE) { + if (req->max_payload_size > 0 + && req->total_size > req->max_payload_size) { DEBUG(SSSDBG_FATAL_FAILURE, "Request too big, aborting client!\n"); return true; @@ -513,6 +514,8 @@ static void sec_recv(struct cli_ctx *cctx) { struct sec_proto_ctx *prctx; struct sec_req_ctx *req; + struct sec_ctx *sec_ctx = talloc_get_type(cctx->rctx->pvt_ctx, + struct sec_ctx); char buffer[SEC_PACKET_MAX_RECV_SIZE]; struct sec_data data = { buffer, SEC_PACKET_MAX_RECV_SIZE }; @@ -531,6 +534,7 @@ static void sec_recv(struct cli_ctx *cctx) return; } req->cctx = cctx; + req->max_payload_size = sec_ctx->max_payload_size; cctx->state_ctx = req; http_parser_init(&prctx->parser, HTTP_REQUEST); prctx->parser.data = req; diff --git a/src/responder/secrets/secsrv_private.h b/src/responder/secrets/secsrv_private.h index 2e68628f6..c4a0c5745 100644 --- a/src/responder/secrets/secsrv_private.h +++ b/src/responder/secrets/secsrv_private.h @@ -75,6 +75,7 @@ struct sec_req_ctx { bool complete; size_t total_size; + size_t max_payload_size; char *request_url; char *mapped_path; @@ -151,7 +152,6 @@ bool sec_req_has_header(struct sec_req_ctx *req, const char *name, const char *value); /* secsrv_cmd.c */ -#define SEC_REQUEST_MAX_SIZE 65536 #define SEC_PACKET_MAX_RECV_SIZE 8192 int sec_send_data(int fd, struct sec_data *data); diff --git a/src/tests/intg/test_secrets.py b/src/tests/intg/test_secrets.py index 000b85a14..e695fd9ce 100644 --- a/src/tests/intg/test_secrets.py +++ b/src/tests/intg/test_secrets.py @@ -493,3 +493,58 @@ def test_per_uid_limit(setup_for_uid_limit, secrets_cli): # FIXME - at this point, it would be nice to test that another UID can still # store secrets, but sadly socket_wrapper doesn't allow us to fake UIDs yet + + +@pytest.fixture +def setup_for_unlimited_quotas(request): + conf = unindent("""\ + [sssd] + domains = local + services = nss + + [domain/local] + id_provider = local + + [secrets] + debug_level = 10 + + [secrets/secrets] + max_secrets = 0 + max_uid_secrets = 0 + max_payload_size = 0 + containers_nest_level = 0 + """).format(**locals()) + + create_conf_fixture(request, conf) + create_sssd_secrets_fixture(request) + return None + + +def test_unlimited_quotas(setup_for_unlimited_quotas, secrets_cli): + """ + Test that setting quotas to zero disabled any checks and lets + store whatever. + """ + cli = secrets_cli + + # test much larger amount of secrets that we allow by default + sec_value = "value" + for x in range(2048): + cli.set_secret(str(x), sec_value) + + # test a much larger secret size than the default one + KILOBYTE = 1024 + payload_size = 32 * KILOBYTE + + sec_value = "x" * payload_size + cli.set_secret("foo", sec_value) + + fooval = cli.get_secret("foo") + assert fooval == sec_value + + # test a deep secret nesting structure + DEFAULT_CONTAINERS_NEST_LEVEL = 128 + container = "mycontainer" + for x in range(DEFAULT_CONTAINERS_NEST_LEVEL): + container += "%s/" % str(x) + cli.create_container(container)
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org