[Freeipa-devel] [PATCHES 509-514] replica promotion: use host credentials when setting up replication

[Simo Sorce]

On Mon, 2015-11-23 at 08:54 +0100, Jan Cholasta wrote:
> On 20.11.2015 17:58, Simo Sorce wrote:
> > On Fri, 2015-11-20 at 16:49 +0100, Jan Cholasta wrote:
> >> On 19.11.2015 17:43, Simo Sorce wrote:
> > [..]
> >>> On the patches
> >>> --
> >>> 509:
> >>> - commit says only: "aci: add IPA servers host group 'ipaservers'"
> >>> but it does other things like changing how CA renewal certificate acis
> >>> are added, I think that must be separated in another patch.
> >>>
> >>> - Why "Manage Host Keytab"  aci has been changed to exclude ipaservers ?
> >>
> >> To allow only members of the admins group to manage keytabs of IPA
> >> servers. This is analogous to how only members of the admins group can
> >> change passwords of other admins.
> >
> > I guess there is an interaction between ACIs I am not getting here, can
> > you give a small overview of how we end up here ?
> 
> This ACI allows admins to modify keytab of any host:
> 
> aci: (targetattr = "krbPrincipalKey || krbLastPwdChange")(target = 
> "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX")(version 3.0;acl 
> "Admins can manage host keytab";allow (write) groupdn = 
> "ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX";)
> 
> The original "Manage Host Keytab" ACI allows any user with the 
> corresponding permission to modify keytab of any host.
> 
> The modified "Manage Host Keytab" ACI allows any user with the 
> correcponding permission to modify keytab of any host except for members 
> of ipaservers.
> 
> The result is that the user has to be member of admins to be able to 
> modify keytab of IPA servers.
> 
> > If IPAservers are not supposed to access those attributes, why are they
> > added to the permissions at all ?
> 
> The effect of the change is not to limit access of IPA servers, but to 
> limit access of non-admin users *to* IPA servers.

Ah, thanks for explaining, this makes a lot of sense.

> > Wouldn't it make sense to have 2 set of permissions ?
> > One for admin type users (with access to all attributes) and one for
> > less privileged users ?
> 
> There already is a separate ACI which allows admins full access to the 
> attributes, see above.
> 
> >
> >>> - Why adding the host to the ipaservers group is done in the
> >>> krbinstance ? I would expect LDAP ops to be mostly done in the
> >>> DSinstance.
> >>
> >> It is the same place where the host entry is created.
> >
> > I will translate this with "historical" :-)
> >
> >>> - I do not see where the host is added to the ipaservers group on
> >>> upgrade.
> >>
> >> It's in install/updates/20-ipaservers_hostgroup.update:
> >>
> >> add: member: fqdn=$FQDN,cn=computers,cn=accounts,$SUFFIX
> >
> > Ahh, I missed that, thanks!
> >
> >>> 510:
> >>> - We should probably tightenup the ACI to allos host X to only add
> >>> memberPrincipal = X and no other value, also the host should not be
> >>> allowed to change the memberPrincipal attribute only the keys.
> >>> If we can't express this in ACIs we can live with the ones you propose
> >>> though.
> >>
> >> I think this can be done.
> >>
> >> memberPrincipal is included in the ACI because KEMLdap.set_key() touches
> >> it. Perhaps it is not intentional?
> >
> > Yeah, it is a bug in set_key(), of course we need to add memberPrincipal
> > when creating a whole new entry, but we should not modify it on existing
> > entries.
> 
> I though so, I will fix that.
> 
> >
> >>> 511:
> >>> ack, but I think you should catch potential exceptions from the
> >>> os.rmdir, as it will throw if there is some other file in the dir (for
> >>> whatever reason). The exception can be safely ignored, we just do not
> >>> want to blow up with a traceback here.
> >>
> >> OK.
> >>
> >>>
> >>> 512:
> >>> - I do not think this is correct, it seem to me you are overriding the
> >>> local ccache (if any) with the host keytab. If I read this right, this
> >>> means that if you run kinit admin && ipa-replica-install then you will
> >>> kind your admin credentials gone and a host TGT instead. Am I reading
> >>> this change correctly ?
> >>
> >> No. :-) Temporary ccache is now used for the install and the host is
> >> kinited into it. The initial ccache is used only for connection check
> >> (and adding the host to ipaservers in patch 514). Your kinit admin will
> >> not be lost.
> >
> > Ok, got it.
> >
> >>>
> >>> 513:
> >>> Nack
> >>> - Should be folded into 510
> >>
> >> These ACIs are required only for patch 514.
> >
> > Yes, but they are close and will give a full picture of the change
> > required for the feature if they are together. They won't hurt anything
> > even if unused until a few patches later (IIUC).
> 
> OK, makes sense.
> 
> >
> >>> - Should not allow all hosts in the domain but only ipaservers for aal
> >>> three changes
> >>
> >> If the host isn't member of ipaservers before ipa-replica-install is
> >> run, the "Check that we don't already have a replication agreement",
> >> "Detect if the other master can handle replication managers" and "Find
> >> if any server has a CA" steps will fail without these ACIs, which breaks
> >> patch 514.
> >
> > Yes I understood that, but it is intentional, these are administrative
> > checks, they need to be done as admin or with a host account that has
> > been pre-added to the ipaservers group. We do not want to expose
> > information to all other hosts in the domain as replication agreements
> > can include passwords in some cases, or other access information the
> > admin may not want to make available to random hosts.
> 
> The access is limited to objectclass and cn only, so the hosts will be 
> able to see only if the entry exists.
> 
> Additionaly, for replication agreements, only the host with the hostname 
> corresponding to the replication agreement can access the entry.

I am not sure I really like to allow every host to find out the topology
(by checking replication agreements on each master), but it is probably
ok for now.

> >>> 514:
> >>> - Should be folded in 512, they are completely interdependent changes.
> >>
> >> Patch 512 works fine without this patch, but the host has to be member
> >> of ipaservers before ipa-replica-install is run.
> >
> > Yeah, this is a problem, we do not want admins to have to do that,
> > right ?
> >
> >>> - I do not understand how this works. promote_check() runs before
> >>> promote() and will fail if the host is not already in the ipaservers
> >>> group, so you will never be able to actually add the host to the group ?
> >>> sounds like chicken-egg ... or what am I missing ?
> >>
> >> The chicken-egg is solved by the ACIs from patch 513. They allow any
> >> host to perform the checks necessary for promote_check() to succeed, so
> >> the host can be added to ipaservers in promote().
> >
> > I do not see how ACIs solve the problem where the code check if the
> > server is already in ipaservers, and the server isn't. My reading is
> > this please tell me where I am wrong:
> >
> > Admin, out of the blue, run ipa-replica-install on a random client.
> > promote_check():
> >   checks if host is in ipaservers
> >   host is not in ipaservers and we bail --> end of the story here ?
> > promote():
> >   add itself to ipaserver <-- how do we get here ?
> 
> It's:
> 
> promote_check():
>    check if host is in ipaservers
>    if host is not in ipaservers:
>      if the user can't modify ipaserver we bail --> end of story here

I found out that I misread the check you did on the [member] attribute,
now it all makes sense to me, thanks.

>    do a bunch of other checks (these require the ACIs if the host is not 
> in ipaservers)
> promote():
>    add the host to ipaservers (if it's not already in and the user can 
> modify ipaservers)

HTH,
Simo.

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