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

[Dmitri Pal]

On 04/11/2012 03:42 AM, Jan Cholasta wrote:
> On 11.4.2012 09:27, Martin Kosek wrote:
>> On Wed, 2012-04-11 at 09:18 +0200, Jan Cholasta wrote:
>>> 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
>>>
>>
>> I think this is OK and expectable - user may use setattr option to set
>> an attribute before it is officially supported by IPA and still have it
>> working when he upgrades.
>
> There's no guarantee the user will still have it working. User
> application might break if it depends on the unmanaged behavior and we
> are too strict in validation of the managed attribute, for example. I
> don't known how much of an issue is this actually, but this kind of
> unpredictability is not a good thing IMHO.
>
>> Though we should make sure that we describe
>> this API well in our documentation to make sure this expectation is
>> shared with users.
>>
>> Martin
>>
>
> Honza
>
Honza,

I do not agree. The difference in behavior will be only in validation.
So instead of letting an invalid data in we would start to prevent it.
IMO it is a bug in the application in the first place so breakage is
justified especially if we document this expectation.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/