[Freeipa-devel] [PATCH] bind-dyndb-ldap: enable/disable PTR synchronization per zone

[Adam Tkac]

On Thu, Dec 01, 2011 at 09:00:18AM -0500, Jiri Kuncar wrote:
> I've added an attribute "idnsAllowSyncPTR" to "idnsZone" to enable or disable synchronization of PTR records. However the bind-dyndb-ldap plugin option "sync_ptr" has to be included in /etc/named.conf to run synchronization feature.

Hello,

This doesn't seem too user-friendly for me. In my opinion better is to allow PTR
synchronization when sync_ptr is "no" and idnsAllowSyncPTR is set to "TRUE". So
admin can either set sync_ptr to allow updates for all zones or set per-zone
idnsAllowSyncPTR attr.

Please check my comments inside the patch.

> My quick fix of LDAP schema in /usr/share/ipa/60basev2.ldif:
> -----
> attributeTypes: (2.16.840.1.113730.3.8.5.11 NAME 'idnsAllowSyncPTR' DESC 'permit synchronization of PTR records' EQUALITY booleanMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.7 SINGLE-VALUE X-ORIGIN 'IPA v2' )
> ...
> objectClasses: (2.16.840.1.113730.3.8.6.1 NAME 'idnsZone' DESC 'Zone class' SUP idnsRecord STRUCTURAL MUST ( idnsName $ idnsZoneActive $ idnsSOAmName $ idnsSOArName $ idnsSOAserial $ idnsSOArefresh $ idnsSOAretry $ idnsSOAexpire $ idnsSOAminimum ) MAY ( idnsUpdatePolicy $ idnsAllowSyncPTR ) )
> -----
> 
> https://fedorahosted.org/bind-dyndb-ldap/ticket/39
> 
> Jiri

> From b1aaef1c05be1a16115eb30bb76a0d1795806bc7 Mon Sep 17 00:00:00 2001
> From: Jiri Kuncar <jkuncar at redhat.com>
> Date: Thu, 1 Dec 2011 08:32:34 -0500
> Subject: [PATCH] Enable/disable PTR synchronization per zone. Class
>  idnsRecord has new attribute idnsAllowSyncPTR (BOOLEAN).
> 
> Signed-off-by: Jiri Kuncar <jkuncar at redhat.com>
> ---
>  src/ldap_helper.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index b60cf11..4530155 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -1801,6 +1801,38 @@ modify_ldap_common(dns_name_t *owner, ldap_instance_t *ldap_inst,
>  	if (ldap_inst->sync_ptr == ISC_TRUE && 
>  	    (rdlist->type == dns_rdatatype_a || rdlist->type == dns_rdatatype_aaaa)) {
>  		
> +		/* 
> +		 * Find parent zone entry.
> +		 * @todo Try the cache first and improve split.
> +		 */
> +		char *zone_dn = strstr(str_buf(owner_dn),", ") + 1;

Since this strstr() rule depends on current src/ldap_convert.c:dnsname_to_dn()
behavior, please add note to dnsname_to_dn() that modification of the

                CHECK(str_cat_char(target, ", "));

line could affect this part of code.

> +		ldap_entry_t *entry;
> +		ldap_valuelist_t values;
> +		char *attrs[] = {"idnsAllowSyncPTR", NULL};
> +		
> +		CHECK(ldap_query(ldap_inst, ldap_conn, zone_dn,
> +						 LDAP_SCOPE_BASE, attrs, 0,
> +						 "(&(objectClass=idnsZone)(idnsZoneActive=TRUE))"));
> +		
> +		/* Search for zone entry with 'idnsAllowSyncPTR == "TRUE"'. */
> +		for (entry = HEAD(ldap_conn->ldap_entries);
> +			 entry != NULL;
> +			 entry = NEXT(entry, link)) {
> +			result = ldap_entry_getvalues(entry, "idnsAllowSyncPTR", &values);
> +			if (result != ISC_R_SUCCESS) 
> +				continue;
> +
> +			if (strcmp(HEAD(values)->value, "TRUE") != 0) {
> +				entry = NULL;
> +			}
> +			break;
> +		}
> +		/* Any valid zone was found. */
> +		if (entry == NULL) {
> +			log_error("Sync PTR is not allowed in zone %s", zone_dn);

idnsAllowSyncPTR set to "FALSE" (or missing idnsAllowSyncPTR attr) shouldn't be
treated as error. Please degrade this msg to

log_debug(3, "...")

for example.

> +			goto cleanup;
> +		}

It would be nice to receive debug msg when idnsAllowSyncPTR is allowed. What
about

log_debug(3, "Sync PTR is allowed for zone %s", zone_dn);

Regards, Adam

-- 
Adam Tkac, Red Hat, Inc.