URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: SUDO: Adding user name alias to sudoRule filter Action: opened
PR body: """ This patch adds another value to sudoUser attribute of sudoRule filter. The value is 'user alias' which means it is cased version of user (in domains where it matters).
Resolves: https://fedorahosted.org/sssd/ticket/3203
-----
This is complement to #39 . The idea is some domains are case sensitive and another not. If we would like to search ```sudoRules``` we need handle user names. With this patch we add user name alias (case sensitive form) of user name to the ```sudoRule``` filter.
Another point is we could save both forms of user name to cached ```sudoRules```. It could be extension of this patch. """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: SUDO: Adding user name alias to sudoRule filter
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: SUDO: Adding user name alias to sudoRule filter
celestian commented: """ I see it, I forgot the tests. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-252249600
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: SUDO: Adding user name alias to sudoRule filter Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: SUDO: Adding user name alias to sudoRule filter
celestian commented: """ Fixed patch pushed. I will add patch for testing new functionality of sudo. And I will add patch for caching both form of user name to sudoRule, if @pbrezina and @jhrozek agree. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-252254830
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: SUDO: Adding user name alias to sudoRule filter
jhrozek commented: """ Can we close PR #39 ? """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-252260308
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: SUDO: Adding user name alias to sudoRule filter
celestian commented: """ PR #39 is for the same issue, but for version 1-13. And I need to change the patch. I would like to have two PR because there are two different patches, one for each version.
If we close PR #39, I will open new one PR after preparing the new proper patch for 1.13.
I see I am not able answer in simple way. :-) """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-252262505
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: SUDO: Adding user name alias to sudoRule filter
jhrozek commented: """ On Fri, Oct 07, 2016 at 07:10:36AM -0700, celestian wrote:
PR #39 is for the same issue, but for version 1-13. And I need to change the patch. I would like to have two PR because there are two different patches, one for each version.
If we close PR #39, I will open new one PR after preparing the new proper patch for 1.13.
I see I am not able answer in simple way. :-)
Then I would suggest to rename the pull requests (there is an edit button) to make it clear what the PRs are targetting ..
"""
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-252276979
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14) Action: edited
Changed field: title Original value: """ SUDO: Adding user name alias to sudoRule filter """
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14) Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14) Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14) Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
celestian commented: """ I sent new version. The tests were added and I addressed comments. Thanks. This new version change the logic of patch. We didn't update query to cache, we simple add some lowercase variant to the cache if it is needed (domain is incasesensitive). """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-253446921
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14) Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
pbrezina commented: """ I didn't do a thorough review but we also need to search with lover case values in responder so also names like "admiNISTRATOR" will match on case insensitive domains. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-253783618
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
pbrezina commented: """ I didn't do a thorough review but we also need to search with lover case values in responder so also names like "admiNISTRATOR" will match on case insensitive domains. The approach looks good to me. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-253783618
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
celestian commented: """ I added user 'teST' on AD. His login on Linux box is 'test'. Patch works. Or missed I something? """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-253791285
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
pbrezina commented: """ User that is stored in AD as teST can have multiple login names since case sensitivity is ignored. It can login as test, but also as teST, TEst, Test, ... all of those logins must work with sudo. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-253792040
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
celestian commented: """ So, I set ```case_sensitive = preserving``` and: ``` ssh -l TEsT@scorpion.domain 192.168.122.65 TEsT@scorpion.domain@192.168.122.65's password: [teST@scorpion.domain@client2 ~]$ sudo less /etc/sssd/sssd.conf ``` still works. AD user was ```teST```. I comment ```case_sensitive = preserving``` and it works again.
AD domain is case insensitive and sudo works for different (case) version of login name. Is there any other corner case, please?
(For example I am not sure if sudoRule have to be applicable on local user. I didn't think about it.) """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-253794517
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
pbrezina commented: """ I see why it works now, what I originally meant was to create a whole new attribute, say sudoUserAlias that would contain lowercased values so we can also distinguish between original and custom data when debugging issues. Can you do it this way, please? The change should be small. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-256000011
On Tue, Oct 25, 2016 at 12:38:57PM +0200, pbrezina wrote:
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14)
pbrezina commented: """ I see why it works now, what I originally meant was to create a whole new attribute, say sudoUserAlias that would contain lowercased values so we can also distinguish between original and custom data when debugging issues. Can you do it this way, please? The change should be small. """
Because in responders, we should be searching ideally only indexed attributes and the indexing would prevent downgrades because the cache version number would have to be increased, we decided to just put all the case variants into the sudoUser attribute.
I'm sorry we're overriding what you asked for when you are away for a week, but we need to fix this issue soon in downstream. If you would prefer to keep the original values as well in the cache for some reason, I would suggest to add additional patch later with the originalSudoUser value.
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.14) Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15) Action: edited
Changed field: title Original value: """ RESPONDER: Enable sudoRule in case insen. domains (1.14) """
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
celestian commented: """ Of course, it is simple. I pushed new version. Thanks for comment. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-256042823
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ On Thu, Oct 27, 2016 at 03:56:21AM -0700, Pavel Březina wrote:
pbrezina requested changes on this pull request.
Hi, there is one indentation which can be simply fixed. Patches works so I can ack them.
However, there is something that surprises me. If you set `case_sensitive = preserving`, I would expect `sss_get_cased_name()` to return the original format of the name. If this was true than you would have to also transform `username` to lower in `sysdb_sudo_filter_userinfo()`. So before pushing these patch: is this correct behaviour of `sss_get_cased_name()`?
I agree this should be checked, but if I remember correctly, at least since 1.14 we should always use the lowercased name internally, but when formatting the output name in the NSS responder we should use the original case with a preserving domain.
"""
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-256685401
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ I reviewed the use of `case_preserve` in the code and I think we use it correctly for formatting output or for interacting with external components. So this part is in my opinion OK.
But I have another question, I noticed that `sudoUser` is indexed but `sudoUserAlias`. And this might turn each sudo invocation into traversing the whole database. Is the new attribute then really worth this potential performance issue? """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-257269928
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15) Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
celestian commented: """ Little note
I rebase my branch with master locally. Result is that @pbrezina's comments from his review are not visible now. I addressed all, so I canceled his review. Maybe it wasn't good idea to rebase my developing branch. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-257514104
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
celestian commented: """ To @pbrezina and @jhrozek
I introduced new attribute ```sudoUserAlias``` as result of discussion with Pavel. The original idea was we did not want to mix the original and added values.
To meet both requirements - do not mix values and not cause a performance issue - would it be reasonable to start indexing new attribute ```sudoUserAlias```?
"""
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-257515716
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ On Tue, Nov 01, 2016 at 01:46:31AM -0700, celestian wrote:
To @pbrezina and @jhrozek
I introduced new attribute ```sudoUserAlias``` as result of discussion with Pavel. The original idea was we did not want to mix the original and added values.
To meet both requirements - do not mix values and not cause a performance issue - would it be reasonable to start indexing new attribute ```sudoUserAlias```?
Sure and for 1.15 this might be doable quite easily, especially if we just file a ticket and index the attribute once before the release (an additional index requires the database to be upgraded so that we should do only one upgrade per release ideally..).
But I wonder about downstream, is the additional attribute worth the database upgrade for downstream customers?
"""
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-257527566
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
celestian commented: """ There is another important question from discussion in #39. We would like to back port fully qualified names too. And then back port will be more simple and save. Isn't it another reason for introducing new attribute and update DB? """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-257556033
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ On Tue, Nov 01, 2016 at 05:36:40AM -0700, celestian wrote:
There is another important question from discussion in #39. We would like to back port fully qualified names too. And then back port will be more simple and save. Isn't it another reason for introducing new attribute and update DB?
I'm not sure what exactly do you propose to backport, but it's really no possible to backport the qualified names in the sysdb cache.
"""
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-257558579
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ Setting changes requested to rework the patch to only include the sudoUser and not sudoUserAlias """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-258154309
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ Oops sorry I accidentally replied to list instead of commenting in the PR. Let me paste my commend again here:
Because in responders, we should be searching ideally only indexed attributes and the indexing would prevent downgrades because the cache version number would have to be increased, we decided to just put all the case variants into the sudoUser attribute.
I'm sorry we're overriding what you asked for when you are away for a week, but we need to fix this issue soon in downstream. If you would prefer to keep the original values as well in the cache for some reason, I would suggest to add additional patch later with the originalSudoUser value. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-258155481
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15) Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
celestian commented: """ So, new version is pushed. It uses only ```sudoUser``` attribute. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-258353222
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
celestian commented: """ I found function: ``` char * sss_get_cased_name(TALLOC_CTX *mem_ctx, const char *orig_name, bool case_sensitive) ``` in ```src/tool/usertools.c``` during backreporting to 1.13. If it is our prefered way how to lowered usernames, I should use it. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-258358616
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
celestian commented: """ I found function: ``` char * sss_get_cased_name(TALLOC_CTX *mem_ctx, const char *orig_name, bool case_sensitive) ``` in ```src/util/usertools.c``` during backreporting to 1.13. If it is our prefered way how to lowered usernames, I should use it. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-258358616
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ On Thu, Nov 03, 2016 at 11:19:02PM -0700, celestian wrote:
So, new version is pushed. It uses only ```sudoUser``` attribute.
Thank you, this patch works in my testing. I will also run some downstream tests to be sure.
"""
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-258394873
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ On Fri, Nov 04, 2016 at 12:06:08AM -0700, celestian wrote:
I found function:
char * sss_get_cased_name(TALLOC_CTX *mem_ctx, const char *orig_name, bool case_sensitive)in ```src/tool/usertools.c``` during backreporting to 1.13. If it is our prefered way how to lowered usernames, I should use it.
This function can be used in cases where you want to use only the original case or only the lowercase. Do we want to store both cases or only the domain-case here in the cache? I think for now and also because downstream is pressing to apply a patch we can use the existing PR and then you can open a ticket for yourself to refactor the code further to only use the lower case.
"""
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-258395624
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
pbrezina commented: """ Ok, I'm fine with this patch due to the indexing issue. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-259093311
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ btw the downstream test against a 389DS server also passed (I'm not sure if we have some downstream tests for sssd+sudo+AD..), so I'm going to push the patches.. """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-259111289
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
jhrozek commented: """ master: f4a1046bb88d7a0ab3617e49ae94bfa849d10645 23637e2fd2b1fe42bdd2335893a11ac8016f56bc sssd-1-14: 143b1dcbbe865a139616a22b139e19bd772e46f0 88239b7f17f599aefa88a8a31c2d0ea44b766c87 """
See the full comment at https://github.com/SSSD/sssd/pull/43#issuecomment-259112827
URL: https://github.com/SSSD/sssd/pull/43 Author: celestian Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15) Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/43/head:pr43 git checkout pr43
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/43 Title: #43: RESPONDER: Enable sudoRule in case insen. domains (1.15)
Label: -Accepted
sssd-devel@lists.fedorahosted.org
-
fidencio -
Jakub Hrozek