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

Wed Mar 13 11:28:03 UTC 2013

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0031-Remove-implicit-Str-to-DN-conversion-using-attr.patch
Type: text/x-patch
Size: 25965 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130313/362b4150/attachment.bin>