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 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 ? Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ Freeipa-devel mailing list Freeipaemail@example.com https://www.redhat.com/mailman/listinfo/freeipa-devel