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 > >>>>ad...@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 ad...@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.
>From 9a98459b258da23529a072b9d77568628d2486e6 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Fri, 17 Aug 2012 11:19:03 +0200 Subject: [PATCH 1/3] ipachangeconf: allow specifying non-default delimeter for options --- ipa-client/ipaclient/ipachangeconf.py | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/ipa-client/ipaclient/ipachangeconf.py b/ipa-client/ipaclient/ipachangeconf.py index 6cf47b807957c245fe03ff4259e35526c49175a9..bdc5579fccd82193e837b5e86cbc694c2619317c 100644 --- a/ipa-client/ipaclient/ipachangeconf.py +++ b/ipa-client/ipaclient/ipachangeconf.py @@ -174,9 +174,12 @@ class IPAChangeConf: self.subsectdel[1])) continue if o['type'] == "option": + delim = o.get('delim', self.dassign) + if delim not in self.assign: + raise ValueError('Unknown delim "%s" must be one of "%s"' % (delim, " ".join([d for d in self.assign]))) output.append(self._dump_line(self.indent[level], o['name'], - self.dassign, + delim, o['value'])) continue if o['type'] == "comment": @@ -200,13 +203,21 @@ class IPAChangeConf: 'type': 'comment', 'value': value.rstrip()} # pylint: disable=E1103 + o = dict() parts = line.split(self.dassign, 1) if len(parts) < 2: - raise SyntaxError('Syntax Error: Unknown line format') + # The default assign didn't match, try the non-default + for d in self.assign[1:]: + parts = line.split(d, 1) + if len(parts) >= 2: + o['delim'] = d + break - return {'name': parts[0].strip(), - 'type': 'option', - 'value': parts[1].rstrip()} + if 'delim' not in o: + raise SyntaxError, 'Syntax Error: Unknown line format' + + o.update({'name':parts[0].strip(), 'type':'option', 'value':parts[1].rstrip()}) + return o def findOpts(self, opts, type, name, exclude_sections=False): @@ -256,13 +267,13 @@ class IPAChangeConf: 'value': val}) continue if o['type'] == 'option': - val = self._dump_line(self.indent[level], - o['name'], - self.dassign, - o['value']) - opts.append({'name': 'comment', - 'type': 'comment', - 'value': val}) + delim = o.get('delim', self.dassign) + if delim not in self.assign: + val = self._dump_line(self.indent[level], + o['name'], + delim, + o['value']) + opts.append({'name':'comment', 'type':'comment', 'value':val}) continue if o['type'] == 'comment': opts.append(o) -- 1.8.0
>From 550e10db91f82db916ac12cc1559eb1dbf901703 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Wed, 31 Oct 2012 10:59:04 +0100 Subject: [PATCH 2/3] Specify includedir in krb5.conf on new installs --- freeipa.spec.in | 2 +- install/share/krb5.conf.template | 2 ++ ipa-client/ipa-install/ipa-client-install | 7 ++++++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/freeipa.spec.in b/freeipa.spec.in index 3f446032360a37d6aeb55c287eea2c0cd088bf31..6c631315506fc0d7dfc05bbb8e7e6cefe3146004 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -84,7 +84,7 @@ BuildRequires: pylint BuildRequires: python-polib BuildRequires: libipa_hbac-python BuildRequires: python-memcached -BuildRequires: sssd >= 1.8.0 +BuildRequires: sssd >= 1.9.2 BuildRequires: python-lxml BuildRequires: python-pyasn1 >= 0.0.9a BuildRequires: python-dns diff --git a/install/share/krb5.conf.template b/install/share/krb5.conf.template index f8b1a6f09868c55e47f21279b6d061fbd8251171..ed30b9e0fe37151c097d25e8b2e9fcf600a0a690 100644 --- a/install/share/krb5.conf.template +++ b/install/share/krb5.conf.template @@ -1,3 +1,5 @@ +includedir /var/lib/sss/pubconf/krb5.include.d + [logging] default = FILE:/var/log/krb5libs.log kdc = FILE:/var/log/krb5kdc.log diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install index 190efb183d8c96e2c9665cf51d5346dc1111ae24..5bfaf3319e3f3f015059150b7cb9030495f3c7b8 100755 --- a/ipa-client/ipa-install/ipa-client-install +++ b/ipa-client/ipa-install/ipa-client-install @@ -729,7 +729,7 @@ def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok, options, filename, client_domain): krbconf = ipaclient.ipachangeconf.IPAChangeConf("IPA Installer") - krbconf.setOptionAssignment(" = ") + krbconf.setOptionAssignment((" = ", " ")) krbconf.setSectionNameDelimiters(("[","]")) krbconf.setSubSectionDelimiters(("{","}")) krbconf.setIndent((""," "," ")) @@ -737,6 +737,11 @@ def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok, opts = [{'name':'comment', 'type':'comment', 'value':'File modified by ipa-client-install'}, {'name':'empty', 'type':'empty'}] + # SSSD include dir + if options.sssd: + opts.append({'name':'includedir', 'type':'option', 'value':'/var/lib/sss/pubconf/krb5.include.d/', 'delim':' '}) + opts.append({'name':'empty', 'type':'empty'}) + #[libdefaults] libopts = [{'name':'default_realm', 'type':'option', 'value':cli_realm}] if not dnsok or not cli_kdc or options.force: -- 1.8.0
>From f51e6c94f57bb0103b1251558c0b2f3d206dbfef Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Wed, 31 Oct 2012 10:15:28 +0100 Subject: [PATCH 3/3] Add the includedir to krb5.conf on upgrades --- freeipa.spec.in | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/freeipa.spec.in b/freeipa.spec.in index 6c631315506fc0d7dfc05bbb8e7e6cefe3146004..9a918ddf7ae9799b78603ef5cc7ed99c8bbfe5cd 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -283,6 +283,7 @@ Requires: libsss_autofs Requires: autofs Requires: libnfsidmap Requires: nfs-utils +Requires(post): policycoreutils Obsoletes: ipa-client >= 1.0 @@ -611,6 +612,21 @@ if [ $1 -eq 0 ]; then fi %endif +%post client +if [ $1 -gt 1 ] ; then + # Has the client been configured? + 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 + echo "includedir /var/lib/sss/pubconf/krb5.include.d/" > /etc/krb5.conf.ipanew + cat /etc/krb5.conf >> /etc/krb5.conf.ipanew + mv /etc/krb5.conf.ipanew /etc/krb5.conf + /sbin/restorecon /etc/krb5.conf + fi + fi +fi %if ! %{ONLY_CLIENT} %files server -f server-python.list -- 1.8.0
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel