[PATCH] Warn if ad_server contains IP address

[Justin Stephenson]

First patch, see attached.

This is for easy fix from ticket
https://fedorahosted.org/sssd/ticket/2789

I am going on the assumption that if the first 2 characters of ad_server 
are digits then it is likely an IP address and not hostname. If you have 
a better idea for this please let me know.

Kind regards,
Justin Stephenson

[Sumit Bose]

On Wed, Feb 24, 2016 at 05:19:50PM -0500, Justin Stephenson wrote:
> First patch, see attached.
>
> This is for easy fix from ticket
> https://fedorahosted.org/sssd/ticket/2789
>
> I am going on the assumption that if the first 2 characters of ad_server are
> digits then it is likely an IP address and not hostname. If you have a
> better idea for this please let me know.

I think the check should be more elaborate, IPv6 addreses must not start
with a number and e.g. 123.com is a valid domain name.

You can try to use getaddrinfo() with AI_NUMERICHOST as hint. I would
expect that getaddrinfo() returns an error if the input is not a IPv4 or
IPv6 address. It would be nice if you can wrap the call to getaddrinfo()
in an extra function and write some unit tests for this function. This
way you can easily make sure getaddrinfo() really behaves as I
suspect/hope.

bye,
Sumit
> Kind regards,
> Justin Stephenson
> From 280f7af2e05304fe4eee8a1803abdb72aedad439 Mon Sep 17 00:00:00 2001
> From: Justin Stephenson 
> Date: Wed, 24 Feb 2016 16:48:39 -0500
> Subject: [PATCH] Warn if ad_server is not a hostname due to GSSAPI problems
> Resolves: https://fedorahosted.org/sssd/ticket/2789
>
> src/providers/ad/ad_common.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
> index 4f8223879a504d1e34b39f4166601c53fd6a73fe..c0e1161d89b727a664826b28c59239860a497299 100644
> --- a/src/providers/ad/ad_common.c
> +++ b/src/providers/ad/ad_common.c
> @@ -378,6 +378,14 @@ ad_get_common_options(TALLOC_CTX *mem_ctx,
>  /* Did we get an explicit server name, or are we discovering it? */
>  server = dp_opt_get_string(opts->basic, AD_SERVER);
> /* Provide warning if IP address is used instead of hostname */
> if (isdigit(server[0]) || isdigit(server[1])) {
>    DEBUG(SSSDBG_CONF_SETTINGS,
>          "Warning: ad_server [%s] detected as IP address, "
>                 "this may cause GSSAPI problems!\n", server);
> }
> if (!server) {
>        DEBUG(SSSDBG_CONF_SETTINGS,
>              "No AD server set, will use service discovery!\n");
>
> -- 
> 2.4.3
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Jakub Hrozek]

On Thu, Feb 25, 2016 at 10:53:43AM +0100, Sumit Bose wrote:
> On Wed, Feb 24, 2016 at 05:19:50PM -0500, Justin Stephenson wrote:
> > First patch, see attached.
> >
> > This is for easy fix from ticket
> > https://fedorahosted.org/sssd/ticket/2789
> >
> > I am going on the assumption that if the first 2 characters of ad_server are
> > digits then it is likely an IP address and not hostname. If you have a
> > better idea for this please let me know.
>
> I think the check should be more elaborate, IPv6 addreses must not start
> with a number and e.g. 123.com is a valid domain name.
>
> You can try to use getaddrinfo() with AI_NUMERICHOST as hint. I would
> expect that getaddrinfo() returns an error if the input is not a IPv4 or
> IPv6 address. It would be nice if you can wrap the call to getaddrinfo()
> in an extra function and write some unit tests for this function. This
> way you can easily make sure getaddrinfo() really behaves as I
> suspect/hope.

I think you can just make the resolv_is_address() function of our
resolver public and write some tests.

btw the check would be better done in _ad_servers_init() and
_ipa_servers_init().

[Justin Stephenson]

Thanks a lot for the feedback, I am new to the cmocka framework so I 
will take some time to learn it and respond after amending the patch.

-Justin

On 02/25/2016 05:00 AM, Jakub Hrozek wrote:
> On Thu, Feb 25, 2016 at 10:53:43AM +0100, Sumit Bose wrote:
> > On Wed, Feb 24, 2016 at 05:19:50PM -0500, Justin Stephenson wrote:
> > > First patch, see attached.
> > >
> > > This is for easy fix from ticket
> > > https://fedorahosted.org/sssd/ticket/2789
> > >
> > > I am going on the assumption that if the first 2 characters of ad_server are
> > > digits then it is likely an IP address and not hostname. If you have a
> > > better idea for this please let me know.
> >
> > I think the check should be more elaborate, IPv6 addreses must not start
> > with a number and e.g. 123.com is a valid domain name.
> >
> > You can try to use getaddrinfo() with AI_NUMERICHOST as hint. I would
> > expect that getaddrinfo() returns an error if the input is not a IPv4 or
> > IPv6 address. It would be nice if you can wrap the call to getaddrinfo()
> > in an extra function and write some unit tests for this function. This
> > way you can easily make sure getaddrinfo() really behaves as I
> > suspect/hope.
>
> I think you can just make the resolv_is_address() function of our
> resolver public and write some tests.
>
> btw the check would be better done in _ad_servers_init() and
> _ipa_servers_init().
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Lukas Slebodnik]

On (26/02/16 09:47), Justin Stephenson wrote:
> Thanks a lot for the feedback, I am new to the cmocka framework so I will
> take some time to learn it and respond after amending the patch.

Justin,
do you plan to update the patch?

LS

[Justin Stephenson]

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.

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 am sure there are some corrections to make so I appreciate any
    feedback.

Kind regards,
    Justin Stephenson

On 08/05/2016 11:49 AM, Lukas Slebodnik wrote:
> On (26/02/16 09:47), Justin Stephenson wrote:
> > Thanks a lot for the feedback, I am new to the cmocka framework so I will
> > take some time to learn it and respond after amending the patch.
>
> Justin,
> do you plan to update the patch?
>
> LS

[Jakub Hrozek]

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.

[Justin Stephenson]

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
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Jakub Hrozek]

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
> > [e-mail]
> > https://lists.fedorahosted.org/admin/lists/[e-mail]

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

[Justin Stephenson]

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
> > > [e-mail]
> > > https://lists.fedorahosted.org/admin/lists/[e-mail]
>
> The code itself is good, but I have two more comments:
> 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
> 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
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Jakub Hrozek]

On Fri, Aug 12, 2016 at 10:08:19AM -0400, Justin Stephenson wrote:
> Thanks Jakub for your help, updated patch set attached.

ACK to both, I'll push the patches when CI finishes.

[Jakub Hrozek]

On Tue, Aug 16, 2016 at 05:50:19PM +0200, Jakub Hrozek wrote:
> On Fri, Aug 12, 2016 at 10:08:19AM -0400, Justin Stephenson wrote:
> > Thanks Jakub for your help, updated patch set attached.
>
> ACK to both, I'll push the patches when CI finishes.

CI: http://sssd-ci.duckdns.org/logs/job/51/75/summary.html

* master:
    * 00f3fbb66e882213a78a7ad0a4f9190d0838c830
    * e915f42093add45a11208e871c9abdf7ab2bfbdc