[Freeipa-devel] [PATCH] 0506 Default read ACIs for hosts

[Simo Sorce]

On Mon, 2014-04-14 at 10:54 +0200, Martin Kosek wrote:
> On 04/11/2014 02:53 PM, Petr Viktorin wrote:
> > On 04/11/2014 02:36 PM, Simo Sorce wrote:
> >> On Fri, 2014-04-11 at 09:48 +0200, Martin Kosek wrote:
> >>> On 04/10/2014 05:29 PM, Petr Viktorin wrote:
> >>>> On 04/10/2014 03:04 PM, Martin Kosek wrote:
> >>>>> On 04/10/2014 02:52 PM, Simo Sorce wrote:
> >>>>>> On Thu, 2014-04-10 at 13:56 +0200, Petr Viktorin wrote:
> >>>>>>> On 04/09/2014 12:25 PM, Martin Kosek wrote:
> >>>>>>>> On 04/03/2014 12:09 PM, Petr Viktorin wrote:
> >>>>>>>>> Hello,
> >>>>>>>>> This adds read permissions to read hosts.
> >>>>>>>>>
> >>>>>>>>> Read access is given to all authenticated users.
> >>>>>>>>> For reading host membership info, there is a separate permission that
> >>>>>>>>> also
> >>>>>>>>> defaults to all authenticated users.
> >>>>>>>>>
> >>>>>>>>> The userPassword attribute is not included for obvious reasons.
> >>>>>>>>
> >>>>>>>> 1) We decided to show hosts only to authenticated users by default. I
> >>>>>>>> am just
> >>>>>>>> thinking - should some portion of hosts be readable just like groups and
> >>>>>>>> users
> >>>>>>>> are? For example at least the host core defined by nsHost objectlass?
> >>>>>>>>
> >>>>>>>> objectClasses: ( nsHost-oid NAME 'nsHost' DESC 'Netscape defined
> >>>>>>>> objectclass'
> >>>>>>>>     SUP top STRUCTURAL MUST cn MAY ( serverHostName $ description $ l $
> >>>>>>>> nsHostLoc
> >>>>>>>>     ation $ nsHardwarePlatform $ nsOsVersion ) X-ORIGIN 'Netscape' )
> >>>>>>>>
> >>>>>>>> Are application supposed to be able to anonymously read that information?
> >>>>>>>
> >>>>>>> I'm not sure. Simo?
> >>>>>>
> >>>>>> Good question, probably not by default, we offer DNS and do not
> >>>>>> recommend people to rely on exposed host maps.
> >>>>>
> >>>>> Question is if should have a separate permission like "System: Read Host Core
> >>>>> Attributes", "System: Read Host", "System: Read Host Membership" or we are
> >>>>> fine
> >>>>> with just "System: Read Host", "System: Read Host Membership".
> >>>>>
> >>>>> If we do not expect people and programs to often read the list of host or the
> >>>>> basic host information anonymously, I would stick with the latter.
> >>>>
> >>>> Let's do that then. It's easy enough to add a custom permission.
> >>>>
> >>>>>>>> 2) Do we want to count enrolledBy and managedBy attribute to "System: Read
> >>>>>>>> Host
> >>>>>>>> Membership" permission or should it be included in the "Read Hosts"
> >>>>>>>> permission?
> >>>>>>>>
> >>>>>>>> If we want to stick with previous behavior, we would want to have only
> >>>>>>>> "memberOf" listed as this is how our original member handling ACI looks
> >>>>>>>> like:
> >>>>>>>>
> >>>>>>>> install/share/default-aci.ldif:aci: (targetattr = "memberOf ||
> >>>>>>>> memberHost ||
> >>>>>>>> memberUser")(version 3.0; acl "No anonymous access to member information";
> >>>>>>>> deny
> >>>>>>>> (read,search,compare) userdn != "ldap:///all";
> >>>>>>>
> >>>>>>> What was the reasoning behind enrolledBy and managedBy? I got it from
> >>>>>>> the notes from devconf; I don't think there was much discussion.
> >>>>>>
> >>>>>> I have no recollection :(
> >>>>>
> >>>>> There was no discussion about these particular attributes. I added then to
> >>>>> Membership permission because they were DN-ish, but when I think more
> >>>>> about it,
> >>>>> it does not seem as a membership in the sense of the ACI above. I would
> >>>>> personally only keep member/memberOf in the Membership attributes.
> >>>>
> >>>> Moved to Read Host.
> >>>>
> >>>>>>>> 3) I could not functionally test if e.g. clients and replicas still
> >>>>>>>> enroll as
> >>>>>>>> we do not have an ACI for krbtpolicy/krbRealmContainer yet and
> >>>>>>>> ipa-client-install searches for it.
> >>>>>>>>
> >>>>>>>> Speaking of which, we will need to have an ACI for reading a portion of
> >>>>>>>> krbRealmContainer anonymously to enable IPA client autodiscovery
> >>>>>>>> (cn+objectclass should be sufficient).
> >>>>>>
> >>>>>> Sad we ended up depending on this, I would have preferred to not depend
> >>>>>> on keeping this around if we ever want to change something.
> >>>>>
> >>>>> Yeah... But we should be OK with exposing just the CN which is not really a
> >>>>> secret as we know what is the realm name...
> >>>>>
> >>>>> Martin
> >>>>>
> >>>
> >>> The basis looks good now. However, when I was checking host's objectclass and
> >>> attributes we allow, I saw many attributes not listed in our DevConf meeting
> >>> minutes, but which are now missing in the entry when I read it as authenticated
> >>> used.
> >>
> >> As an admin, or as a regular user ?
> >>
> >>> krbPrincipalAux attributes like krbCanonicalName, krbExtraData,
> >>> krbLastAdminUnlock, krbLastFailedAuth...
> >>> We may want to list them all, except the ones we chose to now show, like
> >>> krbPrincipalKey :) I would welcome Simo's recommendation in this place.
> >>
> >> I do not think we necessarily need to expose most of them to regular
> >> users (which include keytab bearing services) by default.
> >>
> >>> krbTicketPolicyAux attributes like krbMaxRenewableAge
> >>>
> >>> Objectclasses for reference:
> >>>
> >>> objectClasses: ( 2.16.840.1.113719.1.301.6.8.1 NAME 'krbPrincipalAux' AUXILIAR
> >>>   Y MAY ( krbPrincipalName $ krbCanonicalName $ krbUPEnabled $ krbPrincipalKey
> >>>   $ krbTicketPolicyReference $ krbPrincipalExpiration $ krbPasswordExpiration $
> >>>    krbPwdPolicyReference $ krbPrincipalType $ krbPwdHistory $ krbLastPwdChange
> >>>   $ krbPrincipalAliases $ krbLastSuccessfulAuth $ krbLastFailedAuth $ krbLoginF
> >>>   ailedCount $ krbExtraData $ krbLastAdminUnlock ) )
> >>
> >> Of these we may want to show krbPrincipalName, krbCanonicalName,
> >> krbPrincipalAliases by default.
> > 
> > Added.
> > 
> >> I am uncertain about krbPrincipalExpiration, krbPasswordExpiration,
> >> krbLastPwdChange, but they may be needed for clients like SSSD to do
> >> proper checking and for power users to figure out themselves when they
> >> need to change passwords or ask for an account validity extension etc..
> > 
> > Let's show them then, to make their lives easier. Paranoid admins can always
> > hide them.
> > 
> >> We certainly should not expose:
> >> krbPrincipalKey, krbPwdHistory (though we do not use this IIRC, yet),
> >> krbExtraData
> >>
> >> The rest I lean more on the not showing by default, although they could
> >> be safely shown to authenticated users if necessary.
> > 
> > Similarly, generous admins can always give more access. I've not added these.
> > 
> >>> objectClasses: ( 2.16.840.1.113719.1.301.6.16.1 NAME 'krbTicketPolicyAux' AUXI
> >>>   LIARY MAY ( krbTicketFlags $ krbMaxTicketLife $ krbMaxRenewableAge ) )
> >>
> >> I do not think we need to expose these, although exposing them is not
> >> problematic, you can infer what they are by simply asking for tickets
> >> anyway.
> > 
> > Not added.
> > 
> > 
> > I assume the same will also apply to  users and services?
> > 
> 
> IMO yes. We will see if any issue caused by unreadable attribute pops up. For
> example, user-status command won't work properly without additional permissions
> as it reads krbLastSuccessfulAuth and krbLastFailedAuth. But we will get to
> that part when we get to users.

Admin/Helpdesk people will need access to those attributes indeed.
I guess those should be added to permissions to manage users.

> As for this patch, it works OK so it is an ACK from me.

LGTM.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York