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

[Martin Kosek]

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