[Freeipa-devel] [PATCHES 0031-0032] Improve HBAC rule handling in selinuxusermap-add/mod/find

Wed Mar 13 16:36:32 UTC 2013

On 03/13/2013 12:28 PM, Tomas Babej wrote:
> On 02/27/2013 10:28 AM, Martin Kosek wrote:
>> On 02/20/2013 12:31 PM, Tomas Babej wrote:
>>> On 02/19/2013 10:33 PM, Rob Crittenden wrote:
>>>> Tomas Babej wrote:
>>>>> On 02/06/2013 07:57 PM, Rob Crittenden wrote:
>>>>>> Tomas Babej wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> this pair of patches improves HBAC rule handling in selinuxusermap
>>>>>>> commands.
>>>>>>>
>>>>>>> Patch 0031 deals with:
>>>>>>> https://fedorahosted.org/freeipa/ticket/3349
>>>>>>>
>>>>>>> Patch 0032 takes care of:
>>>>>>> https://fedorahosted.org/freeipa/ticket/3348
>>>>>>>
>>>>>>> and is to be applied on top of Patch 0031.
>>>>>>>
>>>>>>> See commit messages for detailed info.
>>>>>>>
>>>>>>> Tomas
>>>>>>>
>>>>>> ACK for patch 0032.
>>>>>>
>>>>>> For patch 0031 we can't change the data type of an existing attribute.
>>>>>> It will break backwards compatibility. Can you test with an older
>>>>>> client to see if it cares (it may not care about the name of the
>>>>>> type). If older clients will work then this is probably ok.
>>>>>>
>>>>>> I gather that seealso detected as a DN attribute and converted into a
>>>>>> DN class and this is blowing up the Str validator?
>>>>>>
>>>>> Yes, that was exactly the case.
>>>>>> rob
>>>>> I added a workaround for older client versions, tested it with
>>>>> freeipa-client/admintools 2.2, works as expeceted.
>>>>> However, this only should be issue if there is older admintools package
>>>>> on the client than on the server.
>>>>>
>>>>> Outline is such as follows: I added a new flag for DNParam seelalso
>>>>> attribute, called 'allow_malformed' that allows any string to be passed
>>>>> to DNParam. Its value gets wrapped in 'malformed=yes,value=<value>'.
>>>>> This allows to parse out the string in selinuxusermap-add/mod
>>>>> pre_callback out of the DN and search for the rule with such name so
>>>>> that it's DN gets in LDAP instead.
>>>>>
>>>>> Updated patch attached.
>>>>>
>>>>> Tomas
>>>> I like where you're going with this, just a couple of comments:
>>>>
>>>> 1. Should we come up with a more universal name for allow_malformed? Is this
>>>> something that we should allow at a higher level? I was thinking allow_raw,
>>>> or allow_non_dn, or something like that.
>>> To me, allow_non_dn sounds is just as specific as allow_malformed,
>>> they both refer to DN specifically.
>>>
>>> I'd go with allow_raw, if need for such pattern may eventually arise for
>>> other parameter classes than DNParam.
>>>
>>> What do you mean by higher level, turning this hack into a feature
>>> Param class? I don't see how this would work, each parameter
>>> class that implements its own type validation as DNParam needs
>>> to override _convert_scalar(). And in every such class we would need
>>> to wrap our raw value so that it is represented in the type of this parameter,
>>> as we do with DN(('malformed','yes'),('value',value)) now.
>>>
>>> Maybe we could skip type validation in _convert_scalar default
>>> implementation or catch the error raised somehow, and let the type be
>>> invalid, but I'm not aware of the consenquences. I would need to investigate.
>>> Wouldn't it cause failure deeper in the framework?
>>>
>>> Or did you by higher level mean simply picking a more general name for the
>>> flag so it can be reused in other parameter classes with the same name?
>>>> 2. I think that if a bad dn is passed in as a Str the conversion into a DN
>>>> won't be handled:
>>>>
>>>> +            if 'allow_malformed' in self.flags:
>>>> +                dn = DN(('malformed','yes'),('value',value))
>>>>
>>>> Should this be wrapped in a try/except to raise a ConversionError if it fails?
>>> Yes, thanks for that catch.
>>>> rob
>>> Tomas
>> Is it just me, or does the 0031 look overengineered? I think this is a general
>> problem for each Str parameter which we then process/convert to DN in our
>> pre_callbacks.
>>
>> selinuxusermap is one example where this does not work. This fix leaves other
>> examples not working:
>>
>> # ipa trustconfig-mod --setattr "ipantfallbackprimarygroup=cn=Default SMB
>> Group,cn=groups,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com"
>> ipa: ERROR: invalid 'ipantfallbackprimarygroup': must be Unicode text
>>
>> I would rather propose to not automatically encode DN of known attributes set
>> by *attr:
>>
>> diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
>> index 1ebbe7a..e4b9834 100644
>> --- a/ipalib/plugins/baseldap.py
>> +++ b/ipalib/plugins/baseldap.py
>> @@ -768,12 +768,6 @@ last, after all sets and adds."""),
>>                   # None means "delete this attribute"
>>                   value = None
>>
>> -            if ldap.has_dn_syntax(attr):
>> -                try:
>> -                    value = DN(value)
>> -                except ValueError:
>> -                    raise errors.InvalidSyntax(attr=attr)
>> -
>>               if attr in newdict:
>>                   if type(value) in (tuple,):
>>                       newdict[attr] += list(value)
>>
>> I think this conversion is just done too early as this Str param is processed
>> and converted later in the pre_callback, when needed. The code above introduced
>> inconsistent processing of IPA attributes with DN syntax coming from regular
>> option and from *attr option - Str
>>
>> When I did this change, both selinuxusermap-mod and trustconfig-mod started
>> working:
>>
>> # ipa selinuxusermap-mod foo
>> --setattr=seealso=ipaUniqueID=70e42636-75db-11e2-9df6-001a4a104edc,cn=hbac,dc=rhel64,dc=ad,dc=test
>>
>> -------------------------------
>> Modified SELinux User Map "foo"
>> -------------------------------
>>    Rule name: foo
>>    SELinux User: unconfined_u:s0-s0:c0.c1023
>>    HBAC Rule: allow_all
>>    Enabled: TRUE
>> # ipa selinuxusermap-mod foo --setattr=seealso=allow_all
>> ipa: ERROR: no modifications to be performed
>> # ipa selinuxusermap-mod foo --hbacrule=allow_all
>> ipa: ERROR: no modifications to be performed
>>
>> You would just need to investigate if this change would not have other
>> consequences.
>>
>> Martin
> Attaching a version of the patch based on the Martin's proposition.
> 
> This is indeed a simpler solution, that solves both problems. I investigated
> whether removing
> conversion into DN would have any consenquences. However, it turns out that
> DNParam is
> only used in contexts where usage of --*attr options is not allowed:
>   - cosentry class (no CLI)
>   - migration (no *attr options)
>   - ipacertificatesubjectbase in ipa config class (has no_update flag)
> 
> I refactored the patch and retained the unit tests.
> Please note that pushing this renders 0032 invalid.
> 
> Tomas

I think this approach is OK, I also did not discover any bug that it would be
causing.

ACK. Pushed to master, ipa-3-1.

Martin