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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code