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

[Simo Sorce]

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 ?
If IPAservers are not supposed to access those attributes, why are they
added to the permissions at all ?
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 ?

> > - 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.

> > 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).

> > - 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.

> > 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 ?

HTH,
Simo.

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