Re: [PR] proto_tcp.c: fix printing of muliple setsockopt errors

[Björn Jacke]

Hi Willy,

On 07.10.21 08:57, Willy Tarreau wrote:

I'm attaching the experimental patch I added on top of yours to produce
that output. Be careful, it contains the setsockopt() redefinition and
the resizing of the protocol_bind_all() buffer. Also I spotted a leftover
from an earlier "msg" that wasn't modified, I fixed it there.

Please let me know what you think, and if that's OK for you, feel free
to remerge it into yours.


I've worked in your proposed changes, they all look fine to me also. 
Just pushed that to the git branch.


Thanks
Björn

Re: [*EXT*] Re: host-based be routing with H2

[Ionel GARDAIS]

Hi Tim,

Noted for SNI and routing.

User were not able to reproduce the issue when I've re-enable H2.
I've kept the header capture active if I'm ever notified of an issue again.

-- 
Ionel GARDAIS
Tech'Advantage CIO - IT Team manager

- Mail original -
De: "Tim Düsterhus" 
À: "Ionel GARDAIS" , "haproxy" 

Envoyé: Mardi 5 Octobre 2021 21:23:58
Objet: [*EXT*] Re: host-based be routing with H2

Ionel,

On 10/5/21 3:56 PM, Ionel GARDAIS wrote:
> Currently, backend selection is made with
> use_backend %[req.hdr(host),lower]
> 
> Would
> use_backend %[ssl_fc_sni,lower] # Layer 5
> or
> use_backend %[req.ssl_sni,lower] # Layer 6
> help with H2 ?
> 

That would be a big fat NO.

SNI is ***never*** the correct solution to perform routing.

In fact it will make the situation even worse for you.

-

req.hdr(host) is the correct solution and I am surprised that it does 
not work for you.

Consider adding 'capture request header Host len 50' to your frontend 
and then share the log lines for affected requests. With the httplog 
format they should then indicate both the host as seen by HAProxy as 
well as the backed/server selected.

Best regards
Tim Düsterhus
--
232 avenue Napoleon BONAPARTE 92500 RUEIL MALMAISON
Capital EUR 219 300,00 - RCS Nanterre B 408 832 301 - TVA FR 09 408 832 301

Use variables in healthchecks

[Basti Schubert]

Hi there,

i'd like to use variables in healtchecks to set the host header to the
ip of the server that is beeing checked, something like this:

> http-check connect port 443 ssl
> http-check send meth GET uri /ecp/healthcheck.htm ver HTTP/1.1 hdr host 
> $SERVERIP 
> http-check expect status 200

Problem is: i don't know if this is possible and if there is already a
variable for such configs (can %si from the log format section be used
for this?)

I'm on haproxy 2.2.x

cheers
 basti

Re: [PATCH v2] BUILD: SSL: function "ERR_func_error_string" is deprecated in OpenSSL-3.0.0

[Илья Шипицин]

чт, 7 окт. 2021 г. в 12:49, Willy Tarreau :

> On Thu, Oct 07, 2021 at 11:30:54AM +0500,  ??? wrote:
> > > Just thinking about something, given that the new API was already
> adopted
> > > by BoringSSL and will probably be at some point in time by LibreSSL,
> would
> > > it not be better to have a single macro "HA_SSL_USE_API_V3" or
> something
> > > like this that we set based on the various libs' versions, and rely on
> this
> > > one for all other defines ? I think it could significantly simplify the
> > > porting to other libs and avoid a real mess with version numbers
> > > everywhere.
> > >
> >
> > even BoringSSL states "it adopted upstream changes", it is different in
> > details, for example ERR_func_error_string
> > is not deprecated in BoringSSL.
> >
> > Well, there might be a common divisor of course. I'll keep an eye on it
> :)
> >
> > as for this particular patch, it is openssl specific (at least now)
>
> OK. I'll let the SSL maintainers deal with this.
>

I set up Fedora Rawhide builds.
Fedora now uses openssl-3.0.0, all builds are bloody murder:

fedora_clang (#1657157053) · Jobs · Ilya Shipitsin / haproxy-ci-playground
· GitLab




>
> Thanks!
> Willy
>

Re: [PATCH v2] BUILD: SSL: function "ERR_func_error_string" is deprecated in OpenSSL-3.0.0

[Willy Tarreau]

On Thu, Oct 07, 2021 at 11:30:54AM +0500,  ??? wrote:
> > Just thinking about something, given that the new API was already adopted
> > by BoringSSL and will probably be at some point in time by LibreSSL, would
> > it not be better to have a single macro "HA_SSL_USE_API_V3" or something
> > like this that we set based on the various libs' versions, and rely on this
> > one for all other defines ? I think it could significantly simplify the
> > porting to other libs and avoid a real mess with version numbers
> > everywhere.
> >
> 
> even BoringSSL states "it adopted upstream changes", it is different in
> details, for example ERR_func_error_string
> is not deprecated in BoringSSL.
> 
> Well, there might be a common divisor of course. I'll keep an eye on it :)
> 
> as for this particular patch, it is openssl specific (at least now)

OK. I'll let the SSL maintainers deal with this.

Thanks!
Willy

Re: [PR] proto_tcp.c: fix printing of muliple setsockopt errors

[Willy Tarreau]

This is the second patch. Some of your comments in it were useful to
raise some concerns about issues that could be difficult to address,
namely about the hard-coded use of IPPROTO_TCP at some places where
you'd have preferred to use protocol->sock_prot, but this one is not
correct since we're still hijacking the tcpv4 entry.

This also means we won't be able to pass such sockets across reloads,
by the way. So that led me to think about a possible variant of the
approach that could further simplify the patch.

What if we duplicate protocol_tcpv4 and protocol_tcpv6 to
protocol_mptcpv4 and protocol_mptcpv6, and just change the sock_prot
there ? We'll need a new discriminant. We can either create a new
dummy sock_domain like we did for sockpair, so we could have
AF_MPTCP4 and AF_MPTCP6 and make sure the families are properly set
where used. Or cleaner, we'd add an extra dimension to the protocols
array to take care of the proto_type variant. We could consider that
all regular protocols use 0 and that mptcp uses 1, so that we don't
inflate the array that much (and that's not much different from what
was done in the kernel where IPPROTO_MPTCP=262 is just IPPROTO_TCP
with an extra bit set that's easy to mask).

The benefit with this is that you drop the rx/listener option, you
don't need to parse another keyword that comes too late in the process,
you just have to recognize two new address family prefixes "mptcp4"
and "mptcp6" to be used on bind lines like this:

   bind mptcp4@192.168.1.2:443 ...

and that's all. The change is to be operated in str2sa_range() to
take into account this new ipproto_type dimension. By the way I
think that in order to avoid any difficulty in the future, instead
of thinking in terms of ipproto_type we could think in terms of
"protocol variant". That would be the last dimension of the
protocols[] array, and we'd add this "variant" in the protocol
struct so that when you have your protocol, you can easily look
up other info from there. This "variant" would then always be
zero except for mptcp4/6 which would have 1, and would likely
be sufficient for a few more decades.

Another nice benefit is that doing so will instantly provide whatever
is needed to:
  - pass FDs from one process to the new one
  - support mptcp on the backend

I'm seeing that you had to disable the error on TCP_NODELAY, mentioning
that it's not yet supported by MPTCP. Does this just mean that it's always
on by default ? I'm asking because being forced to support nagle is a
no-go for any low-latency protocol: you can't afford to wait 200ms after
sending a request that the stack decides to finally send it.

Be careful with the doc:

+  Is an optional keyword which enables Multicast TCP (RFC 8584). Multicast TCP

As you know it's "multipath" not "multicast" :-) And the RFC is 8684.

Thanks!
Willy

Re: [PR] proto_tcp.c: fix printing of muliple setsockopt errors

[Willy Tarreau]

Hi Björn,

On Mon, Oct 04, 2021 at 04:22:32PM +0200, Björn Jacke wrote:
> Hi Willy,
> 
> I lost track of this issue but I caught this up finally again.
> 
> I updated the setsockopt error patch as part of the mptcp branch here:
> 
> https://gitlab.com/bjacke/haproxy/-/commits/bjacke-mptcp
> 
> The output with failing setsockopt calls is now like this:
> 
> [WARNING]  (66986) : Starting frontend ft_mp:
>   cannot set MSS
>   cannot enable TCP_FASTOPEN
>   for [:::2350]
> [WARNING]  (66986) : Starting frontend ft_test:
>   cannot set MSS
>   for [:::235]

Thanks. To be honest I don't find this easy to read nor to understand.
Also I noticed that the change in protocol_bind_all() caused a socket
binding error to appear like this:

  $ ./haproxy -db -f binderr.cfg
  [NOTICE]   (20118) : haproxy version is 2.5-dev8-e3e46b-117
  [NOTICE]   (20118) : path to executable is ./haproxy
  [ALERT](20118) : Starting proxy http:
  cannot bind socket (Permission denied) [0.0.0.0:80]
  [ALERT](20118) : Starting proxy https:
  cannot bind socket (Permission denied) [0.0.0.0:443]
  [ALERT](20118) : [./haproxy.main()] Some protocols failed to start their 
listeners! Exiting.

I tried to fix it by forcing two spaces after the "\n" and that gave this:

  $ ./haproxy -db -f binderr.cfg
  [NOTICE]   (20308) : haproxy version is 2.5-dev8-e3e46b-117
  [NOTICE]   (20308) : path to executable is ./haproxy
  [ALERT](20308) : Starting proxy http:
cannot bind socket (Permission denied) [0.0.0.0:80]
  [ALERT](20308) : Starting proxy https:
cannot bind socket (Permission denied) [0.0.0.0:443]
  [ALERT](20308) : [./haproxy.main()] Some protocols failed to start their 
listeners! Exiting.

That I find much less clear than the original:

  $ ./haproxy -db -f binderr.cfg
  [NOTICE]   (20213) : haproxy version is 2.5-dev8-e3e46b-117
  [NOTICE]   (20213) : path to executable is ./haproxy
  [ALERT](20213) : Starting proxy http: cannot bind socket (Permission 
denied) [0.0.0.0:80]
  [ALERT](20213) : Starting proxy https: cannot bind socket (Permission 
denied) [0.0.0.0:443]
  [ALERT](20213) : [./haproxy.main()] Some protocols failed to start their 
listeners! Exiting.

I tried something with this config:

  listen http
  bind  :1080 tcp-ut 10 mss 12 tfo defer-accept

I hacked setsockopt() to always fail. With your patch only I'm getting
this (I had to extend the string in protocol_bind_all since 100 would
truncate the output):

  $ ./haproxy -db -f binderr.cfg
  [NOTICE]   (21241) : haproxy version is 2.5-dev8-31b9e1-118
  [NOTICE]   (21241) : path to executable is ./haproxy
  [WARNING]  (21241) : Starting proxy http:
cannot set MSS
cannot enable DEFER_ACCEPT
cannot set TCP User Timeout
cannot enable TCP_FASTOPEN
for [0.0.0.0:1080]

With some small changes instead I'm getting this which I personally
find more parsable and more greppable, and more importantly all errors
continue to be reported similarly:

  $ ./haproxy -db -f binderr.cfg
  [NOTICE]   (20997) : haproxy version is 2.5-dev8-e3e46b-117
  [NOTICE]   (20997) : path to executable is ./haproxy
  [WARNING]  (20997) : Starting proxy http: [0.0.0.0:1080]: cannot set MSS, 
cannot set TCP User Timeout, cannot enable DEFER_ACCEPT, cannot enable 
TCP_FASTOPEN

There's another aspect to think about. With all the evolutions we're
currently bringing towards more dynamic changes, it's important that
we're able to easily report messages both on stderr and on the CLI for
changes that are pushed from there. As such we cannot presume of the
output formatting and in the end providing a single line with all the
relevant info, with multiple errors delimited by commas certainly is
way more versatile and future-proof.

I'm attaching the experimental patch I added on top of yours to produce
that output. Be careful, it contains the setsockopt() redefinition and
the resizing of the protocol_bind_all() buffer. Also I spotted a leftover
from an earlier "msg" that wasn't modified, I fixed it there.

Please let me know what you think, and if that's OK for you, feel free
to remerge it into yours.

Now switching to the second patch :-)

Thanks,
Willy
>From 9866bea00f11ab0091da752dc66e0d402244a298 Mon Sep 17 00:00:00 2001
From: Willy Tarreau 
Date: Thu, 7 Oct 2021 08:44:38 +0200
Subject: [PATCH] EXP with better formatting

---
 src/proto_tcp.c | 22 +++---
 src/protocol.c  | 10 +-
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/proto_tcp.c b/src/proto_tcp.c
index 68f0ffc56..2bd3020bc 100644
--- a/src/proto_tcp.c
+++ b/src/proto_tcp.c
@@ -586,7 +586,7 @@ int tcp_bind_listener(struct listener *listener, char 
*errmsg, int errlen)
int fd, err;
int ready;
struct buffer *msg = alloc_trash_chunk();
-
+inline int setsockopt(int fd, int ipproto, int opt, const void *foo, socklen_t 
len) { return -1; }
err = ERR_NONE;
 
/* ensure we never return garbage */
@@ -597,7 +597,7 @@ 

Re: [PATCH v2] BUILD: SSL: function "ERR_func_error_string" is deprecated in OpenSSL-3.0.0

[Илья Шипицин]

чт, 7 окт. 2021 г. в 10:58, Willy Tarreau :

> Hi Ilya,
>
> On Wed, Oct 06, 2021 at 11:26:13PM +0500, Ilya Shipitsin wrote:
> > +/* ERR_func_error_string is deprecated in OpenSSL-3.0.0 */
> > +#if (OPENSSL_VERSION_NUMBER >= 0x3000L)
> > +#define HA_ERR_func_error_string(ret) "OPENSSL_internal"
> > +#else
> > +#define HA_ERR_func_error_string(ret) ERR_func_error_string(ret)
> > +#endif
>
> Just thinking about something, given that the new API was already adopted
> by BoringSSL and will probably be at some point in time by LibreSSL, would
> it not be better to have a single macro "HA_SSL_USE_API_V3" or something
> like this that we set based on the various libs' versions, and rely on this
> one for all other defines ? I think it could significantly simplify the
> porting to other libs and avoid a real mess with version numbers
> everywhere.
>

even BoringSSL states "it adopted upstream changes", it is different in
details, for example ERR_func_error_string
is not deprecated in BoringSSL.

Well, there might be a common divisor of course. I'll keep an eye on it :)

as for this particular patch, it is openssl specific (at least now)



>
> Just my two cents,
> Willy
>

Re: executable properties (checksec, BinSkim)

[Willy Tarreau]

On Sat, Sep 18, 2021 at 03:05:10PM +0500,  ??? wrote:
> Hello,
> 
> I checked how looks binary shipped in several popular distributions
> (ppa:vbernat/haproxy-2.4, docker haproxytech/haproxy-ubuntu, docker
> haproxy).
> 
> are we aware of those security features ? shall we move them to Makefile ?
> or is it up to distribution ?

It's definitely something up to the distros as it depends on the OS,
the compiler, the linker, and even the target use cases, as there are
environments where users are not willing to trade a single iota of
performance for increased protection, or where executables are run in
short-lived, disposable containers with nothing sensitive on them, that
are replaced every few minutes to hours, and which immediately die if
the process dies or stops working.

I think it can make sense to some extents for generic distros, but I
don't know the impacts on dependencies.

Willy