On Wed, Feb 27, 2013 at 06:48:27PM +0100, 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.

Alexander said on irc that he is fine with the strict case-sensitive
handling of the values, as long as the user input is automatically
upper-cased by the framework as it is currently done for booleans. Since
the WebUI does not allow user entered values this will only affect the
CLI.

bye,
Sumit

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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to