[Freeipa-devel] [PATCH] 0009 Support for IPv6 elements in idnsForwarders attribute
Adam Tkac
atkac at redhat.com
Mon Mar 5 11:32:53 UTC 2012
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 <pspacek at 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 <pspacek at 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.
More information about the Freeipa-devel
mailing list