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