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

[Petr Viktorin]

On 05/18/2012 03:49 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 05/14/2012 04:00 PM, Rob Crittenden wrote:
>>> Martin Kosek wrote:
>>>> On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
>>>>> On 04/10/2012 07:53 PM, Martin Kosek wrote:
>>>>>> On Tue, 2012-04-10 at 19:25 +0200, 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:
>>>>>> [snip]
>>>>>>>>> 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.
>>>>>>
>>>>>> That makes some sense, I could agree with that.
>>>>>>
>>>>>
>>>>> Now that I have a ticket on this
>>>>> (https://fedorahosted.org/freeipa/ticket/2580), I would like to get
>>>>> some
>>>>> wider agreement here.
>>>>>
>>>>> The no_update/no_create attributes are mainly "enabled" flags
>>>>> (ipaenabledflag, nsaccountlock, idnszoneactive), administrative
>>>>> (krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
>>>>> type and data, and various virtual attributes.
>>>>>
>>>>> If setattr etc. is disabled for all of these, it will mainly matter
>>>>> for
>>>>> the "enabled" flags. To be honest I don't know why we only allow
>>>>> modifying those through special commands.
>>>>> If there's some security reason for that, then setattr etc. should be
>>>>> disabled for them; otherwise I think they should be changeable through
>>>>> xyz-mod.
>>>>
>>>> I am not aware of any security reasons why we use special commands for
>>>> enabling/disabling objects. I assume this is to make it different from
>>>> standard object attribute changes and make sure that user does not
>>>> disable the object "by accident" when doing a mod operation. Rob, maybe
>>>> you remember the reason for this interface....
>>>>
>>>> But since we already have this approach, we should keep it and
>>>> implement
>>>> missing "xyz-enable" and "xyz-disable" command so that user's using
>>>> *attr interface to play with enabled/disabled attributes can switch to
>>>> the specialized commands.
>>>>
>>>> So far, I noticed that only DNS zone object misses the enable/disable
>>>> commands, I created a ticket to fix that:
>>>>
>>>> https://fedorahosted.org/freeipa/ticket/2754
>>>>
>>>>> Either way, setattr etc. should honor the no_update flags. Any
>>>>> objections?
>>>>>
>>>>
>>>> Nope - as long as ticket 2754 is fixed.
>>>>
>>>> Martin
>>>>
>>>
>>> I think those are there so they don't show up for the -mod command since
>>> we have a separate interface for doing it.
>>>
>>> rob
>>
>> For that purpose, would a no_cli flag be better than no_create+no_update?
>>
>>
>> I found one more no_update attribute that might be useful with
>> --setattr: externalhost.
>>
>
> I think the flags are really only considered on the cli now. If a
> different flag can make the intention clearer I'm ok with that. Having
> two flags seems more flexible though.
>
> rob

No. When the no_update flag is specified, the param doesn't get cloned 
from the object to the -mod command at all.
(A lot of the getattr problems occured because this cloning also sets 
important data, the "attribute" bit, so the uncloned params are 
incomplete/unusable but the cloned ones aren't available.)

So, I want to have no_create/no_update for things we really don't want 
the user to change (even through setattr), and "hidden" (a better name 
for it than no_cli IMO) for things we just don't advertise because 
there's a different way to do it.
It would also make sense to make the "hidden" attributes available in 
-find ­­– AFAIK, currently you can't search for disabled users for example.

As for two flags being more flexible, I can't find a case where we'd 
want to hide an attribute in -mod but not in -add (or vice versa). I 
think that'd just be an unnecessary inconsistency. (Remember this is 
just hiding the param from the frontend; "krbprincipalname" is rightly 
no_update only.)


Also, in the future, I think the enable/disable commands should call the 
-mod command (or the other way around), so there's just one code path 
for bugs to hide in.

-- 
Petr³