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

[Jan Cholasta]

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

-- 
Jan Cholasta