[sssd PR#33][opened] SECRETS: Some small misc fixes + fixing #3136

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/33
Author: fidencio
 Title: #33: SECRETS: Some small misc fixes + fixing #3136
Action: opened

PR body:
"""
This series was made on top of Jakub's patches adding Secrets'
documentation, that is applied on top of my own patches adding
a the update-confdb-process.

The patches that needs to be reviewed are the last 6 patches, and
they're listed below:
 f587a7c (Fabiano Fidêncio, 3 days ago)
    SECRETS: Add a configurable depth limit for nested containers

    Resolves: https://fedorahosted.org/sssd/ticket/3168

    Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

 e98bd63 (Fabiano Fidêncio, 7 hours ago)
    SECRETS: Use a tmp_context on local_db_check_containers()

    Otherwise the struct ldb_dn will be hanging on the mem_ctx till it gets
    freed.

    Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

 4a90c20 (Fabiano Fidêncio, 7 hours ago)
    SECRETS: Make functions from local.c static

    There's no reason for those functions to be exposed.

    Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

 67e1301 (Fabiano Fidêncio, 10 hours ago)
    CONFIG: Add secrets' provider option

    The provider option specifies where should the secrets be stored.

    Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

 5de385f (Jakub Hrozek, 6 weeks ago)
    CONFIG: List allowed secrets responder options

    Co-author: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

 1f51d50 (Fabiano Fidêncio, 11 hours ago)
    CONFIG: Add secrets responder to the allowed sections

    Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;

In the last patch (f587a7c) I'm adding a test for the maximum depth,
but I decided to, at least for now, do this test based on the default
value set for this option instead of trying to implemenet something in
python to read the config entries from the ConfDB file.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/33/head:pr33
git checkout pr33

[sssd PR#33][+Changes requested] SECRETS: Some small misc fixes + fixing #3136

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3136

Label: +Changes requested

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3136

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3136

jhrozek commented:
"""
Adding "Changes Requested" tag to reflect @lslebodn comment. I will also take a
look at this patch set.
"""

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

[sssd PR#33][edited] SECRETS: Some small misc fixes + fixing #3136

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/33
Author: fidencio
 Title: #33: SECRETS: Some small misc fixes + fixing #3136
Action: edited

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/33/head:pr33
git checkout pr33

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

fidencio commented:
"""
@jhrozek, about the configAPI question, I really missed the others secrets specific
options. I'll re-work and re-submit the patches.
"""

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

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

jhrozek commented:
"""
On Wed, Sep 28, 2016 at 02:16:36AM -0700, fidencio wrote:
>  fidencio commented on this pull request.
>  
>  
>  
>  > @@ -447,6 +468,9 @@ static int local_db_create(TALLOC_CTX *mem_ctx,
>       ret = local_db_check_containers(msg, lctx, msg->dn);
>       if (ret != EOK) goto done;
>   
>  +    ret = local_db_check_containers_nested_level(lctx, msg->dn);
>  +    if (ret != EOK) goto done;
>  
>  I can rebase my patches on top of yours, that wouldn't be a problem. Let me know
> what's your preference. 
I think it's better to get in the code patches and then the DEBUG
patches (it's easier and less error-prone to rebase DEBUG messages
IMO..)

But I guess adding a DEBUG message to this particular place would be
nice..

"""

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

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/33
Author: fidencio
 Title: #33: SECRETS: Some small misc fixes + fixing #3168
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/33/head:pr33
git checkout pr33

[sssd PR#33][-Changes requested] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

Label: -Changes requested

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

fidencio commented:
"""
So, I've pushed a new version of the patches and I tried to address all comments made,
but the one adding a debug message in case of failure on 
local_db_check_containers_nested_level(). IMO would be better to add the debug messages
within Jakub's patches (and I can provide you a patch, that you can squash to yours,
@jhrozek).

In this new series I don't have the work done for #3138 applied. As it will need some
changes I think is just better having these patches pushed ASAP and then, later on,
re-work the patches for #3138.

Also worth to mention that this series fixes #3207 as well.
"""

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

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

fidencio commented:
"""
And CI has passed: http://sssd-ci.duckdns.org/logs/job/54/46/summary.html
Although, github complains that all checks have failed for some reason.

"""

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

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

jhrozek commented:
"""
On Thu, Sep 29, 2016 at 02:37:11PM -0700, fidencio wrote:
>  So, I've pushed a new version of the patches and I tried to
> address
>  all comments made, but the one adding a debug message in case of failure
>  on  local_db_check_containers_nested_level(). IMO would be better to add
>  the debug messages within Jakub's patches (and I can provide you a patch,
>  that you can squash to yours, @jhrozek). 
Sure, I don't mind one way or another.

>  
>  In this new series I don't have the work done for #3138 applied. As it will need some
> changes I think is just better having these patches pushed ASAP and then, later on,
> re-work the patches for #3138. 
Yes, I agree.

>  
>  Also worth to mention that this series fixes #3207 as well. 
I just pushed the documentation patch. Could you please rebase this
series atop origin/master so that I can then only review and push the
new patches?

"""

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

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

jhrozek commented:
"""
On Fri, Sep 30, 2016 at 12:26:46AM -0700, Jakub Hrozek wrote:
>  I just pushed the documentation patch. Could you please rebase this
>  series atop origin/master so that I can then only review and push the
>  new patches? 
In addition, could you please add Reviewed-By to the config validator
patch that is attributed to me? I can't review that one..

"""

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

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/33
Author: fidencio
 Title: #33: SECRETS: Some small misc fixes + fixing #3168
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/33/head:pr33
git checkout pr33

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

fidencio commented:
"""
Done!
"""

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

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/33
Author: fidencio
 Title: #33: SECRETS: Some small misc fixes + fixing #3168
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/33/head:pr33
git checkout pr33

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

fidencio commented:
"""
Okay, with this last patch series config-check works like a charm for [secrets] and
[secrets/users/$uid].
Also the name of the newly introduced property was renamed to containers_nest_limit (and
all the related vars/constants as well).
"""

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

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

fidencio commented:
"""
And here is the diff between the last series and the new one that's arriving in a few
...

 src/config/SSSDConfig/__init__.py.in | 1 +
 src/config/etc/sssd.api.conf         | 1 +
 src/responder/secrets/local.c        | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index 15b9cd1..74c2ca5 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -120,6 +120,7 @@ option_strings = {

     # [secrets]
     'provider': _('The provider where the secrets will be stored in'),
+    'containers_nest_level': _('The maximum allowed number of nested
containers'),
     # secrets - proxy
     'proxy_url': _('The URL Custodia server is listening on'),
     'auth_type': _('The method to use when authenticating to a Custodia
server'),
diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
index f94c8d1..a7757dc 100644
--- a/src/config/etc/sssd.api.conf
+++ b/src/config/etc/sssd.api.conf
@@ -97,6 +97,7 @@ user_attributes = str, None, false
 [secrets]
 # Secrets service
 provider = str, None, false
+containers_nest_level = int, None, false
 # Secrets service - proxy
 proxy_url = str, None, false
 auth_type = str, None, false
diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
index 06260f3..ec84537 100644
--- a/src/responder/secrets/local.c
+++ b/src/responder/secrets/local.c
@@ -334,7 +334,7 @@ done:
 }

 static int local_db_check_containers_nest_level(struct local_context *lctx,
-                                                  struct ldb_dn *leaf_dn)
+                                                struct ldb_dn *leaf_dn)
 {
     int nest_level;

"""

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

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/33
Author: fidencio
 Title: #33: SECRETS: Some small misc fixes + fixing #3168
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/33/head:pr33
git checkout pr33

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[Lukas Slebodnik]

On (30/09/16 16:55), fidencio wrote:
>    URL: https://github.com/SSSD/sssd/pull/33
> Author: fidencio
>  Title: #33: SECRETS: Some small misc fixes + fixing #3168
> Action: synchronized
>
> To pull the PR as Git branch:
> git remote add ghsssd https://github.com/SSSD/sssd
> git fetch ghsssd pull/33/head:pr33
> git checkout pr33 
> From 06a0a81193d6bbe3a0932c8b584433f3cc13fa51 Mon Sep 17 00:00:00
> 2001
> From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> Date: Sun, 25 Sep 2016 20:49:16 +0200
> Subject: [PATCH 1/6] CONFIG: Add secrets responder to the allowed sections
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The regular expression used is quite specific for the two cases we
> support:
> - [secrets]
> - [secrets/users/$uid]
>
> It could be done a bit more generic, but the way it's right now it can
> easily catch errors like: [secrets/usrs/$uid] or [secrets/].
>
> Related:
> https://fedorahosted.org/sssd/ticket/3207
>
> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;
> ---
>  src/config/cfg_rules.ini | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
> index 01be0c6..023ceac 100644
> --- a/src/config/cfg_rules.ini
> +++ b/src/config/cfg_rules.ini
> @@ -8,6 +8,7 @@ section = autofs
>  section = ssh
>  section = pac
>  section = ifp
> +section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
>  section_re = ^domain/.*$ 
Is it expected that section the name "secrets/users/"
is allowed.

Which of following section should be allowed?

sh# cat /etc/sssd/conf.d/10_secrets.conf
[secrets
description = temp

[secrets/users]
description = temp

[secrets/users/]
description = temp

[secrets/users/$uid]
description = temp

[secrets/users/0]
description = temp

[secrets/users/1]
description = temp

[secrets/users/1000]
description = temp

LS

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[Fabiano Fidêncio]

On Mon, Oct 17, 2016 at 11:46 AM, Lukas Slebodnik <lslebodn(a)redhat.com&gt; wrote:
>  On (30/09/16 16:55), fidencio wrote:
> >   URL: https://github.com/SSSD/sssd/pull/33
> >Author: fidencio
> > Title: #33: SECRETS: Some small misc fixes + fixing #3168
> >Action: synchronized
> >
> >To pull the PR as Git branch:
> >git remote add ghsssd https://github.com/SSSD/sssd
> >git fetch ghsssd pull/33/head:pr33
> >git checkout pr33
>
>  >From 06a0a81193d6bbe3a0932c8b584433f3cc13fa51 Mon Sep 17 00:00:00 2001
> >From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> >Date: Sun, 25 Sep 2016 20:49:16 +0200
> >Subject: [PATCH 1/6] CONFIG: Add secrets responder to the allowed sections
> >MIME-Version: 1.0
> >Content-Type: text/plain; charset=UTF-8
> >Content-Transfer-Encoding: 8bit
> >
> >The regular expression used is quite specific for the two cases we
> >support:
> >- [secrets]
> >- [secrets/users/$uid]
> >
> >It could be done a bit more generic, but the way it's right now it can
> >easily catch errors like: [secrets/usrs/$uid] or [secrets/].
> >
> >Related:
> >https://fedorahosted.org/sssd/ticket/3207
> >
> >Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;
> >---
> > src/config/cfg_rules.ini | 1 +
> > 1 file changed, 1 insertion(+)
> >
> >diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
> >index 01be0c6..023ceac 100644
> >--- a/src/config/cfg_rules.ini
> >+++ b/src/config/cfg_rules.ini
> >@@ -8,6 +8,7 @@ section = autofs
> > section = ssh
> > section = pac
> > section = ifp
> >+section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
> > section_re = ^domain/.*$
>
>  Is it expected that section the name "secrets/users/"
>  is allowed. 
I don't think so.
I'll answer your questions based on the my understanding of the
conversation I had with Jakub on the #sssd channel.
Jakub, Simo, please, feel free to jump in and correct me if I'm
mistaken in any point.

>  Which of following section should be allowed?
>
>  sh# cat /etc/sssd/conf.d/10_secrets.conf
>  [secrets
>  description = temp 
Not allowed, but [secrets] is allowed.

>  [secrets/users]
>  description = temp
>  
Shouldn't be allowed.

>  [secrets/users/]
>  description = temp
>  
Shouldn't be allowed.

>  [secrets/users/$uid]
>  description = temp
>  
Shouldn't be allowed.

>  [secrets/users/0]
>  description = temp
>  
Should be allowed.

>  [secrets/users/1]
>  description = temp 
Should be allowed.

>  [secrets/users/1000]
>  description = temp 
Should be allowed.

>  LS
>  _______________________________________________
>  sssd-devel mailing list -- sssd-devel(a)lists.fedorahosted.org
>  To unsubscribe send an email to sssd-devel-leave(a)lists.fedorahosted.org 
Is some of these cases breaking to you?
If yes, please, let me know and I'll provide a follow up patch fixing the issue.

Best Regards,
--
Fabiano Fidêncio

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[Lukas Slebodnik]

On (17/10/16 14:35), Fabiano Fidêncio wrote:
> On Mon, Oct 17, 2016 at 11:46 AM, Lukas Slebodnik
> <lslebodn(a)redhat.com&gt; wrote:
> > On (30/09/16 16:55), fidencio wrote:
> >>   URL: https://github.com/SSSD/sssd/pull/33
> >>Author: fidencio
> >> Title: #33: SECRETS: Some small misc fixes + fixing #3168
> >>Action: synchronized
> >>
> >>To pull the PR as Git branch:
> >>git remote add ghsssd https://github.com/SSSD/sssd
> >>git fetch ghsssd pull/33/head:pr33
> >>git checkout pr33
> >
> > >From 06a0a81193d6bbe3a0932c8b584433f3cc13fa51 Mon Sep 17 00:00:00 2001
> >>From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> >>Date: Sun, 25 Sep 2016 20:49:16 +0200
> >>Subject: [PATCH 1/6] CONFIG: Add secrets responder to the allowed sections
> >>MIME-Version: 1.0
> >>Content-Type: text/plain; charset=UTF-8
> >>Content-Transfer-Encoding: 8bit
> >>
> >>The regular expression used is quite specific for the two cases we
> >>support:
> >>- [secrets]
> >>- [secrets/users/$uid]
> >>
> >>It could be done a bit more generic, but the way it's right now it can
> >>easily catch errors like: [secrets/usrs/$uid] or [secrets/].
> >>
> >>Related:
> >>https://fedorahosted.org/sssd/ticket/3207
> >>
> >>Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;
> >>---
> >> src/config/cfg_rules.ini | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >>diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
> >>index 01be0c6..023ceac 100644
> >>--- a/src/config/cfg_rules.ini
> >>+++ b/src/config/cfg_rules.ini
> >>@@ -8,6 +8,7 @@ section = autofs
> >> section = ssh
> >> section = pac
> >> section = ifp
> >>+section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
> >> section_re = ^domain/.*$
> >
> > Is it expected that section the name "secrets/users/"
> > is allowed.
>
> I don't think so.
> I'll answer your questions based on the my understanding of the
> conversation I had with Jakub on the #sssd channel.
> Jakub, Simo, please, feel free to jump in and correct me if I'm
> mistaken in any point.
>
> >
> > Which of following section should be allowed?
> >
> > sh# cat /etc/sssd/conf.d/10_secrets.conf
> > [secrets
> > description = temp
>
> Not allowed, but [secrets] is allowed.
>  
This one was a typo on my side.

> >
> > [secrets/users]
> > description = temp
> >
>
> Shouldn't be allowed.
>
> > [secrets/users/]
> > description = temp
> >
>
> Shouldn't be allowed.
>  
But it is allowed.

Following change should fix it.
I didn't tested it.

diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
index b6316be..0a654ff 100644
--- a/src/config/cfg_rules.ini
+++ b/src/config/cfg_rules.ini
@@ -8,7 +8,7 @@ section = autofs
 section = ssh
 section = pac
 section = ifp
-section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
+section_re = ^secrets\(/users/[0-9]\+\)\?$
 section_re = ^domain/.*$
 
 [rule/allowed_sssd_options]



> > [secrets/users/$uid]
> > description = temp
> >
>
> Shouldn't be allowed.
>  
yes, it's denied.

> > [secrets/users/0]
> > description = temp
> >
>
> Should be allowed.
>  
OK, I was not sure about root UID.

> > [secrets/users/1]
> > description = temp
>
> Should be allowed.
>
> >
> > [secrets/users/1000]
> > description = temp
>
> Should be allowed.
>
> >
> > LS
>
> Is some of these cases breaking to you?
> If yes, please, let me know and I'll provide a follow up patch fixing the issue.
>  
I think we shoudl also split following rule
"[rule/allowed_sec_options]"

I do not think that following options are read from
sections "secrets/users/.*"

option = timeout
option = debug
option = debug_level
option = debug_timestamps
option = debug_microseconds
option = debug_to_files
option = command
option = reconnection_retries
option = fd_limit
option = client_idle_timeout
option = description

Fabiano,
Could you preapre a patch?

LS

[sssd PR#33][synchronized] SECRETS: Some small misc fixes + fixing #3168

[Fabiano Fidêncio]

Lukaš,

On Mon, Oct 17, 2016 at 4:59 PM, Lukas Slebodnik <lslebodn(a)redhat.com&gt; wrote:
>  On (17/10/16 14:35), Fabiano Fidêncio wrote:
> >On Mon, Oct 17, 2016 at 11:46 AM, Lukas Slebodnik <lslebodn(a)redhat.com&gt; wrote:
> >> On (30/09/16 16:55), fidencio wrote:
> >>>   URL: https://github.com/SSSD/sssd/pull/33
> >>>Author: fidencio
> >>> Title: #33: SECRETS: Some small misc fixes + fixing #3168
> >>>Action: synchronized
> >>>
> >>>To pull the PR as Git branch:
> >>>git remote add ghsssd https://github.com/SSSD/sssd
> >>>git fetch ghsssd pull/33/head:pr33
> >>>git checkout pr33
> >>
> >> >From 06a0a81193d6bbe3a0932c8b584433f3cc13fa51 Mon Sep 17 00:00:00 2001
> >>>From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com&gt;
> >>>Date: Sun, 25 Sep 2016 20:49:16 +0200
> >>>Subject: [PATCH 1/6] CONFIG: Add secrets responder to the allowed sections
> >>>MIME-Version: 1.0
> >>>Content-Type: text/plain; charset=UTF-8
> >>>Content-Transfer-Encoding: 8bit
> >>>
> >>>The regular expression used is quite specific for the two cases we
> >>>support:
> >>>- [secrets]
> >>>- [secrets/users/$uid]
> >>>
> >>>It could be done a bit more generic, but the way it's right now it can
> >>>easily catch errors like: [secrets/usrs/$uid] or [secrets/].
> >>>
> >>>Related:
> >>>https://fedorahosted.org/sssd/ticket/3207
> >>>
> >>>Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com&gt;
> >>>---
> >>> src/config/cfg_rules.ini | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>>diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
> >>>index 01be0c6..023ceac 100644
> >>>--- a/src/config/cfg_rules.ini
> >>>+++ b/src/config/cfg_rules.ini
> >>>@@ -8,6 +8,7 @@ section = autofs
> >>> section = ssh
> >>> section = pac
> >>> section = ifp
> >>>+section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
> >>> section_re = ^domain/.*$
> >>
> >> Is it expected that section the name "secrets/users/"
> >> is allowed.
> >
> >I don't think so.
> >I'll answer your questions based on the my understanding of the
> >conversation I had with Jakub on the #sssd channel.
> >Jakub, Simo, please, feel free to jump in and correct me if I'm
> >mistaken in any point.
> >
> >>
> >> Which of following section should be allowed?
> >>
> >> sh# cat /etc/sssd/conf.d/10_secrets.conf
> >> [secrets
> >> description = temp
> >
> >Not allowed, but [secrets] is allowed.
> >
>  This one was a typo on my side.
>
> >>
> >> [secrets/users]
> >> description = temp
> >>
> >
> >Shouldn't be allowed. 
This is wrong right now :-\

> >
> >> [secrets/users/]
> >> description = temp
> >>
> >
> >Shouldn't be allowed.
> >
>  But it is allowed. 
Anything terminating with a / will cause the following error and it's
not related to secrets only.
(Mon Oct 17 16:59:22:585705 2016) [sssd] [confdb_init_db] (0x0010):
Could not create LDIF for confdb
(Mon Oct 17 16:59:22:585898 2016) [sssd] [confdb_setup] (0x0010):
ConfDB initialization has failed [22]: Invalid argument
(Mon Oct 17 16:59:22:586078 2016) [sssd] [sss_tool_confdb_init]
(0x0010): Unable to setup ConfDB [22]: Invalid argument


>  Following change should fix it.
>  I didn't tested it.
>
>  diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
>  index b6316be..0a654ff 100644
>  --- a/src/config/cfg_rules.ini
>  +++ b/src/config/cfg_rules.ini
>  @@ -8,7 +8,7 @@ section = autofs
>   section = ssh
>   section = pac
>   section = ifp
>  -section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
>  +section_re = ^secrets\(/users/[0-9]\+\)\?$
>   section_re = ^domain/.*$
>
>   [rule/allowed_sssd_options]
>  
Yeah, the full patch should be:

diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
index b6316be..5a4394d 100644
--- a/src/config/cfg_rules.ini
+++ b/src/config/cfg_rules.ini
@@ -8,7 +8,7 @@ section = autofs
 section = ssh
 section = pac
 section = ifp
-section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
+section_re = ^secrets\(/users/[0-9]\+\?\)\?$
 section_re = ^domain/.*$

 [rule/allowed_sssd_options]
@@ -212,7 +212,7 @@ option = user_attributes

 [rule/allowed_sec_options]
 validator = ini_allowed_options
-section_re = ^secrets\(/users/\([0-9]\+\)\?\)\?$
+section_re = ^secrets\(/users/[0-9]\+\?\)\?$

Just tested it on my side here.

> >> [secrets/users/$uid]
> >> description = temp
> >>
> >
> >Shouldn't be allowed.
> >
>  yes, it's denied.
>
> >> [secrets/users/0]
> >> description = temp
> >>
> >
> >Should be allowed.
> >
>  OK, I was not sure about root UID.
>
> >> [secrets/users/1]
> >> description = temp
> >
> >Should be allowed.
> >
> >>
> >> [secrets/users/1000]
> >> description = temp
> >
> >Should be allowed.
> >
> >>
> >> LS
> >
> >Is some of these cases breaking to you?
> >If yes, please, let me know and I'll provide a follow up patch fixing the issue.
> >
>
>  I think we shoudl also split following rule
>  "[rule/allowed_sec_options]"
>
>  I do not think that following options are read from
>  sections "secrets/users/.*"
>
>  option = timeout
>  option = debug
>  option = debug_level
>  option = debug_timestamps
>  option = debug_microseconds
>  option = debug_to_files
>  option = command
>  option = reconnection_retries
>  option = fd_limit
>  option = client_idle_timeout
>  option = description
>
>  Fabiano,
>  Could you preapre a patch? 
Sure. I just want to confirm whether I understand what's your proposal.
You want to have two rules:
- secrets
- secrets/user/[0-9]+?

And have all those options you listed just under the former, right?

Best Regards,
--
Fabiano Fidêncio

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

jhrozek commented:
"""
On Fri, Sep 30, 2016 at 07:55:00AM -0700, fidencio wrote:
>  And here is the diff between the last series and the new one
> that's arriving in a few ...
>  
>   src/config/SSSDConfig/__init__.py.in | 1 +
>   src/config/etc/sssd.api.conf         | 1 +
>   src/responder/secrets/local.c        | 2 +-
>   3 files changed, 3 insertions(+), 1 deletion(-)
>  
>  diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
>  index 15b9cd1..74c2ca5 100644
>  --- a/src/config/SSSDConfig/__init__.py.in
>  +++ b/src/config/SSSDConfig/__init__.py.in
>  @@ -120,6 +120,7 @@ option_strings = {
>  
>       # [secrets]
>       'provider': _('The provider where the secrets will be stored in'),
>  +    'containers_nest_level': _('The maximum allowed number of nested
> containers'),
>       # secrets - proxy
>       'proxy_url': _('The URL Custodia server is listening on'),
>       'auth_type': _('The method to use when authenticating to a Custodia
> server'),
>  diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf
>  index f94c8d1..a7757dc 100644
>  --- a/src/config/etc/sssd.api.conf
>  +++ b/src/config/etc/sssd.api.conf
>  @@ -97,6 +97,7 @@ user_attributes = str, None, false
>   [secrets]
>   # Secrets service
>   provider = str, None, false
>  +containers_nest_level = int, None, false
>   # Secrets service - proxy
>   proxy_url = str, None, false
>   auth_type = str, None, false
>  diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
>  index 06260f3..ec84537 100644
>  --- a/src/responder/secrets/local.c
>  +++ b/src/responder/secrets/local.c
>  @@ -334,7 +334,7 @@ done:
>   }
>  
>   static int local_db_check_containers_nest_level(struct local_context *lctx,
>  -                                                  struct ldb_dn *leaf_dn)
>  +                                                struct ldb_dn *leaf_dn)
>   {
>       int nest_level;
>  
>   
Thank you, all the patches look good to me now, I did some basic manual
testing, I tested the particular case which gives me a 406 when I reach
the nesting limit:

curl -H "Content-Type: application/json" --unix-socket /var/run/secrets.socket 
-XPOST http://localhost/secrets/con1/con2/con3/con4/con5/
<html>
<head>
<title>406 Not Acceptable</title></head>
<body>
<h1>Not Acceptable</h1>
<p>The request cannot be accepted.</p>

CI also passed:
	http://sssd-ci.duckdns.org/logs/job/54/50/summary.html

so ACK to all patches.

"""

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

[sssd PR#33][+Accepted] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

Label: +Accepted

[sssd PR#33][comment] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

jhrozek commented:
"""
btw I also manually tested the config regexes and sssd is OK with known option but not OK
with an unknown option, which is also good:
(Mon Oct  3 15:30:41:545005 2016) [sssd] [sss_ini_call_validators] (0x0020):
[rule/allowed_sec_options]: Attribute 'xxx' is not allowed in section
'secrets'. Check for typos.
"""

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

[sssd PR#33][+Pushed] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

Label: +Pushed

[sssd PR#33][-Accepted] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

  URL: https://github.com/SSSD/sssd/pull/33
Title: #33: SECRETS: Some small misc fixes + fixing #3168

Label: -Accepted

[sssd PR#33][closed] SECRETS: Some small misc fixes + fixing #3168

[fidencio]

   URL: https://github.com/SSSD/sssd/pull/33
Author: fidencio
 Title: #33: SECRETS: Some small misc fixes + fixing #3168
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/33/head:pr33
git checkout pr33