[Freeipa-devel] [PATCH] 761 keytab manipulation permission management

[Jan Cholasta]

Dne 16.10.2014 v 11:24 Petr Vobornik napsal(a):
> On 16.10.2014 09:54, Jan Cholasta wrote:
>> Dne 13.10.2014 v 12:42 Petr Vobornik napsal(a):
>>> On 8.10.2014 18:51, Petr Vobornik wrote:
>>>> On 1.10.2014 18:15, Petr Vobornik wrote:
>>>>> Hello list,
>>>>>
>>>>> Patch for: https://fedorahosted.org/freeipa/ticket/4419
>>>>>
>>>>
>>>> New revisions of 761 and 763 with updated API and ACIs:
>>>>
>>>> ipa host-allow-operation HOSTNAME retrieve-keytab --users=STR --groups
>>>> STR
>>>>    ipa host-disallow-operation HOSTNAME retrieve-keytab --users=STR
>>>> --groups STR
>>>>    ipa host-allow-operation HOSTNAME create-keytab --users=STR
>>>> --groups STR
>>>>    ipa host-disallow-operation HOSTNAME create-keytab --users=STR
>>>> --groups STR
>>>>
>>>>    ipa service-allow-operation PRINCIPAL retrieve-keytab --users=STR
>>>> --groups STR
>>>>    ipa service-disallow-operation PRINCIPAL retrieve-keytab --users=STR
>>>> --groups STR
>>>>    ipa service-allow-operation PRINCIPAL create-keytab --users=STR
>>>> --groups STR
>>>>    ipa service-disallow-operation PRINCIPAL create-keytab --users=STR
>>>> --groups STR
>>>>
>>>> ACIs are targeted to specific operations by including subtypes.
>>>>
>>>
>>> patch #761 rebased because of VERSION bump
>>
>> Since we are apparently not going to treat ipaAllowedToPerform as a
>> member attribute, you should remove reference to it from
>> attribute_members and relationships attributes of service and host.
>
> I still think of it a as member attribute, at least internally.

Given the implementation, I see you can't remove it from 
attribute_members, but it should be removed from relationships nonetheless.

>
>>
>> What's up with ipaallowedtoperform_subtypes_map? Why rename the
>> operations?
>
> It's not renaming. It's a change of internal raw LDAP value into
> self-describing name consistent with terminology of ipa-getkeytab

OK, you are obviously not responsible for this mess, so let's go with it.

>
>>
>> You probably don't want to hardcode 'ipaallowedtoperform_read_keys' in
>> rename_ipaallowedtoperform_to_ldap().
>
> Fixed, but it doesn't make much difference given that the method has
> only one purpose.

Sorry, I missed the comment at the beginning of service_allow_operation, 
it indeed does not make any difference. (I can't say I'm a fan of such 
ugly hacks though.)

>
>>
>> Why do you override get_args() in service_{,dis}allow_operation instead
>> of overriding takes_args?
>
> Fixed
>
>>
>> I'm not particularly happy about the '_subtype' option bussiness, but at
>> least it's not invasive, so I guess it's OK.
>>
>> Note that I still think this API sucks and we should instead go with the
>> generic member-like attribute approach, or take our time to design it
>> properly so that it fits in the framework (no time in 4.1) instead of
>> making it a hacky Franken-API like it is now.
>>


-- 
Jan Cholasta