Thanks Jakub for your help, updated patch set attached.
Kind regards,
Justin Stephenson
On 08/12/2016 06:05 AM, Jakub Hrozek wrote:
On Wed, Aug 10, 2016 at 12:09:10PM -0400, Justin Stephenson wrote:
On 08/09/2016 05:41 AM, Jakub Hrozek wrote:
On Fri, Aug 05, 2016 at 12:09:27PM -0400, Justin Stephenson wrote:
Hi Lukas,
I sent a response on July 6th but perhaps there was an issue with the
mailing list or some reason it did not go through.
Yes, we had issues with the mailing list back then (it was a Fedora
mailman bug that was fixed in the meantime)
Updated patch attached.
I moved the resolv_is_address() function declaration into the
async_resolv.h file(so that it could be included in the
cmocka/test_resolv_fake.c test but I am not sure if this is the
correct approach). I also made the assumption of including my tests
in the already existing test_resolv_fake.c file instead of a
different file.
Also, I wasn't sure whether to use SSSDBG_MINOR_FAILURE or
SSSDBG_CONF_SETTINGS debug log level.
I would say SSSDBG_IMPORTANT_INFO, we want to be loud here.
I am sure there are some corrections to make so I appreciate any
feedback.
I haven't tested the patch yet, just read the diff, but the code looks
OK to me. I would just suggest to split the patch into two, one that
makes the resolv function public and adds the test and the other that
uses the function in the providers.
I split the patch into 2 separate patches as requested, see attached.
Testing with ad_server = <ad-ip-address> was successful with the debug
message being printed in the domain log.
Kind regards,
Justin Stephenson
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
The code itself is good, but I have two more comments:
1) the patches are reversed, the first should introduce the new public
functions and only then can the second patch use them. You can use "git
rebase -i origin/master" and just swap the two commit lines
2) I don't like the commit messages :) I would drop the "Part X of of
fix" altogether and use the next sentence as the first line of the
commit message. So for the first patch this would be:
Subject: [PATCH] Make resolv_is_address() function public and create some basic
tests
Resolves:
https://fedorahosted.org/sssd/ticket/2789
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>From 735edf6687dd404dcce2784a7d9ce52d3d9a2022 Mon Sep 17 00:00:00 2001
From: Justin Stephenson <jstep...@redhat.com>
Date: Wed, 10 Aug 2016 11:27:01 -0400
Subject: [PATCH] [PATCH] Warn if IP address is used as option for
ipa_server/ad_server GSSAPI is dependent on DNS with hostnames and we should
warn about this
Resolves:
https://fedorahosted.org/sssd/ticket/2789
---
src/providers/ad/ad_common.c | 9 +++++++++
src/providers/ipa/ipa_common.c | 9 +++++++++
2 files changed, 18 insertions(+)
diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 9f9f9f1bdac2dc47bf18f9727ccd3b4675a2bb06..9a6fece5d8b5b855e4fe48c0f9420761343f4b8a 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -489,6 +489,7 @@ _ad_servers_init(struct ad_service *service,
bool primary)
{
size_t i;
+ size_t j;
errno_t ret = 0;
char **list;
struct ad_server_data *sdata;
@@ -504,6 +505,14 @@ _ad_servers_init(struct ad_service *service,
goto done;
}
+ for (j = 0; list[j]; j++) {
+ if (resolv_is_address(list[j])) {
+ DEBUG(SSSDBG_IMPORTANT_INFO,
+ "ad_server [%s] is detected as IP address, "
+ "this can cause GSSAPI problems\n", list[j]);
+ }
+ }
+
/* Add each of these servers to the failover service */
for (i = 0; list[i]; i++) {
if (be_fo_is_srv_identifier(list[i])) {
diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index b15ccc6ee87002a5ccb2ce8b2c195372f6b5eecd..657994508e0733e86ba474419380a0081c51ee6e 100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -831,6 +831,7 @@ static errno_t _ipa_servers_init(struct be_ctx *ctx,
char *ipa_domain;
int ret = 0;
int i;
+ int j;
tmp_ctx = talloc_new(NULL);
if (!tmp_ctx) {
@@ -844,6 +845,14 @@ static errno_t _ipa_servers_init(struct be_ctx *ctx,
goto done;
}
+ for (j = 0; list[j]; j++) {
+ if (resolv_is_address(list[j])) {
+ DEBUG(SSSDBG_IMPORTANT_INFO,
+ "ipa_server [%s] is detected as IP address, "
+ "this can cause GSSAPI problems\n", list[j]);
+ }
+ }
+
/* now for each one add a new server to the failover service */
for (i = 0; list[i]; i++) {
--
2.7.4
>From 295051b421d2d438ec92d4c98532ed78a0fd1a74 Mon Sep 17 00:00:00 2001
From: Justin Stephenson <jstep...@redhat.com>
Date: Wed, 10 Aug 2016 11:42:28 -0400
Subject: [PATCH] [PATCH] Make resolv_is_address() function public and create
some basic tests
Resolves:
https://fedorahosted.org/sssd/ticket/2789
---
src/resolv/async_resolv.c | 4 +---
src/resolv/async_resolv.h | 2 ++
src/tests/cmocka/test_resolv_fake.c | 25 +++++++++++++++++++++++++
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/src/resolv/async_resolv.c b/src/resolv/async_resolv.c
index 58d5c6e550bb34cbaa50517323133fad4f900980..e8595567708d1b7fb26d6d4e8dc569631c27f560 100644
--- a/src/resolv/async_resolv.c
+++ b/src/resolv/async_resolv.c
@@ -1062,8 +1062,6 @@ resolv_gethostbyname_address(TALLOC_CTX *mem_ctx, const char *address,
struct resolv_hostent **_rhostent);
static inline int
resolv_gethostbyname_family_init(enum restrict_family family_order);
-static bool
-resolv_is_address(const char *name);
static errno_t
resolv_gethostbyname_step(struct tevent_req *req);
@@ -1133,7 +1131,7 @@ fail:
return NULL;
}
-static bool
+bool
resolv_is_address(const char *name)
{
struct addrinfo hints;
diff --git a/src/resolv/async_resolv.h b/src/resolv/async_resolv.h
index 876abff14e6e22c67585a476a7169423281f01c7..b602a425c21b5f3bfd0098ce3208f6750d4ed1ad 100644
--- a/src/resolv/async_resolv.h
+++ b/src/resolv/async_resolv.h
@@ -189,4 +189,6 @@ errno_t resolv_discover_srv_recv(TALLOC_CTX *mem_ctx,
uint32_t *_ttl,
char **_dns_domain);
+bool
+resolv_is_address(const char *name);
#endif /* __ASYNC_RESOLV_H__ */
diff --git a/src/tests/cmocka/test_resolv_fake.c b/src/tests/cmocka/test_resolv_fake.c
index a1e9ee4cba9b17d506eeae7e6088de28c301364d..bd0d77fd6afd05e005af480a4c2a91da4dbdac33 100644
--- a/src/tests/cmocka/test_resolv_fake.c
+++ b/src/tests/cmocka/test_resolv_fake.c
@@ -32,6 +32,7 @@
#include <resolv.h>
+#include "resolv/async_resolv.h"
#include "tests/cmocka/common_mock.h"
#include "tests/cmocka/common_mock_resp.h"
@@ -333,6 +334,29 @@ void test_resolv_fake_srv(void **state)
assert_int_equal(ret, ERR_OK);
}
+void test_resolv_is_address(void **state)
+{
+ bool ret;
+
+ ret = resolv_is_address("10.192.211.37");
+ assert_true(ret);
+
+ ret = resolv_is_address("127.0.0.1");
+ assert_true(ret);
+
+ ret = resolv_is_address("2001:0db8:85a3:0000:0000:8a2e:0370:7334");
+ assert_true(ret);
+
+ ret = resolv_is_address("sssd.ldap.com");
+ assert_false(ret);
+
+ ret = resolv_is_address("testhostname");
+ assert_false(ret);
+
+ ret = resolv_is_address("localhost");
+ assert_false(ret);
+}
+
int main(int argc, const char *argv[])
{
int rv;
@@ -348,6 +372,7 @@ int main(int argc, const char *argv[])
cmocka_unit_test_setup_teardown(test_resolv_fake_srv,
test_resolv_fake_setup,
test_resolv_fake_teardown),
+ cmocka_unit_test(test_resolv_is_address),
};
/* Set debug level to invalid value so we can deside if -d 0 was used. */
--
2.7.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
