[Freeipa-devel] [RANT] --setattr validation is a minefield.

[Jan Cholasta]

On 10.4.2012 19:56, Dmitri Pal wrote:
> On 04/10/2012 01:48 PM, Rob Crittenden wrote:
>> Petr Viktorin wrote:
>>> On 04/10/2012 07:07 PM, Martin Kosek wrote:
>>>> On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
>>>>> On 10.4.2012 16:00, Petr Viktorin wrote:
>>>>>> I'm aware that we have backwards compatibility requirements so we
>>>>>> have
>>>>>> to stick with unfortunate decisions, but I wanted you to know what I
>>>>>> think. Please tell me I'm wrong!
>>>>>>
>>>>>>
>>>>>>
>>>>>> It is not clear what --{set,add,del}attr and friends should do. On
>>>>>> the
>>>>>> one hand they should be powerful -- presumably as powerful as
>>>>>> ldapmodify. On the other hand they should be checked to ensure they
>>>>>> can't be used to break the system. These requirements are
>>>>>> contradictory.
>>>>>> And in either case we're doing it wrong:
>>>>>> - If they should be all-powerful, we shouldn't validate them.
>>>>>> - If they shouldn't break the system we can just disable them for
>>>>>> IPA-managed attributes. My understanding is that they were originally
>>>>>> only added for attributes IPA doesn't know about. People can still
>>>>>> use
>>>>>> ldapmodify to bypass validation if they want.
>>>>>> - If somewhere in between, we need to clearly define what they should
>>>>>> do, instead of drawing the line ad-hoc based on individual details we
>>>>>> forgot about, as tickets come from QE.
>>>>>>
>>>>>>
>>>>>> I would hope people won't use --setattr for IPA-managed attributes.
>>>>>> Which would however mean we won't get much community testing for all
>>>>>> this extra code.
>>>>>>
>>>>>>
>>>>>> Then, there's an unfortunate detail in IPA implementation: attribute
>>>>>> Params need to be cloned to method objects (Create, Update, etc.) to
>>>>>> work properly (e.g. get the `attribute` flag set). If they are marked
>>>>>> no_update, they don't get cloned, so they don't work properly.
>>>>>> Yet --setattr apparently needs to be able to update and validate
>>>>>> attributes marked no_update (this ties to the confusing
>>>>>> requirements on
>>>>>> --setattr I already mentioned). This leads to doing the same work
>>>>>> again,
>>>>>> slightly differently.
>>>>>>
>>>>>>
>>>>>>
>>>>>> tl;dr: --setattr work on IPA-managed attributes (with validation)
>>>>>> is a
>>>>>> mistake. It adds no functionality, only complexity. We don't want
>>>>>> people
>>>>>> to use it. It will cost us a lot of maintenance work to support.
>>>>>>
>>>>>>
>>>>>> Thank you for listening. A patch for the newest regression is coming
>>>>>> up.
>>>>>>
>>>>>
>>>>> I wholeheartedly agree.
>>>>
>>>> This is indeed a mine field and we need to make a look from at the
>>>> issue
>>>> from all sides before accepting a decision.
>>>
>>> Yes.
>>>
>>>>>
>>>>> Like you said above, we should either not validate --{set,add,del}attr
>>>>> or don't allow them on known attributes.
>>>>
>>>> IMHO, validating attributes we manage in the same way for both
>>>> --setattr
>>>> and standard attrs is not that wrong. It is a good precaution, because
>>>> if we let an unvalidated value in, it can make even a bigger mess later
>>>> in our pre_callbacks or post_callbacks where we assume that at this
>>>> point everything is valid.
>>>
>>> Then we should validate *exactly* the same way, including not allowing
>>> no_update attributes to be updated.
>>>
>>>> If somebody wants to modify attributes in an uncontrolled, unvalidated
>>>> way, he is free to use ldapmodify or other tool to play with raw LDAP
>>>> values. Without our guarantee of course.
>>>
>>> That's clear.
>>>
>>>> But if he chooses to use our --{set,add,del}attr we should at least
>>>> help
>>>> him to not shoot himself to the leg and validate/normalize/encode the
>>>> value. I don't know how many users use this API, but removing a support
>>>> for all managed attributes seems as a big compatibility break to me.
>>>
>>> Well, it was broken (see https://fedorahosted.org/freeipa/ticket/2405,
>>> 2407, 2408), so I don't think many people used it.
>>>
>>> Anyway, what's the use case? Why would the user want to use --setattr
>>> for validated attributes? Is our regular API lacking something?
>>>
>>
>> I think Honza had the best suggestion, enhance the standard API to
>> handle multi-valued attributes better and reduce the need for
>> set/add/delattr. At some future point we can consider dropping them
>> when updating something already covered by a Param. We're stuck with
>> them for now.
>>
>
> The use case I would see is the extensibility. Say a customer wants to
> extend a schema and add an attribute X to the user object. He would
> still be able to manage users  using CLI without writing a plugin for
> the new attribute. Yes plugin is preferred but not everybody would go
> for it. So in absence of the plugin we can't do validation but we still
> should function and be able to deal with this attribute via CLI (and UI
> if this attribute is enabled for UI via UI configuration).
>
> I am generally against dropping this interface. But expectations IMO
> should be:
> 1) If the attribute is managed by us with setattr and friends it should
> behave in the same way as via the direct add/mod/del command
> 2) If attribute is not managed it should not provide any guarantees and
> act in the same way as via LDAP
>
> Hope this helps.
>

This might be the best thing to do, but IMO it is still no good, because 
the behavior of --{set,add,del}attr for a particular attribute might 
change between API versions, when that attribute changes from unmanaged 
to managed.

Honza

-- 
Jan Cholasta