[sssd PR#225][opened] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   URL: https://github.com/SSSD/sssd/pull/225
Author: jhrozek
 Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm
Action: opened

PR body:
"""
While testing the KCM responder some more, I realized that we always checked
the (hardcoded, no less) base DN of cn=secrets when checking for
quotas. These patches make the quota check separate for each of the
cn=secrets/cn=kcm hives, add a test and mention in documentation that the
quota from sssd-secrets applies for how many ccaches can be stored.
"""

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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
cc @simo5 

Simo, are you OK with the quotas being just per cn=secrets/cn=kcm top-level base DNs or do
you think that we need to be more granular (pe-UID?) for the ccaches as well?
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-291531193

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[simo5]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

simo5 commented:
"""
We should probbaly have quotas per UID, or one user can DoS all others including root and
services
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-291556544

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

lslebodn commented:
"""
> We should probably have quotas per UID, or one user can DoS all others
> including root and services
>  
Then we need to change current patch set.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-293249453

[sssd PR#225][+Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: +Changes requested

[sssd PR#225][synchronized] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
The patches I just pushed enforce per-UID limits. @simo5 are you OK with this approach?
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-304682738

[sssd PR#225][-Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: -Changes requested

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[simo5]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

simo5 commented:
"""
Interesting, why maximum number of secrets instead of maximum size ?

"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-304692237

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
On Mon, May 29, 2017 at 08:58:15AM -0700, Simo Sorce wrote:
>  Interesting, why maximum number of secrets instead of maximum size ?
We already have a maximum payload size of a secret:
```
       max_payload_size (integer)
                This option specifies the maximum payload size allowed
                for a secret payload in kilobytes.
```

The intent here is to prevent a user, especially now with KCM to keep
adding ccaches in a loop and starve out the storage.

If we had a per-user max payload size, we'd have to either keep a
running total of the secret lengths or compute it during each write
operation.

Or maybe I just don't see some other solution, do you have
something else in mind?

btw we need to increase the default secret size quite a bit, because KCM
stores each ccache as a secret, so each secret contains multiple creds,
so just adding creds (by calling kvno in a loop) can explode the secret
size...in kernel keyring each cred is of type big_key which holds up to
1MB, but that's /per cred/ and in the secrets storage schema we can
store arbitrary number of creds..so I think the default should be raised
to megabytes in general and we need to make sure that the error messages
are easy to follow in case someone needed to increase the secret payload
size..

"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-304704546

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[simo5]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

simo5 commented:
"""
Yes we need to increase the size per secret, but this is why I am not sure having a
maximum nu,be of secrets will scale well.
If you set the new max to 1MB, and have 256 secreets max then you give 256M of storage per
user ... that seem a lot. On the other hand if you give too few secrets, then a user can
run out o them pretty quickly. Think about a user using secrets to store their browser
keyring, clearly in that case 256 is rather low.

Perhaps we should have different allowed size + # of secrets per hive.
Then the /kcm would have something like 16 secrets allowed but each can be several
megabytes in size, while the regular user space would have a few thousand secrets (4k ?)
allowed but each no more than a few kilobytes in size (say about 16k so that a user can
store large symmetric RSA keys) ?

The other way to use just a size value and allow as many secrets as you want is to use a
"pool of sectors" that can be used to store secrets. Each time you store a
secret, the size is calculated and detracted and runded up to the "sector size"
(say 4k), and that amount is removed from the total allowed size. If that number goes to
zero we return an error. Then when a secret is deleted we have to free up sectors. This
means we have to modify 2 records each time we write to the secrets storage, which may not
be ideal.

There are ways around that though. For example we could write the number of
"sectors" used (or even the size of the value) in the entry itself, and have an
index on this attribute. Keep the size of the per/user data in a hash table in memory and
keep track of quota there. If the daemon is restarted, the user quota is recomputed, the
first time the user makes a write, by searching the database for all user's records
and summing all records, then from there on keep the quota again in memory until the hash
table is full (could be fixed size with FIFO) or the daemon is restarted.

I am sure we can think of other ways, the point is that we should be fair, and it is hard
to privilege number of secrets versus size, because KCM vs passwords vs RSA keys have
quite different per-secret sizes so it would be very difficult to find a balance if we
have ust one quota and we count number of secrets.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-304812108

[sssd PR#225][synchronized] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
After some discussion with @simo5 on IRC, I prepared quite different patches. So far they
don't address the per-client/per-UID quotas. What is implemented so far is that the
quotas can now be set in a per-hive section, for example:
```
[secrets]
# Some generic responder configuration

[secrets/secrets]
max_secrets = 123
max_payload_size = 456

[secrets/kcm]
max_secrets = 345
max_payload_size = 5678
```

Now, the questions I'm wondering are:
 * Is this configuration admin-friendly? Should we explicitly add `quota` to the
subsection? (i.e. `[secrets/secrets/quota]` ?
 * does it make sense to add a per-uid limit? Something like `max_user_secrets`? Note that
the payload size is already per-secret, so it doesn't count the global secret size. I
assume the answer here is yes, just to avoid starving the store by one malicious user
 * The KCM defaults are pulled out of thin air, but they are 256 secrets and the secrets
size is 64MB. This is because one credential in keyring could be up to 1MB and the secret
consists of all credentials in a ccache, so this gives us a little less (because of
encoding) than 64 credentials by default.

The documentation is not yet adjusted, I'd like to do that once we agree on the format
etc to avoid redoing the work. But the tests are there..
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-304908921

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[simo5]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

simo5 commented:
"""
1) I do not think we need to add explicitly a quota subsection, but if I do not want to
set a quota, what values should I use ? 0 ? Why the default is to have a quota ?

2) what is max_secrets? Given you suggest adding max_user_secrets I assume these are not
the same thing ? I am confused.


"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-305440884

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
On Thu, Jun 1, 2017 at 11:30 AM, Simo Sorce <notifications(a)github.com&gt;
wrote:

>     1.
>
>     I do not think we need to add explicitly a quota subsection, but if I
>     do not want to set a quota, what values should I use ? 0 ? Why the default
>     is to have a quota ?
>     2.
>
>     what is max_secrets? Given you suggest adding max_user_secrets I
>     assume these are not the same thing ? I am confused.
>
>  
Let me try to reply the second question (as I've implemented this one). The
original idea was to have it per user but I've noticed that (probably) the
name and the documentation are misleading. /o\.

It was introduced in 65a38b8.


>  You are receiving this because you are subscribed to this thread.
>  Reply to this email directly, view it on GitHub
>  <https://github.com/SSSD/sssd/pull/225#issuecomment-305440884>;, or mute
>  the thread
>
> <https://github.com/notifications/unsubscribe-auth/AAG4ejEGY5XxQok4bb0MQ2H...
>  .
>  
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-305445005

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[simo5]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

simo5 commented:
"""
Patches look good to me.

I have to say I think we could make the quoate system *really* generic by using dynamic
configuration handles to pair the quota to a specific hive, but given the rest of the
infrastructure is currently hardcoded on kcm and secrets hives the current solution is
good enough.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-306824457

[sssd PR#225][synchronized] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
OK, thank you. I added some more commits (the previous commits are more or less unchanged,
except documentation, adding the options to the config API etc)

There is also a fix for the crash of NSS we trigger, which is not so related, but without
the patch, the tests for the last patch (which adds the semantics of using 0 for
unlimited) doesn't work because we flood sssd-secrets with requests and it eventually
crashes.

@fidencio I wonder if you could help with review since @simo5 blessed the approach? I
guess Simo wouldn't have time for a line-by-line review..
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-307063985

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

lslebodn commented:
"""
@jhrozek 
Do you plan to fix nspr problem also for functions `sss_password_encrypt` and
`sss_password_decrypt`?
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-311404875

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
On Tue, Jun 27, 2017 at 09:02:56AM -0700, lslebodn wrote:
>  @jhrozek 
>  Do you plan to fix nspr problem also for functions `sss_password_encrypt` and
> `sss_password_decrypt`? 
I'm sorry, I totally missed your previous comment and nobody set the
"changes required" tag. So I set it now and added the task into my dodo
list to fix later this week..

"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-311450063

[sssd PR#225][+Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: +Changes requested

[sssd PR#225][synchronized] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
I've updated the PR, not only a month after the last nack -_-
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316017605

[sssd PR#225][-Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: -Changes requested

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
Thanks for updating the PR. I'll start the review soon.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316029001

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
Thanks for updating the PR. I'll start the review soon.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316029001

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
Adding changes requested label as per @lslebodn's review.

I'm still going through the patches (as I've lost a big portion of the review due
to a crash on my browser).
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316129409

[sssd PR#225][synchronized] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
On Tue, Jul 18, 2017 at 07:46:35AM -0700, lslebodn wrote:
>  lslebodn commented on this pull request.
>  
>  
>  
>  > @@ -1496,7 +1496,6 @@ sssd_secrets_SOURCES = \
>   sssd_secrets_LDADD = \
>       $(HTTP_PARSER_LIBS) \
>       $(JANSSON_LIBS) \
>  -    $(TDB_LIBS) \
>       $(SSSD_LIBS) \
>  
>  NACK
>  
>  ```
>  sh$ objdump -T .libs/sssd_secrets | grep TDB
>  0000000000000000      DF *UND*  0000000000000000  TDB_1.2.1   tdb_open
>  0000000000000000      DF *UND*  0000000000000000  TDB_1.2.1   tdb_delete
>  0000000000000000      DF *UND*  0000000000000000  TDB_1.2.1   tdb_traverse
>  0000000000000000      DF *UND*  0000000000000000  TDB_1.2.1   tdb_store
>  0000000000000000      DF *UND*  0000000000000000  TDB_1.2.1   tdb_errorstr
>  0000000000000000      DF *UND*  0000000000000000  TDB_1.2.1   tdb_fetch
>  ```
>  
>  We should do opposite. Remove `SSSD_LIBS` and replace with required libraries
>  ```
>  sh$ objdump -p .libs/sssd_secrets | grep NEEDED
>    NEEDED               libhttp_parser_strict.so.2
>    NEEDED               libjansson.so.4
>    NEEDED               libtevent.so.0
>    NEEDED               libtalloc.so.2
>    NEEDED               libpopt.so.0
>    NEEDED               libldb.so.1
>    NEEDED               libdbus-1.so.3
>    NEEDED               libdhash.so.1
>    NEEDED               libselinux.so.1
>    NEEDED               libtdb.so.1
>    NEEDED               libsystemd.so.0
>    NEEDED               libsss_util.so
>    NEEDED               libsss_crypt.so
>    NEEDED               libsss_debug.so
>    NEEDED               libcurl.so.4
>    NEEDED               libc.so.6
>  ```
>  
>  Because there is no reason why `sssd_secrets` shoudl be linked with `PCRE` or `openldap`
> libs which are part of `SSSD_LIBS`  
Because I don't feel strongly about this patch (and arguably it is not
part of what this PR should fix), I dropped it from the set.

"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316162237

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  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&gt;
AuthorDate: Tue Jul 18 14:44:06 2017 +0200
Commit:     Fabiano Fidêncio <fidencio(a)redhat.com&gt;
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&gt;
AuthorDate: Tue Jul 18 14:47:45 2017 +0200
Commit:     Fabiano Fidêncio <fidencio(a)redhat.com&gt;
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&gt;
AuthorDate: Tue Jul 18 15:36:07 2017 +0200
Commit:     Fabiano Fidêncio <fidencio(a)redhat.com&gt;
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&gt;
AuthorDate: Tue Jul 18 22:06:53 2017 +0200
Commit:     Fabiano Fidêncio <fidencio(a)redhat.com&gt;
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&gt;
AuthorDate: Tue Jul 18 22:30:49 2017 +0200
Commit:     Fabiano Fidêncio <fidencio(a)redhat.com&gt;
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

[sssd PR#225][+Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: +Changes requested

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

lslebodn commented:
"""
On (18/07/17 20:38), fidencio wrote:
> - **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&gt;
> AuthorDate: Tue Jul 18 22:06:53 2017 +0200
> Commit:     Fabiano Fidêncio <fidencio(a)redhat.com&gt;
> 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>
>
> ``` 
64 MB is 64000KB
64 MiB is 65536 kiB
https://en.wikipedia.org/wiki/Megabyte :-)

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316441693

[sssd PR#225][synchronized] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
Thanks for the review, I've squashed in @fidencio's fixups (thanks for providing
them) as well as @lslebodn's manpage suggestion.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316638156

[sssd PR#225][-Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: -Changes requested

[sssd PR#225][+Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: +Changes requested

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
Adding the "Changes Requested" label as per @lslebodn's comment.

As I plan to go (one more time) through those patches Today, I'll provide a fixup
(unless @jhrozek is faster than I am).
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-316911468

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

lslebodn commented:
"""
BTW I would prefer at least "CRYPTO: Do not call NSS_Shutdown after every
operation"
in 1.15.3
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-317069831

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

lslebodn commented:
"""
Commit "CRYPTO: Do not call NSS_Shutdown after every operation" was pushed to
upstream

master:
* a6f606117e5cfe64c4b49f94e514bf82054716d3
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-317533228

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
@jhrozek: in the very same branch were I provided you the fix ups, you can find the
patches providing a fixup for @lslebodn's comment + solving the conflict with current
master.

Could you take a look on that branch and re-push the patches?
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-318153056

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
@fidencio yes and thank you very much for providing those. I was slowed down by the
upstream release and some RHEL stuff, but I'll make sure to review those by the end of
the week.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-318170713

[sssd PR#225][synchronized] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

   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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

jhrozek commented:
"""
I squashed in @fidencio's fixup and pushed new version of the patches..
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-318370499

[sssd PR#225][-Changes requested] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[jhrozek]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: -Changes requested

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
CI: http://vm-058-233.${abc}/logs/job/74/96/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-326534539

[sssd PR#225][+Accepted] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: +Accepted

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

fidencio commented:
"""
Please, feel free to push the patches and sorry for the long time it took to review them.
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-326534681

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

lslebodn commented:
"""
master:
* 109ed7ca1a82420798efdc6a9b019675a5bd0f4f
* 6b3bab516355fdf4cc81e6da9d87ec3818ab190f
* 0558f270b3fbb0780e2a94602d455022b89f5381
* 197da163943868216f704fb34031e7d5576e8aee
* 392f48c039d7a6d70bce6ae2d122042391653566
* 4db56d8c90a6467a216590e5ba3bdcd2a2bf1ae9
* 7a162ca3ea0bf8ef6b13795a00baa28d17f6131d
* 9ef185255126b9ed415fa334f585a11c5be4fb1a
* 2d40ce078a9071ac23353848d0849fbbbd600049
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-326653795

[sssd PR#225][closed] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

   URL: https://github.com/SSSD/sssd/pull/225
Author: jhrozek
 Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm
Action: closed

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

[sssd PR#225][comment] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

lslebodn commented:
"""
I fixed few pep8 issues before pushing to master
```
pep8 src/tests/intg/test_secrets.py
src/tests/intg/test_secrets.py:408:1: E302 expected 2 blank lines, found 1
src/tests/intg/test_secrets.py:544:80: E501 line too long (80 > 79 characters)
```

This is a reason why i wrote comment
https://github.com/SSSD/sssd/pull/358#issuecomment-325787750
"""

See the full comment at https://github.com/SSSD/sssd/pull/225#issuecomment-326654091

[sssd PR#225][+Pushed] SECRETS: Apply separate quotas for cn=secrets and cn=kcm

[lslebodn]

  URL: https://github.com/SSSD/sssd/pull/225
Title: #225: SECRETS: Apply separate quotas for cn=secrets and cn=kcm

Label: +Pushed