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

[Petr Viktorin]

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?

-- 
Petr³