URL: https://github.com/SSSD/sssd/pull/53 Author: fidencio Title: #53: Fixes in the config API related to secrets responder Action: opened
PR body: """ Those fixes were suggested by Lukaš in the following thread: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.o...
Changes:
28fa419 (Fabiano Fidêncio, 11 minutes ago) SECRETS: Add allowed_sec_users_options
There are options (the proxying related ones) that only apply to the secrets' subsections. In order to make config API able to catch those, let's create a new section called allowed_sec_users_options) and move there these proxying options.
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com
2aed214 (Fabiano Fidêncio, 2 hours ago) SECRETS: Fix secrets rule in the allowed sections
We have been matching an invalid subsection of the secrets' section, like: [secrets/users]
Let's ensure that we only match the following cases: [secrets] [secrets/users/[0-9]+?]
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/53/head:pr53 git checkout pr53
URL: https://github.com/SSSD/sssd/pull/53 Author: fidencio Title: #53: Fixes in the config API related to secrets responder Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/53/head:pr53 git checkout pr53
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
fidencio commented: """ And CI has passed: http://sssd-ci.duckdns.org/logs/job/55/24/summary.html """
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-254306475
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
fidencio commented: """ @jhrozek I'm removing "Changes requested" label as the last patchset (from 2 days ago) already contains the changes requested by Lukaš. """
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-254770287
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
jhrozek commented: """ On Wed, Oct 19, 2016 at 03:07:28AM -0700, fidencio wrote:
@jhrozek I'm removing "Changes requested" label as the last patchset (from 2 days ago) already contains the changes requested by Lukaš.
Ah, sorry about that, I didn't notice it does.
"""
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-254770870
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
lslebodn commented: """ Jakub is more familiar with that code. """
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-254771806
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
jhrozek commented: """ Sorry for the delay in reviewing. I tried: ``` [secrets/foo] [secrets/123] ``` as an invalid test and sssd noticed them as invalid. It also didn't complain about a positive test: ``` [secrets/users/123] ```
What other tests should I run, especially wrt the second patch? """
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-257296593
URL: https://github.com/SSSD/sssd/pull/53 Author: fidencio Title: #53: Fixes in the config API related to secrets responder Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/53/head:pr53 git checkout pr53
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
fidencio commented: """ @jhrozek: For the first patch the tests are correct. @lslebodn also complained that [secrets/users/] could be a valid case in the way the code is in git right now and it's also fixed by my patch. In any case, seems that we don't allow any config section to end with "/".
For the second test, I guess that good tests are adding configuration options that are only allowed for [secrets] into the [secrets/users/123] section and vice-versa.
Example of a config that should fail: ``` [secrets] proxy_url = foo
[secrets/users/123] timeout = 10 ```
Example of a config that should not fail: ``` [secrets] debug_level = 9
[secrets/users/123] proxy_url = foo ``` @lslebodn, does it make sense for you?
@jhrozek: and I really would like to be sure that the options that I put into secrets section in the second patch are **only** valid for that section or whether those options should be inherited and also allowed to [secrets/users/123] """
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-259133301
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
lslebodn commented: """ On (08/11/16 05:15), fidencio wrote:
@jhrozek: For the first patch the tests are correct. @lslebodn also complained that [secrets/users/] could be a valid case in the way the code is in git right now and it's also fixed by my patch. In any case, seems that we don't allow any config section to end with "/".
For the second test, I guess that good tests are adding configuration options that are only allowed for [secrets] into the [secrets/users/123] section and vice-versa.
Example of a config that should fail:
[secrets] proxy_url = foo [secrets/users/123] timeout = 10Example of a config that should not fail:
[secrets] debug_level = 9 [secrets/users/123] proxy_url = foo@lslebodn, does it make sense for you?
I am fine with the 1st patch. But I am not very familiar with the secrets code therefore It would take me much more time to review 2nd patch. I prefer if @jhrozek could review it.
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-259138537
On Tue, Nov 08, 2016 at 02:40:23PM +0100, lslebodn wrote:
I am fine with the 1st patch. But I am not very familiar with the secrets code therefore It would take me much more time to review 2nd patch. I prefer if @jhrozek could review it.
I see one glitch there. We should move the provider option to the per-user sections. This should be allowed:
``` [secrets/users/123] provider = proxy ```
On the other hand, the "global" section cannot select a provider, the global section is (at the moment) only local.
Otherwise LGTM.
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
jhrozek commented: """ On Tue, Nov 08, 2016 at 05:40:21AM -0800, lslebodn wrote:
On (08/11/16 05:15), fidencio wrote:
@jhrozek: For the first patch the tests are correct. @lslebodn also complained that [secrets/users/] could be a valid case in the way the code is in git right now and it's also fixed by my patch. In any case, seems that we don't allow any config section to end with "/".
For the second test, I guess that good tests are adding configuration options that are only allowed for [secrets] into the [secrets/users/123] section and vice-versa.
Example of a config that should fail:
[secrets] proxy_url = foo [secrets/users/123] timeout = 10Example of a config that should not fail:
[secrets] debug_level = 9 [secrets/users/123] proxy_url = foo@lslebodn, does it make sense for you?
I am fine with the 1st patch. But I am not very familiar with the secrets code therefore It would take me much more time to review 2nd patch. I prefer if @jhrozek could review it.
(Sorry, I accidentally replied to sssd-devel instead of here).
The provider= option should be moved to the per-uid section, because currently only the user sections can select a provider, the global secrets section is always 'local'.
Otherwise LGTM.
"""
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-259797538
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
fidencio commented: """ Change done and patchset updated.
Just to make things easier for the reviewer, here is what the fix up looks like: ```
[ffidenci@cat sssd]$ git diff diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini index 8a5290e..882a185 100644 --- a/src/config/cfg_rules.ini +++ b/src/config/cfg_rules.ini @@ -228,7 +228,6 @@ option = reconnection_retries option = fd_limit option = client_idle_timeout option = description -option = provider option = containers_nest_level option = max_secrets
@@ -237,6 +236,7 @@ validator = ini_allowed_options section_re = ^secrets/users/[0-9]+$
# Secrets service - proxy +option = provider option = proxy_url option = auth_type option = auth_header_name
``` """
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-259799269
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
jhrozek commented: """ Thanks, ACK """
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-259810451
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
jhrozek commented: """ Since there is no ticket, I only pushed the patches to master: 682c9c3467055c2149af28826f7458b857b0f8c4 da8801c363716533f60bc78e10f3a2100cebc3a1
"""
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-259814966
URL: https://github.com/SSSD/sssd/pull/53 Author: fidencio Title: #53: Fixes in the config API related to secrets responder Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/53/head:pr53 git checkout pr53
URL: https://github.com/SSSD/sssd/pull/53 Title: #53: Fixes in the config API related to secrets responder
lslebodn commented: """ On (10/11/16 13:38), Jakub Hrozek wrote:
Since there is no ticket, I only pushed the patches to master: 682c9c3467055c2149af28826f7458b857b0f8c4 da8801c363716533f60bc78e10f3a2100cebc3a1
Such version should have been pushed as part of https://fedorahosted.org/sssd/ticket/3207.
I noticed issues lately due to holidays.
Therefore pushed to 1.14 as well
sssd-1-14: * 9d4cc96f2951412f647223dfe59060fa1e2b7b14 * 2535993d81c7d0dbbd6c6fab6f45b338845535cf
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/53#issuecomment-259918798
sssd-devel@lists.fedorahosted.org
-
fidencio -
Jakub Hrozek