Re: SRV records resolution failure if Authority section is present

2020-07-29 Thread Willy Tarreau
Patch applied, thanks guys!
Willy



Re: SRV records resolution failure if Authority section is present

2020-07-28 Thread Baptiste
On Tue, Jul 28, 2020 at 2:59 PM Jerome Magnin  wrote:

> Hi,
>
> On Sun, Jul 26, 2020 at 10:41:18PM +0200, Willy Tarreau wrote:
> > Thanks Jérôme,
> >
> > CCing Baptiste for approval (in case we've missed anything, I'm clueless
> > about DNS).
> >
>
> Baptiste just reviewed my patch, made a couple suggestions, so please
> find an update attached to this email.
>

Hi,

Patch approved :)
Thx again Jerome !

Baptiste


Re: SRV records resolution failure if Authority section is present

2020-07-28 Thread Jerome Magnin
Hi,

On Sun, Jul 26, 2020 at 10:41:18PM +0200, Willy Tarreau wrote:
> Thanks Jérôme,
> 
> CCing Baptiste for approval (in case we've missed anything, I'm clueless
> about DNS).
>

Baptiste just reviewed my patch, made a couple suggestions, so please
find an update attached to this email.
>From db0198a29ab493796414033b8fb11661e91d0bee Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Sun, 26 Jul 2020 12:13:12 +0200
Subject: [PATCH] BUG/MAJOR: dns: don't treat Authority records as an error

Support for DNS Service Discovery by means of SRV records was enhanced with
commit 13a9232eb ("MEDIUM: dns: use Additional records from SRV responses")
to use the content of the answers Additional records when present.

If there are Authority records before the Additional records we mistakenly
treat that as an invalid response. To fix this, just ignore the Authority
section if it exist and skip to the Additional records.

As 13a9232eb was introduced during 2.2-dev, it must be backported to 2.2.
This is a fix for issue #778
---
 src/dns.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/src/dns.c b/src/dns.c
index 6a8ab831c..613d7308f 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -1043,6 +1043,35 @@ static int dns_validate_dns_response(unsigned char 
*resp, unsigned char *bufend,
/* now parsing additional records for SRV queries only */
if (dns_query->type != DNS_RTYPE_SRV)
goto skip_parsing_additional_records;
+   
+   /* if we find Authority records, just skip them */
+   for (i = 0; i < dns_p->header.nscount; i++) {
+   offset = 0;
+   len = dns_read_name(resp, bufend, reader, tmpname, 
DNS_MAX_NAME_SIZE,
+   , 0);
+   if (len == 0) 
+   continue;
+   
+
+   if (reader + offset + 10 >= bufend)
+   goto invalid_resp;
+
+   reader += offset;
+   /* skip 2 bytes for class */
+   reader += 2;
+   /* skip 2 bytes for type */
+   reader += 2;
+   /* skip 4 bytes for ttl */
+   reader += 4;
+   /* read data len */
+   len = reader[0] * 256 + reader[1];
+   reader += 2;
+
+   if (reader + len >= bufend)
+   goto invalid_resp;
+
+   reader += len;
+   }
 
nb_saved_records = 0;
for (i = 0; i < dns_p->header.arcount; i++) {
-- 
2.27.0



Re: SRV records resolution failure if Authority section is present

2020-07-26 Thread Willy Tarreau
Thanks Jérôme,

CCing Baptiste for approval (in case we've missed anything, I'm clueless
about DNS).

Willy

On Sun, Jul 26, 2020 at 06:04:38PM +0200, Jerome Magnin wrote:
> Hi Tim,
> 
> On Sun, Jul 26, 2020 at 05:47:00PM +0200, Tim Düsterhus wrote:
> > Jerome,
> > 
> > Regarding the commit message: Please add backporting information to the
> > end of the commit message body (I believe it should be 2.2+).
> 
> You're right the commit I mentionned in the message was indeed
> introduced in 2.2-dev, so this must be backported to 2.2.
> > 
> > Regarding the patch:
> > 
> > +   /* skip 2 bytes for ttl */
> > +   reader += 4;
> > 
> > Either the commit or the code is incorrect here.
> The comment was wrong indeed. Thanks for your review.
> 
> -- 
> Jérôme

> >From 363ed1dd2f3ded7837bbb424eabb309803fc6292 Mon Sep 17 00:00:00 2001
> From: Jerome Magnin 
> Date: Sun, 26 Jul 2020 12:13:12 +0200
> Subject: [PATCH] BUG/MAJOR: dns: don't treat Authority records as an error
> 
> Support for DNS Service Discovery by means of SRV records was enhanced with
> commit 13a9232eb ("MEDIUM: dns: use Additional records from SRV responses")
> to use the content of the answers Additional records when present.
> 
> If there are Authority records before the Additional records we mistakenly
> treat that as an invalid response. To fix this, just ignore the Authority
> section if it exist and skip to the Additional records.
> 
> As 13a9232eb was introduced during 2.2-dev, it must be backported to 2.2.
> ---
>  src/dns.c | 27 +++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/src/dns.c b/src/dns.c
> index 6a8ab831c..7ea35ac11 100644
> --- a/src/dns.c
> +++ b/src/dns.c
> @@ -1044,6 +1044,33 @@ static int dns_validate_dns_response(unsigned char 
> *resp, unsigned char *bufend,
>   if (dns_query->type != DNS_RTYPE_SRV)
>   goto skip_parsing_additional_records;
>  
> + for (i = 0; i < dns_p->header.nscount; i++) {
> + offset = 0;
> + len = dns_read_name(resp, bufend, reader, tmpname, 
> DNS_MAX_NAME_SIZE,
> + , 0);
> + if (len == 0) {
> + continue;
> + }
> +
> + if (reader + offset + 10 >= bufend)
> + goto invalid_resp;
> +
> + reader += offset;
> + /* skip 2 bytes for class */
> + reader += 2;
> + /* skip 2 bytes for type */
> + reader += 2;
> + /* skip 4 bytes for ttl */
> + reader += 4;
> + /* read data len */
> + len = reader[0] * 256 + reader[1];
> + reader += 2;
> +
> + if (reader + len >= bufend)
> + goto invalid_resp;
> +
> + reader += len;
> + }
>   nb_saved_records = 0;
>   for (i = 0; i < dns_p->header.arcount; i++) {
>   if (reader >= bufend)
> -- 
> 2.27.0
> 




Re: SRV records resolution failure if Authority section is present

2020-07-26 Thread Jerome Magnin
Hi Tim,

On Sun, Jul 26, 2020 at 05:47:00PM +0200, Tim Düsterhus wrote:
> Jerome,
> 
> Regarding the commit message: Please add backporting information to the
> end of the commit message body (I believe it should be 2.2+).

You're right the commit I mentionned in the message was indeed
introduced in 2.2-dev, so this must be backported to 2.2.
> 
> Regarding the patch:
> 
> + /* skip 2 bytes for ttl */
> + reader += 4;
> 
> Either the commit or the code is incorrect here.
The comment was wrong indeed. Thanks for your review.

-- 
Jérôme
>From 363ed1dd2f3ded7837bbb424eabb309803fc6292 Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Sun, 26 Jul 2020 12:13:12 +0200
Subject: [PATCH] BUG/MAJOR: dns: don't treat Authority records as an error

Support for DNS Service Discovery by means of SRV records was enhanced with
commit 13a9232eb ("MEDIUM: dns: use Additional records from SRV responses")
to use the content of the answers Additional records when present.

If there are Authority records before the Additional records we mistakenly
treat that as an invalid response. To fix this, just ignore the Authority
section if it exist and skip to the Additional records.

As 13a9232eb was introduced during 2.2-dev, it must be backported to 2.2.
---
 src/dns.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/src/dns.c b/src/dns.c
index 6a8ab831c..7ea35ac11 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -1044,6 +1044,33 @@ static int dns_validate_dns_response(unsigned char 
*resp, unsigned char *bufend,
if (dns_query->type != DNS_RTYPE_SRV)
goto skip_parsing_additional_records;
 
+   for (i = 0; i < dns_p->header.nscount; i++) {
+   offset = 0;
+   len = dns_read_name(resp, bufend, reader, tmpname, 
DNS_MAX_NAME_SIZE,
+   , 0);
+   if (len == 0) {
+   continue;
+   }
+
+   if (reader + offset + 10 >= bufend)
+   goto invalid_resp;
+
+   reader += offset;
+   /* skip 2 bytes for class */
+   reader += 2;
+   /* skip 2 bytes for type */
+   reader += 2;
+   /* skip 4 bytes for ttl */
+   reader += 4;
+   /* read data len */
+   len = reader[0] * 256 + reader[1];
+   reader += 2;
+
+   if (reader + len >= bufend)
+   goto invalid_resp;
+
+   reader += len;
+   }
nb_saved_records = 0;
for (i = 0; i < dns_p->header.arcount; i++) {
if (reader >= bufend)
-- 
2.27.0



Re: SRV records resolution failure if Authority section is present

2020-07-26 Thread Tim Düsterhus
Jerome,

Am 26.07.20 um 17:25 schrieb Jerome Magnin:
> Please find a proper fix attached. We already know if we have entries in
> the Authority section (dns_p->header.nscount > 0), so just skip them
> when they are present and only use the Additional records.

Regarding the commit message: Please add backporting information to the
end of the commit message body (I believe it should be 2.2+).

Regarding the patch:

+   /* skip 2 bytes for ttl */
+   reader += 4;

Either the commit or the code is incorrect here.

Best regards
Tim Düsterhus



SRV records resolution failure if Authority section is present

2020-07-26 Thread Jerome Magnin
On Sun, Jul 26, 2020 at 01:21:45PM +0200, Jerome Magnin wrote:
> as I was trying to reproduce the issue with DNS Service Discovery with
> SRV records reported in issue #775 I encountered a different issue. 
> 
> I am using bind as a dns server, and its answers contain an Authority
> field before the Additional records that can be made use of since
> 13a9232eb ("MEDIUM: dns: use Additional records from SRV responses").
> I found out that haproxy doesn't like that and would just consider the
> whole response invalid, no address being set for my servers.
> 
> I've attached a patch to test for the type and just skip to the next
> record, and it seems to work. I'm not sure it's the proper fix, as we
> might want to test for the presence of an Authority field.
> 
> Note that the same config "works" before that commit. I understand
> haproxy makes a lot more DNS requests before that commit, but what I
> mean is that users migrating from earlier versions to 2.2 can have their
> service break because of this.

Please find a proper fix attached. We already know if we have entries in
the Authority section (dns_p->header.nscount > 0), so just skip them
when they are present and only use the Additional records.

Jérôme
>From f77c279a8a3d40629d00238155b7adf941f1ccb6 Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Sun, 26 Jul 2020 12:13:12 +0200
Subject: [PATCH] BUG/MAJOR: dns: don't treat Authority records as an error

Support for DNS Service Discovery by means of SRV records was enhanced with
commit 13a9232eb ("MEDIUM: dns: use Additional records from SRV responses")
to use the content of the answers Additional records when present.

If there are Authority records before the Additional records we mistakenly
treat that as an invalid response. To fix this, just ignore the Authority
section if it exist and skip to the Additional records.
---
 src/dns.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/src/dns.c b/src/dns.c
index 6a8ab831c..292739b2a 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -1044,6 +1044,31 @@ static int dns_validate_dns_response(unsigned char 
*resp, unsigned char *bufend,
if (dns_query->type != DNS_RTYPE_SRV)
goto skip_parsing_additional_records;
 
+   for (i = 0; i < dns_p->header.nscount; i++) {
+   offset = 0;
+   len = dns_read_name(resp, bufend, reader, tmpname, 
DNS_MAX_NAME_SIZE,
+   , 0);
+   if (len == 0) {
+   continue;
+   }
+
+   if (reader + offset + 10 >= bufend)
+   goto invalid_resp;
+
+   reader += offset;
+   /* skip 2 bytes for class */
+   reader += 2;
+   /* skip 2 bytes for type */
+   reader += 2;
+   /* skip 2 bytes for ttl */
+   reader += 4;
+   /* read data len */
+   len = reader[0] * 256 + reader[1];
+   reader += 2;
+   if (reader + len >= bufend)
+   goto invalid_resp;
+   reader += len;
+   }
nb_saved_records = 0;
for (i = 0; i < dns_p->header.arcount; i++) {
if (reader >= bufend)
-- 
2.27.0