On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:
On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
>On (15/01/16 15:06), Sumit Bose wrote:
>>On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
>>>On (15/01/16 10:21), Pavel Březina wrote:
>>>>On 01/14/2016 06:37 PM, Sumit Bose wrote:
>>>>>On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
>>>>>>On 01/13/2016 05:12 PM, Sumit Bose wrote:
>>>>>>>>
>>>>>>>>@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
>>>>>>>>
>>>>>>>> if (ctx == NULL) {
>>>>>>>> DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is
NULL\n");
>>>>>>>>- ctx->ret = ERR_INTERNAL;
>>>>>>>> return false;
>>>>>>>> }
>>>>>>>>
>>>>>>>>@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t
*item,
>>>>>>>>
>>>>>>>> if (ctx == NULL) {
>>>>>>>> DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is
NULL\n");
>>>>>>>>- ctx->ret = ERR_INTERNAL;
>>>>>>>> return false;
>>>>>>>> }
>>>>>>>>
>>>>>>>
>>>>>>>it looks like the last 2 hunks are not available in the
latest tar ball.
>>>>>>
>>>>>>Sorry about that. Added.
>>>>>>
>>>>>>>and there are some trailing whitespaces in patch 9 at line
41.
>>>>>>
>>>>>>Fixed by git.
>>>>>>
>>>>>>I also attached two more patches then converts usn into number
and then use
>>>>>>in filter entryUSN >= (current + 1) instead of (entryUSN >=
cur) &&
>>>>>>(entryUSN != cur) as per Thierry request.
>>>>>>
>>>>>>It could probably be squashed and replace some previous patches
but I think
>>>>>>sending it as new patches is easier for you to review, given the
previous
>>>>>>patches are basically acked.
>>>>>
>>>>>>+ errno = 0;
>>>>>> usn_number = strtoul(usn, &endptr, 10);
>>>>>>- if ((endptr == NULL || (*endptr == '\0' &&
endptr != usn))
>>>>>>- && (usn_number > srv_opts->last_usn)) {
>>>>>>- srv_opts->last_usn = usn_number;
>>>>>>+ if (endptr != NULL && *endptr != '\0') {
>>>>>
>>>>>Currently an empty string in usn would result in usn_number==0, I
wonder
>>>>>if this is valid or if it would be better to error out in this case?
>>>>
>>>>Zero value will not change anything since we always compare if the
current
>>>>usn value is lower than the new one, we switch them only in such case. So
I
>>>>think it is not necessary to error out in this case since strtoul
succeeds.
>>>>
>>>>>
>>>>>Otherwise all looks good, CI and Coverity test are currently
running.
>>>>>
>>>>>bye,
>>>>>Sumit
>>>>>
>>>>>>+ DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN
%s\n", usn);
>>>>>>+ return;
>>>>>>+ } else if (errno != 0) {
>>>>>>+ ret = errno;
>>>>>>+ DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN
%s [%d]:
>>>>>>%s\n",
>>>>>>+ usn, ret, sss_strerror(ret));
>>>>>>+ return;
>>>>>> }
>>>
>>>There are 4 failed test from freeipa sudo test suite.
>>>
>>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
>>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
>>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
>>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
>>>
>>>
>>>=================================== FAILURES
===================================
>>>______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask
______________
>>>
>>>self = <ipatests.test_integration.test_sudo.TestSudo object at
0x7fe0d7ace8d0>
>>>
>>> def test_sudo_rule_restricted_to_one_hostmask(self):
>>> if self.__class__.skip_hostmask_based:
>>> raise pytest.skip("Hostmask could not be detected")
>>>
>>> result1 = self.list_sudo_commands("testuser1")
>>>> assert "(ALL : ALL) NOPASSWD: ALL" in
result1.stdout_text
>>>E assert '(ALL : ALL) NOPASSWD: ALL' in ''
>>>E + where '' = <pytest_multihost.transport.SSHCommand
object at 0x7fe0d7acee10>.stdout_text
>>>
>>>test_integration/test_sudo.py:295: AssertionError
>>>______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user
______
>>>
>>>self = <ipatests.test_integration.test_sudo.TestSudo object at
0x7fe0d79de990>
>>>
>>> def test_sudo_rule_restricted_to_running_as_single_local_user(self):
>>> result1 = self.list_sudo_commands("testuser1",
verbose=True)
>>>> assert "RunAsUsers: localuser" in result1.stdout_text
>>
>>There are just too many options. I tested RunAsUsers and RunAsUsers
>>but only with IPA users and groups. I looks like the handling for the
>>ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
>>
>That's the purpose of automated tests :-)
>To cover all use-case.
These use cases are omitted by design.
We might still need to at least document this change. But with a recent IPA
server, I see:
[jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --externaluser=jhrozek
readfiles
ipa: ERROR: invalid 'externaluser': this option has been deprecated.
[jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternalgroup=jhrozek
readfiles
ipa: ERROR: invalid 'runasexternalgroup': this option has been deprecated.
[jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternaluser=jhrozek
readfiles
ipa: ERROR: invalid 'runasexternaluser': this option has been deprecated.
So I guess we should document that if you use a new sssd version with an
old IPA server AND you use these deprecated options you should revert to
the old schema?