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

[Sumit Bose]

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.

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?

bye,
Sumit
> 
> Martin