Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Mildis]

> Le 27 juin 2019 à 09:08, Vincent Bernat  a écrit :
> 
> ❦ 27 juin 2019 09:06 +02, Mildis :
> 
>>>>> You can workaround this by not chrooting HAProxy. The problem is that
>>>>> once chrooted, it cannot load the library. We should force libpthread to
>>>>> preload this lib.
>>>> 
>>>> This mailing list thread might be relevant / helpful here:
>>>> https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
>>>> (Message-ID: 8e3aa43f-d6ad-4128-bb10-feeb5f315...@gandi.net).
>>> 
>>> Thanks. Adding ADDLIB="-Wl,--no-as-needed -lgcc_s -Wl,--as-needed" fixes
>>> the issue.
>> 
>> Does 2.0.1 include the fix ?
> 
> Yes.
Great, thanks for your help.

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Mildis]

> Le 24 juin 2019 à 19:32, Vincent Bernat  a écrit :
> 
> ❦ 24 juin 2019 19:08 +02, Tim Düsterhus :
> 
>>> You can workaround this by not chrooting HAProxy. The problem is that
>>> once chrooted, it cannot load the library. We should force libpthread to
>>> preload this lib.
>> 
>> This mailing list thread might be relevant / helpful here:
>> https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
>> (Message-ID: 8e3aa43f-d6ad-4128-bb10-feeb5f315...@gandi.net).
> 
> Thanks. Adding ADDLIB="-Wl,--no-as-needed -lgcc_s -Wl,--as-needed" fixes
> the issue.

Does 2.0.1 include the fix ?

Re: haproxy 2.0 - stretch - libgcc_s.so.1

[Mildis]

Le 24 juin 2019 à 19:32, Vincent Bernat  a écrit :
> 
> ❦ 24 juin 2019 19:08 +02, Tim Düsterhus :
> 
>>> You can workaround this by not chrooting HAProxy. The problem is that
>>> once chrooted, it cannot load the library. We should force libpthread to
>>> preload this lib.
>> 
>> This mailing list thread might be relevant / helpful here:
>> https://www.mail-archive.com/haproxy@formilux.org/msg33189.html
>> (Message-ID: 8e3aa43f-d6ad-4128-bb10-feeb5f315...@gandi.net).
> 
> Thanks. Adding ADDLIB="-Wl,--no-as-needed -lgcc_s -Wl,--as-needed" fixes
> the issue.
> 
Thanks Vincent for your prompt action.
As Tim and I point it, 
https://www.mail-archive.com/haproxy@formilux.org/msg33189.html 
 shows this 
behavior and solution.
I was not aware of haproxy 2.0 autodetection of nbthreads highlighted by 
William and the side-effect on packaging.

> 1.9 was already calling pthread_exit() but it doesn't seem to happen
> with 1.9.8. Do you know what could have changed?
> -- 
> The lunatic, the lover, and the poet,
> Are of imagination all compact...
>   -- Wm. Shakespeare, "A Midsummer Night's Dream"

haproxy 2.0 - stretch - libgcc_s.so.1

[Mildis]

Hi,

I'm hitting
https://www.mail-archive.com/haproxy@formilux.org/msg33189.html 
with haproxy 2.0 on Stretch, when doing a hot-reload

Jun 24 18:34:05 haproxy[32347]: libgcc_s.so.1 must be installed for 
pthread_cancel to work
Jun 24 18:34:05 haproxy[32347]: [WARNING] 174/183405 (32347) : Former worker #1 
(32363) exited with code 134 (Aborted)

Is it something to be fixed by Vincent during the build for the packaging ?

Thanks,
Mildis

Re: haproxy segfault

[Mildis]

> Le 12 févr. 2019 à 21:26, Christopher Faulet  a écrit :
>> 
> Hi,
> 
> A recent fix about a double free has been merge in HAProxy 1.9:
> 
>  http://git.haproxy.org/?p=haproxy-1.9.git;a=commit;h=451c5a88 
> 
> 
> Maybe you've hit this bug.

Did this bug has been introduced in 1.9.4 ?
I haven't notice this behavior before.

> 
> -- 
> Christopher Faulet

Re: haproxy segfault

[Mildis]

> Le 12 févr. 2019 à 18:58, Aleksandar Lazic  a écrit :
> 
> Hi.
> 
> Can you activate coredumps
> 
> ulimit -c unlimited
> 
> you should find the core in
> 
> /tmp
> 
> or just search for core on the filesystem
> 

I'm struggling with Stretch/systemd to generate the coredump on crash.
Even running haproxy by hand with ulimit -c unlimited does not generate a 
coredump.

Even after a reboot, the +1e2000 offset is the same.

> You can get a backtrace with the following command as soon as you have a 
> coredump
> 
> gdb /usr/sbin/haproxy #YOUR_CORE_FILE#
> bt full
> 
>> Thanks,
>> mildis
> 
> Regards
> aleks

haproxy segfault

[Mildis]

Hi list,

haproxy is segfaulting multiple times these days for no apparent reason.
At first i thought is was a load issue but even few RPS made it crash.

Symptoms are always the same : segfault of a worker then spawn of a new.
If load is very high, spawned worker segfault immediatly.

In the messages log, the offset is always the same (+1e2000).

I'm running 1.9.4 (from vincent bernat package) in Debian stretch.

In haproxy logs :
Feb 12 11:36:54 ns3089939 haproxy[32688]: [ALERT] 042/113654 (32688) : Current 
worker #1 (32689) exited with code 139 (Segmentation fault)
Feb 12 11:36:54 ns3089939 haproxy[32688]: [ALERT] 042/113654 (32688) : 
exit-on-failure: killing every workers with SIGTERM
Feb 12 11:36:54 ns3089939 haproxy[32688]: [WARNING] 042/113654 (32688) : All 
workers exited. Exiting... (139)

In /var/log/messages
kernel: traps: haproxy[32689] general protection ip:561e5b799375 
sp:7ffe6fd3f2f0 error:0 in haproxy[561e5b72d000+1e2000]

"show errors" is empty.

How could I diagnose further without impacting production too much ?

Thanks,
mildis

Re: RSA and ECC not working as expected

[Mildis]

> Le 3 déc. 2018 à 23:05, Lukas Tribus  a écrit :
> 
> Hello Mildis,
> 
> 
> On Mon, 3 Dec 2018 at 22:19, Mildis  wrote:
>> 
>> Hi,
>> 
>> I'm using 1.8.14 and I tried to follow 
>> https://www.haproxy.com/blog/serving-ecc-and-rsa-certificates-on-same-ip-with-haproxy/
>>  but all I'm getting in the log is
> 
> I'd recommend to ignore this blog post. Haproxy can do ECC/RSA cert
> switching itself since some time now and I have some doubts about
> req.ssl_ec_ext still actually correctly matching ECC support with
> todays browsers and TLS stacks.
> 
> Read the docs about the crt keyword:
> https://cbonte.github.io/haproxy-dconv/1.8/configuration.html#5.1-crt
> 
> The gist of it is that:
> "crt example.pem" loads "example.pem.ecdsa" as ECC and
> "example.pem.rsa" as RSA certificate, and selects the correct one
> based on client support (by actually using the correct openssl
> features, not payload matching in TCP mode). This makes it easy to
> implement ECC/RSA switching without a dedicated TCP based
> frontend/backend.
> 
Thanks Lukas.
I knew I saw something like that in the docs since 1.6 but an official blog 
note had priority on my mind :)
Maybe amending the post could help others wandering around the web for a 
solution ...

RSA and ECC not working as expected

[Mildis]

Hi,

I'm using 1.8.14 and I tried to follow 
https://www.haproxy.com/blog/serving-ecc-and-rsa-certificates-on-same-ip-with-haproxy/
 
<https://www.haproxy.com/blog/serving-ecc-and-rsa-certificates-on-same-ip-with-haproxy/>
 but all I'm getting in the log is 

ssl-relay ssl-rsa/rsa 1/-1/0 0 SC 1/1/0/0/3 0/0

Currently I do not have an ECC cert so I'm doing tests with an RSA only backend

The relevant configuration is 

frontend ssl-relay
mode tcp
bind ${HAPROXY_VRRP}:443
bind ${HAPROXY_IPV4}:443
bind ${HAPROXY_IPV6}:443
default_backend ssl-rsa

backend ssl-rsa
mode tcp
server rsa unix@/var/run/haproxy/haproxy_ssl_rsa.sock send-proxy-v2

listen all-ssl
mode http
bind unix@/var/run/haproxy/haproxy_ssl_rsa.sock accept-proxy ssl crt 
company.crt 

#capture request  header Host len 50
#capture response header Location len 50
#capture request header User-Agent len 50

http-request set-header X-Forwarded-Proto https
http-request set-header X-Forwarded-Port 443
http-request set-header X-Forwarded-Host %[ssl_fc_sni]


http-response set-header Strict-Transport-Security max-age=31536000;\ 
includeSubDomains

acl secured_cookie res.hdr(Set-Cookie),lower -m sub secure
rspirep ^(Set-Cookie:.*) \1;\ Secure unless secured_cookie


The all-ssl section is where all the routing logic take place based on 
hdr(host) and path_beg combinations to use a specific backend.

Why is SC the only answer ?
Am I missing something ?

Thanks,
Mildis

Patch : remove useless test

[Mildis]

Hi,

Very minor patch : remove a useless test that always evaluate to false.



0001-MINOR-same-length-test-is-made-just-before.patch
Description: Binary data


Mildis

Re: PATCH : mux_h2 h2c pointer deref

[Mildis]

OK.
Here you are.


0001-BUG-MINOR-checks-queues-null-deref.patch
Description: Binary data


0001-BUG-MINOR-h2-null-deref.patch
Description: Binary data

Mildis

> Le 2 oct. 2018 à 04:23, Willy Tarreau  a écrit :
> 
> Hi,
> 
> On Sun, Sep 23, 2018 at 06:18:37PM +0200, Mildis wrote:
>> Hi,
>> 
>> Here is a patch for a null-deref.
>> It checks if h2c exists before working on it.
>> 
> 
> For these two patches, I'd prefer to have multiple exit labels
> than adding some "if" in the error exit path. It's important that
> the error path is clear, linear and without any ambiguity. For
> example, just add labels likes "fail_no_h2c" and "fail_no_queue"
> pointing to the return. That easily allows to later add intermediary
> branches if some other entries are allocated.
> 
> Willy

PATCH : checks : queues null-deref

[Mildis]

HI,

Here is a patch for a null-deref.
It checks if queues exists before working on it.


0001-BUG-MINOR-checks-queues-null-deref.patch
Description: Binary data


Mildis

PATCH : mux_h2 h2c pointer deref

[Mildis]

Hi,

Here is a patch for a null-deref.
It checks if h2c exists before working on it.



0001-BUG-MINOR-h2-null-deref.patch
Description: Binary data


Mildis

Re: IPv6 : bug in unique-id-format and hex transormation

[Mildis]

> Le 29 juin 2018 à 14:26, Mildis  a écrit :
> 
>> 
>> Le 29 juin 2018 à 04:51, Willy Tarreau  a écrit :
>> 
>> Hi,
>> 
>> On Thu, Jun 28, 2018 at 11:48:24AM +0200, m...@mildis.org wrote:
>>> 
>>> Hi,
>>> 
>>> When applying hex transform to an IPv6 in unique-id-format, the result is 
>>> an string full of zeros. unique-id-format %{+X}o\ 
>>> %ci:%cp_%fi:%fp_%Ts_%rt:%pid":D142_:01BB_5B348110_:0FC3"
>>> When hex transform is disabled, the IPv6 is printed.
>>> 
>>> Here is a patch that only applies hex transformation to IPv4 addresses.
>> 
>> Hmmm I get your point but then we should have 3 cases handled differently :
>> - IPv4 => hex conversion
>> - IPv6 => no conversion
>> - IPv4 in IPv6 => conversion of the IPv4 part.
> It hits me when I made the patch : wether I should choose the lazy way or the 
> thorough way.
> I did the former.
> 
> So the results should be :
> - 192.168.0.1 => C0A80001
> - 2001:db8:0:85a3::ac1f:8001 => 20010db885a3ac1f8001
> - :::192.168.0.1 => C0A80001
Or even 
2001:db8:0:85a3::ac1f:8001 => 20013Adb83A03A85a33A3Aac1f3A8001
:::192.168.0.1 => 3A3A3AC0A80001

> Applying address compression without commas is not feasible.
> The argument of saving space with hex will not be that obivous then.
> 
> Mildis
> 
> 
>> 
>> In practice it should still boil down to doing IPv4 vs IPv6 and encoding
>> the fields manually for IPv6 without the colons. Indeed, some people will
>> definitely expect the hexa conversion to put a 16-byte block at once and
>> not to insert colons that are used as port delimiters in their format,
>> especially for unique-id. So this should just have its own encoding format
>> for IPv6 addresses in my opinion.
>> 
>> Thanks,
>> willy

Re: IPv6 : bug in unique-id-format and hex transormation

[Mildis]

> Le 29 juin 2018 à 04:51, Willy Tarreau  a écrit :
> 
> Hi,
> 
> On Thu, Jun 28, 2018 at 11:48:24AM +0200, m...@mildis.org wrote:
>> 
>> Hi,
>> 
>> When applying hex transform to an IPv6 in unique-id-format, the result is an 
>> string full of zeros. unique-id-format %{+X}o\ 
>> %ci:%cp_%fi:%fp_%Ts_%rt:%pid":D142_:01BB_5B348110_:0FC3"
>> When hex transform is disabled, the IPv6 is printed.
>> 
>> Here is a patch that only applies hex transformation to IPv4 addresses.
> 
> Hmmm I get your point but then we should have 3 cases handled differently :
>  - IPv4 => hex conversion
>  - IPv6 => no conversion
>  - IPv4 in IPv6 => conversion of the IPv4 part.
It hits me when I made the patch : wether I should choose the lazy way or the 
thorough way.
I did the former.

So the results should be :
- 192.168.0.1 => C0A80001
- 2001:db8:0:85a3::ac1f:8001 => 20010db885a3ac1f8001
- :::192.168.0.1 => C0A80001

Applying address compression without commas is not feasible.
The argument of saving space with hex will not be that obivous then.

Mildis


> 
> In practice it should still boil down to doing IPv4 vs IPv6 and encoding
> the fields manually for IPv6 without the colons. Indeed, some people will
> definitely expect the hexa conversion to put a 16-byte block at once and
> not to insert colons that are used as port delimiters in their format,
> especially for unique-id. So this should just have its own encoding format
> for IPv6 addresses in my opinion.
> 
> Thanks,
> willy

Domain fronting

[Mildis]

Hi list,

I've been across several articles about new rules in domain fronting from AWS 
and Google.

Currently, I'm aware of 3 ways to get the destination host :

%[ssl_fc_sni,lower] # Layer 5
%[req.ssl_sni,lower] # Layer 6
%[req.hdr(host),lower] # Layer 7

Is there a simple way to limit TLS domain fronting by forcing SNI and Host 
header to be the same ?
Maybe add an optional parameter like "strict_sni_host" ?

Regards,
Mildis

Re: Diagnose a PD-- status

[Mildis]

Christopher,

We couldn’t had the error reproduced today : both dataset and client tool were 
updated.
However, I keep the configuration options at hand if I ever need to debug this 
issue further again.

Thanks for your time on this.
Regards,
Mildis


> Le 7 nov. 2017 à 10:14, Christopher Faulet <cfau...@haproxy.com> a écrit :
> 
> Le 06/11/2017 à 16:47, Mildis a écrit :
>> Hi Christopher,
>> Configuration is attached.
>> The domain2backend map sends data mostly to bck-traefik.
> 
> Hi Mildis,
> 
> At first glance, I don't see anything strange here. The compression filter is 
> not supposed to fail this way. So there is definitely something strange I 
> don't understand for now.
> 
> Could you reproduce the bug on a single request on an HAProxy instance 
> without load, maybe using a dedicated frontend ? If yes, it could be useful 
> to enable the trace filter adding following lines in your configuration, in 
> the used frontend section:
> 
>  filter trace name BEFORE
>  filter compression
>  filter trace name AFTER
> 
> Then, you can retry, starting HAProxy in foreground. The output is verbose, 
> but it give more information about the message filtering.
> 
> Thanks,
> -- 
> Christopher Faulet

Re: Diagnose a PD-- status

[Mildis]

Hi Christopher,

Configuration is attached.
The domain2backend map sends data mostly to bck-traefik.

$ haproxy -vv
HA-Proxy version 1.7.91~bpo7+1 2017/08/24
Copyright 2000-2017 Willy Tarreau <wi...@haproxy.org>

Build options :
  TARGET  = linux2628
  CPU = generic
  CC  = gcc
  CFLAGS  = -g -O2 -fPIE -fstack-protector --param=ssp-buffer-size=4 -Wformat 
-Werror=format-security -D_FORTIFY_SOURCE=2
  OPTIONS = USE_GETADDRINFO=1 USE_ZLIB=1 USE_REGPARM=1 USE_OPENSSL=1 USE_LUA=1 
USE_PCRE=1

Default settings :
  maxconn = 2000, bufsize = 16384, maxrewrite = 1024, maxpollevents = 200

Encrypted password support via crypt(3): yes
Built with zlib version : 1.2.7
Running on zlib version : 1.2.7
Compression algorithms supported : identity("identity"), deflate("deflate"), 
raw-deflate("deflate"), gzip("gzip")
Built with OpenSSL version : OpenSSL 1.0.1e 11 Feb 2013
Running on OpenSSL version : OpenSSL 1.0.1t  3 May 2016 (VERSIONS DIFFER!)
OpenSSL library supports TLS extensions : yes
OpenSSL library supports SNI : yes
OpenSSL library supports prefer-server-ciphers : yes
Built with PCRE version : 8.30 2012-02-04
Running on PCRE version : 8.30 2012-02-04
PCRE library supports JIT : no (USE_PCRE_JIT not set)
Built with Lua version : Lua 5.3.1
Built with transparent proxy support using: IP_TRANSPARENT IPV6_TRANSPARENT 
IP_FREEBIND

Available polling systems :
  epoll : pref=300,  test result OK
   poll : pref=200,  test result OK
 select : pref=150,  test result OK
Total: 3 (3 usable), will use epoll.

Available filters :
[COMP] compression
[TRACE] trace
[SPOE] spoe




haproxy.cfg
Description: Binary data


Mildis


> Le 6 nov. 2017 à 10:10, Christopher Faulet <cfau...@haproxy.com> a écrit :
> 
> Hi,
> 
> 
> Le 02/11/2017 à 17:16, Mildis a écrit :
>> [WARNING] 305/144718 (21260) : HTTP compression failed: unexpected behavior 
>> of previous filters
> 
> This warning is very suspicious. It should not happen. Could you share your 
> configuration and "haproxy -vv" output please ?
> 
> 
> -- 
> Christopher Faulet

Re: Diagnose a PD-- status

[Mildis]

I ran in debug mode and found the issue : 

156e:ft-public.clireq[000c:000d]: PUT 
/api/products/5/versions/5/documentations HTTP/1.1
156e:ft-public.clihdr[000c:000d]: X-CSRF-TOKEN: 
de035ec0-58a3-4668-9e43-e4b36911d2ff
156e:ft-public.clihdr[000c:000d]: Content-Type: application/json
156e:ft-public.clihdr[000c:000d]: accept: application/json
156e:ft-public.clihdr[000c:000d]: Content-Length: 12605
156e:ft-public.clihdr[000c:000d]: Host: edc-ci.geomath.fr
156e:ft-public.clihdr[000c:000d]: Connection: Keep-Alive
156e:ft-public.clihdr[000c:000d]: User-Agent: Apache-HttpClient/4.5.3 
(Java/1.8.0_131)
156e:ft-public.clihdr[000c:000d]: Cookie: 
CSRF-TOKEN=de035ec0-58a3-4668-9e43-e4b36911d2ff; 
JSESSIONID=8Xn2-NKJJuaMo-eI6c5PvSTwxNf5BLugv7e7rmes; 
remember-me=Y1luT2VGVXZnMHZvRWN6ZkluY3F6Zz09Olh4UmZmM3BpQVJxaVlZUmhWbkI1MlE9PQ
156e:ft-public.clihdr[000c:000d]: Accept-Encoding: gzip,deflate
156e:bck-traefik.srvrep[000c:000d]: HTTP/1.1 200 OK
156e:bck-traefik.srvhdr[000c:000d]: Cache-Control: no-cache, no-store, 
max-age=0, must-revalidate
156e:bck-traefik.srvhdr[000c:000d]: Content-Type: 
application/json;charset=UTF-8
156e:bck-traefik.srvhdr[000c:000d]: Date: Thu, 02 Nov 2017 13:47:18 GMT
156e:bck-traefik.srvhdr[000c:000d]: Expires: 0
156e:bck-traefik.srvhdr[000c:000d]: Pragma: no-cache
156e:bck-traefik.srvhdr[000c:000d]: Strict-Transport-Security: 
max-age=31536000 ; includeSubDomains
156e:bck-traefik.srvhdr[000c:000d]: X-Application-Context: 
application:prod,mysql:8081
156e:bck-traefik.srvhdr[000c:000d]: X-Content-Type-Options: nosniff
156e:bck-traefik.srvhdr[000c:000d]: X-Xss-Protection: 1; mode=block
156e:bck-traefik.srvhdr[000c:000d]: Transfer-Encoding: chunked
[WARNING] 305/144718 (21260) : HTTP compression failed: unexpected behavior of 
previous filters


Compression was enabled in the default section with :
   compression algo gzip
   compression type text/css text/html text/javascript 
application/javascript text/plain text/xml application/json

By commenting these two lines, it went OK.

However, I still can’t figure out what is causing this behavior : there are 
many other calls to the same URL.

Any clues ?

Regards,
mildis

> Le 1 nov. 2017 à 12:37, Mildis <m...@mildis.org> a écrit :
> 
> Hi,
> 
> I got a request ending in a PD status.
> However, ‘show errors’ does not tell anything about that.
> 
> backend server returned 200, haproxy returned 200 to the client.
> The entire request took 202ms and returned 15k of data : 3/0/0/199/202 200 
> 15262
> 
> Is there a way to diagnose further the PD status ?
> Maybe make haproxy log the reason why it ended in PD ?
> 
> Thanks,
> mildis

Diagnose a PD-- status

[Mildis]

Hi,

I got a request ending in a PD status.
However, ‘show errors’ does not tell anything about that.

backend server returned 200, haproxy returned 200 to the client.
The entire request took 202ms and returned 15k of data : 3/0/0/199/202 200 15262

Is there a way to diagnose further the PD status ?
Maybe make haproxy log the reason why it ended in PD ?

Thanks,
mildis

Use regex for backend selection

[Mildis]

Hi,

I’m in the process of setting HAProxy as an HTTPS frontend switch to different 
backends.
As I have 10+ different backends, I’d like to replace

acl to-server1 hdr_beg(host) -i server1.domain.tld
acl to-server2 hdr_beg(host) -i server2.domain.tld
…
acl to-serverN hdr_beg(host) -i serverN.domain.tld

use_backend bck-server1 if to-server1
use_backend bck-server2 if to-server2
…
use_backend bck-serverN if to-serverN

by something more generic like

use_backend bck-\1 if hdr_reg(host) -i (.*).domain.tld

but I can’t find a way to make it work.

Am I on the right path ?

Thanks,
Mildis

MINOR: ssl: close ssl key file on error

[Mildis]

Hi,

Please find attached a patch which corrects ssl_sock.c.

It closes explicitly the FILE opened to read the ssl key file when parsing 
fails to find a valid key.
Previous behavior : returned from the function after having set the error flags 
but not closed the file.

Regards,
Mildis


0001-MINOR-ssl-close-ssl-key-file-on-error.patch
Description: Binary data

Re: SonarSource static code analysis

[Mildis]

HI again,First analysis has been run.Results can be followed here :https://sonarqube.com/component_issues/index?id=haproxy#resolved=false|types=BUG%2CVULNERABILITYSome rules are just best practices that you could consider irrelevant, some are indications of code that could be refactored …Pick your favorites.Regards,MildisLe 17 juin 2016 à 18:01, Willy Tarreau <w...@1wt.eu> a écrit :Hi Mildis,On Thu, Jun 16, 2016 at 09:09:08PM +0200, Mildis wrote:Hi list, Hi Willy,At my job, we are using SonarSource???s SonarQube code analysis tool to get insights of the code health.This tool allows to highlight defects in the code which might go under the radar of sharp developper eyes.SonarQube is opening its platform to analyse opensource project, see https://sonarqube.com Number of opensource project are actively analyzed, whatever the language used.Integrating a new project in the SonarQube analysis requires the approval of the authors/maintainers of the project.It could be interesting for HAProxy to be analyzed so ???little hands??? could help solving bugs pointed by the tool.Do you think HAProxy could benefit this tool ?Will you agree for HAProxy to be included in the SonarSource portal ?Sure, as long as I'm not bugged by the tool's automatic reports :-)Thanks,Willy

SonarSource static code analysis

[Mildis]

Hi list, Hi Willy,

At my job, we are using SonarSource’s SonarQube code analysis tool to get 
insights of the code health.
This tool allows to highlight defects in the code which might go under the 
radar of sharp developper eyes.

SonarQube is opening its platform to analyse opensource project, see 
https://sonarqube.com <https://sonarqube.com/>
Number of opensource project are actively analyzed, whatever the language used.

Integrating a new project in the SonarQube analysis requires the approval of 
the authors/maintainers of the project.

It could be interesting for HAProxy to be analyzed so ‘little hands’ could help 
solving bugs pointed by the tool.

Do you think HAProxy could benefit this tool ?
Will you agree for HAProxy to be included in the SonarSource portal ?

Regards,
Mildis

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Hi,

I’ve reworked the patch.
No warnings whatsoever, wether using GCC or LLVM.
checkpatch is OK, no warnings either.
Buffers are secured and all loops exit naturally or with an index I’ve 
checked against off-by-one.


I’ve made a custom loop to remove the brackets : I found it easier than 
to do arithmetic around numerous strlen calls.
Also, I’ve declared the address buffer of with a size of 48 as it looks 
that INET6_ADDRSTRLEN is sometime define’d as 48, sometimes as 46.


--
Mildis

Le 2015-10-23 01:02, Willy Tarreau a écrit :

Hi,

On Thu, Oct 22, 2015 at 08:26:01PM +0200, Mildis wrote:

Hi Willy,

Here are two patches, rebased on current HEAD.
The first allows IPv6 with square brackets, the second parses colon to
look for a port.


OK thanks.

The logic is meant to be backward compatible with current behavior : 
as

you suggested, starting from the end, it looks for either a colon or a
closing bracket.
If a colon is found, it???s considered as a port separator, thus 
behaving

like previously.
If something else is found, either a closing bracket or nothing at 
all,

then it consider that no port were provided.

str2ip2 is then in charge of validating wether the string is a valid 
IP

address or not.

Malformed IP addresses, whatever its format, will be rejected.


Description looks good. However I'm still seeing mistakes in the 
patches :


Subject: [PATCH 1/2] OPTIM: config: allow square-bracketed literal 
IPv6.


To enhance readability, allows IPv6 literal addresses with square 
brackets.

Last colon is still mandatory for address/port separation.
---
 src/standard.c | 45 ++---
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..2e064b5 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -620,9 +620,31 @@ const char *invalid_domainchar(const char *name) {
 struct sockaddr_storage *str2ip2(const char *str, struct
sockaddr_storage *sa, int resolve)
 {
struct hostent *he;
+/* add 2 chars for square brackets */
+char tmpip[INET6_ADDRSTRLEN+2] = { '\0' };

There's no point initializing it here since you'll overwrite it later.

+/* point to str by default, if FQDN processing is required */
+char *tmpipptr = (char *)str;

This is strictly forbidden, never cast a const char * into a char *,
that's the best way to modify it without even getting a warning. Just
use a const char * for tmpipptr instead.

+/* check IPv6 with square brackets */
+if (str[0] == '[') {
+/* copy and strip the opening bracket */
+strncpy(tmpip, str+1, sizeof(tmpip));
+size_t iplength = strlen(tmpip);

This last line must have emitted a warning, please do not send patches
which add build warnings, fix them before sending. A warning is very
often an indication that something which has not yet broken will soon 
do.


You have a bug here, strncpy() can fill the whole string, and strlen()
will read past end. Always write a \0 on the last char after strncpy()
to enforce truncation (or reject too large inputs).

+if (tmpip[iplength - 1] != ']') {

Here you still forget to check that iplength is > 0 so you can
actually read tmpip[-1] if the string I pass to your parser is
simply "[".

Also, I find it strange that you first copy the string and then
try to parse it instead of doing the opposite. It's much easier
since you even need to know the length for the strncpy() anyway.

(...)
@@ -747,10 +769,11 @@ struct sockaddr_storage *str2ip2(const char
*str, struct sockaddr_storage *sa, i
  *  the first byte of the address.
  *- "fd@"=> an integer must follow, and is a file descriptor 
number.

  *
- * Also note that in order to avoid any ambiguity with IPv6 addresses, 
the ':'
- * is mandatory after the IP address even when no port is specified. 
NULL is
- * returned if the address cannot be parsed. The  and  
ports are

- * always initialized if non-null, even for non-IP families.
+ * IPv6 addresses can be declared with or without square brackets. 
Also note
+ * that in order to avoid any ambiguity with IPv6 addresses, the last 
colon ':'
+ * is mandatory even when no port is specified. NULL is returned if 
the address
+ * cannot be parsed. The  and  ports are always initialized 
if

+ * non-null, even for non-IP families.

This comment doesn't belong to this patch since the feature is brought 
by

the next patch (at least that's my understanding).

(..)
Subject: [PATCH 2/2] OPTIM: config: make last colon optional for IPv6 
literal

 square-bracketed notation

---
 src/standard.c | 28 ++--
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/src/standard.c b/src/standard.c
index 2e064b5..e93375c 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -879,12 +879,28 @@ struct sockaddr_storage *str2sa_range(const char
*str, int *low, int *high, char
else { /* IPv4 and IPv6 */
  

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Hi Willy,

Here are two patches, rebased on current HEAD.
The first allows IPv6 with square brackets, the second parses colon to 
look for a port.
The logic is meant to be backward compatible with current behavior : as 
you suggested, starting from the end, it looks for either a colon or a 
closing bracket.
If a colon is found, it’s considered as a port separator, thus behaving 
like previously.
If something else is found, either a closing bracket or nothing at all, 
then it consider that no port were provided.


str2ip2 is then in charge of validating wether the string is a valid IP 
address or not.


Malformed IP addresses, whatever its format, will be rejected.


--
Mildis

Le 2015-10-13 21:05, Willy Tarreau a écrit :

On Tue, Oct 13, 2015 at 08:10:03PM +0200, Mildis wrote:


Le 2015-10-10 15:49, Willy Tarreau a écrit :
>Hi,

>@@ -856,11 +883,28 @@ struct sockaddr_storage *str2sa_range(const char
>*str, int *low, int *high, char
>else { /* IPv4 and IPv6 */
>int use_fqdn = 0;
>
>-   port1 = strrchr(str2, ':');
>-   if (port1)
>-   *port1++ = '\0';
>-   else
>+   /* IPv6 wildcard */
>+   if (!strcmp(str2, "::")) {
>+   port1 = "";
>+   }
>
>You're changing the parser here, because it now accepts "::" in a place
>where a port was expected.

May I ask : isn???t the parser just a parser ? Making the caller???s
responsibility to use or not the ports returned either in the
sockaddr_storage or the 2 int depending on the context ? (???bind???
requires a port but ???server??s port is optional)
Is str2sa_range aware of the context it???s called ?


str2sa_range() parses ports, it relies on str2ip2() which only takes
addresses. So according to this it *is* the caller. Your change above
broke this because you're making the former ignore its own 
responsibility
to properly consider the port which is mandatory if the address 
contains

a colon. And that's precisely because str2sa_range() doesn't have to be
aware of the context it's called that we want it to behave 
consistently.


Regards,
WillyFrom 393ec4d4778c126e59fa7bd24e733bb32ca282a9 Mon Sep 17 00:00:00 2001
From: mildis <m...@mildis.org>
Date: Sat, 17 Oct 2015 15:38:51 +0200
Subject: [PATCH 1/2] OPTIM: config: allow square-bracketed literal IPv6.

To enhance readability, allows IPv6 literal addresses with square brackets.
Last colon is still mandatory for address/port separation.
---
 src/standard.c | 45 ++---
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..2e064b5 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -620,9 +620,31 @@ const char *invalid_domainchar(const char *name) {
 struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, int resolve)
 {
 	struct hostent *he;
+/* add 2 chars for square brackets */
+char tmpip[INET6_ADDRSTRLEN+2] = { '\0' };
+/* point to str by default, if FQDN processing is required */
+char *tmpipptr = (char *)str;
+
+/* check IPv6 with square brackets */
+if (str[0] == '[') {
+/* copy and strip the opening bracket */
+strncpy(tmpip, str+1, sizeof(tmpip));
+size_t iplength = strlen(tmpip);
+if (tmpip[iplength - 1] != ']') {
+/* if address started with bracket,
+ it should end with bracket */
+goto fail;
+}
+else  {
+/* switch to tmpip as we are working on a literal bracketed IPv6 */
+tmpipptr = tmpip;
+/* strip last bracket */
+tmpipptr[iplength-1] = '\0';
+}
+}
 
 	/* Any IPv6 address */
-	if (str[0] == ':' && str[1] == ':' && !str[2]) {
+	if (tmpipptr[0] == ':' && tmpipptr[1] == ':' && !tmpipptr[2]) {
 		if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
 			sa->ss_family = AF_INET6;
 		else if (sa->ss_family != AF_INET6)
@@ -631,7 +653,7 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
 	}
 
 	/* Any address for the family, defaults to IPv4 */
-	if (!str[0] || (str[0] == '*' && !str[1])) {
+	if (!tmpipptr[0] || (tmpipptr[0] == '*' && !tmpipptr[1])) {
 		if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
 			sa->ss_family = AF_INET;
 		return sa;
@@ -639,14 +661,14 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
 
 	/* check for IPv6 first */
 	if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family == AF_INET6) &&
-	inet_pton(AF_INET6, str, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
+	inet_pton(AF_INET6, tmpipptr, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
 		sa->ss_family = AF_INET6;
 		return sa;
 	}
 
 	/* then check for IPv4 */
 	if ((!sa->ss_fami

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Hi again,

Let’s go step by step.

Here is a first patch which allows square-brackets literals.
It does not change the port parser, just remove the brackets in str2ip2.
It expects the IP address to be either plain or surrounded with square 
brackets, thus if first and last chars are not respectively opening and 
closing square brackets, it fails.


I’m still trying to figure a good implementation of the port parser to 
make the colon optional for IPv6 addresses.


Regards,
--
Mildis

Le 2015-10-10 15:49, Willy Tarreau a écrit :

Hi,

On Sat, Oct 10, 2015 at 01:50:46PM +0200, Mildis wrote:

Here is a working patch for IPv6 literal with square brackets.
Tested with :
"2001:db8::1234:5678",
"2001:db8::1234:5678:",
"2001:db8::1234:5678:80",
"2001:db8::1234:5678:80:",
"::",
":::",
":::80",
"[2001:db8::1234:5678]",
"[2001:db8::1234:5678]:",
"[2001:db8::1234:5678]:80",
"[::]",
"[::]:",
"[::]:80"


There are several oddities and/or bugs in your implementation, please
look below :

diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..7b49332 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -622,7 +622,7 @@ struct sockaddr_storage *str2ip2(const char *str,
struct sockaddr_storage *sa, i
struct hostent *he;

/* Any IPv6 address */
-   if (str[0] == ':' && str[1] == ':' && !str[2]) {
+   if (str[0] == ':' && str[1] == ':' && (str[2]=='\0' || !str[2])) {

Here you have duplicated the same test on str[2], I don't know what
your original intent was but it's definitely not what you've done.

if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = AF_INET6;
else if (sa->ss_family != AF_INET6)
@@ -637,11 +637,38 @@ struct sockaddr_storage *str2ip2(const char
*str, struct sockaddr_storage *sa, i
return sa;
}

+   /* check IPv6 literal */
+   if (str[0] == '[') {
+   if (strrchr(str, ']') != NULL)

Just a hint, strrchr() is more expensive than strchr() since it needs 
to
walk till the end, so you should only use it when you have no other 
option.

Here it's useless since you're only checking whether you find a closing
bracket or not.

+   /* has opening AND closing bracket */
+   sa->ss_family = AF_INET6;
+   else
+   /* no closing bracket */
+   goto fail;
+   }
+
/* check for IPv6 first */
-   if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family
== AF_INET6) &&
-	inet_pton(AF_INET6, str, &((struct sockaddr_in6 
*)sa)->sin6_addr)) {

-   sa->ss_family = AF_INET6;
-   return sa;
+   if (!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family ==
AF_INET6) {
+   /* IPv6 literal with opening and closing bracket ? */
+   if (str[0] == '[' && strchr(str, ']') != NULL) {

Here you're doing the same check again as you did above, it would have 
been

better to save the result of the initial test and reuse it here.

+   /* strip the closing bracket */
+   char *tmpip6 = strdup(str);

You forgot to free() this strdup so you end up with a memory leak for 
each
address that you're parsing. Also, there's no point allocating then 
freeing
memory just to parse an address, an IPv6 address has a fixed maximum 
length
which is INET6_ADDRSTRLEN, so better declare a local variable of this 
size

(+1) and copy the string there. It also saves malloc checks.

+   if (tmpip6) {
+   *(strchr(tmpip6, ']')) = '\0';

Hint, it's the third time you look for this bracket :-)

Also this time it kills the first closing bracket, thus you don't know
if there's anything after it, which is a problem because it means that
this address will parse without error as 2001:db8::, ignoring the 1234
part and keeping 5678 as the port :

   [2001:db8::]:1234:5678

Better check at the begining if there's anything past the first closing
bracket and report an error.

+   /* if inet_pton is OK, it is a valid IPv6 
address */
+   if (inet_pton(AF_INET6, tmpip6+1, &((struct 
sockaddr_in6
*)sa)->sin6_addr)) {
+   sa->ss_family = AF_INET6;
+   return sa;
+   }
+   }
+   else
+   goto fail;
+   }
+   /* IPv6 without brackets */
+		else if (inet_pton(AF_INET6, str, &((struct sockaddr_in6 
*)sa)->sin6_addr)) {

+   sa->ss_family = AF_INET6;
+   

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Le 2015-10-10 15:49, Willy Tarreau a écrit :

Hi,



@@ -856,11 +883,28 @@ struct sockaddr_storage *str2sa_range(const char
*str, int *low, int *high, char
else { /* IPv4 and IPv6 */
int use_fqdn = 0;

-   port1 = strrchr(str2, ':');
-   if (port1)
-   *port1++ = '\0';
-   else
+   /* IPv6 wildcard */
+   if (!strcmp(str2, "::")) {
+   port1 = "";
+   }

You're changing the parser here, because it now accepts "::" in a place
where a port was expected.


May I ask : isn’t the parser just a parser ? Making the caller’s 
responsibility to use or not the ports returned either in the 
sockaddr_storage or the 2 int depending on the context ? (‘bind’ 
requires a port but ‘server’’s port is optional)

Is str2sa_range aware of the context it’s called ?

Regards,
Mildis

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Aw, man !
My C skills are so rusted :)

I’ll look at your comments and correct all this.

BTW, a bit off-topic : have you looked at a code-review server like 
gerrit ?

Quite useful for multi-round patchset submission like this one.

--
Mildis

Le 2015-10-10 15:49, Willy Tarreau a écrit :

Hi,

On Sat, Oct 10, 2015 at 01:50:46PM +0200, Mildis wrote:

Here is a working patch for IPv6 literal with square brackets.
Tested with :
"2001:db8::1234:5678",
"2001:db8::1234:5678:",
"2001:db8::1234:5678:80",
"2001:db8::1234:5678:80:",
"::",
":::",
":::80",
"[2001:db8::1234:5678]",
"[2001:db8::1234:5678]:",
"[2001:db8::1234:5678]:80",
"[::]",
"[::]:",
"[::]:80"


There are several oddities and/or bugs in your implementation, please
look below :

diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..7b49332 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -622,7 +622,7 @@ struct sockaddr_storage *str2ip2(const char *str,
struct sockaddr_storage *sa, i
struct hostent *he;

/* Any IPv6 address */
-   if (str[0] == ':' && str[1] == ':' && !str[2]) {
+   if (str[0] == ':' && str[1] == ':' && (str[2]=='\0' || !str[2])) {

Here you have duplicated the same test on str[2], I don't know what
your original intent was but it's definitely not what you've done.

if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
sa->ss_family = AF_INET6;
else if (sa->ss_family != AF_INET6)
@@ -637,11 +637,38 @@ struct sockaddr_storage *str2ip2(const char
*str, struct sockaddr_storage *sa, i
return sa;
}

+   /* check IPv6 literal */
+   if (str[0] == '[') {
+   if (strrchr(str, ']') != NULL)

Just a hint, strrchr() is more expensive than strchr() since it needs 
to
walk till the end, so you should only use it when you have no other 
option.

Here it's useless since you're only checking whether you find a closing
bracket or not.

+   /* has opening AND closing bracket */
+   sa->ss_family = AF_INET6;
+   else
+   /* no closing bracket */
+   goto fail;
+   }
+
/* check for IPv6 first */
-   if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family
== AF_INET6) &&
-	inet_pton(AF_INET6, str, &((struct sockaddr_in6 
*)sa)->sin6_addr)) {

-   sa->ss_family = AF_INET6;
-   return sa;
+   if (!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family ==
AF_INET6) {
+   /* IPv6 literal with opening and closing bracket ? */
+   if (str[0] == '[' && strchr(str, ']') != NULL) {

Here you're doing the same check again as you did above, it would have 
been

better to save the result of the initial test and reuse it here.

+   /* strip the closing bracket */
+   char *tmpip6 = strdup(str);

You forgot to free() this strdup so you end up with a memory leak for 
each
address that you're parsing. Also, there's no point allocating then 
freeing
memory just to parse an address, an IPv6 address has a fixed maximum 
length
which is INET6_ADDRSTRLEN, so better declare a local variable of this 
size

(+1) and copy the string there. It also saves malloc checks.

+   if (tmpip6) {
+   *(strchr(tmpip6, ']')) = '\0';

Hint, it's the third time you look for this bracket :-)

Also this time it kills the first closing bracket, thus you don't know
if there's anything after it, which is a problem because it means that
this address will parse without error as 2001:db8::, ignoring the 1234
part and keeping 5678 as the port :

   [2001:db8::]:1234:5678

Better check at the begining if there's anything past the first closing
bracket and report an error.

+   /* if inet_pton is OK, it is a valid IPv6 
address */
+   if (inet_pton(AF_INET6, tmpip6+1, &((struct 
sockaddr_in6
*)sa)->sin6_addr)) {
+   sa->ss_family = AF_INET6;
+   return sa;
+   }
+   }
+   else
+   goto fail;
+   }
+   /* IPv6 without brackets */
+		else if (inet_pton(AF_INET6, str, &((struct sockaddr_in6 
*)sa)->sin6_addr)) {

+   sa->ss_family = AF_INET6;
+   return sa;
+   }

Given the number of different paths to inet_pton() I guess it would be
simpler to just move str to where the actual address to be parsed is 
(eg:

either str or tmpip6) and fall back to the same call.
}

  

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Here is a working patch for IPv6 literal with square brackets.
Tested with :
"2001:db8::1234:5678",
"2001:db8::1234:5678:",
"2001:db8::1234:5678:80",
"2001:db8::1234:5678:80:",
"::",
":::",
":::80",
"[2001:db8::1234:5678]",
"[2001:db8::1234:5678]:",
"[2001:db8::1234:5678]:80",
"[::]",
"[::]:",
"[::]:80"

--
Mildis

Le 2015-10-06 17:47, Willy Tarreau a écrit :

Hi,

On Tue, Oct 06, 2015 at 05:34:07PM +0200, Mildis wrote:

Hi Willy,

My bad : in the doc, I didn???t get the « add a colon without a port 
to

end an address » trick.
That???s why I was lost at first.
The doc obviously says ???[:[port]]??? making the port 
optional
and an ending colon valid but it looks like a typo to me as ending an 
IPv6

address with a colon is not one of my habit.

The provided patch sent initially should handle square brackets but
still makes the colon mandatory even with no port.
I???ll rework it so it accepts :
- 2001:db8::1234: as IPv6 no port (for backward compatibility)
- [2001:db8::1234] as IPv6 no port
- [2001:db8::1234]:80 as IPv6 with port
but forbid ???[2001:db8::1234]:??? and ???2001:db8::1234???


You must not forbid "[2001:db8::1234]:" otherwise people will not adopt
the square brackets notation. Some of them probably already do things 
like

this :

  server srv1 $IP_SRV1:$PORT_SRV1

And fill the respective environment variables with either IPv4 or IPv6.
By preventing a clearly non-ambigous config from being parsed, you'll
simply make them stay away from this new syntax, which is the opposite
to what you're seeking.

And regarding "2001:db8::1234", you can't forbit it simply because you
don't know if 1234 is a port or not in this context, as you have 
reported.


Otherwise it's OK for me. Don't forget to update the doc accordingly!

WillyFrom 44390f0ab9295450635021e59eb9ef04b050b82e Mon Sep 17 00:00:00 2001
From: mildis <m...@mildis.org>
Date: Sat, 10 Oct 2015 12:51:07 +0200
Subject: [PATCH] OPTIM: check IPv6 literal with square brackets

---
 src/standard.c | 70 +++---
 1 file changed, 57 insertions(+), 13 deletions(-)

diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..7b49332 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -622,7 +622,7 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
 	struct hostent *he;
 
 	/* Any IPv6 address */
-	if (str[0] == ':' && str[1] == ':' && !str[2]) {
+	if (str[0] == ':' && str[1] == ':' && (str[2]=='\0' || !str[2])) {
 		if (!sa->ss_family || sa->ss_family == AF_UNSPEC)
 			sa->ss_family = AF_INET6;
 		else if (sa->ss_family != AF_INET6)
@@ -637,11 +637,38 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
 		return sa;
 	}
 
+	/* check IPv6 literal */
+	if (str[0] == '[') {
+		if (strrchr(str, ']') != NULL)
+			/* has opening AND closing bracket */
+			sa->ss_family = AF_INET6;
+		else
+			/* no closing bracket */
+			goto fail;
+	}
+
 	/* check for IPv6 first */
-	if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family == AF_INET6) &&
-	inet_pton(AF_INET6, str, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
-		sa->ss_family = AF_INET6;
-		return sa;
+	if (!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family == AF_INET6) {
+		/* IPv6 literal with opening and closing bracket ? */
+		if (str[0] == '[' && strchr(str, ']') != NULL) {
+			/* strip the closing bracket */
+			char *tmpip6 = strdup(str);
+			if (tmpip6) {
+*(strchr(tmpip6, ']')) = '\0';
+/* if inet_pton is OK, it is a valid IPv6 address */
+if (inet_pton(AF_INET6, tmpip6+1, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
+	sa->ss_family = AF_INET6;
+	return sa;
+}
+			}
+			else
+goto fail;
+		}
+		/* IPv6 without brackets */
+		else if (inet_pton(AF_INET6, str, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
+			sa->ss_family = AF_INET6;
+			return sa;
+		}
 	}
 
 	/* then check for IPv4 */
@@ -747,10 +774,10 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
  *  the first byte of the address.
  *- "fd@"=> an integer must follow, and is a file descriptor number.
  *
- * Also note that in order to avoid any ambiguity with IPv6 addresses, the ':'
- * is mandatory after the IP address even when no port is specified. NULL is
- * returned if the address cannot be parsed. The  and  ports are
- * always initialized if non-null, even for non-IP families.
+ * IPv6 addresses can be declared with or with square brackets. If not using
+ * square brackets, the last colon ':' is mandatory even when no port is
+ * specified. NULL is returned if the address cannot be parsed. The  and
+ *  ports are always initiali

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Hi Willy,

My bad : in the doc, I didn’t get the « add a colon without a port to 
end an address » trick.

That’s why I was lost at first.
The doc obviously says ‘[:[port]]’ making the port optional and 
an ending colon valid but it looks like a typo to me as ending an IPv6 
address with a colon is not one of my habit.


The provided patch sent initially should handle square brackets but 
still makes the colon mandatory even with no port.

I’ll rework it so it accepts :
- 2001:db8::1234: as IPv6 no port (for backward compatibility)
- [2001:db8::1234] as IPv6 no port
- [2001:db8::1234]:80 as IPv6 with port
but forbid ‘[2001:db8::1234]:’ and ‘2001:db8::1234’

Would it makes sense to you ?

mildis



Le 2015-10-06 11:11, Willy Tarreau a écrit :

On Mon, Oct 05, 2015 at 06:42:52PM +0200, Mildis wrote:

Le 2015-10-05 18:07, David du Colombier a écrit :
>>It looks like IPv6 parsing may lead to errors.
>>The logic cannot distinguish from ???2001:db8::1234:80??? as :
>>- a plain IPv6 address 2001:db8::1234:80
>>- IPv6 2001:db8::1234 on port 80
>
>As far I remember, to prevent this confusion, we made the final
>colon mandatory on IPv6 addresses. This way, anything before
>the final colon is the address.
>
>If you don't want to specify the port explicitly, the address
>ends with a colon.

Technically, it???s seem OK.
But it should be less haproxy-centric to stick to an RFC.


We support all the formats mandated by RFCs 1884, 2373, 3513,
and 4291. The address format doesn't mandate the use of a port
which is a transport-layer issue. Many (most) products have
historically supported : to group layer 3 and
layer 4 information in a same word. And haproxy is no exception
and has supported this for 15 years in all places where an
address was expected since the port was almost always there.
Thus when introducing IPv6, it was natural that only the address
part had to be distinguished between IPv4 and IPv6. At some point
we added the ability to support port-less addresses at certain
places (eg: server addresses). In order to leverage the ambiguity
it could raise, these ones had the trailing colon mandatory.

We could also decide to add support for brackets in the address
parser so that it's possible to write [address]:port like in URLs,
I'm fine with this, though I'm not the one who will spend time on
this right now.

And by the way I think that the format is perfectly clear in the
documentation.

Regards,
Willy

RE: OPTIM : IPv6 literal address parsing

[Mildis]

Thanks for your feedback.
I think it need improvement as I currently replace the closing bracket 
with a null directly in the string. Working on a copy and leave the 
original string as-is should be better.

Plus adding some logic here and there to match all cases ...
I’ll work on this asap.

Mildis


Le 2015-10-06 19:39, Lukas Tribus a écrit :

Hi Mildis,


And regarding "2001:db8::1234", you can't forbit it simply because 
you

don't know if 1234 is a port or not in this context, as you have
reported.


Sure. In this very specific case 1234 can’t be a port as 2001:db8:: is
then a subnet.


For the record: you can't know that, unless you know the subnetmask.

I can assign 2001:db8::/128 to a loopback and bind a service to it,
I can bind 2001:db8::/127 to one box and connect it to a box with
2001:db8::1/127 on the other side.

I can also configure 2001:db8::/16 on box on my private network,
where 2001:: is the subnet IP, not 2001:db8::.

A lot of valid configurations out there, you can't assume that all
configurations are simple and straightforward unicast LAN networks.

It is then the kernels job to reject binds to adresses it considers
invalid (for example due to subnetting), but the application does not
(and imho *must not*) be subnet aware.



Regarding the patch: I think this is very useful and I like the square
brackets very much. I'm always scratching my head when I see a ipv6 
bind
configuration in haproxy and the square brackets fix this 
interpretation

problem once and for all (for users that want to use them, others just
keep using current notation).


Thanks for this!



Regards,

Lukas

Re: OPTIM : IPv6 literal address parsing

[Mildis]

Le 2015-10-06 17:47, Willy Tarreau a écrit :

Hi,
You must not forbid "[2001:db8::1234]:" otherwise people will not adopt
the square brackets notation. Some of them probably already do things 
like

this :

  server srv1 $IP_SRV1:$PORT_SRV1

And fill the respective environment variables with either IPv4 or IPv6.
By preventing a clearly non-ambigous config from being parsed, you'll
simply make them stay away from this new syntax, which is the opposite
to what you're seeking.



Got it.



And regarding "2001:db8::1234", you can't forbit it simply because you
don't know if 1234 is a port or not in this context, as you have 
reported.


Sure. In this very specific case 1234 can’t be a port as 2001:db8:: is 
then a subnet.

Is a subnet a valid entry in this context ?




Otherwise it's OK for me. Don’t forget to update the doc accordingly!


I will.

Mildis

OPTIM : IPv6 literal address parsing

[Mildis]

Hi list,

It looks like IPv6 parsing may lead to errors.
The logic cannot distinguish from ‘2001:db8::1234:80’ as :
- a plain IPv6 address 2001:db8::1234:80
- IPv6 2001:db8::1234 on port 80

It always default to the latter, considering any valid word made only of 
digits as a port.

(see https://gist.github.com/mildis/4ab2c3ff3a9ad56c9970)

The proposed patch allows IPv6 literal addresses to be enclose in square 
brackets as per https://www.ietf.org/rfc/rfc2732.txt


MildisFrom 5ab1cdaffbd48d5776dc94365c7581c6b0b84124 Mon Sep 17 00:00:00 2001
From: mildis <m...@mildis.org>
Date: Mon, 5 Oct 2015 09:52:43 +0200
Subject: [PATCH] OPTIM/MINOR: decode IPv6 literal addresses

---
 src/standard.c | 28 
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/standard.c b/src/standard.c
index b5f4bf4..00af8dc 100644
--- a/src/standard.c
+++ b/src/standard.c
@@ -637,11 +637,31 @@ struct sockaddr_storage *str2ip2(const char *str, struct sockaddr_storage *sa, i
 		return sa;
 	}
 
+	/* check IPv6 literal */
+	if (str[0] == '[') {
+		if (strrchr(str, ']') != NULL)
+			sa->ss_family = AF_INET6;
+		else
+			goto fail;
+	}
+
 	/* check for IPv6 first */
-	if ((!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family == AF_INET6) &&
-	inet_pton(AF_INET6, str, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
-		sa->ss_family = AF_INET6;
-		return sa;
+	if (!sa->ss_family || sa->ss_family == AF_UNSPEC || sa->ss_family == AF_INET6) {
+		/* IPv6 literal with opening and closing bracket ? */
+		if (str[0] == '[' && strchr(str, ']') != NULL) {
+			/* strip the closing bracket */
+			*(strchr(str, ']')) = '\0';
+			/* if inet_pton is OK, it is a valid IPv6 address */
+			if (inet_pton(AF_INET6, str+1, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
+sa->ss_family = AF_INET6;
+return sa;
+			}
+		}
+		/* IPv6 without brackets */
+		else if (inet_pton(AF_INET6, str, &((struct sockaddr_in6 *)sa)->sin6_addr)) {
+			sa->ss_family = AF_INET6;
+			return sa;
+		}
 	}
 
 	/* then check for IPv4 */
-- 
1.8.4