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

Thu Feb 28 12:02:54 UTC 2013

On 02/28/2013 12:42 PM, Sumit Bose wrote:
> On Thu, Feb 28, 2013 at 08:44:35AM +0100, Martin Kosek wrote:
>> 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
> 
> Are you thinking of having this in addition to the for-all-services
> default values in cn=ipaConfig,cn=etc or shall those be dropped? I don't
> like the first case because then three different objects needs to be
> consulted to find out which is the right type. This wouldn't be an issue
> for the plugin, but I think it is hard for the user/admin to follow.

Hm, you are right.

> 
> If the current defaults shall be dropped I think this is a major change
> because it will require changes in the current CLI and WebUI which will
> be visible to the users. I'm not against this change, I'm just wondering
> if it is worth the effort for the next release?
> 
> Maybe an argument to keep this is in global default is that the settings
> are used for the host/*.* services as well which are in a different
> sub-tree of the cn=accounts container. Additionally in future we might
> want apply those setting to the user TGTs as well?

Yeah, that was actually my point. That we are mixing service-specific PAC
"rules" to the global setting. Which may be shared with host/*.* principals and
user principals. This automatic PAC rules may require some designing so that is
is generally usable.

As for your current patch set, I just finished my tests (Kerberos secured NFS
share, accessed from Linux/AD with Trust) and it worked fine... Though in my
case I did not have to disable PAC for the NFS service principal - its size
probably did not exceed the kernel ticket size limit.

I am now just concerned to this error message:

+            root_logger.info("search results for NFS services have been "
+                             "truncated by the server; please check manually "
+                              "if the PAC type is set correctly for all NFS "
+                              "services")

I am thinking we may want to increase the error message to ERROR so that it is
visible during rpm update. Otherwise the error message could get lost easily.

Martin