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;
+}
+