URL:
https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm
fidencio commented:
"""
For each of the requested changes you'll find fixup patch in
https://gitlab.com/fidencio/sssd/commits/jhrozek/kcm_review
- **SECRETS: Remove unused declarations**: CHANGES REQUESTED
This patch actually breaks the build as SEC_NET_TIMEOUT is still used in
src/responder/secsrv.c.
My suggestion here is to move this to the next patch in the series.
```
Author: Fabiano Fidêncio <fidencio(a)redhat.com>
AuthorDate: Tue Jul 18 14:44:06 2017 +0200
Commit: Fabiano Fidêncio <fidencio(a)redhat.com>
CommitDate: Tue Jul 18 21:51:27 2017 +0200
fixup! SECRETS: Remove unused declarations
---
src/responder/secrets/secsrv.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/responder/secrets/secsrv.h b/src/responder/secrets/secsrv.h
index fad7ee514..0575cbaba 100644
--- a/src/responder/secrets/secsrv.h
+++ b/src/responder/secrets/secsrv.h
@@ -30,6 +30,8 @@
#include <tevent.h>
#include <ldb.h>
+#define SEC_NET_TIMEOUT 5
+
struct sec_ctx {
struct resolv_ctx *resctx;
struct resp_ctx *rctx;
```
- **SECRETS: Do not link with c-ares**: CHANGES REQUESTED
Add the part removed from the previous patch to this one.
```
Author: Fabiano Fidêncio <fidencio(a)redhat.com>
AuthorDate: Tue Jul 18 14:47:45 2017 +0200
Commit: Fabiano Fidêncio <fidencio(a)redhat.com>
CommitDate: Tue Jul 18 21:51:37 2017 +0200
fixup! SECRETS: Do not link with c-ares
---
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 df11c7143..1aad272da 100644
--- a/src/responder/secrets/secsrv.h
+++ b/src/responder/secrets/secsrv.h
@@ -30,8 +30,6 @@
#include <tevent.h>
#include <ldb.h>
-#define SEC_NET_TIMEOUT 5
-
struct sec_ctx {
struct resp_ctx *rctx;
int fd_limit;
```
- **SECRETS: Store quotas in a per-hive configuration structure**: ACK
- **SECRETS: Read the quotas for cn=secrets from [secrets/secrets] configuration
subsection**: CHANGES REQUEST
There are just a few small changes that I'd like to ask for:
- therefor -> therefore
- mention that max_secrets is "(...) the maximum number of secrets that can be
stored in the hive."
- add an extra line before the done label
```
Author: Fabiano Fidêncio <fidencio(a)redhat.com>
AuthorDate: Tue Jul 18 15:36:07 2017 +0200
Commit: Fabiano Fidêncio <fidencio(a)redhat.com>
CommitDate: Tue Jul 18 22:19:35 2017 +0200
fixup! SECRETS: Read the quotas for cn=secrets from [secrets/secrets] configuration
subsection
---
src/man/sssd-secrets.5.xml | 4 ++--
src/responder/secrets/secsrv.c | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/man/sssd-secrets.5.xml b/src/man/sssd-secrets.5.xml
index 8aa78dacc..44383ec18 100644
--- a/src/man/sssd-secrets.5.xml
+++ b/src/man/sssd-secrets.5.xml
@@ -173,7 +173,7 @@ systemctl enable sssd-secrets.service
</variablelist>
<para>
The following options affect only the secrets
<quote>hive</quote>
- and therefor should be set in a per-hive subsection.
+ and therefore should be set in a per-hive subsection.
</para>
<variablelist>
<varlistentry>
@@ -193,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
diff --git a/src/responder/secrets/secsrv.c b/src/responder/secrets/secsrv.c
index 76b2e14df..db12cbbc3 100644
--- a/src/responder/secrets/secsrv.c
+++ b/src/responder/secrets/secsrv.c
@@ -121,6 +121,7 @@ static int sec_get_hive_config(struct sec_ctx *sctx,
}
ret = EOK;
+
done:
talloc_free(tmp_ctx);
return ret;
```
And one question ... do we really want to/have to keep backward-compatibility here?
Secrets hasn't been used by anyone so far ...
- **SECRETS: Rename local_db_req.basedn to local_db_req.req_dn**: ACK
- **SECRETS: Use separate quotas for /kcm and /secrets hives**: CHANGES REQUESTED
There's just one thing that bothers me in the manpage ... By having the default value
for kcm hive described as "64 MB" gives the users the impression that
"MB" can actually be used, which is not true.
```
Author: Fabiano Fidêncio <fidencio(a)redhat.com>
AuthorDate: Tue Jul 18 22:06:53 2017 +0200
Commit: Fabiano Fidêncio <fidencio(a)redhat.com>
CommitDate: Tue Jul 18 22:06:53 2017 +0200
fixup! SECRETS: Use separate quotas for /kcm and /secrets hives
---
src/man/sssd-secrets.5.xml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/man/sssd-secrets.5.xml b/src/man/sssd-secrets.5.xml
index e71cbec58..18b5c81b3 100644
--- a/src/man/sssd-secrets.5.xml
+++ b/src/man/sssd-secrets.5.xml
@@ -208,7 +208,7 @@ systemctl enable sssd-secrets.service
a secret payload in kilobytes.
</para>
<para>
- Default: 16 (secrets hive), 64 MB (kcm hive)
+ Default: 16 (secrets hive), 65536 (64 MB) (kcm hive)
</para>
</listitem>
</varlistentry>
```
- **TESTS: Test that ccaches can be stored after max_secrets is reached for regular
non-ccache secrets**: ACK
- **SECRETS: Add a new option to control per-UID limits**: CHANGES REQUESTED
Sorry for nitpicking here, but please, add an extra line before the done label
```
Author: Fabiano Fidêncio <fidencio(a)redhat.com>
AuthorDate: Tue Jul 18 22:30:49 2017 +0200
Commit: Fabiano Fidêncio <fidencio(a)redhat.com>
CommitDate: Tue Jul 18 22:30:49 2017 +0200
fixup! SECRETS: Add a new option to control per-UID limits
---
src/responder/secrets/local.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
index fe32007f7..53093ed3b 100644
--- a/src/responder/secrets/local.c
+++ b/src/responder/secrets/local.c
@@ -480,6 +480,7 @@ static int local_db_check_peruid_number_of_secrets(TALLOC_CTX
*mem_ctx,
}
ret = EOK;
+
done:
talloc_free(tmp_ctx);
return ret;
```
- **CRYPTO: Do not call NSS_Shutdown after every operation**: ACK
- **SECRETS: Support 0 as unlimited for the quotas**: ACK
I'm firing a CI build and I'll actually run some manual tests Tomorrow.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/225#issuecomment-316190393