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

[Rob Crittenden]

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.

rob