On Thu, Mar 01, 2012 at 07:55:33PM +0100, Petr Spacek wrote:
> Hello,
> 
> here is (again) reworked patch for
> https://fedorahosted.org/bind-dyndb-ldap/ticket/49 .
> 
> Adam pointed me to existing BIND parser, which I missed. Now is all
> parsing & socket magic done inside BIND libraries. Our code is a bit
> shorter and syntax is 100% BIND-compatible. (But it means same as
> discussed yesterday :-)
> 
> Adam, please review it.

Thanks for the patch Petr, see my comments below.

> Petr^2 Spacek
> 
> On 03/01/2012 03:22 PM, Petr Spacek wrote:
> >Hello,
> >
> >here is reworked patch for
> >https://fedorahosted.org/bind-dyndb-ldap/ticket/49 .
> >
> >Changes after yesterday's discussion on IRC with Simo and Mkosek:
> >It follows BIND9 syntax for optional specification of port & adds
> >documentation for this new syntax.
> >
> >Petr^2 Spacek
> >
> >On 02/29/2012 05:33 PM, Martin Kosek wrote:
> >>I agree that we should keep the BIND syntax and separate port and IP
> >>address with a space. We will at least avoid possible issues with IP
> >>address decoding in the future.
> >>
> >>Since this is a new attribute we have a good chance to do changes now so
> >>that it is used correctly. I created an upstream ticket to change the
> >>behavior and validators in FreeIPA:
> >>
> >>https://fedorahosted.org/freeipa/ticket/2462
> >>
> >>Martin
> >>
> >>On Wed, 2012-02-29 at 16:44 +0100, Petr Spacek wrote:
> >>>On 02/29/2012 04:30 PM, Simo Sorce wrote:
> >>>>Either way looks ok to me.
> >>>>I agree that using a space may be less confusing if this syntax never
> >>>>allows to specify multiple addresses.
> >>>>If multiple address can be specified than it may be less ideal to use
> >>>>spaces.
> >>>>
> >>>>Simo.
> >>>
> >>>idnsForwarders is multi-value attribute, so each value contain single
> >>>forwarder address.
> >>>
> >>>Petr^2 Spacek
> >>>
> >>>>On Wed, 2012-02-29 at 15:14 +0100, Petr Spacek wrote:
> >>>>>And there is the patch, sorry.
> >>>>>
> >>>>>Petr^2
> >>>>>
> >>>>>On 02/29/2012 03:10 PM, Petr Spacek wrote:
> >>>>>>Hello,
> >>>>>>
> >>>>>>this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/49 ,
> >>>>>>but I want to discuss one (unimplemented) change:
> >>>>>>
> >>>>>>I propose a change in (currently very strange) forwarders syntax.
> >>>>>>
> >>>>>>Current syntax:
> >>>>>><IP>[.port]
> >>>>>>
> >>>>>>examples:
> >>>>>>1.2.3.4 (without optional port)
> >>>>>>1.2.3.4.5553 (optional port 5553)
> >>>>>>A::B (IPv6, without optional port)
> >>>>>>A::B.5553
> >>>>>>::FFFF:1.2.3.4 (6to4, without optional port)
> >>>>>>::FFFF:1.2.3.4.5553 (6to4, with optional port 5553)
> >>>>>>
> >>>>>>I find this syntax confusing, non-standard and not-typo-proof.
> >>>>>>
> >>>>>>
> >>>>>>IMHO better choice is to follow BIND forwarders syntax:
> >>>>>><IP> [port ip_port] (port is string delimited with spaces)
> >>>>>>
> >>>>>>(From: http://www.zytrax.com/books/dns/ch7/queries.html#forwarders)
> >>>>>>
> >>>>>>
> >>>>>>*Current syntax is not documented*, so probably is not used anywhere.
> >>>>>>(And DNS server on non-standard port is probably useful only for
> >>>>>>testing
> >>>>>>purposes, but it's another story.)
> >>>>>>
> >>>>>>
> >>>>>>What is you opinion?

> From 14056200915a90c2a957e8a2219819bd3b7f9365 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspa...@redhat.com>
> Date: Thu, 1 Mar 2012 15:08:10 +0100
> Subject: [PATCH] Add support for IPv6 elements in idnsForwarders attribute
>  and make syntax compatible with BIND9 forwarders.
>  Signed-off-by: Petr Spacek <pspa...@redhat.com>
> 
> ---
>  NEWS              |    3 +
>  README            |    8 ++-
>  src/acl.c         |   89 ++++++++++++++++++++++++++++++++++
>  src/acl.h         |    3 +
>  src/ldap_helper.c |  136 
> ++++++-----------------------------------------------
>  5 files changed, 116 insertions(+), 123 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index ced6250..a97a5f3 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,3 +1,6 @@
> +[1] Add support for IPv6 elements in idnsForwarders attribute
> +    and make syntax compatible with BIND9 forwarders.
> +
>  1.1.0a2
>  ======
>  
> diff --git a/README b/README
> index 914abdc..aedb88c 100644
> --- a/README
> +++ b/README
> @@ -89,8 +89,12 @@ example zone ldif is available in the doc directory.
>       with a valid idnsForwarders attribute.
>  
>  * idnsForwarders
> -     Defines multiple IP addresses (TODO: optional port numbers) to which
> -     queries will be forwarded.
> +     Defines multiple IP addresses to which queries will be forwarded.
> +     It is multi-value attribute: Each IP address (and optional port) has to
> +     be in own value. BIND9 syntax for "forwarders" is required.
> +     Optional port can be specified by adding " port <number>" after IP 
> +     address. IPv4 and IPv6 addresses are supported.
> +     Examples: "1.2.3.4" or "1.2.3.4 port 553" or "A::B" or "A::B port 553"

In my opinion we should mark the idnsForwarders attribute single-value and deal
with them as with idnsAllow{Query,Transfer} (i.e. specify forwarders splitted by
semi-colon). It will be more consistent with idnsAllow{Query,Transfer}. What do
you think about this?

>  5. Configuration
>  ================
> diff --git a/src/acl.c b/src/acl.c
> index 32ca702..b888aea 100644
> --- a/src/acl.c
> +++ b/src/acl.c
> @@ -69,6 +69,7 @@ static isc_once_t once = ISC_ONCE_INIT;
>  static cfg_type_t *update_policy;
>  static cfg_type_t *allow_query;
>  static cfg_type_t *allow_transfer;
> +static cfg_type_t *forwarders;
>  
>  static cfg_type_t *
>  get_type_from_tuplefield(const cfg_type_t *cfg_type, const char *name)
> @@ -139,6 +140,7 @@ init_cfgtypes(void)
>       update_policy = get_type_from_clause_array(zoneopts, "update-policy");
>       allow_query = get_type_from_clause_array(zoneopts, "allow-query");
>       allow_transfer = get_type_from_clause_array(zoneopts, "allow-transfer");
> +     forwarders = get_type_from_clause_array(zoneopts, "forwarders");
>  }
>  
>  static isc_result_t
> @@ -351,6 +353,24 @@ cleanup:
>       return result;
>  }
>  
> +static isc_result_t
> +semicolon_bracket_str(isc_mem_t *mctx, const char *str, ld_string_t 
> **bracket_strp)
> +{
> +     ld_string_t *tmp = NULL;
> +     isc_result_t result;
> +
> +     CHECK(str_new(mctx, &tmp));
> +     CHECK(str_sprintf(tmp, "{ %s; }", str));
> +
> +     *bracket_strp = tmp;
> +
> +     return ISC_R_SUCCESS;
> +
> +cleanup:
> +     str_destroy(&tmp);
> +     return result;
> +}
> +

This function won't be needed if we mark idnsForwarders single-value.

>  isc_result_t
>  acl_configure_zone_ssutable(const char *policy_str, dns_zone_t *zone)
>  {
> @@ -480,3 +500,72 @@ cleanup:
>       return result;
>  }
>  
> +
> +/**
> + * Parse nameserver IP address with or without port specified in BIND9 
> syntax.
> + * IPv4 and IPv6 addresses are supported, see examples.
> + *
> + * @param[in] forwarder_str String with IP address and optionally port.
> + * @param[in] mctx Memory for allocating temporary and sa structure.
> + * @param[out] sa Socket address structure.
> + *
> + * @return Pointer to newly allocated isc_sockaddr_t structure.
> + *
> + * @example
> + * "192.168.0.100" -> { address:192.168.0.100, port:53 }
> + * "192.168.0.100 port 553" -> { address:192.168.0.100, port:553 }
> + * "0102:0304:0506:0708:090A:0B0C:0D0E:0FAA" ->
> + *           { address:0102:0304:0506:0708:090A:0B0C:0D0E:0FAA, port:53 }
> + * "0102:0304:0506:0708:090A:0B0C:0D0E:0FAA port 553" ->
> + *           { address:0102:0304:0506:0708:090A:0B0C:0D0E:0FAA, port:553 }
> + *
> + */
> +isc_result_t
> +acl_parse_forwarder(const char *forwarder_str, isc_mem_t *mctx, 
> isc_sockaddr_t **sa)

I would recommend to change the last param to isc_sockaddrlist_t* when 
idnsForwarders
is single-value & modify the function appropriately.

> +{
> +     isc_result_t result = ISC_R_SUCCESS;
> +     cfg_parser_t *parser = NULL;
> +
> +     cfg_obj_t *forwarders_cfg = NULL;
> +     ld_string_t *new_forwarder_str = NULL;
> +     const cfg_obj_t *faddresses;
> +     const cfg_listelt_t *element;
> +
> +     in_port_t port = 53;
> +
> +     REQUIRE(forwarder_str != NULL);
> +     REQUIRE(mctx != NULL);

REQUIRE(mctx != NULL); is not needed, every isc_mem_* function checks mctx
validity.

> +     REQUIRE(sa != NULL);
> +     REQUIRE(*sa == NULL);

REQUIRE(sa != NULL && *sa == NULL); is better.

> +
> +     /* add semicolon and brackets as necessary for parser */
> +     if (!index(forwarder_str, ';'))
> +             CHECK(semicolon_bracket_str(mctx, forwarder_str, 
> &new_forwarder_str));
> +     else
> +             CHECK(bracket_str(mctx, forwarder_str, &new_forwarder_str));
> +
> +     CHECK(cfg_parser_create(mctx, dns_lctx, &parser));
> +     CHECK(parse(parser, str_buf(new_forwarder_str), &forwarders, 
> &forwarders_cfg));
> +
> +     faddresses = cfg_tuple_get(forwarders_cfg, "addresses");
> +     element = cfg_list_first(faddresses);
> +     if (!element) {
> +             result = ISC_R_FAILURE;
> +             goto cleanup;
> +     }
> +
> +     const cfg_obj_t *forwarder = cfg_listelt_value(element);
> +     *sa = isc_mem_get(mctx, sizeof(isc_sockaddr_t));
> +     if (*sa == NULL) {
> +             result = ISC_R_NOMEMORY;
> +             goto cleanup;
> +     }
> +     **sa = *cfg_obj_assockaddr(forwarder);
> +     if (isc_sockaddr_getport(*sa) == 0)
> +             isc_sockaddr_setport(*sa, port);
> +
> +cleanup:
> +     if (new_forwarder_str)
> +             str_destroy(&new_forwarder_str);

Destroy the new_forwarder_str unconditionally, str_destroy handles NULL strings
correctly.

> +     return result;
> +}
> diff --git a/src/acl.h b/src/acl.h
> index d0ab216..7e4471b 100644
> --- a/src/acl.h
> +++ b/src/acl.h
> @@ -42,4 +42,7 @@ acl_from_ldap(isc_mem_t *mctx, const char *aclstr, 
> acl_type_t type,
>   * Please refer to BIND 9 ARM (Administrator Reference Manual) about ACLs.
>   */
>  
> +isc_result_t
> +acl_parse_forwarder(const char *forwarders_str, isc_mem_t *mctx, 
> isc_sockaddr_t **sa);
> +
>  #endif /* !_LD_ACL_H_ */
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index d9e8629..21c3690 100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -776,108 +776,6 @@ cleanup:
>  }
>  
>  
> -/**
> - * @brief 
> - *
> - * @param nameserver
> - * @param sa
> - *
> - * @return 
> - */
> -static isc_result_t
> -sockaddr_fromchar(char *nameserver, struct sockaddr *sa)
> -{
> -     isc_result_t result = ISC_R_FAILURE;
> -     struct addrinfo *ai;
> -     struct addrinfo hints;
> -     int res;
> -
> -     REQUIRE(sa != NULL);
> -
> -     memset(&hints, 0, sizeof(hints));
> -     hints.ai_flags = AI_NUMERICHOST;
> -
> -     res = getaddrinfo(nameserver, NULL, &hints, &ai);
> -     if (res == 0) {
> -             if ((ai->ai_family == AF_INET) || (ai->ai_family == AF_INET6)) {
> -                     memcpy(sa, ai->ai_addr, ai->ai_addrlen);
> -                     result = ISC_R_SUCCESS;
> -             }
> -             freeaddrinfo(ai);
> -     }
> -     return result;
> -}
> -
> -/**
> - * Parse nameserver IP address with or without
> - * port separated with a dot.
> - *
> - * @example
> - * "192.168.0.100.53" -> { address:192.168.0.100,  port:53 }
> - *
> - * @param token 
> - * @param sa Socket address structure.
> - */
> -static isc_result_t
> -parse_nameserver(char *token, struct sockaddr *sa)
> -{
> -     isc_result_t result = ISC_R_FAILURE;
> -     char *dot;
> -     long number;
> -
> -     REQUIRE(token != NULL);
> -     REQUIRE(sa != NULL);
> -
> -     result = sockaddr_fromchar(token, sa);
> -     if (result == ISC_R_SUCCESS)
> -             return result;
> -
> -     dot = strrchr(token, '.');
> -     if (dot == NULL)
> -             return ISC_R_FAILURE;
> -     
> -     number = strtol(dot + 1, NULL, 10);
> -     if ((number < 0) || (number > UINT16_MAX))
> -             return ISC_R_FAILURE;
> -     
> -     *dot = '\0';
> -     result = sockaddr_fromchar(token, sa);
> -     *dot = '.'; /* restore value */
> -     if (result == ISC_R_SUCCESS) {
> -             in_port_t port = htons(number);
> -             switch (sa->sa_family) {
> -             case AF_INET :
> -                     ((struct sockaddr_in *)sa)->sin_port = port;
> -                     break;
> -             case AF_INET6 :
> -                     ((struct sockaddr_in6 *)sa)->sin6_port = port;
> -                     break;
> -             default:
> -                     log_bug("Unknown sa_family type");
> -                     return ISC_R_FAILURE;
> -             }
> -     }
> -     return result;
> -}
> -
> -/* TODO: Code hardering. */
> -static void *
> -get_in_addr(struct sockaddr *sa)
> -{
> -     if (sa->sa_family == AF_INET)
> -             return &(((struct sockaddr_in*)sa)->sin_addr);
> -     else
> -             return &(((struct sockaddr_in6*)sa)->sin6_addr);
> -}
> -
> -static in_port_t
> -get_in_port(struct sockaddr *sa)
> -{
> -     if (sa->sa_family == AF_INET)
> -             return (((struct sockaddr_in*)sa)->sin_port);
> -     else
> -             return (((struct sockaddr_in6*)sa)->sin6_port);
> -}
>  
>  static isc_result_t
>  configure_zone_forwarders(ldap_entry_t *entry, ldap_instance_t *inst, 
> @@ -887,7 +785,6 @@ configure_zone_forwarders(ldap_entry_t *entry, 
> ldap_instance_t *inst,
>       isc_result_t result;
>       ldap_value_t *value;
>       isc_sockaddrlist_t addrs;
> -     isc_sockaddr_t *addr;
>  
>       REQUIRE(entry != NULL && inst != NULL && name != NULL && values != 
> NULL);
>  
> @@ -905,28 +802,19 @@ configure_zone_forwarders(ldap_entry_t *entry, 
> ldap_instance_t *inst,
>  
>       ISC_LIST_INIT(addrs);
>       for (value = HEAD(*values); value != NULL; value = NEXT(value, link)) {
> -             struct sockaddr sa;
> -
> -             addr = isc_mem_get(inst->mctx, sizeof(*addr));
> -             if (addr == NULL) {
> -                     result = ISC_R_NOMEMORY;
> -                     goto cleanup;
> -             }
> -             ISC_LINK_INIT(addr, link);
> +             isc_sockaddr_t *addr = NULL;
> +             char forwarder_txt[ISC_SOCKADDR_FORMATSIZE];
>  
> -             result = parse_nameserver(value->value, &sa);
> -             if (result != ISC_R_SUCCESS) {
> -                     log_bug("Could not convert IP address "
> -                             "from string '%s'.", value->value);
> +             if( acl_parse_forwarder(value->value, inst->mctx, &addr)
> +                             != ISC_R_SUCCESS) {
> +                     log_error("Could not parse forwarder '%s'", 
> value->value);
> +                     continue;
>               }
>  
> -             /* Convert port from network byte order. */
> -             in_port_t port = ntohs(get_in_port(&sa));
> -             port = (port != 0) ? port : 53; /* use well known port */       
>                 
> -
> -             isc_sockaddr_fromin(addr, get_in_addr(&sa), port);
> +             ISC_LINK_INIT(addr, link);
>               ISC_LIST_APPEND(addrs, addr, link);
> -             log_debug(5, "Adding forwarder %s (:%d) for %s", value->value, 
> port, dn);
> +             isc_sockaddr_format(addr, forwarder_txt, 
> ISC_SOCKADDR_FORMATSIZE);
> +             log_debug(5, "Adding forwarder %s for %s", forwarder_txt, dn);
>       }
>  
>       /*
> @@ -944,6 +832,11 @@ configure_zone_forwarders(ldap_entry_t *entry, 
> ldap_instance_t *inst,
>                       fwdpolicy = dns_fwdpolicy_only;
>       }
>  
> +     if (ISC_LIST_EMPTY(addrs)) {
> +             log_debug(5, "No forwarders seen; disabling forwarding");
> +             fwdpolicy = dns_fwdpolicy_none;
> +     }
> +
>       log_debug(5, "Forward policy: %d", fwdpolicy);
>               
>       /* Set forward table up. */
> @@ -951,6 +844,7 @@ configure_zone_forwarders(ldap_entry_t *entry, 
> ldap_instance_t *inst,
>  
>  cleanup:
>       while (!ISC_LIST_EMPTY(addrs)) {
> +             isc_sockaddr_t *addr = NULL;
>               addr = ISC_LIST_HEAD(addrs);
>               ISC_LIST_UNLINK(addrs, addr, link);
>               isc_mem_put(inst->mctx, addr, sizeof(*addr));
> -- 
> 1.7.7.6
> 


-- 
Adam Tkac, Red Hat, Inc.

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

Reply via email to