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

[Tomas Babej]

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