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

[Jan Cholasta]

On 23.11.2015 15:34, Simo Sorce wrote:
> 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.

Ad alternative is to add the host to ipaservers before the checks are 
done and remove it again if any of them fail.

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


-- 
Jan Cholasta