[Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Rob Crittenden]

Petr Viktorin wrote:
> On 02/29/2012 11:14 AM, Jan Cholasta wrote:
>> On 29.2.2012 11:09, Petr Viktorin wrote:
>>> On 02/28/2012 03:19 PM, Jan Cholasta wrote:
>>>> On 28.2.2012 11:54, Petr Viktorin wrote:
>>>>> On 02/27/2012 10:44 PM, Rob Crittenden wrote:
>>>>>> Petr Viktorin wrote:
>>>>>>> On 02/20/2012 08:51 PM, Rob Crittenden wrote:
>>>>>>>> Petr Viktorin wrote:
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2159 says various config
>>>>>>>>> options
>>>>>>>>> are not marked Required, so entering an empty value for it will
>>>>>>>>> pass
>>>>>>>>> validation (and IPA will blow up later when it expects a
>>>>>>>>> string,not
>>>>>>>>> None). Forexample the following:
>>>>>>>>> $ ipa config-mod --groupsearch=
>>>>>>>>> fails with AttributeError: 'NoneType' object has no attribute
>>>>>>>>> 'split'
>>>>>>>>>
>>>>>>>>> There is a more general problem behind this, though: even if the
>>>>>>>>> attributes *are* marked as Required, an empty string will pass
>>>>>>>>> validation. This is because `None` is used in `Param.validate` to
>>>>>>>>> mean
>>>>>>>>> both "No value supplied" and "Empty value supplied". The method
>>>>>>>>> currently assumes the former, and skips validation entirely for
>>>>>>>>> `None`
>>>>>>>>> values to optional parameters.
>>>>>>>>>
>>>>>>>>> For example, the following will delete "membergroup", even though
>>>>>>>>> it's a
>>>>>>>>> required attribute :
>>>>>>>>>
>>>>>>>>> $ ipa delegation-add --attrs=street --group=editors \
>>>>>>>>> --membergroup=admins td1
>>>>>>>>> $ ipa delegation-mod --membergroup= td1
>>>>>>>>>
>>>>>>>>> Note that some LDAPObjects handle this with a _check_empty_attrs
>>>>>>>>> function, so they aren't affected. That function is specific to
>>>>>>>>> LADP
>>>>>>>>> objects, though. So I needed to tackle this on a lower level.
>>>>>>>>>
>>>>>>>>> This patch solves the problem by
>>>>>>>>> * adding a 'nonempty' flag when a required parameter of a CRUD
>>>>>>>>> Update
>>>>>>>>> object is auto-converted to a non-required parameter
>>>>>>>>> * making the`validate` method aware of whether the parameter was
>>>>>>>>> supplied; and if it was, honor the "nonempty" flag.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The second patch fixes
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2159 by
>>>>>>>>> marking required config options as required.
>>>>>>>>
>>>>>>>> This looks good but I think there are other things to protect in
>>>>>>>> config
>>>>>>>> as well such as the default e-mail domain. It is probably safe to
>>>>>>>> say
>>>>>>>> that everything in there is required.
>>>>>>>>
>>>>>>>> rob
>>>>>>>
>>>>>>> Let me just double-check this with you.
>>>>>>>
>>>>>>> According to code in the user plugin (around line 330), if the
>>>>>>> default
>>>>>>> e-mail domain is not set, users don't get an address
>>>>>>> auto-assigned. Do
>>>>>>> we really want to require user e-mails?
>>>>>>>
>>>>>>> ipaconfigstring (the password plugin flags) are a set (multivalue,
>>>>>>> not
>>>>>>> required).
>>>>>>>
>>>>>>> The rest of the values I left as not required are for optional
>>>>>>> features
>>>>>>> or limits: search results & time limit, max. username length,
>>>>>>> password
>>>>>>> expiry notification. Currently if these are missing, the
>>>>>>> feature/limit
>>>>>>> is disabled (well, except for the time limit).
>>>>>>> But, there are also special values (0 or -1) that have the same
>>>>>>> effect
>>>>>>> as a missing value. Sometimes they're documented.
>>>>>>> So we want to enforce that users use these special values instead of
>>>>>>> removing the config entry?
>>>>>>
>>>>>> I think we want to enforce that these are defined. It will be
>>>>>> confusing
>>>>>> for users if these are not there at all. I don't think we need to
>>>>>> show
>>>>>> the special options, just declare that the attribute is required.
>>>>>>
>>>>>> rob
>>>>>>
>>>>>
>>>>> Attaching updated patch 13.
>>>>>
>>>>> Only the default e-mail domain
>>>>> (https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
>>>>> still optional.
>>>>>
>>>>
>>>> You have removed all the config-related defensive code in the patch, is
>>>> this a good idea? What will happen if someone e.g. deletes a required
>>>> config attribute directly from LDAP?
>>>>
>>>
>>> Then IPA crashes. The defensive code wasn't there for all cases anyway,
>>> as ticket #2159 shows.
>>>
>>> If we want to protect against this it would probably be better to make
>>> the config class itself give the default when a required value is
>>> missing.
>>>
>>
>> This, and raise an error in cases where no default is available (the
>> check should probably be done in ldap.get_ipa_config).
>>
>> Honza
>>
>
> Would a better approach be to modify the LDAP schema to require these
> values?
>

I think that may be a longer-term fix. I propose we keep the defensive 
code in for now and correct it in the future.

rob