On 01/18/2016 10:54 AM, Lukas Slebodnik wrote:
On (18/01/16 10:18), Jakub Hrozek wrote:
> 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.
>
We might ask whether Dreamworks need this feature.
Otherwise these patches will not help them.
We talked with Jakub about it and decided that we will add support of
ipaSudoRunAsExt* attributes. So with this patch set only hostmask test
should fail, we will not add support for hostmask unless requested.