On Wed, Jul 15, 2015 at 07:40:29PM +0200, Pavel Reichl wrote:
On 07/13/2015 12:04 PM, Martin Basti wrote:
>On 10/07/15 20:16, Jakub Hrozek wrote:
>>On Fri, Jul 10, 2015 at 05:52:20PM +0200, Martin Basti wrote:
>>>On 10/07/15 16:55, Jakub Hrozek wrote:
>>>>On Thu, Jul 09, 2015 at 01:17:44PM +0200, Pavel Reichl wrote:
>>>>>While reading #2558 I noticed that there is a further request
>>>>>relating to
>>>>>these patches - mbasti asks If there could be some mean how to
>>>>>send IPs of
>>>>>all interfaces?
>>>>>Is this a good idea in general?
>>>>>I suppose I could implement it with some special value for
>>>>>'dyndns_iface' -
>>>>>ideally the special value would contain characters prohibited to
>>>>>be part of
>>>>>interface name if there are such...
>>>>I would say this looks like a useful feature in general, but can you
>>>>ask
>>>>Martin why he needs this feature? I think the time has come for 1.13
>>>>development to start turning into bug fixing more, so unless there is
>>>>really some particular use-case, please move the ticket into 1.14.
>>>>_______________________________________________
>>>>sssd-devel mailing list
>>>>sssd-devel(a)lists.fedorahosted.org
>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>This is blocker for
https://fedorahosted.org/freeipa/ticket/4249
>>>
>>>Without this patch we cannot add dualstack and multihomed support to
>>>IPA
>>>clients, because when IPA adds all IP and IPv6 addresses to DNS, then
>>>SSSD
>>>just deletes all records and put there just one address.
>>I can see the argument for dualstack -- we shouldn't remove v6 addresses
>>if we're talking to the server over v4 and conversely. I suspect the
>>multi-homed use-case is a bit less important, right?
>Yes multihomed support is less important.
>
>>
>>>We want to support dualstack in IPA for long time. Several users wanted
>>>dualstack, even multihomed support.
>>Is there a corresponding IPA ticket or some link to a freeipa-users
>>thread?
>>
>>Please note I agree with the technical requirement, but there's limited
>>time, so we should prioritize.
>>_______________________________________________
>>sssd-devel mailing list
>>sssd-devel(a)lists.fedorahosted.org
>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>IPv6 IPA tickets:
>
>https://fedorahosted.org/freeipa/query?keywords=~ipv6&status=!closed
>
>Martin^2
>
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello, please see updated patch set, 1st and 2nd patch were not changed
(except for the ticket number in commit message).
I have tested the patches against IPA successfully, patches also work
against AD but I'm seeing some problems in log - I have to investigate this
thoroughly but I believe that the problem is rather miscommunication of AD
then bug in my patches.
Thanks!
This is mostly a code review as Martin Basti already did some testing.
From ad26074e4c569ed32eeb40ae950a48db489d7c5c Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Wed, 8 Jul 2015 09:01:24 -0400
Subject: [PATCH 1/5] DYNDNS: sss_iface_addr_list_get return ENOENT
If none of eligible interfaces matches ifname then ENOENT is returned.
Resolves:
https://fedorahosted.org/sssd/ticket/2549
---
src/providers/dp_dyndns.c | 8 +++++++-
src/providers/ldap/sdap_dyndns.c | 4 ++++
src/tests/cmocka/test_dyndns.c | 20 ++++++++++++++++++++
3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
index 1cac3d0fae2454cea823ed640a4325f27580353f..9552e83d8d4ba615c157c9bae275c8ab867ec274
100644
--- a/src/providers/dp_dyndns.c
+++ b/src/providers/dp_dyndns.c
@@ -222,7 +222,13 @@ sss_iface_addr_list_get(TALLOC_CTX *mem_ctx, const char *ifname,
}
}
- ret = EOK;
+ if (addrlist != NULL) {
+ /* OK, some result was found */
+ ret = EOK;
+ } else {
+ /* No result was found */
+ ret = ENOENT;
I think we should have some non-verbose DEBUG message here.
+ }
*_addrlist = addrlist;
done:
freeifaddrs(ifaces);
diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
index 0d9c9205792062378aa25aad6ac706058001433a..bb98f8e1ec6852b04e9ea3e8d5807fecc7e6958d
100644
--- a/src/providers/ldap/sdap_dyndns.c
+++ b/src/providers/ldap/sdap_dyndns.c
@@ -504,6 +504,10 @@ sdap_dyndns_get_addrs_send(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
"Cannot get list of addresses from interface %s\n", iface);
+ /* non critical failure */
+ if (ret == ENOENT) {
+ ret = EOK;
Can you move this block before the debug message or lower the verbosity
of the debug message (ret == ENOENT ? MINOR_FAILURE : OP_FAILURE)
+ }
}
/* We're done. Just fake an async request completion */
goto done;
diff --git a/src/tests/cmocka/test_dyndns.c b/src/tests/cmocka/test_dyndns.c
index 689e333d4e68c4a2582894d06b8b7b20c76b9be8..3214e90c063ea9a4cf6f6bc6507bf4e37b7d23a4
100644
--- a/src/tests/cmocka/test_dyndns.c
+++ b/src/tests/cmocka/test_dyndns.c
@@ -247,6 +247,23 @@ void dyndns_test_get_multi_ifaddr(void **state)
assert_true(check_leaks_pop(dyndns_test_ctx) == true);
From b8740437b7a1e6fbe2f7221e5b4c4b44dba2d4d4 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Wed, 8 Jul 2015 09:08:03 -0400
Subject: [PATCH 2/5] DYNDNS: support mult. interfaces for dyndns_iface opt
Resolves:
https://fedorahosted.org/sssd/ticket/2549
---
src/man/sssd-ad.5.xml | 7 ++--
src/man/sssd-ipa.5.xml | 7 ++--
src/providers/dp_dyndns.c | 6 ++++
src/providers/dp_dyndns.h | 4 +++
src/providers/ldap/sdap_dyndns.c | 72 +++++++++++++++++++++++++++++++++++-----
5 files changed, 84 insertions(+), 12 deletions(-)
diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
index 938a443e027b9bf83c75c240a7d6b2a0876b92c8..4e2aedb57e3d8e1ce5b674e31c6a3d1fe293d4cb
100644
--- a/src/man/sssd-ad.5.xml
+++ b/src/man/sssd-ad.5.xml
@@ -754,15 +754,18 @@ ad_gpo_map_deny = +my_pam_service
<listitem>
<para>
Optional. Applicable only when dyndns_update
- is true. Choose the interface whose IP address
+ is true. Choose the interfaces whose IP addresses
should be used for dynamic DNS updates.
</para>
<para>
- NOTE: This option currently supports only one interface.
+ NOTE: This option supports multiple interfaces.
I think we can just remove this note. If you like, we can say something
like "Choose the interface or a list of interfaces...".
</para>
<para>
Default: Use the IP address of the AD LDAP connection
</para>
+ <para>
+ Example: dyndns_iface = em1, vnet1, vnet2
+ </para>
</listitem>
</varlistentry>
[...]
--- a/src/providers/ldap/sdap_dyndns.c
+++ b/src/providers/ldap/sdap_dyndns.c
@@ -482,6 +482,65 @@ static void sdap_dyndns_get_addrs_done(struct tevent_req *subreq);
static errno_t sdap_dyndns_add_ldap_conn(struct sdap_dyndns_get_addrs_state *state,
struct sdap_handle *sh);
+static errno_t get_ifaces_addrs(TALLOC_CTX *mem_ctx,
+ const char *iface,
+ struct sss_iface_addr **_result)
+{
+ struct sss_iface_addr *result_addrs = NULL;
+ struct sss_iface_addr *intf_addrs;
+ TALLOC_CTX *tmp_ctx;
+ char **list_of_intfs;
+ int num_of_intfs;
+ errno_t ret;
+ int i;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ ret = ENOENT;
looks like a typo, please use ENOMEM.
+ goto done;
+ }
+
+ ret = split_on_separator(tmp_ctx, iface, ',', true, true,
&list_of_intfs,
+ &num_of_intfs);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "Parsing names of interfaces failed - %d:[%s].\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+
+ for (i = 0; i < num_of_intfs; i++) {
+ ret = sss_iface_addr_list_get(tmp_ctx, list_of_intfs[i], &intf_addrs);
+ if (ret == EOK) {
+ if (result_addrs != NULL) {
+ /* If there is already an existing list, head of this existing
+ * list will be considered as parent talloc context for the
+ * new list.
+ */
+ talloc_steal(result_addrs, intf_addrs);
+ }
+ sss_iface_addr_concatenate(&result_addrs, intf_addrs);
+ } else if (ret == ENOENT) {
+ /* non-critical failure */
non-critical failure should not emit OP failure.
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Cannot get interface %s or there are no addresses "
+ "bind to it.\n", list_of_intfs[i]);
+ } else {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Cannot get list of addresses from interface %s -
%d:[%s]\n",
+ list_of_intfs[i], ret, sss_strerror(ret));
+ goto done;
+ }
+ }
+
+ ret = EOK;
+
+done:
+ *_result = talloc_steal(mem_ctx, result_addrs);
is it correct to touch the return pointer also if ret != OK ?
+ talloc_free(tmp_ctx);
+ return ret;
+}
+
static struct tevent_req *
sdap_dyndns_get_addrs_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
@@ -500,14 +559,11 @@ sdap_dyndns_get_addrs_send(TALLOC_CTX *mem_ctx,
}
if (iface) {
- ret = sss_iface_addr_list_get(state, iface, &state->addresses);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE,
- "Cannot get list of addresses from interface %s\n", iface);
- /* non critical failure */
- if (ret == ENOENT) {
- ret = EOK;
- }
+ ret = get_ifaces_addrs(state, iface, &state->addresses);
+ if (ret != EOK || state->addresses == NULL) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "get_ifaces_addrs() failed: %d:[%s]\n",
+ ret, sss_strerror(ret));
}
/* We're done. Just fake an async request completion */
goto done;
--
2.4.3
From 793de4b7b5762689e011dc220564380cb8094598 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Tue, 14 Jul 2015 04:21:34 -0400
Subject: [PATCH 3/5] DYNDNS: special value '*' for dyndns_iface option
Option dyndns_iface has now special value '*' which implies that IPs
from add interfaces should be sent during DDNS update.
---
src/man/sssd-ad.5.xml | 4 +++-
src/man/sssd-ipa.5.xml | 4 +++-
src/providers/dp_dyndns.c | 17 +++++++++++++----
3 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
index 4e2aedb57e3d8e1ce5b674e31c6a3d1fe293d4cb..f925790a0cff419dfd4c904fdd45fa4fd76a4a25
100644
--- a/src/man/sssd-ad.5.xml
+++ b/src/man/sssd-ad.5.xml
@@ -755,7 +755,9 @@ ad_gpo_map_deny = +my_pam_service
<para>
Optional. Applicable only when dyndns_update
is true. Choose the interfaces whose IP addresses
- should be used for dynamic DNS updates.
+ should be used for dynamic DNS updates. Special
+ value <quote>*</quote> implies that IPs from
all
+ interfaces should be used.
</para>
<para>
NOTE: This option supports multiple interfaces.
diff --git a/src/man/sssd-ipa.5.xml b/src/man/sssd-ipa.5.xml
index ca59ffbb005a6f4129362276ce57faa6a9e1af5c..c484204ceb08e34169de5df18b6a01cb09d44ed6
100644
--- a/src/man/sssd-ipa.5.xml
+++ b/src/man/sssd-ipa.5.xml
@@ -167,7 +167,9 @@
<para>
Optional. Applicable only when dyndns_update
is true. Choose the interfaces whose IP addresses
- should be used for dynamic DNS updates.
+ should be used for dynamic DNS updates. Special
+ value <quote>*</quote> implies that IPs from
all
+ interfaces should be used.
</para>
<para>
NOTE: This option currently supports multiple interfaces.
diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
index dec521054e18e083ba0557dbf9c8bfe918d4575e..a1533387c1fc980713fbc01c25efba2fbaed4a43
100644
--- a/src/providers/dp_dyndns.c
+++ b/src/providers/dp_dyndns.c
@@ -171,6 +171,16 @@ ok_for_dns(struct sockaddr *sa)
return true;
}
+static bool supported_address_family(sa_family_t sa_family)
+{
+ return sa_family == AF_INET || sa_family == AF_INET6;
+}
+
+static bool matching_name(const char *ifname, const char *ifname2)
+{
+ return (strcmp("*", ifname) == 0) || (strcasecmp(ifname, ifname2) == 0);
nitpick - can you #define the asterisk as a constant and add a comment
that it's a special value that means all ifaces?
+}
+
/* Collect IP addresses associated with an interface */
errno_t
sss_iface_addr_list_get(TALLOC_CTX *mem_ctx, const char *ifname,
@@ -200,10 +210,9 @@ sss_iface_addr_list_get(TALLOC_CTX *mem_ctx, const char *ifname,
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, ifname) == 0 &&
- ok_for_dns(ifa->ifa_addr)) {
+ if (supported_address_family(ifa->ifa_addr->sa_family)
+ && matching_name(ifname, ifa->ifa_name)
+ && ok_for_dns(ifa->ifa_addr)) {
thanks, this is more readable.
/* Add this address to the IP address list */
address = talloc_zero(mem_ctx, struct sss_iface_addr);
--
2.4.3
From 24c659db3abd0a9db880ecf8ae040302a4614de6 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Wed, 15 Jul 2015 10:58:38 -0400
Subject: [PATCH 4/5] TESTS: dyndns tests support AAAA addresses
ACK
From 8680c9df4accf269bdc9d2cb6c3440cbb0dc8405 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Tue, 14 Jul 2015 09:56:59 -0400
Subject: [PATCH 5/5] DYNDNS: support for dualstack
When dyndns_iface option was not used, address of connection to LDAP
was used. This patch proposes following change:
* Interface containing address of connection is found.
* All A and AAAA addresses of this interface are collected.
* Collected addresses are sent during DDNS update.
* Function sss_iface_addr_add() is removed.
Resolves:
https://fedorahosted.org/sssd/ticket/2558
---
src/providers/dp_dyndns.c | 150 +++++++++++++++++++++++++++------
src/providers/dp_dyndns.h | 8 +-
src/providers/ldap/sdap_dyndns.c | 20 ++---
src/tests/cmocka/test_dyndns.c | 178 +++++++++++++++++++++++++++++++++++++++
4 files changed, 317 insertions(+), 39 deletions(-)
diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
index a1533387c1fc980713fbc01c25efba2fbaed4a43..4310a7c6c74787eada934e56380375611801245b
100644
--- a/src/providers/dp_dyndns.c
+++ b/src/providers/dp_dyndns.c
@@ -55,31 +55,6 @@ void sss_iface_addr_concatenate(struct sss_iface_addr **list,
DLIST_CONCATENATE((*list), list2, struct sss_iface_addr*);
}
-struct sss_iface_addr *
-sss_iface_addr_add(TALLOC_CTX *mem_ctx, struct sss_iface_addr **list,
- struct sockaddr_storage *ss)
-{
- struct sss_iface_addr *address;
-
- address = talloc(mem_ctx, struct sss_iface_addr);
- if (address == NULL) {
- return NULL;
- }
-
- address->addr = talloc_memdup(address, ss,
- sizeof(struct sockaddr_storage));
- if(address->addr == NULL) {
- talloc_zfree(address);
- return NULL;
- }
-
- /* steal old dlist to the new head */
- talloc_steal(address, *list);
- DLIST_ADD(*list, address);
-
- return address;
-}
-
errno_t
sss_iface_addr_list_as_str_list(TALLOC_CTX *mem_ctx,
struct sss_iface_addr *ifaddr_list,
@@ -1252,3 +1227,128 @@ errno_t be_nsupdate_init_timer(struct be_nsupdate_ctx *ctx,
return ERR_OK;
}
+
+static errno_t match_ip(const struct sockaddr *sa,
+ const struct sockaddr *sb,
+ bool *_res)
Don't you think it would be simpler to just return bool? For cases where
the address family is totally different, can you just return false?
+{
+ size_t addrsize;
+ bool res;
+ errno_t ret;
+ const void *addr_a;
+ const void *addr_b;
+
+ if (sa->sa_family == AF_INET) {
+ addrsize = sizeof(struct in_addr);
+ addr_a = (const void *) &((const struct sockaddr_in *) sa)->sin_addr;
+ addr_b = (const void *) &((const struct sockaddr_in *) sb)->sin_addr;
+ } else if (sa->sa_family == AF_INET6) {
+ addrsize = sizeof(struct in6_addr);
+ addr_a = (const void *) &((const struct sockaddr_in6 *) sa)->sin6_addr;
+ addr_b = (const void *) &((const struct sockaddr_in6 *) sb)->sin6_addr;
Hmm, I was surprised I couldn't find any existing function or macro to
compare a sockaddr or a sockaddr_storage except IN6_ARE_ADDR_EQUAL..
+ } else {
+ ret = EINVAL;
+ goto done;
+ }
+
+ if (sa->sa_family != sb->sa_family) {
+ ret = EOK;
+ res = false;
+ goto done;
+ }
+
+ res = memcmp(addr_a, addr_b, addrsize) == 0;
+ ret = EOK;
+
+done:
+ if (ret == EOK) {
+ *_res = res;
+ }
I think a more idiomatic way is to call this assignment to output
pointer along with setting ret = EOK.
+ return ret;
+}
+
+static errno_t find_iface_by_addr(TALLOC_CTX *mem_ctx,
+ const struct sockaddr *ss,
+ const char **_iface_name)
+{
+ struct ifaddrs *ifaces = NULL;
+ struct ifaddrs *ifa;
+ errno_t ret;
+
+ ret = getifaddrs(&ifaces);
+ if (ret == -1) {
+ ret = errno;
+ DEBUG(SSSDBG_OP_FAILURE,
+ "Could not read interfaces [%d][%s]\n", ret,
sss_strerror(ret));
+ goto done;
+ }
+
+ for (ifa = ifaces; ifa != NULL; ifa = ifa->ifa_next) {
+ bool ip_matched;
+
+ /* Some interfaces don't have an ifa_addr */
+ if (!ifa->ifa_addr) continue;
+
+ ret = match_ip(ss, ifa->ifa_addr, &ip_matched);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "match_ip failed: %d:[%s]\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+ if (ip_matched) {
+ const char *iface_name;
Please put a blank line here. Actually, I'm not thrilled to have the EOK
branch in the middle of the function. Normally, you want to outmost
branch to be the success one...here it's ENOENT..
+ iface_name = talloc_strdup(mem_ctx, ifa->ifa_name);
+ if (iface_name == NULL) {
+ ret = ENOMEM;
+ } else {
+ *_iface_name = iface_name;
+ ret = EOK;
+ }
+ goto done;
+ }
+ }
+ ret = ENOENT;
+
+done:
+ freeifaddrs(ifaces);
+ return ret;
+}
+
+errno_t sss_get_dualstack_addresses(TALLOC_CTX *mem_ctx,
+ struct sockaddr *ss,
+ struct sss_iface_addr **_iface_addrs)
+{
+ struct sss_iface_addr *iface_addrs;
+ const char *iface_name = NULL;
+ TALLOC_CTX *tmp_ctx;
+ errno_t ret;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ ret = find_iface_by_addr(tmp_ctx, ss, &iface_name);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "find_iface_by_addr failed: %d:[%s]\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+
+ ret = sss_iface_addr_list_get(tmp_ctx, iface_name, &iface_addrs);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "sss_iface_addr_list_get failed: %d:[%s]\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+
+done:
+ if (ret == EOK) {
+ *_iface_addrs = talloc_steal(mem_ctx, iface_addrs);
Again, it would be more idiomatic and consistent with the rest of the
code to write:
ret = EOK
*_iface_addrs = <assign>
done:
+ }
+
+ talloc_free(tmp_ctx);
+ return ret;
+}
The rest looks OK to me. I've started Coverity scan.