Lukaš,

On Mon, Oct 17, 2016 at 4:59 PM, Lukas Slebodnik <lsleb...@redhat.com> wrote:
> On (17/10/16 14:35), Fabiano Fidêncio wrote:
>>On Mon, Oct 17, 2016 at 11:46 AM, Lukas Slebodnik <lsleb...@redhat.com> 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?= <fiden...@redhat.com>
>>>>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 <fiden...@redhat.com>
>>>>---
>>>> 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-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

Reply via email to