On 03/13/2013 06:06 PM, Rob Crittenden wrote:
> Martin Kosek wrote:
>> On 03/11/2013 09:39 AM, Petr Spacek wrote:
>>> On 11.3.2013 09:09, Martin Kosek wrote:
>>>> On 03/08/2013 09:49 AM, Petr Spacek wrote:
>>>>> On 8.3.2013 00:14, Rob Crittenden wrote:
>>>>>> Martin Kosek wrote:
>>>>>>> Remove obsolete BIND GSSAPI configuration options tkey-gssapi-credential
>>>>>>> and tkey-domain and replace them with tkey-gssapi-keytab which avoids
>>>>>>> unnecessary Kerberos checks on BIND startup and can cause issues when
>>>>>>> KDC is not available.
>>>>>>>
>>>>>>> Both new and current IPA installations are updated.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/3429
>>>>>>>
>>>>>>
>>>>>> Still reviewing this but I noticed that after upgrading my 3.1.99 server
>>>>>> pre-patch to with with-patch version the connections argument in 
>>>>>> named.conf
>>>>>> got set to 4 (courtesy of ipa-upgradeconfig). Should we be setting that 
>>>>>> to 4
>>>>>> during the initial install too?
>>>>>
>>>>> For 3.2 it doesn't matter. Anything >= 2 should be okay, but more 
>>>>> connections
>>>>> should not harm.
>>>>>
>>>>> Higher value should allow higher level of parallelism, it is one of tuning
>>>>> parameters. Value 4 was necessary to prevent deadlocks in some previous
>>>>> versions of bind-dyndb-ldap.
>>>>>
>>>>
>>>> Previously, when I implemented the upgrade script, I set connections arg 
>>>> only
>>>> if it was present in named.conf and thus bind-dyndb-ldap could not use a
>>>> reasonable default on its own decision.
>>>>
>>>> This was changed in e578183ea25a40aedf6dcc3e1ee4bcb19b73e70f and 
>>>> connections
>>>> is set always. Rob is correct, that in that case we might want to add it to
>>>> named.conf by default to make it consistent... or we could also fix upgrade
>>>> script to change connections only if it is present in named.conf.
>>>>
>>>> Petr, what does make more sense bind-dyndb-ldap wise?
>>>
>>> Default values should work. Personally I would include only "override" 
>>> values
>>> in named.conf, but technically it doesn't matter.
>>>
>>> Note: Latest bind-dyndb-ldap versions refuse to start if configuration is
>>> insane. Fatal error will point admin to errors in configuration and should
>>> prevent surprises from auto-magically changed values.
>>>
>>> Invalid configurations - examples:
>>> connections < 1
>>> connections < 2 && psearch is enabled
>>> serial_autoincrement enabled && psearch disabled
>>>
>>
>> Ok, lets set the connections argument only if it is in named.conf _and_ it is
>> lower than the required minimum. All patches attached.
>>
>> Martin
>>
> 
> This works ok if the format of named.conf is stable.
> 
> If, for example, there are extra spaces between options and { then the values
> are not updated. This is nothing new with this patch, our previous code was
> also space dependent (augeas will address this eventually)
> 
> I just wonder: Should we log if a warning if a change has not been applied?
> 
> rob

There is already a warning if we could not match tkey-gssapi-credential,
tkey-domain or tkey-gssapi-keytab. It would look like that:

# ipa-upgradeconfig --quiet
Either tkey-gssapi-credential or tkey-domain is missing in /etc/named.conf.
Skip update.

At least I made our crappy named.conf parser more resilient to spaces in
section start (i.e. it should now work with "options      {" and made the
regular expression object naming more consistent. But you are right, this will
be eventually improved by augueas.

Important thing for now is, that our updater works fine with our template
named.conf we ship with freeipa including user changes, if user does not go too
wild into breaking what's already there...

Martin
From 16063578811053f5a6cd6ed281c868847cb7d492 Mon Sep 17 00:00:00 2001
From: Martin Kosek <mko...@redhat.com>
Date: Tue, 5 Mar 2013 12:02:58 +0100
Subject: [PATCH 1/3] Update named.conf parser

Refactor the named.conf parsing and editing functions in bindinstance
so that both "dynamic-db" and "options" sections of named.conf can
be read and updated

https://fedorahosted.org/freeipa/ticket/3429
---
 ipaserver/install/bindinstance.py | 69 +++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index c14f2423ebecbe5bfb7ae9a719754077f826fcf0..20d3349558e6e916ab0c26d3fa8ba0baf12994ad 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -42,8 +42,13 @@ from ipalib.util import (validate_zonemgr, normalize_zonemgr,
 NAMED_CONF = '/etc/named.conf'
 RESOLV_CONF = '/etc/resolv.conf'
 
-named_conf_ipa_re = re.compile(r'(?P<indent>\s*)arg\s+"(?P<name>\S+)\s(?P<value>[^"]+)";')
-named_conf_ipa_template = "%(indent)sarg \"%(name)s %(value)s\";\n"
+named_conf_section_ipa_start_re = re.compile('\s*dynamic-db\s+"ipa"\s+{')
+named_conf_section_options_start_re = re.compile('\s*options\s+{')
+named_conf_section_end_re = re.compile('};')
+named_conf_arg_ipa_re = re.compile(r'(?P<indent>\s*)arg\s+"(?P<name>\S+)\s(?P<value>[^"]+)";')
+named_conf_arg_options_re = re.compile(r'(?P<indent>\s*)(?P<name>\S+)\s+"(?P<value>[^"]+)"\s*;')
+named_conf_arg_ipa_template = "%(indent)sarg \"%(name)s %(value)s\";\n"
+named_conf_arg_options_template = "%(indent)s%(name)s \"%(value)s\";\n"
 
 def check_inst(unattended):
     has_bind = True
@@ -85,26 +90,36 @@ def named_conf_exists():
             return True
     return False
 
-def named_conf_get_directive(name):
+NAMED_SECTION_OPTIONS = "options"
+NAMED_SECTION_IPA = "ipa"
+def named_conf_get_directive(name, section=NAMED_SECTION_IPA):
     """Get a configuration option in bind-dyndb-ldap section of named.conf"""
+    if section == NAMED_SECTION_IPA:
+        named_conf_section_start_re = named_conf_section_ipa_start_re
+        named_conf_arg_re = named_conf_arg_ipa_re
+    elif section == NAMED_SECTION_OPTIONS:
+        named_conf_section_start_re = named_conf_section_options_start_re
+        named_conf_arg_re = named_conf_arg_options_re
+    else:
+        raise NotImplementedError('Section "%s" is not supported' % section)
 
     with open(NAMED_CONF, 'r') as f:
-        ipa_section = False
+        target_section = False
         for line in f:
-            if line.startswith('dynamic-db "ipa"'):
-                ipa_section = True
+            if named_conf_section_start_re.match(line):
+                target_section = True
                 continue
-            if line.startswith('};'):
-                if ipa_section:
+            if named_conf_section_end_re.match(line):
+                if target_section:
                     break
 
-            if ipa_section:
-                match = named_conf_ipa_re.match(line)
+            if target_section:
+                match = named_conf_arg_re.match(line)
 
                 if match and name == match.group('name'):
                     return match.group('value')
 
-def named_conf_set_directive(name, value):
+def named_conf_set_directive(name, value, section=NAMED_SECTION_IPA):
     """
     Set configuration option in bind-dyndb-ldap section of named.conf.
 
@@ -116,25 +131,37 @@ def named_conf_set_directive(name, value):
     """
     new_lines = []
 
+    if section == NAMED_SECTION_IPA:
+        named_conf_section_start_re = named_conf_section_ipa_start_re
+        named_conf_arg_re = named_conf_arg_ipa_re
+        named_conf_arg_template = named_conf_arg_ipa_template
+    elif section == NAMED_SECTION_OPTIONS:
+        named_conf_section_start_re = named_conf_section_options_start_re
+        named_conf_arg_re = named_conf_arg_options_re
+        named_conf_arg_template = named_conf_arg_options_template
+    else:
+        raise NotImplementedError('Section "%s" is not supported' % section)
+
     with open(NAMED_CONF, 'r') as f:
-        ipa_section = False
+        target_section = False
         matched = False
         last_indent = "\t"
         for line in f:
-            if line.startswith('dynamic-db "ipa"'):
-                ipa_section = True
-            if line.startswith('};'):
-                if ipa_section and not matched:
+            if named_conf_section_start_re.match(line):
+                target_section = True
+            if named_conf_section_end_re.match(line):
+                if target_section and not matched and \
+                        value is not None:
                     # create a new conf
-                    new_conf = named_conf_ipa_template \
+                    new_conf = named_conf_arg_template \
                             % dict(indent=last_indent,
                                    name=name,
                                    value=value)
                     new_lines.append(new_conf)
-                ipa_section = False
+                target_section = False
 
-            if ipa_section and not matched:
-                match = named_conf_ipa_re.match(line)
+            if target_section and not matched:
+                match = named_conf_arg_re.match(line)
 
                 if match:
                     last_indent = match.group('indent')
@@ -143,7 +170,7 @@ def named_conf_set_directive(name, value):
                         if value is not None:
                             if not isinstance(value, basestring):
                                 value = str(value)
-                            new_conf = named_conf_ipa_template \
+                            new_conf = named_conf_arg_template \
                                     % dict(indent=last_indent,
                                            name=name,
                                            value=value)
-- 
1.8.1.4

From 49abec0af46e03accb95b463c09616b4bccfff5f Mon Sep 17 00:00:00 2001
From: Martin Kosek <mko...@redhat.com>
Date: Thu, 14 Mar 2013 10:30:32 +0100
Subject: [PATCH 2/3] Use tkey-gssapi-keytab in named.conf

Remove obsolete BIND GSSAPI configuration options tkey-gssapi-credential
and tkey-domain and replace them with tkey-gssapi-keytab which avoids
unnecessary Kerberos checks on BIND startup and can cause issues when
KDC is not available.

Both new and current IPA installations are updated.

https://fedorahosted.org/freeipa/ticket/3429
---
 install/share/bind.named.conf.template |  3 +-
 install/tools/ipa-upgradeconfig        | 69 +++++++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/install/share/bind.named.conf.template b/install/share/bind.named.conf.template
index 9fdd91319947f6cfd3034f8d2a4fe8bb60d1af77..b12df593ad902dfbe815c2292992f26204756cd8 100644
--- a/install/share/bind.named.conf.template
+++ b/install/share/bind.named.conf.template
@@ -14,8 +14,7 @@ options {
 	// Any host is permitted to issue recursive queries
 	allow-recursion { any; };
 
-	tkey-gssapi-credential "DNS/$FQDN";
-	tkey-domain "$REALM";
+	tkey-gssapi-keytab "/etc/named.keytab";
 };
 
 /* If you want to enable debugging, eg. using the 'rndc trace' command,
diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig
index 9bd706ad0beb45ba7a4b20f30f564f5e234bd133..f310ff76d283e582b2191d26c9773b388f24482b 100644
--- a/install/tools/ipa-upgradeconfig
+++ b/install/tools/ipa-upgradeconfig
@@ -451,6 +451,72 @@ def named_enable_serial_autoincrement():
 
     return changed
 
+def named_update_gssapi_configuration():
+    """
+    Update GSSAPI configuration in named.conf to a recent API.
+    tkey-gssapi-credential and tkey-domain is replaced with tkey-gssapi-keytab.
+    Details can be found in https://fedorahosted.org/freeipa/ticket/3429.
+
+    When some change in named.conf is done, this functions returns True
+    """
+
+    root_logger.info('[Updating GSSAPI configuration in DNS]')
+
+    if not bindinstance.named_conf_exists():
+        # DNS service may not be configured
+        root_logger.info('DNS is not configured')
+        return False
+
+    if sysupgrade.get_upgrade_state('named.conf', 'gssapi_updated'):
+        root_logger.debug('Skip GSSAPI configuration check')
+        return False
+
+    try:
+        gssapi_keytab = bindinstance.named_conf_get_directive('tkey-gssapi-keytab',
+                bindinstance.NAMED_SECTION_OPTIONS)
+    except IOError, e:
+        root_logger.error('Cannot retrieve tkey-gssapi-keytab option from %s: %s',
+                bindinstance.NAMED_CONF, e)
+        return False
+    else:
+        if gssapi_keytab:
+            root_logger.debug('GSSAPI configuration already updated')
+            sysupgrade.set_upgrade_state('named.conf', 'gssapi_updated', True)
+            return False
+
+    try:
+        tkey_credential = bindinstance.named_conf_get_directive('tkey-gssapi-credential',
+                bindinstance.NAMED_SECTION_OPTIONS)
+        tkey_domain = bindinstance.named_conf_get_directive('tkey-domain',
+                bindinstance.NAMED_SECTION_OPTIONS)
+    except IOError, e:
+        root_logger.error('Cannot retrieve tkey-gssapi-credential option from %s: %s',
+                bindinstance.NAMED_CONF, e)
+        return False
+
+    if not tkey_credential or not tkey_domain:
+        root_logger.error('Either tkey-gssapi-credential or tkey-domain is missing in %s. '
+            'Skip update.', bindinstance.NAMED_CONF)
+        return False
+
+    try:
+        bindinstance.named_conf_set_directive('tkey-gssapi-credential', None,
+                                              bindinstance.NAMED_SECTION_OPTIONS)
+        bindinstance.named_conf_set_directive('tkey-domain', None,
+                                              bindinstance.NAMED_SECTION_OPTIONS)
+        bindinstance.named_conf_set_directive('tkey-gssapi-keytab', '/etc/named.keytab',
+                                              bindinstance.NAMED_SECTION_OPTIONS)
+    except IOError, e:
+        root_logger.error('Cannot update GSSAPI configuration in %s: %s',
+                bindinstance.NAMED_CONF, e)
+        return False
+    else:
+        root_logger.debug('GSSAPI configuration updated')
+
+    sysupgrade.set_upgrade_state('named.conf', 'gssapi_updated', True)
+    return True
+
+
 def enable_certificate_renewal(ca):
     """
     If the CA subsystem certificates are not being tracked for renewal then
@@ -741,7 +807,8 @@ def main():
     add_server_cname_records()
     changed_psearch = named_enable_psearch()
     changed_autoincrement = named_enable_serial_autoincrement()
-    if changed_psearch or changed_autoincrement:
+    changed_gssapi_conf = named_update_gssapi_configuration()
+    if changed_psearch or changed_autoincrement or changed_gssapi_conf:
         # configuration has changed, restart the name server
         root_logger.info('Changes to named.conf have been made, restart named')
         bind = bindinstance.BindInstance(fstore)
-- 
1.8.1.4

From 08642f6db6297b1b6a74ab33b40f4c8fb4c3b691 Mon Sep 17 00:00:00 2001
From: Martin Kosek <mko...@redhat.com>
Date: Wed, 13 Mar 2013 10:55:48 +0100
Subject: [PATCH 3/3] Do not force named connections on upgrades

We used to set connections argument for bind-dyndb-ldap even when
the attribute was not in named.conf. This is not necessary as
the bind-dyndb-ldap plugin chooses a sane default instead of us.
---
 install/tools/ipa-upgradeconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig
index f310ff76d283e582b2191d26c9773b388f24482b..f5652139d6e8632970e6c558275b69a61f2b0510 100644
--- a/install/tools/ipa-upgradeconfig
+++ b/install/tools/ipa-upgradeconfig
@@ -383,7 +383,7 @@ def named_enable_psearch():
         # "connections" option, bail out
         pass
     else:
-        if connections is None or connections < minimum_connections:
+        if connections is not None and connections < minimum_connections:
             try:
                 bindinstance.named_conf_set_directive('connections',
                                                         minimum_connections)
-- 
1.8.1.4

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

Reply via email to