On 01/18/2016 12:23 PM, Pavel Březina wrote:
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.
The previous
tarball contained one patch that was already pushed...