Re: [PATCH] BUG/MINOR: init: enforce strict-limits when using master-worker

2021-01-13 Thread Jerome Magnin
On Wed, Jan 13, 2021 at 11:34:07AM +0100, William Dauchy wrote:
> On Wed, Jan 13, 2021 at 11:06 AM William Dauchy  wrote:
> I spend some time on it, trying to find a good explanation but it
> seems like I truly screw up. I probably overlooked the master worker
> test. So at the end forget my comment on the commit message, it seems
> like it never worked in master worker mode.
> 
> Sorry for that!

no worries, I've updated the commit message to mention your review.
thanks for your time reviewing the issue.
regards,
-- 
Jérôme
>From ca260ac46cd441ed4108cdef7b304b6c0baec68c Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Tue, 12 Jan 2021 20:19:38 +0100
Subject: [PATCH] BUG/MINOR: init: enforce strict-limits when using
 master-worker

The strict-limits global option was introduced with commit 0fec3ab7b
("MINOR: init: always fail when setrlimit fails"). When used in
conjuction with master-worker, haproxy will not fail when a setrlimit
fails. This happens because we only exit() if master-worker isn't used.

This patch removes all tests for master-worker mode for all cases covered
by strict-limits scope.

This should be backported from 2.1 onward.
This should fix issue #1042.

Reviewed by William Dauchy 
---
 src/haproxy.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index fcc4f6c70..7612e4c45 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -3111,8 +3111,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Cannot raise FD limit to 
%d, limit is %d.\n",
 argv[0], global.rlimit_nofile, 
(int)limit.rlim_cur);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else {
/* try to set it to the max possible at least */
@@ -3135,8 +3134,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
 argv[0], global.rlimit_memmax);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_warning("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
@@ -3147,8 +3145,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
 argv[0], global.rlimit_memmax);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_warning("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
@@ -3320,8 +3317,7 @@ int main(int argc, char **argv)
 "Please raise 'ulimit-n' to %d or more to 
avoid any trouble.\n",
 argv[0], (int)limit.rlim_cur, global.maxconn, 
global.maxsock,
 global.maxsock);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_alert("[%s.main()] FD limit (%d) too low for 
maxconn=%d/maxsock=%d. "
@@ -3608,8 +3604,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Failed to set the raise 
the maximum "
 "file size.\n", argv[0]);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_warning("[%s.main()] Failed to set the raise 
the maximum "
@@ -3622,8 +3617,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Failed to set the raise 
the core "
 "dump size.\n", argv[0]);
-   if (!(global.mode & MODE_MWORKER))
-   

Re: [PATCH] BUG/MINOR: init: enforce strict-limits when using master-worker

2021-01-13 Thread Jerome Magnin
Hi William,

On Wed, Jan 13, 2021 at 08:57:47AM +0100, William Dauchy wrote:
> On Tue, Jan 12, 2021 at 08:36:57PM +0100, Jerome Magnin wrote:
> > From ca260ac46cd441ed4108cdef7b304b6c0baec68c Mon Sep 17 00:00:00 2001
> > From: Jerome Magnin 
> > Date: Tue, 12 Jan 2021 20:19:38 +0100
> > Subject: [PATCH] BUG/MINOR: init: enforce strict-limits when using
> >  master-worker
> > 
> > The strict-limits global option was introduced with commit 0fec3ab7b
> > ("MINOR: init: always fail when setrlimit fails"). When used in
> > conjuction with master-worker, haproxy will not fail when a setrlimit
> > fails. This happens because we only exit() if master-worker isn't used.
> > 
> > This patch removes all tests for master-worker mode for all cases covered
> > by strict-limits scope.
> > 
> > This should be backported from 2.1 onward.
> > This should fix issue #1042.
> 
> just for the context we discussed in the issue:
> 
> this feature was originially implemented in a reload scenario where it
> made the reload fail, without leaving the master worker.
> I completely overlooked the start and I agree this patch is more aligned
> with what the documentation is stating.
> 
> Could be nice to have this context in the commit message.
> Outside of that I'm ok with the patch itself.
> 
> Reviewed by William Dauchy 
> 

Apologies for forgetting about adding this in my commit message. Out of
curiosity which use case is currently covered for you ? I did most of my
testing on the number of FD and wasn't able to confirm that only startup
is broken, and reload behaves as expected. Either way I first get
alerted, then warned, and I have a new haproxy process with not so
strict limits.

-- 
Jérôme



[PATCH] BUG/MINOR: init: enforce strict-limits when using master-worker

2021-01-12 Thread Jerome Magnin
Hi William, list,

This is a patch for issue 1042.
I removed all tests for master-worker mode for everything related to
strict-limits.

regards,
-- 
Jérôme
>From ca260ac46cd441ed4108cdef7b304b6c0baec68c Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Tue, 12 Jan 2021 20:19:38 +0100
Subject: [PATCH] BUG/MINOR: init: enforce strict-limits when using
 master-worker

The strict-limits global option was introduced with commit 0fec3ab7b
("MINOR: init: always fail when setrlimit fails"). When used in
conjuction with master-worker, haproxy will not fail when a setrlimit
fails. This happens because we only exit() if master-worker isn't used.

This patch removes all tests for master-worker mode for all cases covered
by strict-limits scope.

This should be backported from 2.1 onward.
This should fix issue #1042.
---
 src/haproxy.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/haproxy.c b/src/haproxy.c
index fcc4f6c70..7612e4c45 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -3111,8 +3111,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Cannot raise FD limit to 
%d, limit is %d.\n",
 argv[0], global.rlimit_nofile, 
(int)limit.rlim_cur);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else {
/* try to set it to the max possible at least */
@@ -3135,8 +3134,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
 argv[0], global.rlimit_memmax);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_warning("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
@@ -3147,8 +3145,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
 argv[0], global.rlimit_memmax);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_warning("[%s.main()] Cannot fix MEM limit to 
%d megs.\n",
@@ -3320,8 +3317,7 @@ int main(int argc, char **argv)
 "Please raise 'ulimit-n' to %d or more to 
avoid any trouble.\n",
 argv[0], (int)limit.rlim_cur, global.maxconn, 
global.maxsock,
 global.maxsock);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_alert("[%s.main()] FD limit (%d) too low for 
maxconn=%d/maxsock=%d. "
@@ -3608,8 +3604,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Failed to set the raise 
the maximum "
 "file size.\n", argv[0]);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_warning("[%s.main()] Failed to set the raise 
the maximum "
@@ -3622,8 +3617,7 @@ int main(int argc, char **argv)
if (global.tune.options & GTUNE_STRICT_LIMITS) {
ha_alert("[%s.main()] Failed to set the raise 
the core "
 "dump size.\n", argv[0]);
-   if (!(global.mode & MODE_MWORKER))
-   exit(1);
+   exit(1);
}
else
ha_warning("[%s.main()] Failed to set the raise 
the core "
-- 
2.30.0



[PATCH] DOC: ssl-load-extra-files only applies to certificates on bind lines.

2020-09-07 Thread Jerome Magnin
Hi,

this is a small doc patch for ssl-load-extra-files.
I will create a feature request to support separating the key from the
certificate when used on server lines, as discussed privately with
William.

-- 
Jérôme
>From 01cfd0dcd2f7efbb90a25bd2f72053bdbd5f559c Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Mon, 7 Sep 2020 11:55:57 +0200
Subject: [PATCH] DOC: ssl-load-extra-files only applies to certificates on
 bind lines.

Be explicit about ssl-load-extra-files not applying to certificates
referenced with the crt keyword on server lines.
---
 doc/configuration.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index a8242793a..c1f6f8219 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1373,7 +1373,8 @@ ssl-dh-param-file 
 
 ssl-load-extra-files *
   This setting alters the way HAProxy will look for unspecified files during
-  the loading of the SSL certificates.
+  the loading of the SSL certificates associated to "bind" lines. It does not
+  apply to certificates used for client authentication on "server" lines.
 
   By default, HAProxy discovers automatically a lot of files not specified in
   the configuration, and you may want to disable this behavior if you want to
-- 
2.28.0



Re: Is the "source" keyword supported on FreeBSD?

2020-08-12 Thread Jerome Magnin
Hi Frank,

On Wed, Aug 12, 2020 at 11:50:05AM +0200, Frank Wall wrote:
> Hi,
> 
> this *feels* like a silly question and I may have missed something
> pretty obvious, but... I've tried to use the "source" keyword and
> it doesn't work. HAProxy does not use the specified IP address when
> connecting to the server.
> 
> Is this keyword supposed to work on FreeBSD or are there any known caveats?

Yes it is supposed to work on FreeBSD. The only caveat I know is that
you must use ipfw if you want to do "full transparent proxy" mode, which
mean using the client IP addresses to establish connections on the
backend side, because divert-reply is not available in FreeBSD's pf but
this is not what you are trying to do.

Can you tell us how it does not work ? Do you have any error message ?

> Below is my HAProxy config. I've tried both, adding the keyword on the
> "server" line and just adding it to a backend section, either way it
> does not work. The config currently contains both variants for
> demonstration purposes.

That config should work as long as 192.168.77.20 is assigned to your
FreeBSD host. If it isn't you should see haproxy complain about not
being able to bind a source address for a connect().

-- 
Jérôme



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



[PATCH] BUG/MAJOR: dns: fix null pointer dereference in snr_update_srv_status

2020-07-28 Thread Jerome Magnin
Hi,

this is a patch for issue #775.

-- 
Jérôme
>From 68e8b71c50d0805faf5facba587f1c8c3f1760b7 Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Tue, 28 Jul 2020 13:38:22 +0200
Subject: [PATCH] BUG/MAJOR: dns: fix null pointer dereference in
 snr_update_srv_status

Since commit 13a9232eb ("MEDIUM: dns: use Additional records from SRV
responses"), a struct server can have a NULL dns_requester->resolution,
when SRV records are used and DNS answers contain an Additional section.

This is a problem when we call snr_update_srv_status() because it does
not check that resolution is NULL, and dereferences it. This patch
simply adds a test for resolution being NULL. When that happens, it means
we are using SRV records with Additional records, and an entry was removed.

This should fix issue #775.
This should be backported to 2.2.
---
 src/server.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/server.c b/src/server.c
index a622e22bd..918294b2f 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3731,6 +3731,15 @@ int snr_update_srv_status(struct server *s, int 
has_no_ip)
struct dns_resolution *resolution = s->dns_requester->resolution;
int exp;
 
+   /* If resolution is NULL we're dealing with SRV records Additional 
records */
+   if (resolution == NULL) {
+   if (s->next_admin & SRV_ADMF_RMAINT)
+   return 1;
+
+   srv_set_admin_flag(s, SRV_ADMF_RMAINT, "entry removed from SRV 
record");
+   return 0;
+   }
+
switch (resolution->status) {
case RSLV_STATUS_NONE:
/* status when HAProxy has just (re)started.
-- 
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



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



haproxy@formilux.org

2020-07-26 Thread Jerome Magnin
Hi,

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.
-- 
Jérôme
>From 9637655e5ee0d4d51056cbdb948f4c2b1da272e4 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, we can skip to the next
record when the type is DNS_RTYPE_NS for the current record.
---
 include/haproxy/dns-t.h | 1 +
 src/dns.c   | 6 ++
 2 files changed, 7 insertions(+)

diff --git a/include/haproxy/dns-t.h b/include/haproxy/dns-t.h
index b1b43976c..1a8e6e49f 100644
--- a/include/haproxy/dns-t.h
+++ b/include/haproxy/dns-t.h
@@ -71,6 +71,7 @@ extern struct pool_head *dns_requester_pool;
 
 /* dns record types (non exhaustive list) */
 #define DNS_RTYPE_A 1   /* IPv4 address */
+#define DNS_RTYPE_NS2   /* NS record type */
 #define DNS_RTYPE_CNAME 5   /* canonical name */
 #define DNS_RTYPE_  28  /* IPv6 address */
 #define DNS_RTYPE_SRV   33  /* SRV record */
diff --git a/src/dns.c b/src/dns.c
index 6a8ab831c..b840bf9de 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -1076,6 +1076,12 @@ static int dns_validate_dns_response(unsigned char 
*resp, unsigned char *bufend,
dns_answer_record->type = reader[0] * 256 + reader[1];
reader += 2;
 
+   /* skip if record type is NS */
+   if (dns_answer_record->type == DNS_RTYPE_NS) {
+pool_free(dns_answer_item_pool, dns_answer_record);
+dns_answer_record = NULL;
+   continue;
+   }
/* 2 bytes for class (2) */
if (reader + 2 > bufend)
goto invalid_resp;
-- 
2.27.0



Re: [ANNOUNCE] haproxy-2.0.16

2020-07-18 Thread Jerome Magnin
Hi Dmitry

On Sat, Jul 18, 2020 at 12:29:10PM +0300, Dmitry Sivachenko wrote:
> 
> 1) new warnings:
> 
> src/log.c:1692:10: warning: logical not is only applied to the left hand side 
> of this comparison [-Wlogical-not-parentheses]
> while (HA_SPIN_TRYLOCK(LOGSRV_LOCK, >lock) != 0) {
>^   ~~
> include/common/hathreads.h:1026:33: note: expanded from macro 
> 'HA_SPIN_TRYLOCK'
> #define HA_SPIN_TRYLOCK(lbl, l) !pl_try_s(l)
> ^
> src/log.c:1692:10: note: add parentheses after the '!' to evaluate the 
> comparison first
> include/common/hathreads.h:1026:33: note: expanded from macro 
> 'HA_SPIN_TRYLOCK'
> #define HA_SPIN_TRYLOCK(lbl, l) !pl_try_s(l)
> ^
> src/log.c:1692:10: note: add parentheses around left hand side expression to 
> silence this warning
> while (HA_SPIN_TRYLOCK(LOGSRV_LOCK, >lock) != 0) {
>^
>(  )
> include/common/hathreads.h:1026:33: note: expanded from macro 
> 'HA_SPIN_TRYLOCK'
> #define HA_SPIN_TRYLOCK(lbl, l) !pl_try_s(l)
> ^
>

This looks like a missing backport because the issue was encountered and
fixed in master.
https://github.com/haproxy/haproxy/commit/db57a142c31016ff3e0dd533cb2b4de14445781e

-- 
Jérôme



Re: Log levels when logging to stdout

2020-07-16 Thread Jerome Magnin
Hi Martin,

On Thu, Jul 16, 2020 at 10:05:40AM +0300, Martin Grigorov wrote:
> 
> I am using such logging configuration (HAProxy built from master branch):
> 
> global
>   log stdout format raw local0 err
>   ...
> defaults
>   log global
>   option dontlog-normal
>   option httplog
>   option dontlognull
>   ...
> 
> But HAProxy still logs entries like the following:
> 
> 0001:test_fe.clireq[001c:]: GET /haproxy-load HTTP/1.1
> 0002:test_fe.clihdr[001e:]: host: 192.168.0.206:8080
> 0006:test_fe.accept(0004)=0020 from [:::192.168.0.72:46768]
> ALPN=
> 0001:test_fe.clihdr[001c:]: host: 192.168.0.206:8080
> 0002:test_fe.clihdr[001e:]: content-type: application/json
> 0004:test_fe.accept(0004)=0024 from [:::192.168.0.72:46754]
> ALPN=
> 0005:test_fe.accept(0004)=001f from [:::192.168.0.72:46766]
> ALPN=
> 0001:test_fe.clihdr[001c:]: content-type: application/json
> 0007:test_fe.accept(0004)=0025 from [:::192.168.0.72:46776]
> ALPN=
> 0003:test_fe.clireq[0022:]: GET /haproxy-load HTTP/1.1
> 0008:test_fe.accept(0004)=001d from [:::192.168.0.72:46756]
> ALPN=
> 0004:test_fe.clireq[0024:]: GET /haproxy-load HTTP/1.1
> 0003:test_fe.clihdr[0022:]: host: 192.168.0.206:8080
> 0004:test_fe.clihdr[0024:]: host: 192.168.0.206:8080
> 
> Those do not look like errors but in any case I tried also with 'emerg' log
> level instead of 'err'
>  and this didn't change anything.

This is haproxy debug output, available when you start with haproxy -d.

> 
> Do I configure it in a wrong way ?
> I want HAProxy to log only when there is a problem. Because now it logs few
> GBs of those when I load it and this affects the performance.
> https://medium.com/@martin.grigorov/hi-willy-476dee6439d3

How do you start haproxy ?

-- 
Jérôme



Re: HTTP/2 in 2.1.x behaves different than in 2.0.x

2020-07-03 Thread Jerome Magnin
Hi Christian,

On Fri, Jul 03, 2020 at 11:02:48AM +0200, Christian Ruppert wrote:
> Hi List,
> 
> we've just noticed and confirmed some strange change in behavior, depending
> on whether the request is made with HTTP 1.x or 2.x.
> [...] 
> That also affects ACLs like url*/path* and probably others.
> I don't think that is intended, isn't it?
> That looks like a regression to me. If that is a bug/regression, than it
> might be good if it's possible to catch that one via test case (regtest).
>

This change is intentional and not a regression, it was introduced by
this commit:
http://git.haproxy.org/?p=haproxy.git;a=commit;h=30ee1efe676e8264af16bab833c621d60a72a4d7

-- 
Jérôme



Re: Ubuntu 20.04 + TLSv1

2020-06-12 Thread Jerome Magnin
On Fri, Jun 12, 2020 at 03:09:18PM +0200, bjun...@gmail.com wrote:
> Hi,
> 
> currently i'm testing Ubuntu 20.04 and HAProxy 2.0.14.
> 
> I'm trying to get TLSv1 working (we need this for some legacy clients), so
> far without success.
> 
> I've read different things, on the one hand Ubuntu has removed
> TLSv1/TLSv1.1 support completely, otherwise that it can be enabled:
> http://changelogs.ubuntu.com/changelogs/pool/main/o/openssl/openssl_1.1.1f-1ubuntu2/changelog
> 
> 
> Is there anything that can be set in HAProxy? (apart from
> "ssl-default-bind-options ssl-min-ver TLSv1.0 ssl-max-ver TLSv1.2")
> 
> Has anybody more information on this matter or has TLSv1 working in Ubuntu
> 20.04 + HAProxy?
>

Hi,

appending @SECLEVEL=1 to the cipher string I can perform the handshakes
using TLSv1.0 and higher on ubuntu 20.04. You don't need to rebuild
openssl. I was not able to use s_client -tls1 or -tls1_2 on the 20.04
though, had to try with a different client. It's probably something that
you can handle with openssl.cnf, just like the ciphers.

frontend in
  bind *:8443 ssl crt ssl.pem ssl-min-ver TLSv1.0  ciphers ALL:@SECLEVEL=1


-- 
Jérôme



Re: missing backports in haproxy-1.8

2020-06-12 Thread Jerome Magnin
On Fri, Jun 12, 2020 at 11:10:08AM +0200, William Lallemand wrote:
> I pushed them in the 1.8 git. I couldn't reproduce the issue though,
> which compiler do you use?
>
I ran into the issue with gcc 10.1.0. Thanks for the backports!

Jérôme



Re: missing backports in haproxy-1.8

2020-06-11 Thread Jerome Magnin
On Thu, Jun 11, 2020 at 07:27:26PM +0200, William Lallemand wrote:
> On Thu, Jun 11, 2020 at 12:41:51PM +0200, Jerome Magnin wrote:
> > 72d9f3351 BUILD: chunk: properly declare pool_head_trash as extern
> > 2231b6388 BUILD: cache: avoid a build warning with some compilers/linkers
> > 
> The 1.8 didn't receive any new backports since the 1.8.25 release, but I
> take notes of these two, thanks!
> 
Hi William,

I thought they might have been forgotten because they are supposed to be
backported as far as 1.9 as per commit message, but I needed them to
build 1.8 today.

Jérôme



missing backports in haproxy-1.8

2020-06-11 Thread Jerome Magnin
Hi list,

haproxy-1.8 is missing two backports, and can't be built with recent gcc
as a result.
72d9f3351 BUILD: chunk: properly declare pool_head_trash as extern
2231b6388 BUILD: cache: avoid a build warning with some compilers/linkers

regards,
Jérôme



[PATCH] DOC: retry-on can only be used with mode http

2020-05-13 Thread Jerome Magnin
Hi,

with github issue #627 we've had at least one report of someone using retry-on
with mode tcp. Olivier fixed the crash, and I propose the attached patch to
better document that retry-on is only valid when used with mode http and
ignored otherwise.

Jérôme
>From e030ea97758cc8b6af5f655637137230e9a1791f Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Wed, 13 May 2020 20:09:57 +0200
Subject: [PATCH] DOC: retry-on can only be used with mode http

The documentation for retry-on hints at it being meant to be used
in conjuction with mode http, but since we've a had bug report
involving mode tcp and retry-on, lets make it explicit in the
documentation that it only works with mode http and will be
ignored otherwise.
---
 doc/configuration.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 7594992b4..3071c655d 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -8749,7 +8749,9 @@ retries 
 
 
 retry-on [list of keywords]
-  Specify when to attempt to automatically retry a failed request
+  Specify when to attempt to automatically retry a failed request.
+  This setting is only valid when "mode" is set to http and is silently ignored
+  otherwise.
   May be used in sections:defaults | frontend | listen | backend
  yes   |no|   yes  |   yes
   Arguments :
-- 
2.26.2



Re: [PATCH 1/3] BUG/MINOR: pollers: remove uneeded free in global init

2020-05-13 Thread Jerome Magnin
Hi Willy,

On Wed, May 13, 2020 at 03:52:54PM +0200, Willy Tarreau wrote:
> Hi Jérôme,
> [...] 
> Ah crap! I didn't notice this part which didn't appear in the context
> of the patch. I didn't notice we still had a few such labels in very
> old files. Do you mind if instead I edit your patch to completely remove
> the line ?

I don't mind at all.

Jérôme



Re: [PATCH 1/3] BUG/MINOR: pollers: remove uneeded free in global init

2020-05-13 Thread Jerome Magnin
Hi,

On Wed, May 13, 2020 at 11:46:50AM +0200, Willy Tarreau wrote:
> On Mon, May 11, 2020 at 03:20:03PM +0200, William Dauchy wrote:
> > Since commit d4604adeaa8c ("MAJOR: threads/fd: Make fd stuffs
> > thread-safe"), we init pollers per thread using a helper. It was still
> > correct for mono-thread mode until commit cd7879adc2c4 ("BUG/MEDIUM:
> > threads: Run the poll loop on the main thread too"). We now use a deinit
> > helper for all threads, making those free uneeded.
> > 
> > Only poll and select are affected by this very minor issue.
> > 
> > it could be backported from v1.8 to v2.1.
> > 
> > Fixes: cd7879adc2c4 ("BUG/MEDIUM: threads: Run the poll loop on the main
> > thread too")
> > Signed-off-by: William Dauchy 
> 
> Applied, thank you William.
> 

This one breaks clang builds because it removes the fail_revt label but it is
still declared as a local label, and clang errors on it.

Please find a patch attached.

Jérôme
>From 7549f1648f4e32ded652eabc07cd1dd7f0e7f38f Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Wed, 13 May 2020 15:11:02 +0200
Subject: [PATCH] BUILD: select: only declare existing local labels to appease
 clang

Commit 42a50bd19 ("BUG/MINOR: pollers: remove uneeded free in global
init") removed the 'fail_revt' label from the _do_init() function
in src/ev_select.c but left the local label declaration, which makes
clang unhappy and unable to build.

This should be backported where 42a50bd19 is backported.
---
 src/ev_select.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/ev_select.c b/src/ev_select.c
index d02669f0f..8bf712585 100644
--- a/src/ev_select.c
+++ b/src/ev_select.c
@@ -253,7 +253,7 @@ static void deinit_select_per_thread()
  */
 static int _do_init(struct poller *p)
 {
-   __label__ fail_swevt, fail_srevt, fail_revt;
+   __label__ fail_swevt, fail_srevt;
int fd_set_bytes;
 
p->private = NULL;
-- 
2.26.2



Re: [PATCH] DOC: give a more accuration description of what check does

2020-04-28 Thread Jerome Magnin
Hi Willy,

On Tue, Apr 28, 2020 at 05:46:08PM +0200, Willy Tarreau wrote:
> 
> Thanks for this, however I don't completely agree with the wording
> there (but I do agree that the current one is totally outdated).
> 
> The main point is that users don't configure "handshakes", they
> rather define transport protocols (typically SSL) so in the end
> this is not necessarily clearer this way and may even lead to new
> confusion regarding ssl vs check-ssl for example.
> 
> Probably that it would be better to take this opportunity to
> entirely revisit the description of the "check" keyword to cover
> the following points:

Thanks for the feedback. I've attached an updated patch to this email.

> 
>   - by default, checks are performed on the same address and port as
> configured on the server, using the same encapsulation parameters
> (SSL/TLS, SNI, proxy-protocol header etc).  If "port" or "addr"
> directives are set, it is assumed that the server isn't checked on
> the traffic port but on a proxy port that possibly uses a different
> service. In this case, connection headers such as the proxy protocol
> are only sent if "check-send-proxy" is set; SSL/TLS is only used if
> "check-ssl" is used, and similarly the SNI used on the connection
> will be defined by "check-sni". See also "check-ssl", "check-sni" etc.

This part is not entirely accurate in my experience:
  - SNI configured with "sni" on server lines is not used with checks. One must
set "check-sni", if only because it's almost always 'sni ssl_fc_sni' or 
'sni hdr(host)' which has no meaning in the context of checks. I don't think
there are ways around this. 
  - the "alpn" setting of a server line is also not used for checks, one must
define it with check-alpn. This will probably change soon now that haproxy
can do h2 checks natively.

Jérôme
>From 6a8e8ecfa1f6c9e8a4d1a5000296f650b45d9bdc Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Sun, 26 Apr 2020 14:23:04 +0200
Subject: [PATCH] DOC: give a more accurate description of what check does

The documentation for check implies that without an application
level check configured, it only enables simple tcp checks. What it
actually does is verify that the configured transport layer is available,
and that optional application level checks succeed.
---
 doc/configuration.txt | 61 ++-
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 3853d406f..12582d9f2 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -12520,18 +12520,55 @@ ca-file 
   server's certificate.
 
 check
-  This option enables health checks on the server. By default, a server is
-  always considered available. If "check" is set, the server is available when
-  accepting periodic TCP connections, to ensure that it is really able to serve
-  requests. The default address and port to send the tests to are those of the
-  server, and the default source is the same as the one defined in the
-  backend. It is possible to change the address using the "addr" parameter, the
-  port using the "port" parameter, the source address using the "source"
-  address, and the interval and timers using the "inter", "rise" and "fall"
-  parameters. The request method is define in the backend using the "httpchk",
-  "smtpchk", "mysql-check", "pgsql-check" and "ssl-hello-chk" options. Please
-  refer to those options and parameters for more information. See also
-  "no-check" option.
+  This option enables health checks on a server:
+- when not set, no health checking is performed, and the server is always
+  considered available.
+- when set and no other check method is configured, the server is 
considered
+  available when a connection can be established at the highest configured
+  transport layer. This means TCP by default, or SSL/TLS when "ssl" or
+  "check-ssl" are set, both possibly combined with connection prefixes such
+  as a PROXY protocol header when "send-proxy" or "check-send-proxy" are
+  set.
+- when set and an application-level health check is defined, the
+  application-level exchanges are performed on top of the configured
+  transport layer and the server is considered available if all of the
+  exchanges succeed.
+
+  By default, health checks are performed on the same address and port as
+  configured on the server, using the same encapsulation parameters (SSL/TLS,
+  proxy-protocol header, etc... ). It is possible to change the destination
+  address using "addr

[PATCH] DOC: give a more accuration description of what check does

2020-04-26 Thread Jerome Magnin
Hi,

here's a documentation patch for the check keyword.

regards,
Jérôme
>From 10e90939d9fd1bd4f1e651d679d0b99e8da91afb Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Sun, 26 Apr 2020 14:23:04 +0200
Subject: [PATCH] DOC: give a more accurate description of what check does

The documentation for check implies that without an optional l7
check configured, it enables simple tcp checks. What it actually
does is check that every configured handshake on the server line
can be established.

# simple tcp connect
backend foo
  server s1 192.168.0.1:80 check
# this does a tcp connect + tls handshake
backend foo
  server s1 192.168.0.1;443 ssl check
# simple tcp connect is enough for check success
backend foo
  option tcp-check
  tcp-check connect
  server s1 192.168.0.1:443 ssl check
---
 doc/configuration.txt | 38 +++---
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 676d5a075..faf5a54bc 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -12112,17 +12112,33 @@ ca-file 
 
 check
   This option enables health checks on the server. By default, a server is
-  always considered available. If "check" is set, the server is available when
-  accepting periodic TCP connections, to ensure that it is really able to serve
-  requests. The default address and port to send the tests to are those of the
-  server, and the default source is the same as the one defined in the
-  backend. It is possible to change the address using the "addr" parameter, the
-  port using the "port" parameter, the source address using the "source"
-  address, and the interval and timers using the "inter", "rise" and "fall"
-  parameters. The request method is define in the backend using the "httpchk",
-  "smtpchk", "mysql-check", "pgsql-check" and "ssl-hello-chk" options. Please
-  refer to those options and parameters for more information. See also
-  "no-check" option.
+  always considered available. If "check" is set, the server is considered
+  available when all the handshakes configured on the server line can be
+  established, or when an optional layer 7 health check succeeds. This is to
+  ensure that the server is really able to handle requests. The default address
+  and port to send health checks to are those of the server, and the default
+  source address is the same as the one defined in the backend. It is possible
+  to change the destination address using the "addr" parameter, the port using
+  the "port" parameter, the source address using the "source" parameter, and 
the
+  interval and timers using the "inter", "rise" and "fall" parameters. Optional
+  layer 7 checks can be configured with the "httpchk", "smtpchk", 
"mysql-check",
+  "pgsql-check" and "ssl-hello-check" options. When ssl is configured on the
+  server line, "option tcp-check" and "tcp-check connect" can be used to 
refrain
+  from establishing the tls hanshake during health checks. Please refer to 
those
+  options and parameters for more information. See also "no-check" and 
"check-ssl".
+
+  Example:
+  # simple tcp connect
+  backend foo
+server s1 192.168.0.1:80 check
+  # this does a tcp connect + tls handshake
+  backend foo
+server s1 192.168.0.1;443 ssl check
+  # simple tcp connect is enough for check success
+  backend foo
+option tcp-check
+tcp-check connect
+server s1 192.168.0.1:443 ssl check
 
 check-send-proxy
   This option forces emission of a PROXY protocol line with outgoing health
-- 
2.26.2



[PATCH] option logasap does not depend on mode

2020-04-23 Thread Jerome Magnin
Hi,

this patch is to disambiguate option logasap.

regards,
Jérôme
>From b7feb6d24341c15320ec961ebf1f8fc39342c0da Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Thu, 23 Apr 2020 19:01:17 +0200
Subject: [PATCH] DOC: option logasap does not depend on mode

The documentation for option logasap misleads into thinking it is
only valid for mode http. It is actually valid for mode tcp too,
so this patch tries to disambiguate the current wording.
---
 doc/configuration.txt | 28 +---
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index f3e6aa147..d3dc3884c 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -7383,21 +7383,27 @@ no option log-separate-errors
 
 option logasap
 no option logasap
-  Enable or disable early logging of HTTP requests
+  Enable or disable early logging.
   May be used in sections :   defaults | frontend | listen | backend
  yes   |yes   |   yes  |   no
   Arguments : none
 
-  By default, HTTP requests are logged upon termination so that the total
-  transfer time and the number of bytes appear in the logs. When large objects
-  are being transferred, it may take a while before the request appears in the
-  logs. Using "option logasap", the request gets logged as soon as the server
-  sends the complete headers. The only missing information in the logs will be
-  the total number of bytes which will indicate everything except the amount
-  of data transferred, and the total time which will not take the transfer
-  time into account. In such a situation, it's a good practice to capture the
-  "Content-Length" response header so that the logs at least indicate how many
-  bytes are expected to be transferred.
+  By default, logs are emitted when all the log format variables and sample
+  fetches used in the definition of the log-format string return a value, or
+  when the session is terminated. This allows the built in log-format strings
+  to account for the transfer time, or the number of bytes in log messages.
+
+  If you handle long lived connections such as large file transfers or RDP,
+  it may take a while for the request or connection to appear in the logs.
+  Using "option logasap", the log message is created as soon as the server
+  connection is established in mode tcp, or as soon as the server sends the
+  complete headers in mode http. Missing information in the logs will be the
+  total number of bytes which will only indicate the amount of data transfered
+  before the message was created and the total time which will not take the
+  remainder of the connection life or transfer time into account. For the case
+  of HTTP, it is good practice to capture the Content-Length response header
+  so that the logs at least indicate how many bytes are expected to be
+  transfered.
 
   Examples :
   listen http_proxy 0.0.0.0:80
-- 
2.26.2



Re: How to suppress weak ciphers

2020-04-23 Thread Jerome Magnin
On Thu, Apr 23, 2020 at 03:59:59AM +, Branitsky, Norman wrote:
> Jerome,
> 
> Thanks for the clarification.
> 
> This string:
> 
> CHACHA20:AESGCM:AESCCM:!RSA
> resulted in an F grade from SSL Labs due to the inclusion of TLS_DH_anon 
> ciphers:
> 
> [cid:image001.jpg@01D61902.1FDF86A0]
> 
> After adding the following to the end of the string, scored an A+:
> 
> :!aNULL
>

Interesting, I didn't have to append !aNULL, I guess this depends on operating
systems and openssl defaults. Congrats on the A+ :-)

Jérôme



Re: How to suppress weak ciphers

2020-04-22 Thread Jerome Magnin
On Wed, Apr 22, 2020 at 06:20:14PM +, Branitsky, Norman wrote:
> As you can see from my pasted configuration, I was specifying exactly 4 
> ciphers.
> The 2 weak CBC ciphers were magically appearing in the SSL Labs report.
> I tried to explicitly delete them - but the delete request is ignored.
> 
> It seems that this entry, for example, must actually be a family:
> ECDHE-RSA-AES256-SHA384
> which includes
> ECDHE-RSA-AES256-CBC-SHA384
> Not clear why the explicit delete command doesn't delete the CBC cipher.
> 

the configuration you shared excludes ciphers that are not actually ciphers. I'm
guessing this is why you still see what you try to disable when you test with
ssllabs.

there is no ECDHE-RSA-AES256-CBC-SHA384 in man ciphers(1), and no
ECDHE-RSA-AES128-CBC-SHA384.

On the other hand ECDHE-RSA-AES256-SHA384 is the openssl equivalent to 
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 so you probably got things mixed up.


> Do you use the following specification and do you find sufficient support of 
> existing browsers?
> ssl-default-bind-ciphers CHACHA20:AESGCM:AESCCM:!RSA
> Or is this too aggressive?
>
It does not support safari from 6 to 8 on IOS and OSX, and IE11 on windows 
phone 8.1.
I can share ssllabs report privately if you want.

Jérôme



Re: How to suppress weak ciphers

2020-04-22 Thread Jerome Magnin
Hi Norman,
On Wed, Apr 22, 2020 at 03:29:28PM +, Branitsky, Norman wrote:
> HA-Proxy version 1.7.10-a7dcc3b 2018/01/02
> SSL Labs reports the CBC ciphers are "weak":
> 
> [cid:image002.jpg@01D6117D.1C8AC910]
> 
> I've tried to explicitly negate these ciphers with an "!" in haproxy.cfg to 
> no avail:
> 
> 
> ssl-default-bind-options no-sslv3 no-tlsv10 no-tlsv11 no-tls-tickets
> 
> ssl-default-bind-ciphers 
> ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA256:!ECDHE-RSA-AES256-CBC-SHA384:!ECDHE-RSA-AES128-CBC-SHA384
> 
> ssl-default-server-options no-sslv3 no-tlsv10 no-tlsv11 no-tls-tickets
> 
> ssl-default-server-ciphers 
> ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA256:!ECDHE-RSA-AES256-CBC-SHA384:!ECDHE-RSA-AES128-CBC-SHA384
> 
> How do I delete the "weak" ciphers?
> 


If you list all the ciphers you want to support, it does not make sense to
negate those you don't want. just don't list them. 
You would use ! to exclude specific ciphers or ciphers "families", ie:

 ssl-default-bind-ciphers CHACHA20:AESGCM:AESCCM:!RSA

you can find additional information on this in the manpage for ciphers(1).

regards,
Jérôme



Re: [PATCH] ssl defaults enhancements

2020-04-22 Thread Jerome Magnin
On Wed, Apr 22, 2020 at 12:06:15PM +0200, Jerome Magnin wrote:
> Hi,
> [...] 
> The other patch adds a new keyword in global section to set default bind 
> curves.
> 
I updated the second patch to remove the ability to set the default curves at
build time because I did it wrong and I'm not sure it's actually useful. 

Jérôme
>From aafd1cc7fd97de2d0e395197cd2a80a3d885e60d Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Fri, 3 Apr 2020 15:28:22 +0200
Subject: [PATCH] MINOR: config: add a global directive to set default SSL
 curves

This commit adds a new keyword to the global section to set default
curves for ssl binds:
  - ssl-default-bind-curves
---
 doc/configuration.txt |  8 
 src/ssl_sock.c| 35 +++
 2 files changed, 43 insertions(+)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 2e548b66c..9b0b1d4f7 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -622,6 +622,7 @@ The following keywords are supported in the "global" 
section :
- stats
- ssl-default-bind-ciphers
- ssl-default-bind-ciphersuites
+   - ssl-default-bind-curves
- ssl-default-bind-options
- ssl-default-server-ciphers
- ssl-default-server-ciphersuites
@@ -1270,6 +1271,13 @@ ssl-default-bind-ciphersuites 
   "ssl-default-bind-ciphers" keyword. Please check the "bind" keyword for more
   information.
 
+ssl-default-bind-curves 
+  This setting is only available when support for OpenSSL was built in. It sets
+  the default string describing the list of elliptic curves algorithms ("curve
+  suite") that are negotiated during the SSL/TLS handshake with ECDHE. The 
format
+  of the string is a colon-delimited list of curve name.
+  Please check the "bind" keyword for more information.
+
 ssl-default-bind-options []...
   This setting is only available when support for OpenSSL was built in. It sets
   default ssl-options to force on all "bind" lines. Please check the "bind"
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 9077e9114..0f9b7f62c 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -175,6 +175,9 @@ static struct {
 #if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L)
char *listen_default_ciphersuites;
char *connect_default_ciphersuites;
+#endif
+#if ((HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL) || 
defined(LIBRESSL_VERSION_NUMBER))
+   char *listen_default_curves;
 #endif
int listen_default_ssloptions;
int connect_default_ssloptions;
@@ -9516,6 +9519,10 @@ static int bind_parse_ssl(char **args, int cur_arg, 
struct proxy *px, struct bin
 
if (global_ssl.listen_default_ciphers && !conf->ssl_conf.ciphers)
conf->ssl_conf.ciphers = 
strdup(global_ssl.listen_default_ciphers);
+#if ((HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL) || 
defined(LIBRESSL_VERSION_NUMBER))
+   if (global_ssl.listen_default_curves && !conf->ssl_conf.curves)
+   conf->ssl_conf.curves = 
strdup(global_ssl.listen_default_curves);
+#endif
 #if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L)
if (global_ssl.listen_default_ciphersuites && 
!conf->ssl_conf.ciphersuites)
conf->ssl_conf.ciphersuites = 
strdup(global_ssl.listen_default_ciphersuites);
@@ -10493,6 +10500,31 @@ static int ssl_parse_global_ciphersuites(char **args, 
int section_type, struct p
 }
 #endif
 
+#if ((HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL) || 
defined(LIBRESSL_VERSION_NUMBER))
+/*
+ * parse the "ssl-default-bind-curves" keyword in a global section.
+ * Returns <0 on alert, >0 on warning, 0 on success.
+ */
+static int ssl_parse_global_curves(char **args, int section_type, struct proxy 
*curpx,
+   struct proxy *defpx, const char *file, int 
line,
+  char **err)
+{
+   char **target;
+   target = _ssl.listen_default_curves;
+
+   if (too_many_args(1, args, err, NULL))
+   return -1;
+
+   if (*(args[1]) == 0) {
+   memprintf(err, "global statement '%s' expects a curves suite as 
an arguments.", args[0]);
+   return -1;
+   }
+
+   free(*target);
+   *target = strdup(args[1]);
+   return 0;
+}
+#endif
 /* parse various global tune.ssl settings consisting in positive integers.
  * Returns <0 on alert, >0 on warning, 0 on success.
  */
@@ -13008,6 +13040,9 @@ static struct cfg_kw_list cfg_kws = {ILH, {
{ CFG_GLOBAL, "tune.ssl.capture-cipherlist-size", 
ssl_parse_global_capture_cipherlist },
{ CFG_GLOBAL, "ssl-default-bind-ciphers", ssl_parse_global_ciphers },
{ CFG_GLOBAL, "ssl-default-server-ciphers", ssl_parse_global_ciphers },
+#if ((HA_OPENSSL_VERSION_NUMBER >= 0x1000200fL) || 
defined(LIBRESSL_VERSION_NUMBER))
+   { CFG_GLOBAL, "ssl-defau

[PATCH] ssl defaults enhancements

2020-04-22 Thread Jerome Magnin
Hi,

please find attached to this mail two patches.
One aims at addressing issue #595 on github, where Anit reports some server
ssl options default values aren't applied when set with default-server or 
ssl-default-server-options directives. 
The other patch adds a new keyword in global section to set default bind curves.
 
Jérôme
>From d86993cbd4476e1901eafdc7fbe88d31ca6f8e90 Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Wed, 22 Apr 2020 11:40:18 +0200
Subject: [PATCH] BUG/MINOR: ssl: default settings for ssl server options are
 not used

Documentation states that default settings for ssl server options can be set
using either ssl-default-server-options or default-server directives. In 
practice,
not all ssl server options can have default values, such as ssl-min-ver, 
ssl-max-ver,
etc..

This patch adds the missing ssl options in srv_ssl_settings_cpy() and 
srv_parse_ssl(),
making it possible to write configurations like the following examples, and 
have them
behave as expected.

   global
 ssl-default-server-options ssl-max-ver TLSv1.2

   defaults
 mode http

   listen l1
 bind 1.2.3.4:80
 default-server ssl verify none
 server s1 1.2.3.5:443

   listen l2
 bind 2.2.3.4:80
 default-server ssl verify none ssl-max-ver TLSv1.3 ssl-min-ver TLSv1.2
 server s1 1.2.3.6:443

This should be backported as far as 1.8.
This fixes issue #595.
---
 src/server.c   |  9 +
 src/ssl_sock.c | 10 ++
 2 files changed, 19 insertions(+)

diff --git a/src/server.c b/src/server.c
index 4c745d655..f90cfff5a 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1643,6 +1643,15 @@ static void srv_ssl_settings_cpy(struct server *srv, 
struct server *src)
srv->ssl_ctx.verify_host = strdup(src->ssl_ctx.verify_host);
if (src->ssl_ctx.ciphers != NULL)
srv->ssl_ctx.ciphers = strdup(src->ssl_ctx.ciphers);
+   if (src->ssl_ctx.options)
+   srv->ssl_ctx.options = src->ssl_ctx.options;
+   if (src->ssl_ctx.methods.flags)
+   srv->ssl_ctx.methods.flags = src->ssl_ctx.methods.flags;
+   if (src->ssl_ctx.methods.min)
+   srv->ssl_ctx.methods.min = src->ssl_ctx.methods.min;
+   if (src->ssl_ctx.methods.max)
+   srv->ssl_ctx.methods.max = src->ssl_ctx.methods.max;
+
 #if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L && !defined OPENSSL_IS_BORINGSSL)
if (src->ssl_ctx.ciphersuites != NULL)
srv->ssl_ctx.ciphersuites = strdup(src->ssl_ctx.ciphersuites);
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 9077e9114..2d52facb2 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -10050,6 +10050,16 @@ static int srv_parse_ssl(char **args, int *cur_arg, 
struct proxy *px, struct ser
if (global_ssl.connect_default_ciphersuites && 
!newsrv->ssl_ctx.ciphersuites)
newsrv->ssl_ctx.ciphersuites = 
strdup(global_ssl.connect_default_ciphersuites);
 #endif
+   newsrv->ssl_ctx.options |= global_ssl.connect_default_ssloptions;
+   newsrv->ssl_ctx.methods.flags |= 
global_ssl.connect_default_sslmethods.flags;
+
+   if (!newsrv->ssl_ctx.methods.min)
+   newsrv->ssl_ctx.methods.min = 
global_ssl.connect_default_sslmethods.min;
+
+   if (!newsrv->ssl_ctx.methods.max)
+   newsrv->ssl_ctx.methods.max = 
global_ssl.connect_default_sslmethods.max;
+
+
    return 0;
 }
 
-- 
2.26.2

>From e2d311f55f3a3eb5728f5dcf376ed54c672160a3 Mon Sep 17 00:00:00 2001
From: Jerome Magnin 
Date: Fri, 3 Apr 2020 15:28:22 +0200
Subject: [PATCH] MINOR: config: add a global directive to set default SSL
 curves

This commit adds a new keyword to the global section to set default
curves for ssl binds:
  - ssl-default-bind-curves

It is also possible to preset them at build time by setting the macro
LISTEN_DEFAULT_CURVES.
---
 Makefile  |  2 ++
 doc/configuration.txt |  8 
 src/ssl_sock.c| 40 
 3 files changed, 50 insertions(+)

diff --git a/Makefile b/Makefile
index 1e4213989..9e4cdef90 100644
--- a/Makefile
+++ b/Makefile
@@ -238,6 +238,8 @@ ADDLIB =
 #   ciphers on "bind" lines instead of using OpenSSL's defaults.
 #   CONNECT_DEFAULT_CIPHERS is a cipher suite string used to set the default
 #   SSL ciphers on "server" lines instead of using OpenSSL's defaults.
+#   LISTEN_DEFAULT_CURVES is a curve suite string sued to set the default SSL
+#   curves on "bind" lines instead of using OpenSSL's defaults.
 DEFINE =
 SILENT_DEFINE =
 
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 2e548b66c..9b0b1d4f7 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -622,6 +622,7 @@ The following keywords are supported in the "global" 
section :
- stats
- ssl-default-bind-ciphers
- ssl-default-

Re: FreeBSD CI builds fail

2019-07-29 Thread Jerome Magnin
On Tue, Jul 23, 2019 at 08:37:37PM +0200, Jerome Magnin wrote:
> On Tue, Jul 23, 2019 at 07:09:57PM +0200, Tim Düsterhus wrote:
> > Jérôme,
> > Ilya,
> > 
> > I noticed that FreeBSD CI fails since
> > https://github.com/haproxy/haproxy/commit/885f64fb6da0a349dd3182d21d337b528225c517.
> > 
> > 
> > One example is here: https://github.com/haproxy/haproxy/runs/169980019
> > 
> > It should be investigated whether the reg-test is valid for FreeBSD and
> > either be fixed or disabled.
> > 
> > Best regards
> > Tim Düsterhus
> > 
> Thanks Tim and Ilya,
> 
> This one fails because there's a L4 timeout, I can probably update the regex 
> to
> take that into account, the interesting part is the failure and the step at
> which it fails, but for now we expect a connection failure and not a timeout.
> 
> I'm a bit more concerned about the other one reported by Ilya where the 
> backend
> server started by VTest won't accept connection. I'll look into this one
> further.

We have decided to exclude this test on non Linux system for the time being as
it triggers a race condition in VTest.
https://github.com/haproxy/haproxy/commit/0d00b544c3bdc9dc1796aca28bad46b3c1867184

Jérôme



Re: FreeBSD CI builds fail

2019-07-23 Thread Jerome Magnin
On Tue, Jul 23, 2019 at 07:09:57PM +0200, Tim Düsterhus wrote:
> Jérôme,
> Ilya,
> 
> I noticed that FreeBSD CI fails since
> https://github.com/haproxy/haproxy/commit/885f64fb6da0a349dd3182d21d337b528225c517.
> 
> 
> One example is here: https://github.com/haproxy/haproxy/runs/169980019
> 
> It should be investigated whether the reg-test is valid for FreeBSD and
> either be fixed or disabled.
> 
> Best regards
> Tim Düsterhus
> 
Thanks Tim and Ilya,

This one fails because there's a L4 timeout, I can probably update the regex to
take that into account, the interesting part is the failure and the step at
which it fails, but for now we expect a connection failure and not a timeout.

I'm a bit more concerned about the other one reported by Ilya where the backend
server started by VTest won't accept connection. I'll look into this one
further.

Jérôme



Re: fullconn not working

2019-07-16 Thread Jerome Magnin
Hi Patrick,

On Tue, Jul 16, 2019 at 09:40:31AM -0400, Patrick Hemmer wrote:
> 
> 
> 
> *From:* Pavlos Parissis [mailto:pavlos.paris...@gmail.com]
> *Sent:* Tuesday, July 16, 2019, 09:32 EDT
> *To:* haproxy@formilux.org
> *Cc:* Patrick Hemmer 
> *Subject:* fullconn not working
> 
> > On Παρασκευή, 28 Ιουνίου 2019 5:50:48 Μ.Μ. CEST Patrick Hemmer wrote:
> > > I'm trying to get fullconn working, and can't seem to do so. I dunno if
> > > it's a bug, or if it's my understanding that's wrong.
> > > Basically my goal is to prevent the cumulative total of all connections
> > > to all servers in a pool from exceeding a certain value.
> > > For example I might have 10 servers, each with a maxconn of 10. But I
> > > want to configure haproxy with a pool-wide limit of 50, so that even if
> > > the connections are well distributed and no one server is maxed out,
> > > after 50 connections to all servers, haproxy will still start to queue
> > > instead.
> > > 
> > > fullconn seems like the right way to accomplish this, however I cannot
> > > get it to work. I've tried a simple setup of 2 servers, each with
> > > `maxconn 3`, and then a backend `fullconn 2`, which should result in
> > > queuing after 2 simultaneous connections, however it doesn't. If I send
> > > 4 connections, all 4 are simultaneously sent to the backend servers.
> > > 
> > > Here's my test config:
> > > defaults
> > >   log 127.0.0.1:1234 daemon
> > >   mode http
> > >   option httplog
> > >   timeout queue 5s
> > > frontend f1
> > >   bind :8001
> > >   default_backend b1
> > > backend b1
> > >   fullconn 2
> > >   server s1 127.0.0.1:8081 minconn 1 maxconn 3
> > >   server s2 127.0.0.1:8081 minconn 1 maxconn 3
> > > 
> > > Here's how I test:
> > > for i in {1..4}; do curl "http://localhost:8001/?sleep=2=$i; & done
> > > 
> > > And here's the logs:
> > > <30>Jun 28 11:37:47 haproxy[75322]: 127.0.0.1:55119
> > > [28/Jun/2019:11:37:45.658] f1 b1/s2 0/0/0/2003/2003 200 75 - - 
> > > 4/4/3/2/0 0/0 "GET /?sleep=2=3 HTTP/1.1"
> > > <30>Jun 28 11:37:47 haproxy[75322]: 127.0.0.1:55117
> > > [28/Jun/2019:11:37:45.658] f1 b1/s2 0/0/0/2003/2003 200 75 - - 
> > > 4/4/2/1/0 0/0 "GET /?sleep=2=4 HTTP/1.1"
> > > <30>Jun 28 11:37:47 haproxy[75322]: 127.0.0.1:55118
> > > [28/Jun/2019:11:37:45.658] f1 b1/s1 0/0/0/2003/2003 200 75 - - 
> > > 4/4/1/2/0 0/0 "GET /?sleep=2=1 HTTP/1.1"
> > > <30>Jun 28 11:37:47 haproxy[75322]: 127.0.0.1:55120
> > > [28/Jun/2019:11:37:45.658] f1 b1/s1 0/0/0/2003/2003 200 75 - - 
> > > 4/4/0/1/0 0/0 "GET /?sleep=2=2 HTTP/1.1"
> > > 
> > > 
> > Your e-mail client mangled above log lines and as a result they are bit 
> > unreadable.
> > The 4th field from `4/4/3/2/0`  is srv_conn  *2* which is below the maxconn 
> > of *3*, so haproxy did
> > the right thing as it didn't allow more than *full_conn* connections to be 
> > concurrently opened
> > against the server.
> > 
> > Cheers,
> > Pavlos
> maxconn and fullconn are different settings.
> maxconn: 
> https://cbonte.github.io/haproxy-dconv/1.9/configuration.html#maxconn%20(Server%20and%20default-server%20options)
> fullconn:
> https://cbonte.github.io/haproxy-dconv/1.9/configuration.html#fullconn
> 
> -Patrick

fullconn is not used to set a limit on the amount of connections handled by the
backend. It is used to have a 'dynamic' maxconn value on server lines.

this dymanic maxconn value will never be below minconn, and never higher than
maxconn. When the amount of connections handled by backend is between 0 and
fullconn, this dynamic maxconn value is somewhere between minconn and maxconn,
proportionnate to where we are between 0 and fullconn. When fullconn is reached,
dynamic maxconn equals the maxconn you set on server line.

With the values you set, when you reach 2 connections at backend level, servers
will allow 3 at most, each.

Jérôme



Re: Server IP address not being preserved from server state file

2019-07-11 Thread Jerome Magnin
Hi

On Thu, Jul 11, 2019 at 12:15:19PM -0400, Shaun Tarves wrote:
> Hi -
> 
> I am trying to determine why my servers' IP address is not being preserved
> through a reload when written to the server state file. I'm using version
> 1.9.8 on alpine linux.
> 
> CONFIGURATION:
> global
>   server-state-file /usr/local/etc/haproxy/haproxy.state
> 
> defaults
>   load-server-state-from-file global
> 
> backend rdp-proxy
>   mode tcp
> 
> 
>   server-template rdp 1-5 0.0.0.0:443 disabled init-addr last
> 
> During some time, the servers' IP addresses and/or ports are configured
> through the stats socket.
> 
> Right before restarting, I dump the server state using show servers state
> and it correctly shows me the configured server information:
> 11 rdp-proxy 1 rdp1 172.18.0.2 2 4 1 1 778186 1 0 0 8 0 0 0 - 80 -
> 11 rdp-proxy 2 rdp2 172.18.0.3 2 4 1 1 778186 1 0 0 8 0 0 0 - 80 -
> 11 rdp-proxy 3 rdp3 0.0.0.0 0 5 1 1 778186 1 0 0 8 0 0 0 - 443 -
> 11 rdp-proxy 4 rdp4 0.0.0.0 0 5 1 1 778186 1 0 0 8 0 0 0 - 443 -
> 11 rdp-proxy 5 rdp5 0.0.0.0 0 5 1 1 778186 1 0 0 8 0 0 0 - 443 -
> 
> 
> When I reload via the HUP or USR2 signals, all server information (admin
> state, port, etc.) is correctly restored EXCEPT the IP address, as seen
> below:
> 11 rdp-proxy 1 rdp1 0.0.0.0 2 4 1 1 778186 1 0 4 8 0 0 0 - 80 -
> 11 rdp-proxy 2 rdp2 0.0.0.0 2 4 1 1 778186 1 0 4 8 0 0 0 - 80 -
> 11 rdp-proxy 3 rdp3 0.0.0.0 0 5 1 1 778186 1 0 0 8 0 0 0 - 443 -
> 11 rdp-proxy 4 rdp4 0.0.0.0 0 5 1 1 778186 1 0 0 8 0 0 0 - 443 -
> 11 rdp-proxy 5 rdp5 0.0.0.0 0 5 1 1 778186 1 0 0 8 0 0 0 - 443 -

if you declare the server address with an IP on the server line, the IP from
server state file will never be used. If you want to use it you need to declare
the server address with a name and not an address, like this:

server-template rdp 1-5 foo.bar disabled init-addr last,1.2.3.4

this will set the IP to 1.2.3.4 by default, and you can change it with the stats
socket. when you reload, you will use the IP from the server state file.

cheers,
Jérôme



Re: http2-issue with http2 enabled on frontend and on backend

2019-02-26 Thread Jerome Magnin
On Tue, Feb 26, 2019 at 11:19:12AM +0100, Tom wrote:
> Hi list
> 
> When I enable health-checks on the backend, then the backend comes not up,
> because of "Layer7 invalid response". The backend is a simple nginx with
> http2 enabled. As I mentioned: When I directly talk to the backend with
> http2, then everything is fine. So it has something to do regarding my
> haproxy-config, but I'm not sure whats wrong.
> 
Sorry I missed the part about health checks, I believe you need to use
check-alpn http/1.1 on your server lines for checks to work. They are not
supported over http2 for now and since you have alpn h2,http/1.1 on your
server lines, haproxy will try to send http/1 traffic to a server expecting
h2, that is, unless you set check-alpn.

Jérôme



Re: http2-issue with http2 enabled on frontend and on backend

2019-02-26 Thread Jerome Magnin
On Tue, Feb 26, 2019 at 11:19:12AM +0100, Tom wrote:
> Hi list
> 
> I'm using haproxy-1.9.4 and trying to enable http2 in frontend and on one
> backend server (nginx with http2 enabled). I'm always receiving a http/502
> from haproxy. I'm successfully able to directly talk to the backend with
> http2, but not via haproxy.
> 
> The haproxy-log looks like this (curl-request like "curl --http2 -k -L -v
> https://10.10.10.10;)
> Feb 26 11:07:10 localhost haproxy[24088]: srcip=1.1.1.1:37468
> feip=10.10.10.10:443(http-in,http-in~,1) beip=10.10.10.10:37530(server1,0)
> serverip=10.20.20.20:443(webserver1) GET / HTTP/1.1 1/1/0/0/0 0/0 requests=0
> resptime=-1 bytesread=244 status=502 tsc=PH-- sslv=TLSv1.2 ms=998
> 
> 
> My config looks like this:
> global
>   log 127.0.0.1 local1 info
>   chroot /home/haproxy
>   user haproxy
>   group haproxy
>   master-worker
>   debug
>   ssl-server-verify none
>   tune.ssl.default-dh-param 2048
>   ssl-default-bind-ciphers EECDH+AESGCM:EDH+AESGCM:AES256+EECDH:AES256+EDH
>   ssl-default-bind-options no-sslv3 no-tls-tickets
> defaults
>   log global
>   mode http
>   option dontlognull
>   timeout connect 5s
>   timeout client  50s
>   timeout server 60s
> frontend http-in
>   bind 10.10.10.10:443 ssl crt /etc/haproxy/ssl/wildcard.pem crt
> /etc/haproxy/ssl/ alpn h2,http/1.1
>   log-format "srcip=%ci:%cp feip=%fi:%fp(%f,%ft,%fc) beip=%bi:%bp(%b,%bc)
> serverip=%si:%sp(%s) "%r" %ac/%fc/%bc/%sc/%rc %sq/%bq requests=%rt
> resptime=%Tr bytesread=%B status=%ST tsc=%tsc sslv=%sslv ms=%ms"
>   default_backend server1
> 
> 
> backend server1
>   balance roundrobin
>   #http-check expect status 200
>   #option httpchk GET "/test"
>   server webserver1 10.20.20.20:443 ssl verify none alpn h2,http/1.1
> 
> 
> 
> When I enable health-checks on the backend, then the backend comes not up,
> because of "Layer7 invalid response". The backend is a simple nginx with
> http2 enabled. As I mentioned: When I directly talk to the backend with
> http2, then everything is fine. So it has something to do regarding my
> haproxy-config, but I'm not sure whats wrong.
> 
> Any hints for this?

if you want to use http2 on both sides you need to enable htx.
add 'option http-use-htx' in your defaults, or in the frontend and backend where
you want http2 enabled.

Jérôme



Re: DNS resolution issue with Docker swarm and HAProxy 1.8.15/1.9.0

2018-12-20 Thread Jerome Magnin
Hi Vincent,

On Thu, Dec 20, 2018 at 10:22:25PM +0100, Vincent Bernat wrote:
>  ❦ 20 décembre 2018 17:14 +01, Willy Tarreau :
> 
> >> this is indeed a regression in haproxy.  thanks for reporting it.
> >> attached patch should fix it.
> >> CC'ing Remi as the original author, and Baptiste, as DNS maintainer.
> >
> > Good catch, the patch looks obviously good, I've just merged it.
> > Thanks for fixing this one, Jérôme.
> 
> Is it important enough for an 1.8.16? Is it important enough for
> distributors to release a fixed version? Why doesn't it affect most DNS
> implementations?

the bug only triggers when the resolver used by haproxy sends back DNS responses
with nothing after the Answers section, ie: no additional records. I'm guessing
the use of DNSSEC hides the problem :-) I must get better with commit messages.

Jérôme



Re: DNS resolution issue with Docker swarm and HAProxy 1.8.15/1.9.0

2018-12-20 Thread Jerome Magnin
Hi,

On Thu, Dec 20, 2018 at 03:42:40PM +0100, Leonhard Wimmer wrote:
> Hello,
> 
> We are running HAProxy in our Docker (18.09.0) swarm and we are relying on
> the Docker embedded DNS server for service discovery.
> 
> The backend servers are configured to resolve the IP addresses via a
> "resolvers" config entry pointing to the Docker embedded DNS running on
> "127.0.0.11".
> 
> Up to HAProxy 1.8.14 this worked like charm, but it stopped working with
> version 1.8.15. Also the newly released version 1.9.0 is affected by this
> problem.
> 
> I've looked through the changes between 1.8.14 and 1.8.15 and I could narrow
> it down to commit 2e53fe8:
> "BUG: dns: Prevent out-of-bounds read in dns_validate_dns_response()".
> If I revert this commit on haproxy-1.8 it works perfectly, just as before.
> 
> DNS resolution does not seem to be generally broken though. If I use a regular
> (non-docker-internal) hostname, it can be resolved normally, even using the
> Docker embedded DNS server.
> 
> I'm not yet sure if it is the Docker DNS server returning an invalid result
> or HAProxy having a problem with the validation.
> 
> I'm happy to help with debugging. I can provide packet captures of the DNS
> resolution and a sample config to reproduce the problem if you are interested.
>

this is indeed a regression in haproxy.  thanks for reporting it.
attached patch should fix it.
CC'ing Remi as the original author, and Baptiste, as DNS maintainer.

Jérôme
>From 7868e9bfdc26c2a9e777470eae9d098b75b9070b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Magnin?= 
Date: Thu, 20 Dec 2018 16:47:31 +0100
Subject: [PATCH] BUG/MEDIUM: Don't prevent reading the last byte of the payload 
in dns_validate_response()

A regression was introduced with efbbdf72 BUG: dns: Prevent out-of-bounds read 
in dns_validate_dns_response()
as it prevented from taking into account the last byte of the payload.
this patch aims at fixing it.

this must be backported in 1.8.
---
 src/dns.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/dns.c b/src/dns.c
index c1396f52..78d8f52f 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -810,7 +810,7 @@ static int dns_validate_dns_response(unsigned char *resp, 
unsigned char *bufend,
/* Move forward 2 bytes for data len */
reader += 2;
 
-   if (reader + dns_answer_record->data_len >= bufend) {
+   if (reader + dns_answer_record->data_len > bufend) {
pool_free(dns_answer_item_pool, dns_answer_record);
return DNS_RESP_INVALID;
}
-- 
2.20.1



Re: sample fetch: add bc_http_major

2018-12-07 Thread Jerome Magnin
Hi Aleks,

On Fri, Dec 07, 2018 at 01:46:53PM +0100, Aleksandar Lazic wrote:
> Hi Jerome.
> [...] 
> I suggest to use a dedicated function for that, jm2c.
> 
> { "bc_http_major", smp_fetch_bc_http_major, 0, NULL, SMP_T_SINT, 
> SMP_USE_L4SRV },
> 

If you look at src/ssl_sock.c there are several fetches applying to both
frontend and backend connection, and each pair uses the same function. I
shamelessly copied^W^Wtook example from them.

Jérôme



sample fetch: add bc_http_major

2018-12-07 Thread Jerome Magnin
Hi,

the attached patch adds bc_http_major. It returns the HTTP major encoding of the
backend connection, based on the the on-wire encoding. 

Jérôme
>From e0a28394ea2da5757c1e72773ab4c9fb97565a35 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Magnin?= 
Date: Fri, 7 Dec 2018 09:03:11 +0100
Subject: [PATCH] sample: add bc_http_major

---
 doc/configuration.txt | 5 +
 src/connection.c  | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index e6678c17..18951f8c 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -14352,6 +14352,11 @@ table may be specified with the "sc*" form, in which 
case the currently
 tracked key will be looked up into this alternate table instead of the table
 currently being tracked.
 
+bc_http_major: integer
+  Returns the backend connection's HTTP major version encoding, which may be 1
+  for HTTP/0.9 to HTTP/1.1 or 2 for HTTP/2. Note, this is based on the on-wire
+  encoding and not the version present in the request header.
+
 be_id : integer
   Returns an integer containing the current backend's id. It can be used in
   frontends with responses to check which backend processed the request.
diff --git a/src/connection.c b/src/connection.c
index 054e1998..2b0063e6 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -1258,7 +1258,8 @@ int make_proxy_line_v2(char *buf, int buf_len, struct 
server *srv, struct connec
 static int
 smp_fetch_fc_http_major(const struct arg *args, struct sample *smp, const char 
*kw, void *private)
 {
-   struct connection *conn = objt_conn(smp->sess->origin);
+   struct connection *conn = (kw[0] != 'b') ? objt_conn(smp->sess->origin) 
:
+   
smp->strm ? cs_conn(objt_cs(smp->strm->si[1].end)) : NULL;
 
smp->data.type = SMP_T_SINT;
smp->data.u.sint = (conn && strcmp(conn_get_mux_name(conn), "H2") == 0) 
? 2 : 1;
@@ -1293,6 +1294,7 @@ int smp_fetch_fc_rcvd_proxy(const struct arg *args, 
struct sample *smp, const ch
  */
 static struct sample_fetch_kw_list sample_fetch_keywords = {ILH, {
{ "fc_http_major", smp_fetch_fc_http_major, 0, NULL, SMP_T_SINT, 
SMP_USE_L4CLI },
+   { "bc_http_major", smp_fetch_fc_http_major, 0, NULL, SMP_T_SINT, 
SMP_USE_L4SRV },
{ "fc_rcvd_proxy", smp_fetch_fc_rcvd_proxy, 0, NULL, SMP_T_BOOL, 
SMP_USE_L4CLI },
{ /* END */ },
 }};
-- 
2.19.2



Re: Difference between rspdel and http-response del-header use case?

2018-11-15 Thread Jerome Magnin
Hi,

On Thu, Nov 15, 2018 at 02:01:18PM +, Ricardo Fraile wrote:
> Hello,
> 
> 
> What is the difference between using one of the following rules instead
> of the other?
> 
> I think that rspdel is the historic way to do, but maybe it have other
> implications.
> 
> 
> rspdel ^Server.*
> 
> or
> 
> http-response del-header Server
> 
> 

one of the differences is that rspdel (and other rsp* or req* commands) happen
after the rule processing, whereas http-resonse del-header happens during the
rule processing. while it might not make a big difference when suppressing
headers, it can make a difference if you add headers and want further rules to
act based on the value of said header.

also you can break the request or response with a badly crafted req* or rsp*
command.

-- 
Jérôme



Re: Combine different ACLs under same name

2018-10-05 Thread Jerome Magnin
Hello,

On Fri, Oct 05, 2018 at 10:46:20AM +0200, Ricardo Fraile wrote:
> Hello,
> 
> 
> I have tested that some types of acls can't be combined, as example:
> 
> Server 192.138.1.1, acl with combined rules:
> 
> acl rule1 hdr_dom(host) -i test.com
> acl rule1 src 192.168.1.2/24
> redirect prefix https://yes.com code 301 if rule1 
> redirect prefix https://no.com
> 
> Request from 192.168.1.2:
> 
>   $ curl -I -H "host: test.com" 192.138.1.1
>   HTTP/1.1 301 Moved Permanently
>   Content-length: 0
>   Location: https://yes.com/
> 
> Request from 192.168.1.3:
> 
>   $ curl -I -H "host: test.com" 192.138.1.1
>   HTTP/1.1 301 Moved Permanently
>   Content-length: 0
>   Location: https://yes.com/
> 
> 
> 
> Server 192.138.1.1, acl with two rules:
> 
> acl rule1 hdr_dom(host) -i test.com
> acl rule2 src 192.168.1.2/24
> redirect prefix https://yes.com code 301 if rule1 rule2
> redirect prefix https://no.com
> 
> Request from 192.168.1.2:
> 
>   $ curl -I -H "host: test.com" 192.138.1.1
>   HTTP/1.1 301 Moved Permanently
>   Content-length: 0
>   Location: https://yes.com/
> 
> Request from 192.168.1.3:
> 
>   $ curl -I -H "host: test.com" 192.138.1.1
>   HTTP/1.1 301 Moved Permanently
>   Content-length: 0
>   Location: https://no.com/
> 
> I look for this behaviour on the documentation but I don't find any
> reference to it. Please, can someone know where it is documented?
> 
> 

This is expected behavior.

when you declare acls with the same name such as:

acl foo src 1.2.3.4
acl foo hdr(host) foo.bar


and use foo as a condition for anything, foo equivalent to :

 { src 1.2.3.4 } || { hdr(host) foo.bar }

There is at least an example of this behavior in the documentation:
https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#7.2

your splitting of the acl in two acls leads to implying an && between the two
acls, and the behavior is different.

regards,
Jérôme



Re: srv_is_up : unable to find server.

2018-06-05 Thread Jerome Magnin
Hi Brent,

On Tue, Jun 05, 2018 at 01:18:36PM +0200, Brent Clark wrote:
> Good day Guys
> 
> I am at a total loss, and Im hoping someone on this list, would be so kind
> to review my setup.
> 
> I am trying to get haproxy to monitor redis / sentinel. But I keep getting.
> 
> [WARNING] 155/110602 (309) : config : log format ignored for frontend
> 'ft_redis' since it has no log address.
> [ALERT] 155/110602 (309) : parsing [/usr/local/etc/haproxy/haproxy.cfg:29] :
> unable to find server '10.42.131.120' in proxy 'bk_redis', referenced in arg
> 1 of ACL keyword 'srv_is_up' in proxy 'bk_redis'.
> [ALERT] 155/110602 (309) : parsing [/usr/local/etc/haproxy/haproxy.cfg:30] :
> unable to find server '10.42.40.236' in proxy 'bk_redis', referenced in arg
> 1 of ACL keyword 'srv_is_up' in proxy 'bk_redis'.
> [ALERT] 155/110602 (309) : parsing [/usr/local/etc/haproxy/haproxy.cfg:31] :
> unable to find server '10.42.224.133' in proxy 'bk_redis', referenced in arg
> 1 of ACL keyword 'srv_is_up' in proxy 'bk_redis'.
> [ALERT] 155/110602 (309) : Fatal errors found in configuration.
> 
> What I cant understand is, I changed to ips as opposed to hostnames. But
> haproxy still cant  see the peer.
> 
> Here is my configuration file.
> https://pastebin.com/raw/DGTsNRDs
> 
> If someone can assist it would be appreciated.
> 

srv_is_up takes an optionnal backend name and a mandatory server name as
argument. server name is the second argument on a server line, it does not have
to be a (resolvable) fqdn.

example: 

use-server redis-server-0 if { srv_is_up(10.42.131.120/sentinel0) } ...

I'm not sure I understand what you want to do, though.

-- 
Jérôme



Re: Use SNI with healthchecks

2018-04-23 Thread Jerome Magnin
Hi Vincent,

On Mon, Apr 23, 2018 at 02:38:32PM +, GALLISSOT VINCENT wrote:
> Hi all,
> 
> 
> I want to use SNI with httpchk on HAProxy 1.7.10 to connect to  CloudFront 
> distributions as backend servers.
> 
> I saw in this mailing-list archives that SNI is not used by default even when 
> using the ssl directive.
> 
> We don't pay for SNI on that distribution, that means CloudFront doesn't 
> provide a certificate on its default vhost.
> 
> Because of that, all healthchecks fail with "handshake failure".
> 
> 
> I temporarily by-passed the issue by adding "port 80" to allow healthchecks 
> over HTTP:
> 
> 
> option httpchk HEAD /check HTTP/1.1\r\nHost:\ 
> mydistribution.cloudfront.net
> server mydistribution mydistribution.cloudfront.net:443 check resolvers 
> mydns port 80 cookie no-sslv3 ssl verify required ca-file ca-certificates.crt
> 
> 
> Does anybody know how can I use healthchecks over HTTPS with SNI support ?
>

Prior to 1.8 if you want SNI in the health checks you have to use something
along these lines:

backend moo
mode http
option httpchk GET / HTTP/1.0
server s1 my.example.host:443 check addr 127.0.0.1 port 1234 ssl sni 
str("my.example.host") 


listen foo
bind 127.0.0.1:1234
server s1 my.example.host:443 sni str("my.example.host") ssl

That's because sni keyword only applies to proxied traffic, and not checks, so
you check through a listener that will add the sni.

With 1.8 and later, you just use check-sni  on server lines.

cheers,
Jérôme 



Re: Alpn in debian/ubuntu ppa 1.8

2018-01-25 Thread Jerome Magnin
Hi Igor,

On Thu, Jan 25, 2018 at 11:26:14PM +1100, Igor Cicimov wrote:
> Hi,
> 
> I was testing haproxy 1.8 from the ppa repository and noticed it is not
> build with alpn support so just wonder why?

what's the output of haproxy -vv ?

Jérôme