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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel