[Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

[Martin Kosek]

On 02/27/2013 06:48 PM, Sumit Bose wrote:
> On Wed, Feb 27, 2013 at 08:37:18AM -0500, Simo Sorce wrote:
>> On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
>>> On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
>>>> On 02/21/2013 04:24 PM, Sumit Bose wrote:
>>>>> Hi,
>>>>>
>>>>> this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
>>>>> The related design page is
>>>>> http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
>>>>>
>>>>> It was agreed while discussing the design page that the currently
>>>>> hardcoded value for the NFS service will be dropped completely, hence
>>>>> the first patch reverts to patch which added the hardcoded value.
>>>>>
>>>>> The second patch adds the needed attribute to compensate the now missing
>>>>> hardcoded value.
>>>>>
>>>>> In the following three patches the PAC type is read and used
>>>>> accordingly. The last patch adds a unit test for a new function.
>>>>>
>>>>> bye,
>>>>> Sumit
>>>>
>>>> I did only sanity testing to the C part of the RFE so far, but by reading it it
>>>> looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb part
>>>> is OK.
>>>
>>> Alexander asked in irc to change strcmp() to strcasecmp() so that
>>> options like 'Ms-Pac' or 'none' are accepted as well.
>>
>> Is there a good reason to allow insensitive matching ?
>>
>> in LDAP you can't use true or false either for booleans, you have to use
>> TRUE and FALSE, so I am not so thrilled to allow insensitive matching
>> for this option as well.
> 
> I'm fine with this. Alexander, any comments.
> 
>>
>>>> I will comment on the Python parts:
>>>>
>>>> 1) Your update check testing if there is any NFS service with ipakrbauthzdata
>>>> looks like a good heuristics to prevent admin frustration. Though an ideal
>>>> solution would require
>>>> https://fedorahosted.org/freeipa/ticket/2961
>>>> to prevent multiple updates when admin purposefully removes this attribute. We
>>>> may want to reference the ticket in a comment in the update plugin...
>>>
>>> I added a comment in the code and in #2961.
>>>
>>>>
>>>>
>>>> 2) The upgrade plugin may get into infinite loop if ldap.find_entries returns
>>>> truncated result. As you do not update entries directly but only return update
>>>> instructions when you have no truncated results, you will loop.
>>>>
>>>> To simulate this, I just created 2 NFS principals and set size_limit=1 in your
>>>> find_entries call.
>>>
>>> Thanks for catching this, I removed the truncated logic and added a
>>> messages if truncated was set.
>>>
>>>>
>>>>
>>>> 3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
>>>> service principals added by service-add? Shouldn't we set ipakrbauthzdata for
>>>> new services too? As a default when no user ipakrhbauthzdata is set...
>>>> Otherwise admins could be surprised why their new NFS services do not work
>>>> while the others do.
>>>>
>>>> Also, I think we should have this NFS culprit documented somewhere (i.e. why we
>>>> set ipakrbauthzdata to NONE by default). At least as a small section in the
>>>> online help for services plugin...
>>>
>>> I added comments to the help and the doc string in patch #100. If you
>>> think it is necessary to implicitly set PAC type to 'NONE' if NFS
>>> services are added and no type is given explicitly, I would prefer if
>>> you can open a new tickets so that it can be discussed independently.
>>>
>>> Thank you for the review. New versions attached.
>>
>> I do not think we should set the policy to NONE by default, OTOH I see
>> the problem with upgrades. Maybe we should have a way to express
>> additional defaults, per service type.
>>
>> Ie add additional values like:
>>
>> ipakrbAuthsData: nfs:NONE
>>
>> This would be a default but only for services that have the form nfs/*@*
>>
>> This would allow us to avoid magical special casing in the code and
>> allow admins to change the overall default for specific services as
>> needed.
>>
>> Maybe we should add a new ticket for this ?
> 
> This sounds like a good compromise and will make updating much easier.
> If we agree to do it this way I can add ipakrbAuthsData: nfs:NONE and
> fix the code with this ticket. But we would need tickets for the CLI and
> WebUI to handle this new case.
> 
> For the WebUI there already is
> https://fedorahosted.org/freeipa/ticket/3404 , maybe it can be extended
> to handle this new case as well?
> 
> As for design documents I think I can added the needed information to
> http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
> 
> bye,
> Sumit

Hi Sumit,

This looks like a good idea and would prevent the magic default PAC type, yes.
Though I would not add this service-specific setting to global IPA config object.

I would rather like to see that in the service tree, for example as a
configuration option of the service root which could be controlled with
serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:

# ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
# ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
# ipa serviceconfig-show
  Default PAC Map: nfs:NONE, cifs:PAD

Martin