RESPONDER: Enable sudoRule in case insen. domains​ (1.15)

[celestian]

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.

[celestian]

I see it, I forgot the tests.

[celestian]

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.

[jhrozek]

Can we close PR #39 ?

[celestian]

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. :-)

[jhrozek]

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 ..

[fidencio]

I didn't follow closely what @jhrozek suggested, so I'm just reviewing the code, not the approach taken to write the patch.

[fidencio]

Here you leaked the tmp_ctx. Please, set ret to EOK and go to done.

[celestian]

Thanks, addressed.

[fidencio]

What does lowered == NULL mean? I didn't find anything useful in libunistring documentation.
If NULL means an error, would be nice to split the if, print a debug message and treat the error accordingly.

By the way, it's not related to your code, but I personally don't like comparing the lowered string with users[i] to check whether the users[i] string was already a lower cased string or not. Would be better to do this check before converting the string, I know there's a u8_islowercase() function in libunistring and I hope it's being used somewhere down in the stack. I didn't find anything similar in GLib though.

[celestian]

So... lowered == NULL should be a test for error. After rethinking it would be more logic then I want in this function. I think it is possible to have empty sudoUser. So I removed this condition.

And on note about islowercase(). I didn't find it in our code. I kept the original.

[celestian]

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).

[fidencio]

lowered has to be non-NULL, otherwise you'll crash in the strcmp() call.

So, checking for it is fine. I just wanted to understand which are the cases that NULL was returned and I didn't find any documentation about this.

I'd go for:

if (lowered == NULL) {`
    ret = errno;
    DEBUG();
    continue; /* in case it's not fatal ... or goto done in case it is. */
}

if (strcmp(...) == 0) {
    DEBUG();
    continue;
}

Does the suggestion make sense?

[celestian]

Yes, it makes sense. Thanks, @fidencio
I addressed it. New version is pushed.

[pbrezina]

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.

[celestian]

I added user 'teST' on AD. His login on Linux box is 'test'. Patch works.
Or missed I something?

[pbrezina]

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.

[celestian]

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.)

[pbrezina]

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.

[celestian]

Of course, it is simple. I pushed new version. Thanks for comment.

Old patch version.

[pbrezina]

Hi, there is one indentation which can be simply fixed. Patches works so ACK.

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 patches: is this correct behaviour of sss_get_cased_name()?

[pbrezina]

I'd prefer a space here, but it's up to you.

[pbrezina]

Indentation error.

[jhrozek]

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.

[jhrozek]

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?

Thanks, Pavel. I addressed all your comments.

[celestian]

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.

[celestian]

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?

[jhrozek]

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.

[celestian]

So, new version is pushed. It uses only sudoUser attribute.

[celestian]

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.

[jhrozek]

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.

[jhrozek]

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.

[pbrezina]

Ok, I'm fine with this patch due to the indexing issue.

[jhrozek]

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..

[jhrozek]

master:
f4a1046
23637e2
sssd-1-14:
143b1dc
88239b7