[Freeipa-devel] [PATCH] client: include the directory with domain-realm mappings in krb5.conf

[Rob Crittenden]

Jakub Hrozek wrote:
> On Mon, Nov 12, 2012 at 05:42:21PM +0100, Jan Cholasta wrote:
>> On 9.11.2012 17:58, Jakub Hrozek wrote:
>>> On Wed, Nov 07, 2012 at 05:20:30PM +0100, Martin Kosek wrote:
>>>> On 11/07/2012 12:53 AM, Jakub Hrozek wrote:
>>>>> On Wed, Oct 31, 2012 at 01:22:52PM +0100, Martin Kosek wrote:
>>>>>> On 10/31/2012 11:00 AM, Jakub Hrozek wrote:
>>>>>>> On Mon, Oct 22, 2012 at 02:14:00PM -0400, Simo Sorce wrote:
>>>>>>>> On Mon, 2012-10-22 at 17:15 +0200, Martin Kosek wrote:
>>>>>>>>> On 10/08/2012 08:27 PM, Rob Crittenden wrote:
>>>>>>>>>> Jakub Hrozek wrote:
>>>>>>>>>>> On Fri, Aug 17, 2012 at 12:20:27PM -0400, Simo Sorce wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ----- Original Message -----
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> the attached patches add the directory the SSSD writes domain-realm
>>>>>>>>>>>>> mappings as includedir to krb5.conf when installing the client.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for
>>>>>>>>>>>>> options
>>>>>>>>>>>>> ipachangeconf only allows one delimeter between keys and values. This
>>>>>>>>>>>>> patch adds the possibility of also specifying "delim" in the option
>>>>>>>>>>>>> dictionary to override the default delimeter.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On a slightly-unrelated note, we really should think about adopting
>>>>>>>>>>>>> Augeas. Changing configuration with home-grown scripts is getting
>>>>>>>>>>>>> tricky.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [PATCH 2/3] Specify includedir in krb5.conf on new installs
>>>>>>>>>>>>> This patch utilizes the new functionality from the previous patch to
>>>>>>>>>>>>> add
>>>>>>>>>>>>> the includedir on top of the krb5.conf file
>>>>>>>>>>>>>
>>>>>>>>>>>>> [PATCH 3/3] Add the includedir to krb5.conf on upgrades
>>>>>>>>>>>>> This patch is completely untested and I'm only posting it to get
>>>>>>>>>>>>> opinions. At first I was going to use an upgrade script in %post but
>>>>>>>>>>>>> then I thought it would be overengineering when all we want to do is
>>>>>>>>>>>>> prepend one line.. Would a simple munging like this be acceptable or
>>>>>>>>>>>>> shall I write a full script?
>>>>>>>>>>>>
>>>>>>>>>>>> NACK, using a scriptlet is fine, but not the way you did, as it has a huge
>>>>>>>>>>>> race condition where krb5.conf exists and has only one line in it (the
>>>>>>>>>>>> include line).
>>>>>>>>>>>>
>>>>>>>>>>>> You should first create the new file: echo "include ..." > /etc/krb.conf.ipanew
>>>>>>>>>>>> Then cat the contents of the existing file in i:t cat /etc/krb.conf >>
>>>>>>>>>>>> /etc/krb.conf.ipanew
>>>>>>>>>>>> And finally atomically rename it: mv /etc/krb.conf.ipanew /etc/krb.conf
>>>>>>>>>>>>
>>>>>>>>>>>> This method is also safe wrt something killing the yum process ...
>>>>>>>>>>>>
>>>>>>>>>>>> Simo.
>>>>>>>>>>>
>>>>>>>>>>> I'm attaching a new revision of the patches not even two months after
>>>>>>>>>>> the original nack.
>>>>>>>>>>>
>>>>>>>>>>> I also think it might be nice to have a more general way of upgrading
>>>>>>>>>>> the client config so I filed
>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3149
>>>>>>>>>>
>>>>>>>>>> I don't think grepping for a string is an effective way to determine if the
>>>>>>>>>> client has been configured. Someone could have removed that line.
>>>>>>>>>>
>>>>>>>>>> I'd prefer using /var/lib/ipa-client/sysrestore/sysrestore.index. If it exists
>>>>>>>>>> and has more than 2 lines in it ([files] + one other file) then it is safe to
>>>>>>>>>> say the client is configured, or at least partially configured.
>>>>>>>>>>
>>>>>>>>>> rob
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I just found one more issue. What if ipa-client-install is run with --no-sssd
>>>>>>>>> option? In that case I assume we should not include the SSSD's krb5.include.d,
>>>>>>>>> right? The same would also appy for upgrades, we would need to check if client
>>>>>>>>> was actually configured with SSSD before mangling their krb5.conf.
>>>>>>>>
>>>>>>>> Yeah that's right, we should have all sssd related changes under a
>>>>>>>> conditional that is true only when sssd is enabled.
>>>>>>>>
>>>>>>>> Simo.
>>>>>>>
>>>>>>> OK, new patches are attached. In new installs, the includedir is only
>>>>>>> added when options.sssd is true. During upgrades, I checked for
>>>>>>> sssd.conf's existence, I'm not sure if there's any other way to check if
>>>>>>> the client was configured with sssd?
>>>>>>
>>>>>> Hello Jakub, thanks for these patches. I think that checking if /etc/sssd.conf
>>>>>> exists as actually not so bad way to test if it was configured. Anyway, I did
>>>>>> few tests on server and client but I still see few issues:
>>>>>>
>>>>>> 1) SELinux context of krb5.conf is not as expected (but I am not sure what real
>>>>>> issue could that cause):
>>>>>>
>>>>>> # restorecon -FvvR /etc/krb5.conf
>>>>>> restorecon reset /etc/krb5.conf context
>>>>>> unconfined_u:object_r:etc_t:s0->system_u:object_r:krb5_conf_t:s0
>>>>>>
>>>>>
>>>>> Fixed. Thanks, I shouldn have noticed that doing mv would just replace
>>>>> the original context.
>>>>>
>>>>>> 2) I can no longer kinit on IPA server after applying your patch
>>>>>> # rpm -q sssd
>>>>>> sssd-1.9.90-0.20121030T1436Zgitf46bf56.fc17.x86_64
>>>>>> # rpm -Uvh --force freeipa-*.rpm
>>>>>> # head -n 5 /etc/krb5.conf
>>>>>> includedir /var/lib/sss/pubconf/krb5.include.d/
>>>>>> [logging]
>>>>>>   default = FILE:/var/log/krb5libs.log
>>>>>>   kdc = FILE:/var/log/krb5kdc.log
>>>>>>   admin_server = FILE:/var/log/kadmind.log
>>>>>> # KRB5_TRACE=/dev/stdout kinit admin
>>>>>> [21059] 1351684052.658548: Getting initial credentials for
>>>>>> admin at IDM.LAB.BOS.REDHAT.COM
>>>>>> [21059] 1351684052.665269: Sending request (200 bytes) to IDM.LAB.BOS.REDHAT.COM
>>>>>> [21059] 1351684052.665989: Resolving hostname vm-044.idm.lab.bos.redhat.com
>>>>>> [21059] 1351684052.667511: Sending initial UDP request to dgram 10.16.78.44:88
>>>>>> [21059] 1351684052.672514: Received answer from dgram 10.16.78.44:88
>>>>>> [21059] 1351684052.672653: Response was from master KDC
>>>>>> [21059] 1351684052.672751: Received error from KDC: -1765328370/KDC has no
>>>>>> support for encryption type
>>>>>> kinit: KDC has no support for encryption type while getting initial credentials
>>>>>>
>>>>>>
>>>>>> Now when I comment includedir:
>>>>>> # head -n 5 /etc/krb5.conf
>>>>>> # kinit admin
>>>>>> Password for admin at IDM.LAB.BOS.REDHAT.COM:
>>>>>> # echo $?
>>>>>> 0
>>>>>>
>>>>>> When I upgraded client machine (without krb5kdc), kinit worked fine. Does that
>>>>>> mean that krb5.conf can only be changed on client machines?
>>>>>>
>>>>>
>>>>> I'm still looking into this. I'm not sure why kinit does that and why it
>>>>> does that on the IPA server only. Unfortunately the default krb5 package
>>>>> is so optimized that I need to rebuild one without optimizations.
>>>>>
>>>>>> 3) We should also add Requires on sssd >= 1.9.0 in FreeIPA spec file to pick up
>>>>>> the new feature. Otherwise I just get an error on client:
>>>>>>
>>>>>> # kinit admin
>>>>>> kinit: Included profile directory could not be read while initializing Kerberos
>>>>>> 5 library
>>>>>>
>>>>>
>>>>> Done
>>>>>
>>>>>> 4) (Optional) I think we can make the process of checking if IPA is configured
>>>>>> easier and follow a similar way that Sumit did:
>>>>>> https://fedorahosted.org/freeipa/changeset/fe66fbe637132ac5eb22eea388e2261f33497bf5/
>>>>>>
>>>>>> This section:
>>>>>>
>>>>>> +restore=0
>>>>>> +test -f '/var/lib/ipa-client/sysrestore/sysrestore.index' && restore=$(wc -l
>>>>>> '/var/lib/ipa-client/sysrestore/sysrestore.index' | awk '{print $1}')
>>>>>> +
>>>>>> +if [ -f '/etc/sssd/sssd.conf' -a $restore -ge 2 ]; then
>>>>>> +    if ! egrep -q '/var/lib/sss/pubconf/krb5.include.d/' /etc/krb5.conf
>>>>>> 2>/dev/null ; then
>>>>>>
>>>>>> could then be replaced by something like this:
>>>>>>
>>>>>> python -c "import sys; from ipapython import ipautil; sys.exit(0 if
>>>>>> ipapython.is_ipaclient_configured() else 1);" > /dev/null 2>&1
>>>>>> if [  $? -eq 0 ]; then
>>>>>>
>>>>>> I am not saying you need to do this step, this can be done later by us.
>>>>>
>>>>> That code currently only exists for IPA server, right? At least judging
>>>>> by:
>>>> >from ipaserver.install import installutils;
>>>>>
>>>>> Then I would prefer to do it separately. It's a good idea, though, the
>>>>> postscript as it is now is ugly.
>>>>>
>>>>
>>>> Thanks for updated patch, now when I updated to the most recent sssd, kinit
>>>> worked for me even though IPA master krb5.conf was updated. Few more issues I
>>>> found follows:
>>>>
>>>
>>> That must have been krb5 updated, sssd does not have much to do with
>>> bare kinit.
>>>
>>>> rpmbuild --define "_topdir /root/freeipa-master/rpmbuild" -ba freeipa.spec
>>>> error: line 179: Bad Requireflags: qualifiers: Requires(postttrans):
>>>> policycoreutils
>>>> make: *** [rpms] Error 1
>>>>
>>>> This is the reason:
>>>> +Requires(postttrans): policycoreutils
>>>> should be:
>>>> +Requires(posttrans): policycoreutils
>>>>
>>>
>>> Thanks, the requires were misplaced, they were present in the server
>>> section and should have been in the client section..and because I only
>>> tested with make client-rpms (see below), I didn't notice the typo.
>>>
>>>> 2) IPA server krb5.conf is not updated for clean server/replica installation.
>>>> The includedir can get there only with next package update.
>>>>
>>>> install/share/krb5.conf.template would also need to be updated.
>>>>
>>>
>>> Done. I didn't realize the codepaths were different.
>>>
>>>>
>>>> Besides these 2 issues (and the SELinux ones), the patch should be good to go.
>>>>
>>>> Martin
>>>
>>> New patches are attached.
>>>
>>
>> We have discussed the patch with Jakub off-list and decided that the
>> upgrade should be done in %post (with an appropriate $1 check)
>> instead of %posttrans.
>>
>
> With a little bit more context about why I chose %posttrans initially at
> all..I wasn't sure if yum/rpm guarantees it would install the
> dependencies (in this case sssd) before installing ipa-client-install,
> so I initially did the upgrade in %posttrans to make sure all packages
> were in place.
>
>> Besides that, ACK.
>
> Thank you, new patches are attached.

I was waiting for selinux-policy to be updated so the included files 
would work in enforcing mode. That new package is on its way to 
updates-testing now.

I pushed these three patches plus one which sets a new minimum for 
selinux-policy.

pushed to master and ipa-3-0

rob