From a741d0c4345dceb27dfbb20b81fd858a514b47cd Mon Sep 17 00:00:00 2001 From: Pavel Reichl Date: Fri, 24 Jul 2015 13:25:56 -0400 Subject: DYNDNS: improve nsupdate_msg_add_fwd() Update nsupdate_msg_add_fwd() to group commands by address family processed IP address belongs to. It's better to group removing old A addresses and adding new A addresses in a single transaction. Same goes for AAAA addresses. Separate transaction for A and AAAA addresses updates are important because server might block updates for one of these families and thus the update even for the non-blocked address family would unnecessarily fail. For more details please see: https://fedorahosted.org/sssd/wiki/DesignDocs/DDNSMessagesUpdate Resolves: https://fedorahosted.org/sssd/ticket/2495 Reviewed-by: Jakub Hrozek --- src/providers/dp_dyndns.c | 56 ++++++--- src/tests/cmocka/test_dyndns.c | 280 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 316 insertions(+), 20 deletions(-) diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c index 782dcb60a..f717bf6cd 100644 --- a/src/providers/dp_dyndns.c +++ b/src/providers/dp_dyndns.c @@ -273,40 +273,62 @@ nsupdate_msg_add_fwd(char *update_msg, struct sss_iface_addr *addresses, char ip_addr[INET6_ADDRSTRLEN]; errno_t ret; + /* A addresses first */ /* Remove existing entries as needed */ if (remove_af & DYNDNS_REMOVE_A) { update_msg = talloc_asprintf_append(update_msg, - "update delete %s. in A\nsend\n", + "update delete %s. in A\n", hostname); if (update_msg == NULL) { return NULL; } } + DLIST_FOR_EACH(new_record, addresses) { + if (new_record->addr->ss_family == AF_INET) { + ret = addr_to_str(new_record->addr, ip_addr, INET6_ADDRSTRLEN); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, "addr_to_str failed: %d:[%s],\n", + ret, sss_strerror(ret)); + return NULL; + } + + /* Format the record update */ + update_msg = talloc_asprintf_append(update_msg, + "update add %s. %d in %s %s\n", + hostname, ttl, "A", ip_addr); + if (update_msg == NULL) { + return NULL; + } + } + } + update_msg = talloc_asprintf_append(update_msg, "send\n"); + + /* AAAA addresses next */ + /* Remove existing entries as needed */ if (remove_af & DYNDNS_REMOVE_AAAA) { update_msg = talloc_asprintf_append(update_msg, - "update delete %s. in AAAA\nsend\n", + "update delete %s. in AAAA\n", hostname); if (update_msg == NULL) { return NULL; } } - DLIST_FOR_EACH(new_record, addresses) { - ret = addr_to_str(new_record->addr, ip_addr, INET6_ADDRSTRLEN); - if (ret != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, "addr_to_str failed: %d:[%s],\n", - ret, sss_strerror(ret)); - return NULL; - } + if (new_record->addr->ss_family == AF_INET6) { + ret = addr_to_str(new_record->addr, ip_addr, INET6_ADDRSTRLEN); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, "addr_to_str failed: %d:[%s],\n", + ret, sss_strerror(ret)); + return NULL; + } - /* Format the record update */ - update_msg = talloc_asprintf_append(update_msg, - "update add %s. %d in %s %s\n", - hostname, ttl, - new_record->addr->ss_family == AF_INET ? "A" : "AAAA", - ip_addr); - if (update_msg == NULL) { - return NULL; + /* Format the record update */ + update_msg = talloc_asprintf_append(update_msg, + "update add %s. %d in %s %s\n", + hostname, ttl, "AAAA", ip_addr); + if (update_msg == NULL) { + return NULL; + } } } diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c index 815acf696..43df0e1b5 100644 --- a/src/tests/cmocka/test_dyndns.c +++ b/src/tests/cmocka/test_dyndns.c @@ -355,6 +355,266 @@ void dyndns_test_addr_list_as_str_list(void **state) assert_true(check_leaks_pop(dyndns_test_ctx) == true); } +void dyndns_test_create_fwd_msg(void **state) +{ + errno_t ret; + char *msg; + struct sss_iface_addr *addrlist; + int i; + + check_leaks_push(dyndns_test_ctx); + + /* getifaddrs is called twice in sss_get_dualstack_addresses() */ + for (i = 0; i < 2; i++) { + will_return_getifaddrs("eth0", "192.168.0.2", AF_INET); + will_return_getifaddrs("eth1", "192.168.0.1", AF_INET); + will_return_getifaddrs("eth0", "2001:cdba::555", AF_INET6); + will_return_getifaddrs("eth1", "2001:cdba::444", AF_INET6); + will_return_getifaddrs(NULL, NULL, 0); /* sentinel */ + } + + struct sockaddr_in sin; + memset(&sin, 0, sizeof (sin)); + sin.sin_family = AF_INET; + sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); + ret = sss_get_dualstack_addresses(dyndns_test_ctx, + (struct sockaddr *) &sin, + &addrlist); + assert_int_equal(ret, EOK); + + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, NULL, NULL, "bran_stark", + 1234, DYNDNS_REMOVE_A | DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "\nupdate delete bran_stark. in A\n" + "update add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + /* fallback case realm and server */ + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, "North", "Winterfell", + "bran_stark", + 1234, DYNDNS_REMOVE_A | DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "server Winterfell\n" + "realm North\n" + "update delete bran_stark. in A\n" + "update add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + /* just realm */ + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, "North", NULL, + "bran_stark", + 1234, DYNDNS_REMOVE_A | DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "realm North\n" + "update delete bran_stark. in A\n" + "update add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + /* just server */ + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, NULL, "Winterfell", + "bran_stark", + 1234, DYNDNS_REMOVE_A | DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "server Winterfell\n" + "\n" + "update delete bran_stark. in A\n" + "update add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + /* remove just A */ + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, NULL, NULL, "bran_stark", + 1234, DYNDNS_REMOVE_A, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "\nupdate delete bran_stark. in A\n" + "update add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + /* remove just AAAA */ + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, NULL, NULL, "bran_stark", + 1234, DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "\nupdate add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + talloc_free(addrlist); + assert_true(check_leaks_pop(dyndns_test_ctx) == true); +} + +void dyndns_test_create_fwd_msg_mult(void **state) +{ + errno_t ret; + char *msg; + struct sss_iface_addr *addrlist; + int i; + + check_leaks_push(dyndns_test_ctx); + + /* getifaddrs is called twice in sss_get_dualstack_addresses() */ + for (i = 0; i < 2; i++) { + will_return_getifaddrs("eth0", "192.168.0.2", AF_INET); + will_return_getifaddrs("eth0", "192.168.0.1", AF_INET); + will_return_getifaddrs("eth0", "2001:cdba::555", AF_INET6); + will_return_getifaddrs("eth0", "2001:cdba::444", AF_INET6); + will_return_getifaddrs(NULL, NULL, 0); /* sentinel */ + } + + struct sockaddr_in sin; + memset(&sin, 0, sizeof (sin)); + sin.sin_family = AF_INET; + sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); + ret = sss_get_dualstack_addresses(dyndns_test_ctx, + (struct sockaddr *) &sin, + &addrlist); + assert_int_equal(ret, EOK); + + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, NULL, NULL, "bran_stark", + 1234, DYNDNS_REMOVE_A | DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "\nupdate delete bran_stark. in A\n" + "update add bran_stark. 1234 in A 192.168.0.1\n" + "update add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::444\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + talloc_free(addrlist); + assert_true(check_leaks_pop(dyndns_test_ctx) == true); +} + +void dyndns_test_create_fwd_msg_A(void **state) +{ + errno_t ret; + char *msg; + struct sss_iface_addr *addrlist; + int i; + + check_leaks_push(dyndns_test_ctx); + + /* getifaddrs is called twice in sss_get_dualstack_addresses() */ + for (i = 0; i < 2; i++) { + will_return_getifaddrs("eth0", "192.168.0.2", AF_INET); + will_return_getifaddrs("eth0", "192.168.0.1", AF_INET); + will_return_getifaddrs(NULL, NULL, 0); /* sentinel */ + } + + struct sockaddr_in sin; + memset(&sin, 0, sizeof (sin)); + sin.sin_family = AF_INET; + sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); + ret = sss_get_dualstack_addresses(dyndns_test_ctx, + (struct sockaddr *) &sin, + &addrlist); + assert_int_equal(ret, EOK); + + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, NULL, NULL, "bran_stark", + 1234, DYNDNS_REMOVE_A | DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "\nupdate delete bran_stark. in A\n" + "update add bran_stark. 1234 in A 192.168.0.1\n" + "update add bran_stark. 1234 in A 192.168.0.2\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "send\n"); + talloc_zfree(msg); + + talloc_free(addrlist); + assert_true(check_leaks_pop(dyndns_test_ctx) == true); +} + +void dyndns_test_create_fwd_msg_AAAA(void **state) +{ + errno_t ret; + char *msg; + struct sss_iface_addr *addrlist; + int i; + + check_leaks_push(dyndns_test_ctx); + + /* getifaddrs is called twice in sss_get_dualstack_addresses() */ + for (i = 0; i < 2; i++) { + will_return_getifaddrs("eth0", "2001:cdba::555", AF_INET6); + will_return_getifaddrs("eth0", "2001:cdba::444", AF_INET6); + will_return_getifaddrs(NULL, NULL, 0); /* sentinel */ + } + + struct sockaddr_in6 sin; + memset(&sin, 0, sizeof (sin)); + sin.sin6_family = AF_INET6; + ret = inet_pton(AF_INET6, "2001:cdba::555", &sin.sin6_addr.s6_addr); + assert_int_equal(ret, 1); + ret = sss_get_dualstack_addresses(dyndns_test_ctx, + (struct sockaddr *) &sin, + &addrlist); + assert_int_equal(ret, EOK); + + ret = be_nsupdate_create_fwd_msg(dyndns_test_ctx, NULL, NULL, "bran_stark", + 1234, DYNDNS_REMOVE_A | DYNDNS_REMOVE_AAAA, + addrlist, &msg); + assert_int_equal(ret, EOK); + + assert_string_equal(msg, + "\nupdate delete bran_stark. in A\n" + "send\n" + "update delete bran_stark. in AAAA\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::444\n" + "update add bran_stark. 1234 in AAAA 2001:cdba::555\n" + "send\n"); + talloc_zfree(msg); + + talloc_free(addrlist); + assert_true(check_leaks_pop(dyndns_test_ctx) == true); +} + void dyndns_test_dualstack(void **state) { errno_t ret; @@ -375,7 +635,7 @@ void dyndns_test_dualstack(void **state) } struct sockaddr_in sin; - memset (&sin, 0, sizeof (sin)); + memset(&sin, 0, sizeof (sin)); sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); ret = sss_get_dualstack_addresses(dyndns_test_ctx, @@ -436,7 +696,7 @@ void dyndns_test_dualstack_multiple_addresses(void **state) } struct sockaddr_in sin; - memset (&sin, 0, sizeof (sin)); + memset(&sin, 0, sizeof (sin)); sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr ("192.168.0.2"); ret = sss_get_dualstack_addresses(dyndns_test_ctx, @@ -511,7 +771,7 @@ void dyndns_test_dualstack_no_iface(void **state) will_return_getifaddrs(NULL, NULL, 0); /* sentinel */ struct sockaddr_in sin; - memset (&sin, 0, sizeof (sin)); + memset(&sin, 0, sizeof (sin)); sin.sin_family = AF_INET; sin.sin_addr.s_addr = inet_addr ("192.168.0.3"); ret = sss_get_dualstack_addresses(dyndns_test_ctx, @@ -769,6 +1029,20 @@ int main(int argc, const char *argv[]) cmocka_unit_test_setup_teardown(dyndns_test_dualstack_no_iface, dyndns_test_simple_setup, dyndns_test_teardown), + + /* Messages for nsupdate */ + cmocka_unit_test_setup_teardown(dyndns_test_create_fwd_msg, + dyndns_test_setup, + dyndns_test_teardown), + cmocka_unit_test_setup_teardown(dyndns_test_create_fwd_msg_mult, + dyndns_test_setup, + dyndns_test_teardown), + cmocka_unit_test_setup_teardown(dyndns_test_create_fwd_msg_A, + dyndns_test_setup, + dyndns_test_teardown), + cmocka_unit_test_setup_teardown(dyndns_test_create_fwd_msg_AAAA, + dyndns_test_setup, + dyndns_test_teardown), }; /* Set debug level to invalid value so we can deside if -d 0 was used. */ -- cgit