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?
>From 184e44e5209e2cdbe03446e3e00754bfc5b115df 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.7.12.1

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

---
 ipa-client/ipa-install/ipa-client-install | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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

>From e2bcbe0424be113c53b8288cfe194deb0b6a711e 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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 
5845770576fe1408b244e8404eb9b16225deb68d..48a0f94fc3be13f0d056460738479ef33937920f
 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -610,6 +610,18 @@ if [ $1 -eq 0 ]; then
 fi
 %endif
 
+%posttrans client
+# 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
+    fi   
+fi
 
 %if ! %{ONLY_CLIENT}
 %files server -f server-python.list
-- 
1.7.12.1

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to