[PATCH] Add dynamic DNS updates to FreeIPA

[Stephen Gallagher]

Addresses https://fedorahosted.org/sssd/ticket/368

This adds two new options:

ipa_dyndns_update: Boolean value to select whether this client
should automatically update its IP address in FreeIPA DNS.

ipa_dyndns_iface: Choose an interface manually to use for
updating dynamic DNS. Default is to use the interface associated
with the LDAP connection to FreeIPA.

This patch supports A and AAAA records. It relies on the presence
of the nsupdate tool from the bind-utils package to perform the
actual update step. The location of this utility is set at build
time, but its availability is determined at runtime (so clients
that do not require dynamic update capability do not need to meet
this dependency).

PTR records are not updated, since there is no guarantee that the IP 
address being updated has an address in a zone handled by FreeIPA. If 
this is a serious lack, it can be added in a later patch.

[Stephen Gallagher]

On 05/03/2010 11:32 AM, Stephen Gallagher wrote:
> Addresses https://fedorahosted.org/sssd/ticket/368
>
> This adds two new options:
>
> ipa_dyndns_update: Boolean value to select whether this client
> should automatically update its IP address in FreeIPA DNS.
>
> ipa_dyndns_iface: Choose an interface manually to use for
> updating dynamic DNS. Default is to use the interface associated
> with the LDAP connection to FreeIPA.
>
> This patch supports A and AAAA records. It relies on the presence
> of the nsupdate tool from the bind-utils package to perform the
> actual update step. The location of this utility is set at build
> time, but its availability is determined at runtime (so clients
> that do not require dynamic update capability do not need to meet
> this dependency).
>
> PTR records are not updated, since there is no guarantee that the IP
> address being updated has an address in a zone handled by FreeIPA. If
> this is a serious lack, it can be added in a later patch.

Jakub noticed during review that I forgot to handle IPv6 addresses when 
retrieving the address from the LDAP connection.

New patch attached.

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/03/2010 10:02 PM, Stephen Gallagher wrote:
> Jakub noticed during review that I forgot to handle IPv6 addresses when
> retrieving the address from the LDAP connection.
>
> New patch attached.

Some minor nitpicks:

* Do we want to keep these new options undocumented? Ditto for ConfigAPI
* The comment of NSUPDATE_PATH says "The patch to nscd"
* there is a new blank line added at the end of nsupdate.m4

I haven't tested the patch yet but otherwise the patch looks good to me.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkvfOvgACgkQHsardTLnvCUgEwCfUtupw2z2GmE9kJxVxp2X61V6
> PEUAn1qEYItYE6WuiCQMVVmCfLyXh3Dp
> =WD5w
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

On 05/03/2010 05:07 PM, Jakub Hrozek wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 05/03/2010 10:02 PM, Stephen Gallagher wrote:
> > Jakub noticed during review that I forgot to handle IPv6 addresses when
> > retrieving the address from the LDAP connection.
> >
> > New patch attached.
>
> Some minor nitpicks:
> Do we want to keep these new options undocumented? Ditto for ConfigAPI

No, I forgot to add the documentation. Upcoming patch will rectify this.
> The comment of NSUPDATE_PATH says "The patch to nscd"

Copy-paste fail.
> there is a new blank line added at the end of nsupdate.m4

Not sure what you mean here. It's a completely new file.
> I haven't tested the patch yet but otherwise the patch looks good to me.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkvfOvgACgkQHsardTLnvCUgEwCfUtupw2z2GmE9kJxVxp2X61V6
> PEUAn1qEYItYE6WuiCQMVVmCfLyXh3Dp
> =WD5w
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

Hi,

my comments are in line.

bye,
Sumit

On Mon, May 03, 2010 at 04:02:38PM -0400, Stephen Gallagher wrote:
> On 05/03/2010 11:32 AM, Stephen Gallagher wrote:
> > Addresses https://fedorahosted.org/sssd/ticket/368
> >
> > This adds two new options:
> >
> > ipa_dyndns_update: Boolean value to select whether this client
> > should automatically update its IP address in FreeIPA DNS.
> >
> > ipa_dyndns_iface: Choose an interface manually to use for
> > updating dynamic DNS. Default is to use the interface associated
> > with the LDAP connection to FreeIPA.
> >
> > This patch supports A and AAAA records. It relies on the presence
> > of the nsupdate tool from the bind-utils package to perform the
> > actual update step. The location of this utility is set at build
> > time, but its availability is determined at runtime (so clients
> > that do not require dynamic update capability do not need to meet
> > this dependency).
> >
> > PTR records are not updated, since there is no guarantee that the IP
> > address being updated has an address in a zone handled by FreeIPA. If
> > this is a serious lack, it can be added in a later patch.
>
> Jakub noticed during review that I forgot to handle IPv6 addresses
> when retrieving the address from the LDAP connection.
>
> New patch attached.
>
> -- 
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 22ac3581ec625690687afc69e65676c822189f59..420bb8472dc1a74d2742c6e41bf2a76db834c09d 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -348,6 +348,7 @@ dist_noinst_HEADERS = \
>      providers/ipa/ipa_access.h \
>      providers/ipa/ipa_timerules.h \
>      providers/ipa/ipa_auth.h \
> providers/ipa/ipa_dyndns.h \
> tools/tools_util.h \
> tools/sss_sync_ops.h \
> resolv/async_resolv.h \
>
> @@ -727,6 +728,7 @@ libsss_ldap_la_SOURCES = \
>      providers/ldap/sdap_child_helpers.c \
>      providers/ldap/sdap_fd_events.c \
>      providers/ldap/sdap.c \
> providers/ipa/ipa_dyndns.c \

Why does the LDAP provider need ipa_dyndns.c ?
>  util/user_info_msg.c \
>  util/sss_ldap.c \
>  util/sss_krb5.c
>
> @@ -788,6 +790,7 @@ libsss_ipa_la_SOURCES = \
>      providers/ipa/ipa_auth.c \
>      providers/ipa/ipa_access.c \
>      providers/ipa/ipa_timerules.c \
> providers/ipa/ipa_dyndns.c \
> providers/ldap/ldap_id.c \
> providers/ldap/ldap_id_enum.c \
> providers/ldap/ldap_id_cleanup.c \
> diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..c9f64bed50647e8a25dab109491de63abd9035b9
> --- /dev/null
> +++ b/src/providers/ipa/ipa_dyndns.c
>
> +struct ipa_ipv4 {
> struct ipa_ipv4 *next;
> struct ipa_ipv4 *prev;
> struct sockaddr_in *addr;
> bool matched;
>
> +};
>
> +struct ipa_ipv6 {
> struct ipa_ipv6 *next;
> struct ipa_ipv6 *prev;
> struct sockaddr_in6 *addr;
> bool matched;
>
> +};

I would recommend to only have one list with a family (AF_INET,
AF_INET6, ...) flag and a struct sockaddr_storage to hold the address.
> +struct ipa_dyndns_ctx {
> struct ipa_options *ipa_ctx;
> char *hostname;
> struct ipa_ipv4 *ipv4;
> struct ipa_ipv6 *ipv6;
> int child_status;
>
> +};
> if (iface) {
>    /* Get the IP addresses associated with the
>     * specified interface
>     */
>    errno = 0;
>    ret = getifaddrs(&ifaces);
>    if (ret == -1) {
>        ret = errno;
>        DEBUG(0, ("Could not read interfaces [%d][%s]\n",
>                  ret, strerror(ret)));
>        goto failed;
>    }
>    for(ifa = ifaces; ifa != NULL; ifa=ifa->ifa_next) {
>        /* Some interfaces don't have an ifa_addr */
>        if (!ifa->ifa_addr) continue;

Although it is safe here I think using a
switch(ifa->ifa_addr->sa_family) would make the code more clear, e.g

if (strcasecmp(ifa->ifa_name, iface) == 0)
    switch(ifa->ifa_addr->sa_family == AF_INET)
....
>        /* Add IPv4 addresses to the list */
>        if(ifa->ifa_addr->sa_family == AF_INET &&
>           strcasecmp(ifa->ifa_name, iface) == 0) {
>            /* Add this address to the IPv4 list */
>            address4 = talloc_zero(state, struct ipa_ipv4);
>            if (!address4) {
>                goto failed;
>            }
>            address4->addr = talloc_memdup(address4, ifa->ifa_addr,
>                                           sizeof(struct sockaddr_in));
>            if(address4->addr == NULL) {
>                goto failed;
>            }
>            DLIST_ADD(state->ipv4, address4);
>        }
>        /* Add IPv6 addresses to the list */
>        if(ifa->ifa_addr->sa_family == AF_INET6 &&
>           strcasecmp(ifa->ifa_name, iface) == 0) {
>            /* Add this address to the IPv6 list */
>            address6 = talloc_zero(state, struct ipa_ipv6);
>            if (!address6) {
>                goto failed;
>            }
>            address6->addr = talloc_memdup(address6, ifa->ifa_addr,
>                                           sizeof(struct sockaddr_in6));
>            if(!address6->addr) {
>                goto failed;
>            }
>            DLIST_ADD(state->ipv6, address6);
>        }
>    }
>    freeifaddrs(ifaces);
> }
> else {
>    /* Get the file descriptor for the primary LDAP connection */
>    ret = get_fd_from_ldap(ctx->id_ctx->gsh->ldap, &fd);
>    if (ret != EOK) {
>        goto failed;
>    }
>    ret = getsockname(fd, &sa, &sa_len);
>    if (ret == -1) {
>        DEBUG(0,("Failed to get socket name\n"));
>        goto failed;
>    }

switch(sa.sa_family) ?
>    if (sa.sa_family == AF_INET) {
>        address4 = talloc(state, struct ipa_ipv4);
>        if (!address4) {
>            goto failed;
>        }
>        address4->addr = talloc_memdup(address4, &sa,
>                                       sizeof(struct sockaddr_in));
>        if(address4->addr == NULL) {
>            goto failed;
>        }
>        DLIST_ADD(state->ipv4, address4);
>    }
>    if (sa.sa_family == AF_INET6) {
>        address6 = talloc(state, struct ipa_ipv6);
>        if (!address6) {
>            goto failed;
>        }
>        address6->addr = talloc_memdup(address6, &sa,
>                                       sizeof(struct sockaddr_in6));
>        if(address6->addr == NULL) {
>            goto failed;
>        }
>        DLIST_ADD(state->ipv6, address6);
>    }
> }
> +static int create_nsupdate_message(struct ipa_nsupdate_ctx *ctx)
> +{
> int ret, i;
> char *servername;
> char *zone;
> char ip_addr[128];
> const char *ip;
> struct ipa_ipv4 *a_record;
> struct ipa_ipv6 *aaaa_record;
> servername = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
>                               IPA_SERVER);
> if (!servername) {
>    return EIO;
> }
> zone = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
>                         IPA_DOMAIN);
> if (!zone) {
>    return EIO;
> }
> /* The DNS zone for IPA is the lower-case
> * version of hte IPA domain
> */
> for(i = 0; zone[i] != '\0'; i++) {
>    zone[i] = tolower(zone[i]);
> }
> /* Add the server and zone headers */
> ctx->update_msg = talloc_asprintf(ctx, "server %s\nzone %s.\n",
>                                       servername,
>                                       zone);
> if (ctx->update_msg == NULL) {
>    ret = ENOMEM;
>    goto done;
> }

I think you never check if both lists are empty. Does it make sense to
send only the delete messages in this case?
> /* Remove any existing entries */
> ctx->update_msg = talloc_asprintf_append(ctx->update_msg,
>                                         "update delete %s. in A\nsend\n"
>                                         "update delete %s. in AAAA\nsend\n",
>                                         ctx->dyndns_ctx->hostname,
>                                         ctx->dyndns_ctx->hostname);
> if (ctx->update_msg == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> /* Determine if we're updating IPv4, IPv6 or both */
> if (ctx->dyndns_ctx->ipv4) {
>    /* At least one A record */
>    DLIST_FOR_EACH(a_record, ctx->dyndns_ctx->ipv4) {
>        ip = inet_ntop(a_record->addr->sin_family,
>                       &a_record->addr->sin_addr,
>                       ip_addr, 128);
>        if (ip == NULL) {
>            ret = EIO;
>            goto done;
>        }
>        /* Format the A record update */
>        ctx->update_msg = talloc_asprintf_append(
>                ctx->update_msg,
>                "update add %s. 86400 in A %s\n",
>                ctx->dyndns_ctx->hostname, ip_addr);
>        if (ctx->update_msg == NULL) {
>            ret = ENOMEM;
>            goto done;
>        }
>    }
>    ctx->update_msg = talloc_asprintf_append(ctx->update_msg, "send\n");
>    if (ctx->update_msg == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
> }
> if (ctx->dyndns_ctx->ipv6) {
>    /* At least one AAAA record */
>    DLIST_FOR_EACH(aaaa_record, ctx->dyndns_ctx->ipv6) {
>        ip = inet_ntop(aaaa_record->addr->sin6_family,
>                       &aaaa_record->addr->sin6_addr,
>                       ip_addr, 128);
>        if (ip == NULL) {
>            ret = EIO;
>            goto done;
>        }
>        /* Format the AAAA message */
>        ctx->update_msg = talloc_asprintf_append(
>                ctx->update_msg,
>                "update add %s. 86400 in AAAA %s\n",
>                ctx->dyndns_ctx->hostname, ip_addr);
>        if(ctx->update_msg == NULL) {
>            ret = ENOMEM;
>            goto done;
>        }
>    }
>    ctx->update_msg = talloc_asprintf_append(ctx->update_msg, "send\n");
>    if(ctx->update_msg == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
> }
> ret = EOK;
>
> +done:
> return ret;
>
> +}
> ret = pipe(pipefd_to_child);
> if (ret == -1) {
>    err = errno;
>    DEBUG(1, ("pipe failed [%d][%s].\n", errno, strerror(errno)));

use err instead of errno as you have done below
>    return NULL;
> }
> pid = fork();
> if (pid == 0) { /* child */
>    args[0] = talloc_strdup(ctx, NSUPDATE_PATH);
>    args[1] = talloc_strdup(ctx, "-g");
>    args[2] = NULL;
>    if (args[0] == NULL || args[1] == NULL) {
>        return NULL;
>    }
>    close(pipefd_to_child[1]);
>    ret = dup2(pipefd_to_child[0], STDIN_FILENO);
>    if (ret == -1) {
>        err = errno;
>        DEBUG(1, ("dup2 failed [%d][%s].\n", err, strerror(err)));
>        return NULL;
>    }
>    errno = 0;
>    ret = execv(NSUPDATE_PATH, args);
>    if(ret == -1) {
>        err = errno;
>        DEBUG(1, ("execv failed [%d][%s].\n", err, strerror(err)));
>    }
>    return NULL;
> }
> else if (pid > 0) { /* parent */
>    close(pipefd_to_child[0]);
>    ctx->pipefd_to_child = pipefd_to_child[1];
>    /* Write the update message to the nsupdate child */
>    subreq = write_pipe_send(req,
>                             ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>                             (uint8_t *)ctx->update_msg,
>                             strlen(ctx->update_msg)+1,
>                             ctx->pipefd_to_child);
>    if (subreq == NULL) {
>        return NULL;
>    }
>    tevent_req_set_callback(subreq, ipa_dyndns_stdin_done, req);
>    /* Set up SIGCHLD handler */
>    ret = child_handler_setup(req,
>                              ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>                              pid, ipa_dyndns_child_handler, req, &ctx->sige);
>    if (ret != EOK) {
>        return NULL;
>    }
>    talloc_steal(ctx, ctx->sige);
>    /* Set up timeout handler */
>    tv = tevent_timeval_current_ofs(15,0);

it would be nice to have a #define IPA_DNYDNS_TIMEOUT 15
>    ctx->timeout_handler = tevent_add_timer(
>            ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>            req, tv, ipa_dyndns_timeout, req);
>    if(ctx->timeout_handler == NULL) {
>        return NULL;
>    }
> }
> else { /* error */
>    err = errno;
>    DEBUG(1, ("fork failed [%d][%s].\n", errno, strerror(errno)));

use err, see above
>    return NULL;
> }
> return req;
>
> +}

[Stephen Gallagher]

On 05/04/2010 06:44 AM, Sumit Bose wrote:
> Hi,
>
> my comments are in line.
>
> bye,
> Sumit
> > @@ -727,6 +728,7 @@ libsss_ldap_la_SOURCES = \
> >       providers/ldap/sdap_child_helpers.c \
> >       providers/ldap/sdap_fd_events.c \
> >       providers/ldap/sdap.c \
> > providers/ipa/ipa_dyndns.c \
>
> Why does the LDAP provider need ipa_dyndns.c ?

That's a mistake. That should have been removed.
> >   util/user_info_msg.c \
> >   util/sss_ldap.c \
> >   util/sss_krb5.c
> >
> > @@ -788,6 +790,7 @@ libsss_ipa_la_SOURCES = \
> >       providers/ipa/ipa_auth.c \
> >       providers/ipa/ipa_access.c \
> >       providers/ipa/ipa_timerules.c \
> > providers/ipa/ipa_dyndns.c \
> >  providers/ldap/ldap_id.c \
> >  providers/ldap/ldap_id_enum.c \
> >  providers/ldap/ldap_id_cleanup.c \
> > diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..c9f64bed50647e8a25dab109491de63abd9035b9
> > --- /dev/null
> > +++ b/src/providers/ipa/ipa_dyndns.c
> >
> > +struct ipa_ipv4 {
> > struct ipa_ipv4 *next;
> > struct ipa_ipv4 *prev;
> > struct sockaddr_in *addr;
> > bool matched;
> >
> > +};
> >
> > +struct ipa_ipv6 {
> > struct ipa_ipv6 *next;
> > struct ipa_ipv6 *prev;
> > struct sockaddr_in6 *addr;
> > bool matched;
> >
> > +};
>
> I would recommend to only have one list with a family (AF_INET,
> AF_INET6, ...) flag and a struct sockaddr_storage to hold the address.

Yeah, the separate lists were because originally I was only going to 
base updates on the lookup_family option, but I changed that. I'll 
combine them.
> > +struct ipa_dyndns_ctx {
> > struct ipa_options *ipa_ctx;
> > char *hostname;
> > struct ipa_ipv4 *ipv4;
> > struct ipa_ipv6 *ipv6;
> > int child_status;
> >
> > +};
> > if (iface) {
> >    /* Get the IP addresses associated with the
> >     * specified interface
> >     */
> >    errno = 0;
> >    ret = getifaddrs(&ifaces);
> >    if (ret == -1) {
> >        ret = errno;
> >        DEBUG(0, ("Could not read interfaces [%d][%s]\n",
> >                  ret, strerror(ret)));
> >        goto failed;
> >    }
> >    for(ifa = ifaces; ifa != NULL; ifa=ifa->ifa_next) {
> >        /* Some interfaces don't have an ifa_addr */
> >        if (!ifa->ifa_addr) continue;
>
> Although it is safe here I think using a
> switch(ifa->ifa_addr->sa_family) would make the code more clear, e.g
>
> if (strcasecmp(ifa->ifa_name, iface) == 0)
>      switch(ifa->ifa_addr->sa_family == AF_INET)
> ....

I'll clean that up.
> >        /* Add IPv4 addresses to the list */
> >        if(ifa->ifa_addr->sa_family == AF_INET&&
> >           strcasecmp(ifa->ifa_name, iface) == 0) {
> >            /* Add this address to the IPv4 list */
> >            address4 = talloc_zero(state, struct ipa_ipv4);
> >            if (!address4) {
> >                goto failed;
> >            }
> >            address4->addr = talloc_memdup(address4, ifa->ifa_addr,
> >                                           sizeof(struct sockaddr_in));
> >            if(address4->addr == NULL) {
> >                goto failed;
> >            }
> >            DLIST_ADD(state->ipv4, address4);
> >        }
> >        /* Add IPv6 addresses to the list */
> >        if(ifa->ifa_addr->sa_family == AF_INET6&&
> >           strcasecmp(ifa->ifa_name, iface) == 0) {
> >            /* Add this address to the IPv6 list */
> >            address6 = talloc_zero(state, struct ipa_ipv6);
> >            if (!address6) {
> >                goto failed;
> >            }
> >            address6->addr = talloc_memdup(address6, ifa->ifa_addr,
> >                                           sizeof(struct sockaddr_in6));
> >            if(!address6->addr) {
> >                goto failed;
> >            }
> >            DLIST_ADD(state->ipv6, address6);
> >        }
> >    }
> >    freeifaddrs(ifaces);
> > }
> > else {
> >    /* Get the file descriptor for the primary LDAP connection */
> >    ret = get_fd_from_ldap(ctx->id_ctx->gsh->ldap,&fd);
> >    if (ret != EOK) {
> >        goto failed;
> >    }
> >    ret = getsockname(fd,&sa,&sa_len);
> >    if (ret == -1) {
> >        DEBUG(0,("Failed to get socket name\n"));
> >        goto failed;
> >    }
>
> switch(sa.sa_family) ?

Agreed.
> >    if (sa.sa_family == AF_INET) {
> >        address4 = talloc(state, struct ipa_ipv4);
> >        if (!address4) {
> >            goto failed;
> >        }
> >        address4->addr = talloc_memdup(address4,&sa,
> >                                       sizeof(struct sockaddr_in));
> >        if(address4->addr == NULL) {
> >            goto failed;
> >        }
> >        DLIST_ADD(state->ipv4, address4);
> >    }
> >    if (sa.sa_family == AF_INET6) {
> >        address6 = talloc(state, struct ipa_ipv6);
> >        if (!address6) {
> >            goto failed;
> >        }
> >        address6->addr = talloc_memdup(address6,&sa,
> >                                       sizeof(struct sockaddr_in6));
> >        if(address6->addr == NULL) {
> >            goto failed;
> >        }
> >        DLIST_ADD(state->ipv6, address6);
> >    }
> > }
> > +static int create_nsupdate_message(struct ipa_nsupdate_ctx *ctx)
> > +{
> > int ret, i;
> > char *servername;
> > char *zone;
> > char ip_addr[128];
> > const char *ip;
> > struct ipa_ipv4 *a_record;
> > struct ipa_ipv6 *aaaa_record;
> > servername = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
> >                               IPA_SERVER);
> > if (!servername) {
> >    return EIO;
> > }
> > zone = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
> >                         IPA_DOMAIN);
> > if (!zone) {
> >    return EIO;
> > }
> > /* The DNS zone for IPA is the lower-case
> > * version of hte IPA domain
> > */
> > for(i = 0; zone[i] != '\0'; i++) {
> >    zone[i] = tolower(zone[i]);
> > }
> > /* Add the server and zone headers */
> > ctx->update_msg = talloc_asprintf(ctx, "server %s\nzone %s.\n",
> >                                       servername,
> >                                       zone);
> > if (ctx->update_msg == NULL) {
> >    ret = ENOMEM;
> >    goto done;
> > }
>
> I think you never check if both lists are empty. Does it make sense to
> send only the delete messages in this case?

Yes. If both lists are empty then we should remove the entries from DNS 
so that they aren't pointing at potentially the wrong address (which may 
or may not be a different host)
> > /* Remove any existing entries */
> > ctx->update_msg = talloc_asprintf_append(ctx->update_msg,
> >                                         "update delete %s. in A\nsend\n"
> >                                         "update delete %s. in AAAA\nsend\n",
> >                                         ctx->dyndns_ctx->hostname,
> >                                         ctx->dyndns_ctx->hostname);
> > if (ctx->update_msg == NULL) {
> >    ret = ENOMEM;
> >    goto done;
> > }
> > /* Determine if we're updating IPv4, IPv6 or both */
> > if (ctx->dyndns_ctx->ipv4) {
> >    /* At least one A record */
> >    DLIST_FOR_EACH(a_record, ctx->dyndns_ctx->ipv4) {
> >        ip = inet_ntop(a_record->addr->sin_family,
> >
> > +&a_record->addr->sin_addr,
> >                       ip_addr, 128);
> >        if (ip == NULL) {
> >            ret = EIO;
> >            goto done;
> >        }
> >        /* Format the A record update */
> >        ctx->update_msg = talloc_asprintf_append(
> >                ctx->update_msg,
> >                "update add %s. 86400 in A %s\n",
> >                ctx->dyndns_ctx->hostname, ip_addr);
> >        if (ctx->update_msg == NULL) {
> >            ret = ENOMEM;
> >            goto done;
> >        }
> >    }
> >    ctx->update_msg = talloc_asprintf_append(ctx->update_msg, "send\n");
> >    if (ctx->update_msg == NULL) {
> >        ret = ENOMEM;
> >        goto done;
> >    }
> > }
> > if (ctx->dyndns_ctx->ipv6) {
> >    /* At least one AAAA record */
> >    DLIST_FOR_EACH(aaaa_record, ctx->dyndns_ctx->ipv6) {
> >        ip = inet_ntop(aaaa_record->addr->sin6_family,
> >
> > +&aaaa_record->addr->sin6_addr,
> >                       ip_addr, 128);
> >        if (ip == NULL) {
> >            ret = EIO;
> >            goto done;
> >        }
> >        /* Format the AAAA message */
> >        ctx->update_msg = talloc_asprintf_append(
> >                ctx->update_msg,
> >                "update add %s. 86400 in AAAA %s\n",
> >                ctx->dyndns_ctx->hostname, ip_addr);
> >        if(ctx->update_msg == NULL) {
> >            ret = ENOMEM;
> >            goto done;
> >        }
> >    }
> >    ctx->update_msg = talloc_asprintf_append(ctx->update_msg, "send\n");
> >    if(ctx->update_msg == NULL) {
> >        ret = ENOMEM;
> >        goto done;
> >    }
> > }
> > ret = EOK;
> >
> > +done:
> > return ret;
> >
> > +}
> > ret = pipe(pipefd_to_child);
> > if (ret == -1) {
> >    err = errno;
> >    DEBUG(1, ("pipe failed [%d][%s].\n", errno, strerror(errno)));
>
> use err instead of errno as you have done below

Whoops.
> >    return NULL;
> > }
> > pid = fork();
> > if (pid == 0) { /* child */
> >    args[0] = talloc_strdup(ctx, NSUPDATE_PATH);
> >    args[1] = talloc_strdup(ctx, "-g");
> >    args[2] = NULL;
> >    if (args[0] == NULL || args[1] == NULL) {
> >        return NULL;
> >    }
> >    close(pipefd_to_child[1]);
> >    ret = dup2(pipefd_to_child[0], STDIN_FILENO);
> >    if (ret == -1) {
> >        err = errno;
> >        DEBUG(1, ("dup2 failed [%d][%s].\n", err, strerror(err)));
> >        return NULL;
> >    }
> >    errno = 0;
> >    ret = execv(NSUPDATE_PATH, args);
> >    if(ret == -1) {
> >        err = errno;
> >        DEBUG(1, ("execv failed [%d][%s].\n", err, strerror(err)));
> >    }
> >    return NULL;
> > }
> > else if (pid>  0) { /* parent */
> >    close(pipefd_to_child[0]);
> >    ctx->pipefd_to_child = pipefd_to_child[1];
> >    /* Write the update message to the nsupdate child */
> >    subreq = write_pipe_send(req,
> >                             ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
> >                             (uint8_t *)ctx->update_msg,
> >                             strlen(ctx->update_msg)+1,
> >                             ctx->pipefd_to_child);
> >    if (subreq == NULL) {
> >        return NULL;
> >    }
> >    tevent_req_set_callback(subreq, ipa_dyndns_stdin_done, req);
> >    /* Set up SIGCHLD handler */
> >    ret = child_handler_setup(req,
> >                              ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
> >                              pid, ipa_dyndns_child_handler, req,&ctx->sige);
> >    if (ret != EOK) {
> >        return NULL;
> >    }
> >    talloc_steal(ctx, ctx->sige);
> >    /* Set up timeout handler */
> >    tv = tevent_timeval_current_ofs(15,0);
>
> it would be nice to have a #define IPA_DNYDNS_TIMEOUT 15

Yeah, I meant to do that, then promptly forgot :)
> >    ctx->timeout_handler = tevent_add_timer(
> >            ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
> >            req, tv, ipa_dyndns_timeout, req);
> >    if(ctx->timeout_handler == NULL) {
> >        return NULL;
> >    }
> > }
> > else { /* error */
> >    err = errno;
> >    DEBUG(1, ("fork failed [%d][%s].\n", errno, strerror(errno)));
>
> use err, see above

Thanks.
> >    return NULL;
> > }
> > return req;
> >
> > +}

Thanks for the review. Updated patch coming soon.

[Stephen Gallagher]

On 05/04/2010 08:29 AM, Stephen Gallagher wrote:
> >
>
> Thanks for the review. Updated patch coming soon.

Updated patch should address both Sumit's and Jakub's concerns.

[Sumit Bose]

On Tue, May 04, 2010 at 01:01:37PM -0400, Stephen Gallagher wrote:
> On 05/04/2010 08:29 AM, Stephen Gallagher wrote:
> > >
> >
> > Thanks for the review. Updated patch coming soon.
>
> Updated patch should address both Sumit's and Jakub's concerns.

There is a typo in SSSDConfig.py:

+    'ipa_dyndns_update' : _("Whether to automatically update the
client's DNS entry in FreeIPA"),
+    'ipa_dyndns_update' : _("The interface whose IP should be used for
dynamic DNS updates"),

And I see error messages from nsupdate like 'Check your Kerberos ticket,
it may have expired.'. Maybe you should catch stdout and stderr from
nsupdate child.

How does nsupdate find the ccache file? I think you may need to set
KRB5CCNAME.

bye,
Sumit
> -- 
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> From 40e669a1eb8914c85c691d601783a2a7336964fd Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher 
> Date: Sun, 2 May 2010 07:48:26 -0400
> Subject: [PATCH] Add dynamic DNS updates to FreeIPA
>
> This adds two new options:
>
> ipa_dyndns_update: Boolean value to select whether this client
> should automatically update its IP address in FreeIPA DNS.
>
> ipa_dyndns_iface: Choose an interface manually to use for
> updating dynamic DNS. Default is to use the interface associated
> with the LDAP connection to FreeIPA.
>
> This patch supports A and AAAA records. It relies on the presence
> of the nsupdate tool from the bind-utils package to perform the
> actual update step. The location of this utility is set at build
> time, but its availability is determined at runtime (so clients
> that do not require dynamic update capability do not need to meet
> this dependency).
>
> contrib/sssd.spec.in                           |    1 +
>  src/Makefile.am                                |    2 +
>  src/config/SSSDConfig.py                       |    2 +
>  src/config/etc/sssd.api.d/sssd-ipa.conf        |    2 +
>  src/configure.ac                               |    1 +
>  src/external/nsupdate.m4                       |    8 +
>  src/man/sssd-ipa.5.xml                         |   28 ++
>  src/providers/ipa/ipa_common.c                 |    2 +
>  src/providers/ipa/ipa_common.h                 |    2 +
>  src/providers/ipa/ipa_dyndns.c                 |  585 ++++++++++++++++++++++++
>  src/providers/ipa/{ipa_auth.h => ipa_dyndns.h} |   15 +-
>  src/providers/ipa/ipa_init.c                   |   41 ++
>  src/providers/ldap/sdap_async_private.h        |    2 +
>  src/providers/ldap/sdap_fd_events.c            |   28 +-
>  14 files changed, 697 insertions(+), 22 deletions(-)
>  create mode 100644 src/external/nsupdate.m4
>  create mode 100644 src/providers/ipa/ipa_dyndns.c
>  copy src/providers/ipa/{ipa_auth.h => ipa_dyndns.h} (73%)
>
> diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
> index 10aa391780b00f2a0144b2430c35f9ef4c1219b4..9bdf3265b368e53243984a0fe9aab0144a32aa46 100644
> --- a/contrib/sssd.spec.in
> +++ b/contrib/sssd.spec.in
> @@ -74,6 +74,7 @@ BuildRequires: check-devel
>  BuildRequires: doxygen
>  BuildRequires: libselinux-devel
>  BuildRequires: libsemanage-devel
> +BuildRequires: bind-utils
>
> %description
>  Provides a set of daemons to manage access to remote directories and
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 22ac3581ec625690687afc69e65676c822189f59..49cd228578b64c2fcc7c0da578b71dd6020e0d15 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -348,6 +348,7 @@ dist_noinst_HEADERS = \
>      providers/ipa/ipa_access.h \
>      providers/ipa/ipa_timerules.h \
>      providers/ipa/ipa_auth.h \
> providers/ipa/ipa_dyndns.h \
> tools/tools_util.h \
> tools/sss_sync_ops.h \
> resolv/async_resolv.h \
>
> @@ -788,6 +789,7 @@ libsss_ipa_la_SOURCES = \
>      providers/ipa/ipa_auth.c \
>      providers/ipa/ipa_access.c \
>      providers/ipa/ipa_timerules.c \
> providers/ipa/ipa_dyndns.c \
> providers/ldap/ldap_id.c \
> providers/ldap/ldap_id_enum.c \
> providers/ldap/ldap_id_cleanup.c \
>
> diff --git a/src/config/SSSDConfig.py b/src/config/SSSDConfig.py
> index 18df97904de5128d34e4de7c36db1a4743f916d6..be2b7b3f9090a000e5c3c09067aa7f9627466081 100644
> --- a/src/config/SSSDConfig.py
> +++ b/src/config/SSSDConfig.py
> @@ -87,6 +87,8 @@ option_strings = {
>      'ipa_domain' : _('IPA domain'),
>      'ipa_server' : _('IPA server address'),
>      'ipa_hostname' : _('IPA client hostname'),
>
> 'ipa_dyndns_update' : _("Whether to automatically update the client's DNS entry in FreeIPA"),
>
> 'ipa_dyndns_update' : _("The interface whose IP should be used for dynamic DNS updates"),
>
> # [provider/krb5]
> 'krb5_kdcip' : _('Kerberos server address'),
>
> diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf
> index c2a12d5a6068ac42f6dfcdac3d0733871a3fe1d5..500109663724621a973987d687a3adfd24d8ead8 100644
> --- a/src/config/etc/sssd.api.d/sssd-ipa.conf
> +++ b/src/config/etc/sssd.api.d/sssd-ipa.conf
> @@ -2,6 +2,8 @@
>  ipa_domain = str, None, true
>  ipa_server = str, None, true
>  ipa_hostname = str, None, false
> +ipa_dyndns_update = bool, None, false
> +ipa_dyndns_iface = str, None, false
>  ldap_uri = str, None, false
>  ldap_search_base = str, None, false
>  ldap_schema = str, None, false
> diff --git a/src/configure.ac b/src/configure.ac
> index 93debb60abe69456551b8d3d77068875bfae4815..4cc5d853f40e67d6c7e4d44e90a569ea7dc9667c 100644
> --- a/src/configure.ac
> +++ b/src/configure.ac
> @@ -94,6 +94,7 @@ m4_include([external/python.m4])
>  m4_include([external/selinux.m4])
>  m4_include([external/crypto.m4])
>  m4_include([external/nscd.m4])
> +m4_include([external/nsupdate.m4])
>  m4_include([util/signal.m4])
>
> PKG_CHECK_MODULES([DBUS],[dbus-1])
> diff --git a/src/external/nsupdate.m4 b/src/external/nsupdate.m4
> new file mode 100644
> index 0000000000000000000000000000000000000000..6e18f017baf603d48c87951f1c445af9413e9506
> --- /dev/null
> +++ b/src/external/nsupdate.m4
> @@ -0,0 +1,8 @@
> +AC_PATH_PROG(NSUPDATE, nsupdate)
> +AC_MSG_CHECKING(for nsupdate)
> +if test -x "$NSUPDATE"; then
> AC_DEFINE_UNQUOTED([NSUPDATE_PATH], ["$NSUPDATE"], [The path to nsupdate])
> AC_MSG_RESULT(yes)
>
> +else
> AC_MSG_ERROR([no. nsupdate is not available])
>
> +fi
> diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml
> index d1ba1c526f6996561226d32169ef90251f0c3290..8fae9d2bc92f96d2d4ead2d8554676587b522ef8 100644
> --- a/src/man/sssd-ipa.5.xml
> +++ b/src/man/sssd-ipa.5.xml
> @@ -98,6 +98,34 @@
>                  </varlistentry>
>              <varlistentry>
>                <term>ipa_dyndns_update (boolean)</term>
>                <listitem>
>                    <para>
>                        Optional. This option tells SSSD to automatically
>                        update the DNS server built into FreeIPA v2 with
>                        the IP address of this client.
>                    </para>
>                    <para>
>                        Default: false
>                    </para>
>                </listitem>
>            </varlistentry>
>            <varlistentry>
>                <term>ipa_dyndns_iface (string)</term>
>                <listitem>
>                    <para>
>                        Optional. Applicable only when ipa_dyndns_update
>                        is true. Choose the interface whose IP address
>                        should be used for dynamic DNS updates.
>                    </para>
>                    <para>
>                        Default: Use the IP address of the IPA LDAP connection
>                    </para>
>                </listitem>
>            </varlistentry>
>            <varlistentry>
>                <term>krb5_validate (boolean)</term>
>                <listitem>
>                    <para>
>
> diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
> index bed0666bd53b9729400f6078f78487b54edbac03..f30e9680cba2b9ba40c03a776e09ddaeb6486b3a 100644
> --- a/src/providers/ipa/ipa_common.c
> +++ b/src/providers/ipa/ipa_common.c
> @@ -32,6 +32,8 @@ struct dp_option ipa_basic_opts[] = {
>      { "ipa_domain", DP_OPT_STRING, NULL_STRING, NULL_STRING },
>      { "ipa_server", DP_OPT_STRING, NULL_STRING, NULL_STRING },
>      { "ipa_hostname", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> { "ipa_dyndns_update", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE },
> { "ipa_dyndns_iface", DP_OPT_STRING, NULL_STRING, NULL_STRING}
>
> };
>
> struct dp_option ipa_def_ldap_opts[] = {
> diff --git a/src/providers/ipa/ipa_common.h b/src/providers/ipa/ipa_common.h
> index 03a082efd410512330a8b19c0a9a97261aad3e80..9cf3d251b3e7fd0b44673d10752e06be398c5e31 100644
> --- a/src/providers/ipa/ipa_common.h
> +++ b/src/providers/ipa/ipa_common.h
> @@ -46,6 +46,8 @@ enum ipa_basic_opt {
>      IPA_DOMAIN = 0,
>      IPA_SERVER,
>      IPA_HOSTNAME,
>
> IPA_DYNDNS_UPDATE,
>
> IPA_DYNDNS_IFACE,
>
> IPA_OPTS_BASIC /* opts counter */
>
> };
> diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..7f337f04a8ef9dba892ac0c3b1ce1537b624a228
> --- /dev/null
> +++ b/src/providers/ipa/ipa_dyndns.c
> @@ -0,0 +1,585 @@
> +/*
> SSSD
> ipa_dyndns.c
> Authors:
>    Stephen Gallagher <[e-mail]>
> Copyright (C) 2010 Red Hat
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program.  If not, see http://www.gnu.org/licenses/.
>
> +*/
>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/ioctl.h>
> +#include <arpa/inet.h>
> +#include <net/if.h>
> +#include <ifaddrs.h>
> +#include <ctype.h>
> +#include "util/util.h"
> +#include "confdb/confdb.h"
> +#include "providers/ipa/ipa_common.h"
> +#include "providers/ipa/ipa_dyndns.h"
> +#include "providers/child_common.h"
> +#include "providers/data_provider.h"
> +#include "providers/ldap/ldap_common.h"
> +#include "providers/ldap/sdap_async_private.h"
> +#include "resolv/async_resolv.h"
>
> +#define IPA_DYNDNS_TIMEOUT 15
>
> +struct ipa_ipaddress {
> struct ipa_ipaddress *next;
> struct ipa_ipaddress *prev;
> struct sockaddr *addr;
> bool matched;
>
> +};
>
> +struct ipa_dyndns_ctx {
> struct ipa_options *ipa_ctx;
> char *hostname;
> struct ipa_ipaddress *addresses;
> int child_status;
>
> +};
>
> +static struct tevent_req * ipa_dyndns_update_send(struct ipa_options *ctx);
>
> +static void ipa_dyndns_update_done(struct tevent_req *req);
>
> +void ipa_dyndns_update(void *pvt)
> +{
> struct ipa_options *ctx = talloc_get_type(pvt, struct ipa_options);
> struct tevent_req *req = ipa_dyndns_update_send(ctx);
> if (req == NULL) {
>    DEBUG(1, ("Could not update DNS\n"));
>    return;
> }
> tevent_req_set_callback(req, ipa_dyndns_update_done, req);
>
> +}
>
> +static struct tevent_req *
> +ipa_dyndns_gss_tsig_update_send(struct ipa_dyndns_ctx *ctx);
>
> +static void ipa_dyndns_gss_tsig_update_done(struct tevent_req *subreq);
>
> +static struct tevent_req *
> +ipa_dyndns_update_send(struct ipa_options *ctx)
> +{
> int ret;
> int fd;
> char *iface;
> char *ipa_hostname;
> struct ipa_dyndns_ctx *state;
> struct sockaddr sa;
> socklen_t sa_len = sizeof(sa);
> struct ifaddrs *ifaces;
> struct ifaddrs *ifa;
> struct ipa_ipaddress *address;
> struct tevent_req *req, *subreq;
> DEBUG (9, ("Performing update\n"));
> req = tevent_req_create(ctx, &state, struct ipa_dyndns_ctx);
> if (req == NULL) {
>    return NULL;
> }
> state->ipa_ctx = ctx;
> iface = dp_opt_get_string(ctx->basic, IPA_DYNDNS_IFACE);
> if (iface) {
>    /* Get the IP addresses associated with the
>     * specified interface
>     */
>    errno = 0;
>    ret = getifaddrs(&ifaces);
>    if (ret == -1) {
>        ret = errno;
>        DEBUG(0, ("Could not read interfaces [%d][%s]\n",
>                  ret, strerror(ret)));
>        goto failed;
>    }
>    for(ifa = ifaces; ifa != NULL; ifa=ifa->ifa_next) {
>        /* Some interfaces don't have an ifa_addr */
>        if (!ifa->ifa_addr) continue;
>        /* Add IP addresses to the list */
>        if((ifa->ifa_addr->sa_family == AF_INET ||
>            ifa->ifa_addr->sa_family == AF_INET6) &&
>           strcasecmp(ifa->ifa_name, iface) == 0) {
>            /* Add this address to the IP address list */
>            address = talloc_zero(state, struct ipa_ipaddress);
>            if (!address) {
>                goto failed;
>            }
>            address->addr = talloc_memdup(address, ifa->ifa_addr,
>                                          sizeof(struct sockaddr));
>            if(address->addr == NULL) {
>                goto failed;
>            }
>            DLIST_ADD(state->addresses, address);
>        }
>    }
>    freeifaddrs(ifaces);
> }
> else {
>    /* Get the file descriptor for the primary LDAP connection */
>    ret = get_fd_from_ldap(ctx->id_ctx->gsh->ldap, &fd);
>    if (ret != EOK) {
>        goto failed;
>    }
>    ret = getsockname(fd, &sa, &sa_len);
>    if (ret == -1) {
>        DEBUG(0,("Failed to get socket name\n"));
>        goto failed;
>    }
>    switch(sa.sa_family) {
>    case AF_INET:
>    case AF_INET6:
>        address = talloc(state, struct ipa_ipaddress);
>        if (!address) {
>            goto failed;
>        }
>        address->addr = talloc_memdup(address, &sa,
>                                      sizeof(struct sockaddr));
>        if(address->addr == NULL) {
>            goto failed;
>        }
>        DLIST_ADD(state->addresses, address);
>        break;
>    default:
>        DEBUG(1, ("Connection to LDAP is neither IPv4 nor IPv6\n"));
>        ret = EIO;
>        goto failed;
>    }
> }
> /* Get the IPA hostname */
> ipa_hostname = dp_opt_get_string(state->ipa_ctx->basic,
>                                 IPA_HOSTNAME);
> if (!ipa_hostname) {
>    /* This should never happen, but we'll protect
>     * against it anyway.
>     */
>    talloc_free(req);
>    return NULL;
> }
> state->hostname = talloc_strdup(state, ipa_hostname);
> if(state->hostname == NULL) {
>    talloc_free(req);
>    return NULL;
> }
> /* In the future, it might be best to check that an update
> * needs to be run before running it, but this is such a
> * rare event that it's probably fine to just run an update
> * every time we come online.
> */
> subreq = ipa_dyndns_gss_tsig_update_send(state);
> if(subreq == NULL) {
>    tevent_req_error(req, EIO);
> }
> tevent_req_set_callback(subreq,
>                        ipa_dyndns_gss_tsig_update_done,
>                        req);
> return req;
>
> +failed:
> talloc_free(req);
> return NULL;
>
> +}
>
> +struct ipa_nsupdate_ctx {
> char *update_msg;
> struct ipa_dyndns_ctx *dyndns_ctx;
> int pipefd_to_child;
> struct tevent_timer *timeout_handler;
> struct tevent_signal *sige;
>
> +};
>
> +static int create_nsupdate_message(struct ipa_nsupdate_ctx *ctx);
>
> +static struct tevent_req *
> +fork_nsupdate_send(struct ipa_nsupdate_ctx *ctx);
>
> +static void fork_nsupdate_done(struct tevent_req *subreq);
>
> +static struct tevent_req *
> +ipa_dyndns_gss_tsig_update_send(struct ipa_dyndns_ctx *ctx)
> +{
> int ret;
> struct ipa_nsupdate_ctx *state;
> struct tevent_req *req;
> struct tevent_req *subreq;
> req = tevent_req_create(ctx, &state, struct ipa_nsupdate_ctx);
> if(req == NULL) {
>    return NULL;
> }
> state->dyndns_ctx = ctx;
> /* Format the message to pass to the nsupdate command */
> ret = create_nsupdate_message(state);
> if (ret != EOK) {
>    goto failed;
> }
> /* Fork a child process to perform the DNS update */
> subreq = fork_nsupdate_send(state);
> if(subreq == NULL) {
>    goto failed;
> }
> tevent_req_set_callback(subreq, fork_nsupdate_done, req);
> return req;
>
> +failed:
> talloc_free(req);
> return NULL;
>
> +}
>
> +struct nsupdate_send_ctx {
> struct ipa_nsupdate_ctx *nsupdate_ctx;
>
> +};
>
> +static int create_nsupdate_message(struct ipa_nsupdate_ctx *ctx)
> +{
> int ret, i;
> char *servername;
> char *zone;
> char ip_addr[INET6_ADDRSTRLEN];
> const char *ip;
> struct ipa_ipaddress *new_record;
> servername = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
>                               IPA_SERVER);
> if (!servername) {
>    return EIO;
> }
> zone = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
>                         IPA_DOMAIN);
> if (!zone) {
>    return EIO;
> }
> /* The DNS zone for IPA is the lower-case
> * version of hte IPA domain
> */
> for(i = 0; zone[i] != '\0'; i++) {
>    zone[i] = tolower(zone[i]);
> }
> /* Add the server and zone headers */
> ctx->update_msg = talloc_asprintf(ctx, "server %s\nzone %s.\n",
>                                       servername,
>                                       zone);
> if (ctx->update_msg == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> /* Remove any existing entries */
> ctx->update_msg = talloc_asprintf_append(ctx->update_msg,
>                                         "update delete %s. in A\nsend\n"
>                                         "update delete %s. in AAAA\nsend\n",
>                                         ctx->dyndns_ctx->hostname,
>                                         ctx->dyndns_ctx->hostname);
> if (ctx->update_msg == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> DLIST_FOR_EACH(new_record, ctx->dyndns_ctx->addresses) {
>    switch(new_record->addr->sa_family) {
>    case AF_INET:
>        ip = inet_ntop(new_record->addr->sa_family,
>                       &(((struct sockaddr_in *)new_record->addr)->sin_addr),
>                       ip_addr, INET6_ADDRSTRLEN);
>        if (ip == NULL) {
>            ret = EIO;
>            goto done;
>        }
>        break;
>    case AF_INET6:
>        ip = inet_ntop(new_record->addr->sa_family,
>                       &(((struct sockaddr_in6 *)new_record->addr)->sin6_addr),
>                       ip_addr, INET6_ADDRSTRLEN);
>        if (ip == NULL) {
>            ret = EIO;
>            goto done;
>        }
>        break;
>    default:
>        DEBUG(0, ("Unknown address family\n"));
>        ret = EIO;
>        goto done;
>    }
>    /* Format the record update */
>    ctx->update_msg = talloc_asprintf_append(
>            ctx->update_msg,
>            "update add %s. 86400 in %s %s\n",
>            ctx->dyndns_ctx->hostname,
>            new_record->addr->sa_family == AF_INET ? "A" : "AAAA",
>            ip_addr);
>    if (ctx->update_msg == NULL) {
>        ret = ENOMEM;
>        goto done;
>    }
> }
> ctx->update_msg = talloc_asprintf_append(ctx->update_msg, "send\n");
> if (ctx->update_msg == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> ret = EOK;
>
> +done:
> return ret;
>
> +}
>
> +static void ipa_dyndns_stdin_done(struct tevent_req *subreq);
>
> +static void ipa_dyndns_child_handler(int child_status,
>                                 struct tevent_signal *sige,
>                                 void *pvt);
>
> +static void ipa_dyndns_timeout(struct tevent_context *ev,
>                           struct tevent_timer *te,
>                           struct timeval tv, void *pvt);
>
> +static struct tevent_req *
> +fork_nsupdate_send(struct ipa_nsupdate_ctx *ctx)
> +{
> int pipefd_to_child[2];
> pid_t pid;
> int ret;
> errno_t err;
> struct timeval tv;
> struct tevent_req *req = NULL;
> struct tevent_req *subreq = NULL;
> struct nsupdate_send_ctx *state;
> char *args[3];
> req = tevent_req_create(ctx, &state, struct nsupdate_send_ctx);
> if (req == NULL) {
>    return NULL;
> }
> state->nsupdate_ctx = ctx;
> ret = pipe(pipefd_to_child);
> if (ret == -1) {
>    err = errno;
>    DEBUG(1, ("pipe failed [%d][%s].\n", err, strerror(err)));
>    return NULL;
> }
> pid = fork();
> if (pid == 0) { /* child */
>    args[0] = talloc_strdup(ctx, NSUPDATE_PATH);
>    args[1] = talloc_strdup(ctx, "-g");
>    args[2] = NULL;
>    if (args[0] == NULL || args[1] == NULL) {
>        return NULL;
>    }
>    close(pipefd_to_child[1]);
>    ret = dup2(pipefd_to_child[0], STDIN_FILENO);
>    if (ret == -1) {
>        err = errno;
>        DEBUG(1, ("dup2 failed [%d][%s].\n", err, strerror(err)));
>        return NULL;
>    }
>    errno = 0;
>    ret = execv(NSUPDATE_PATH, args);
>    if(ret == -1) {
>        err = errno;
>        DEBUG(1, ("execv failed [%d][%s].\n", err, strerror(err)));
>    }
>    return NULL;
> }
> else if (pid > 0) { /* parent */
>    close(pipefd_to_child[0]);
>    ctx->pipefd_to_child = pipefd_to_child[1];
>    /* Write the update message to the nsupdate child */
>    subreq = write_pipe_send(req,
>                             ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>                             (uint8_t *)ctx->update_msg,
>                             strlen(ctx->update_msg)+1,
>                             ctx->pipefd_to_child);
>    if (subreq == NULL) {
>        return NULL;
>    }
>    tevent_req_set_callback(subreq, ipa_dyndns_stdin_done, req);
>    /* Set up SIGCHLD handler */
>    ret = child_handler_setup(req,
>                              ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>                              pid, ipa_dyndns_child_handler, req, &ctx->sige);
>    if (ret != EOK) {
>        return NULL;
>    }
>    talloc_steal(ctx, ctx->sige);
>    /* Set up timeout handler */
>    tv = tevent_timeval_current_ofs(IPA_DYNDNS_TIMEOUT, 0);
>    ctx->timeout_handler = tevent_add_timer(
>            ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>            req, tv, ipa_dyndns_timeout, req);
>    if(ctx->timeout_handler == NULL) {
>        return NULL;
>    }
> }
> else { /* error */
>    err = errno;
>    DEBUG(1, ("fork failed [%d][%s].\n", err, strerror(err)));
>    return NULL;
> }
> return req;
>
> +}
>
> +static void ipa_dyndns_timeout(struct tevent_context *ev,
>                           struct tevent_timer *te,
>                           struct timeval tv, void *pvt)
>
> +{
> struct tevent_req *req =
>        talloc_get_type(pvt, struct tevent_req);
> DEBUG(1, ("Timeout reached for dynamic DNS update\n"));
> tevent_req_error(req, ETIMEDOUT);
>
> +}
>
> +static void ipa_dyndns_stdin_done(struct tevent_req *subreq)
> +{
> /* Verify that the buffer was sent, then return
> * and wait for the sigchld handler to finish.
> */
> DEBUG(9, ("Sending nsupdate data complete\n"));
> int ret;
> struct tevent_req *req =
>        tevent_req_callback_data(subreq, struct tevent_req);
> struct nsupdate_send_ctx *state =
>        tevent_req_data(req, struct nsupdate_send_ctx);
> ret = write_pipe_recv(subreq);
> talloc_zfree(subreq);
> if (ret != EOK) {
>    DEBUG(1, ("Sending nsupdate data failed\n"));
>    tevent_req_error(req, ret);
>    return;
> }
> close(state->nsupdate_ctx->pipefd_to_child);
> state->nsupdate_ctx->pipefd_to_child = -1;
>
> +}
>
> +static void ipa_dyndns_child_handler(int child_status,
>                                 struct tevent_signal *sige,
>                                 void *pvt)
>
> +{
> struct tevent_req *req = talloc_get_type(pvt, struct tevent_req);
> talloc_zfree(sige);
> if (WEXITSTATUS(child_status) != 0) {
>    DEBUG(1, ("Dynamic DNS child failed with status [%d]\n",
>              child_status));
>    tevent_req_error(req, EIO);
>    return;
> }
> tevent_req_done(req);
>
> +}
>
> +static int ipa_dyndns_generic_recv(struct tevent_req *req)
> +{
> TEVENT_REQ_RETURN_ON_ERROR(req);
> return EOK;
>
> +}
>
> +static void fork_nsupdate_done(struct tevent_req *subreq)
> +{
> int ret;
> struct tevent_req *req =
>        tevent_req_callback_data(subreq, struct tevent_req);
> ret = ipa_dyndns_generic_recv(subreq);
> talloc_zfree(subreq);
> if (ret != EOK) {
>    tevent_req_error(req, ret);
>    return;
> }
> tevent_req_done(req);
>
> +}
>
> +static void ipa_dyndns_gss_tsig_update_done(struct tevent_req *subreq)
> +{
> /* Check the return code from the sigchld handler
> * and return it to the parent request.
> */
> int ret;
> struct tevent_req *req =
>        tevent_req_callback_data(subreq, struct tevent_req);
> ret = ipa_dyndns_generic_recv(subreq);
> talloc_zfree(subreq);
> if (ret != EOK) {
>    tevent_req_error(req, ret);
>    return;
> }
> tevent_req_done(req);
>
> +}
>
> +static void ipa_dyndns_update_done(struct tevent_req *req)
> +{
> int ret = ipa_dyndns_generic_recv(req);
> talloc_free(req);
> if (ret != EOK) {
>    DEBUG(1, ("Updating DNS entry failed\n"));
>    return;
> }
> DEBUG(1,("Updated DNS entry\n"));
>
> +}
> diff --git a/src/providers/ipa/ipa_auth.h b/src/providers/ipa/ipa_dyndns.h
> similarity index 73%
> copy from src/providers/ipa/ipa_auth.h
> copy to src/providers/ipa/ipa_dyndns.h
> index 3079bbd1b8ffc21cc6c6b5b945717bc4e93f5f25..406e8b2f0cce0931f3babd1120655718622c871b 100644
> --- a/src/providers/ipa/ipa_auth.h
> +++ b/src/providers/ipa/ipa_dyndns.h
> @@ -1,12 +1,12 @@
>  /*
>      SSSD
> IPA Backend Module -- Authentication
>
> ipa_dyndns.h
>
> Authors:
>    Sumit Bose <[e-mail]>
>    Stephen Gallagher <[e-mail]>
>
> Copyright (C) 2009 Red Hat
>
> Copyright (C) 2010 Red Hat
>
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
>
> @@ -22,11 +22,10 @@
>      along with this program.  If not, see http://www.gnu.org/licenses/.
>  */
>
> -#ifndef _IPA_AUTH_H_
> -#define _IPA_AUTH_H_
> +#ifndef IPA_DYNDNS_H_
> +#define IPA_DYNDNS_H_
>
> -#include "providers/dp_backend.h"
> +void ipa_dyndns_update(void *pvt);
>
> -void ipa_auth(struct be_req *be_req);
>
> -#endif /* _IPA_AUTH_H_ */
> +#endif /* IPA_DYNDNS_H_ */
> diff --git a/src/providers/ipa/ipa_init.c b/src/providers/ipa/ipa_init.c
> index bd5a9e03c8bf3af6ba2c8864c9291d92a5efa1d8..dbef772a4817833f8dbf21c3482ca195be7304b6 100644
> --- a/src/providers/ipa/ipa_init.c
> +++ b/src/providers/ipa/ipa_init.c
> @@ -33,6 +33,7 @@
>  #include "providers/ipa/ipa_auth.h"
>  #include "providers/ipa/ipa_access.h"
>  #include "providers/ipa/ipa_timerules.h"
> +#include "providers/ipa/ipa_dyndns.h"
>
> struct ipa_options *ipa_options = NULL;
>
> @@ -97,6 +98,8 @@ int sssm_ipa_id_init(struct be_ctx *bectx,
>                       void **pvt_data)
>  {
>      struct sdap_id_ctx *ctx;
>
> struct stat stat_buf;
>
> errno_t err;
> int ret;
>
> if (!ipa_options) {
>
> @@ -128,6 +131,44 @@ int sssm_ipa_id_init(struct be_ctx *bectx,
>          goto done;
>      }
> if(dp_opt_get_bool(ipa_options->basic, IPA_DYNDNS_UPDATE)) {
>    /* Perform automatic DNS updates when the
>     * IP address changes.
>     * Register a callback for successful LDAP
>     * reconnections. This is the easiest way to
>     * identify that we have gone online.
>     */
>    /* Ensure that nsupdate exists */
>    errno = 0;
>    ret = stat(NSUPDATE_PATH, &stat_buf);
>    if (ret == -1) {
>        err = errno;
>        if (err == ENOENT) {
>            DEBUG(0, ("%s does not exist. Dynamic DNS updates disabled\n",
>                      NSUPDATE_PATH));
>        }
>        else {
>            DEBUG(0, ("Could not set up dynamic DNS updates: [%d][%s]\n",
>                      err, strerror(err)));
>        }
>    }
>    else {
>        /* nsupdate is available. Dynamic updates
>         * are supported
>         */
>        ret = be_add_online_cb(ctx, ctx->be,
>                               ipa_dyndns_update,
>                               ipa_options, NULL);
>        if (ret != EOK) {
>            DEBUG(1,("Failure setting up automatic DNS update\n"));
>            /* We will continue without DNS updating */
>        }
>    }
> }
> ret = setup_tls_config(ctx->opts->basic);
>    if (ret != EOK) {
>        DEBUG(1, ("setup_tls_config failed [%d][%s].\n",
>
> diff --git a/src/providers/ldap/sdap_async_private.h b/src/providers/ldap/sdap_async_private.h
> index 75597c6d3ed1380971029e2a3b4174fcb7155c27..c74a7e60c38f752af108b7cb00ff1e019f9b0d9d 100644
> --- a/src/providers/ldap/sdap_async_private.h
> +++ b/src/providers/ldap/sdap_async_private.h
> @@ -34,6 +34,8 @@ void sdap_ldap_result(struct tevent_context *ev, struct tevent_fd *fde,
>  int setup_ldap_connection_callbacks(struct sdap_handle *sh,
>                                      struct tevent_context *ev);
>
> +int get_fd_from_ldap(LDAP *ldap, int *fd);
>
> errno_t sdap_set_connected(struct sdap_handle *sh, struct tevent_context *ev);
>
> int sdap_op_add(TALLOC_CTX *memctx, struct tevent_context *ev,
> diff --git a/src/providers/ldap/sdap_fd_events.c b/src/providers/ldap/sdap_fd_events.c
> index 11527789f6e2834883b85efc983b57b7eb1c0a37..3278296308fcb1a745dbd4cb89a4cc0c05aa065b 100644
> --- a/src/providers/ldap/sdap_fd_events.c
> +++ b/src/providers/ldap/sdap_fd_events.c
> @@ -33,6 +33,20 @@ struct sdap_fd_events {
>  #endif
>  };
>
> +int get_fd_from_ldap(LDAP *ldap, int *fd)
> +{
> int ret;
> ret = ldap_get_option(ldap, LDAP_OPT_DESC, fd);
> if (ret != LDAP_OPT_SUCCESS) {
>    DEBUG(1, ("Failed to get fd from ldap!!\n"));
>    *fd = -1;
>    return EIO;
> }
> return EOK;
>
> +}
>
> #ifdef HAVE_LDAP_CONNCB
>  static int remove_connection_callback(TALLOC_CTX *mem_ctx)
>  {
> @@ -135,20 +149,6 @@ static void sdap_ldap_connect_callback_del(LDAP *ld, Sockbuf *sb,
>
> #else
>
> -static int get_fd_from_ldap(LDAP *ldap, int *fd)
> -{
> int ret;
> ret = ldap_get_option(ldap, LDAP_OPT_DESC, fd);
> if (ret != LDAP_OPT_SUCCESS) {
>    DEBUG(1, ("Failed to get fd from ldap!!\n"));
>    *fd = -1;
>    return EIO;
> }
> return EOK;
>
> -}
>
> static int sdap_install_ldap_callbacks(struct sdap_handle *sh,
>                                         struct tevent_context *ev)
>  {
> -- 
> 1.7.0.1
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

On 05/04/2010 02:27 PM, Sumit Bose wrote:
> On Tue, May 04, 2010 at 01:01:37PM -0400, Stephen Gallagher wrote:
> > On 05/04/2010 08:29 AM, Stephen Gallagher wrote:
> > > >
> > >
> > > Thanks for the review. Updated patch coming soon.
> >
> > Updated patch should address both Sumit's and Jakub's concerns.
>
> There is a typo in SSSDConfig.py:
> 'ipa_dyndns_update' : _("Whether to automatically update the
>
> client's DNS entry in FreeIPA"),
> 'ipa_dyndns_update' : _("The interface whose IP should be used for
>
> dynamic DNS updates"),

Whoops, I'll fix that.
> And I see error messages from nsupdate like 'Check your Kerberos ticket,
> it may have expired.'. Maybe you should catch stdout and stderr from
> nsupdate child.

You see them where? Those shouldn't appear if you're running daemonized.
> How does nsupdate find the ccache file? I think you may need to set
> KRB5CCNAME.

That should already be in the environment, because the IPA backend is 
already kinited with the host keytab. It seemed to work for me on my 
test VMs.
> bye,
> Sumit

[Stephen Gallagher]

On 05/04/2010 02:33 PM, Stephen Gallagher wrote:
> On 05/04/2010 02:27 PM, Sumit Bose wrote:
> > On Tue, May 04, 2010 at 01:01:37PM -0400, Stephen Gallagher wrote:
> > > On 05/04/2010 08:29 AM, Stephen Gallagher wrote:
> > > > >
> > > >
> > > > Thanks for the review. Updated patch coming soon.
> > >
> > > Updated patch should address both Sumit's and Jakub's concerns.
> >
> > There is a typo in SSSDConfig.py:
> > 'ipa_dyndns_update' : _("Whether to automatically update the
> >
> > client's DNS entry in FreeIPA"),
> > 'ipa_dyndns_update' : _("The interface whose IP should be used for
> >
> > dynamic DNS updates"),
>
> Whoops, I'll fix that.

Fixed.
> > And I see error messages from nsupdate like 'Check your Kerberos ticket,
> > it may have expired.'. Maybe you should catch stdout and stderr from
> > nsupdate child.
>
> You see them where? Those shouldn't appear if you're running daemonized.

You will see them if you're running SSSD in -d, but they're suppressed 
when running daemonized. I just tested this. I think that's probably fine.
> > How does nsupdate find the ccache file? I think you may need to set
> > KRB5CCNAME.
> That should already be in the environment, because the IPA backend is
> already kinited with the host keytab. It seemed to work for me on my
> test VMs.

Just retested as well. It's getting the ccache from the ID provider.

New patch attached.

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/04/2010 08:54 PM, Stephen Gallagher wrote:
> New patch attached.

fwiw, I don't have any more concerns, ack from me. Let's see if Sumit
has any more comments :)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkvgjZcACgkQHsardTLnvCWYagCgz6voyc0nRgBj3sln91577uLs
> JwMAniGTNyBJjGNy6sHYFMGXdINaWFgw
> =T/Xu
> -----END PGP SIGNATURE-----

[Sumit Bose]

On Tue, May 04, 2010 at 11:11:51PM +0200, Jakub Hrozek wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 05/04/2010 08:54 PM, Stephen Gallagher wrote:
> > New patch attached.
>
> fwiw, I don't have any more concerns, ack from me. Let's see if Sumit
> has any more comments :)

The patch looks fine, but currently I'm not able to really test it. So
it is also an ACK from me, but if you want me to do more testing you
have to wait until tomorrow.

bye,
Sumit
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkvgjZcACgkQHsardTLnvCWYagCgz6voyc0nRgBj3sln91577uLs
> JwMAniGTNyBJjGNy6sHYFMGXdINaWFgw
> =T/Xu
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Stephen Gallagher]

On 05/05/2010 04:37 AM, Sumit Bose wrote:
> On Tue, May 04, 2010 at 11:11:51PM +0200, Jakub Hrozek wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > On 05/04/2010 08:54 PM, Stephen Gallagher wrote:
> > > New patch attached.
> >
> > fwiw, I don't have any more concerns, ack from me. Let's see if Sumit
> > has any more comments :)
>
> The patch looks fine, but currently I'm not able to really test it. So
> it is also an ACK from me, but if you want me to do more testing you
> have to wait until tomorrow.
>
> bye,
> Sumit

I found a double-free error in the patch, and in doing so ended up 
updating the SIGCHLD patch to address it. The attached patch is rebased 
atop those changes.

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/06/2010 07:54 PM, Stephen Gallagher wrote:
> On 05/05/2010 04:37 AM, Sumit Bose wrote:
> > On Tue, May 04, 2010 at 11:11:51PM +0200, Jakub Hrozek wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > >
> > > On 05/04/2010 08:54 PM, Stephen Gallagher wrote:
> > > > New patch attached.
> > >
> > > fwiw, I don't have any more concerns, ack from me. Let's see if Sumit
> > > has any more comments :)
> >
> > The patch looks fine, but currently I'm not able to really test it. So
> > it is also an ACK from me, but if you want me to do more testing you
> > have to wait until tomorrow.
> >
> > bye,
> > Sumit
>
> I found a double-free error in the patch, and in doing so ended up
> updating the SIGCHLD patch to address it. The attached patch is rebased
> atop those changes.

I don't have any more comments to the code and also my testing shows
that this feature works correctly - ACK.

Great work!
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.14 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkvkbakACgkQHsardTLnvCXsDQCfZokstM/aXl8YyarOBBczth5l
> duoAoJFV+ZuTEBhSnglrQEwHPUdK5kfn
> =uKPZ
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

On 05/07/2010 03:44 PM, Jakub Hrozek wrote:
> I don't have any more comments to the code and also my testing shows
> that this feature works correctly - ACK.
>
> Great work!

Pushed to sssd-1-2.