Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

log-format %HPO variable should log default value (/) when request path is missing #2573

Closed
fabfurnari opened this issue May 20, 2024 · 8 comments
Labels
status: fixed This issue is a now-fixed bug. type: bug This issue describes a bug.

Comments

@fabfurnari
Copy link

fabfurnari commented May 20, 2024

Detailed Description of the Problem

Using the %HPO log-format variable doesn't log default path (/) in requests with missing path.

While these requests are quite uncommon, it could happen that some requests are formed just by the URL in absolute form, eg.

GET http://test.domain.com HTTP/1.1

In this case the %HPO is logged just as blank space.

Expected Behavior

In case of missing path in a (absolute-form) request, the default behavior should be to interpret it as /, as also suggested by https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.3

Comparing also this behavior with the path fetch:

[... omissis ...]
http-request set-var(txn.path) path
log-format "%HM %[var(txn.path)] %HV"
[... omissis ...]

In this case a missing path in the request is logged as GET - HTTP/1.1

Steps to Reproduce the Behavior

  1. With the following log-format configuration it's easy to trigger this, issuing a very simple request like:
echo 'GET http://localhost HTTP/1.1
' | nc localhost 80

The log line (see configuration below) in this case should be GET / HTTP/1.1 while it's actually GET HTTP/1.1

Do you have any idea what may have caused this?

No response

Do you have an idea how to solve the issue?

No response

What is your configuration?

global  
        log stdout format raw local0
        chroot /var/lib/haproxy
        stats socket /run/haproxy/admin.sock mode 660 level admin
        stats timeout 30s
        user haproxy
        group haproxy
        daemon

        # Default SSL material locations
        ca-base /etc/ssl/certs
        crt-base /etc/ssl/private

        # See: https://ssl-config.mozilla.org/#server=haproxy&server-version=2.0.3&config=intermediate
        ssl-default-bind-ciphers ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384
        ssl-default-bind-ciphersuites TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256
        ssl-default-bind-options ssl-min-ver TLSv1.2 no-tls-tickets

defaults
        log     global
        mode    http
        option  dontlognull
        log-format "%HM %HPO %HV"
        timeout connect 5000
        timeout client  50000
        timeout server  50000
        errorfile 400 /etc/haproxy/errors/400.http
        errorfile 403 /etc/haproxy/errors/403.http
        errorfile 408 /etc/haproxy/errors/408.http
        errorfile 500 /etc/haproxy/errors/500.http
        errorfile 502 /etc/haproxy/errors/502.http
        errorfile 503 /etc/haproxy/errors/503.http
        errorfile 504 /etc/haproxy/errors/504.http

listen test
        mode http
        bind :80 v4v6
        http-request return status 200 content-type "text/plain" string "OK!"

Output of haproxy -vv

HAProxy version 2.6.12-1+deb12u1 2023/12/16 - https://haproxy.org/
Status: long-term supported branch - will stop receiving fixes around Q2 2027.
Known bugs: http://www.haproxy.org/bugs/bugs-2.6.12.html
Running on: Linux 6.5.0-0.deb12.4-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.10-1~bpo12+1 (2023-11-23) x86_64
Build options :
  TARGET  = linux-glibc
  CPU     = generic
  CC      = cc
  CFLAGS  = -O2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wundef -Wdeclaration-after-statement -Wfatal-errors -Wtype-limits -Wshift-negative-value -Wshift-overflow=2 -Wduplicated-cond -Wnull-dereference -fwrapv -Wno-address-of-packed-member -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-clobbered -Wno-missing-field-initializers -Wno-cast-function-type -Wno-string-plus-int -Wno-atomic-alignment
  OPTIONS = USE_PCRE2=1 USE_PCRE2_JIT=1 USE_OPENSSL=1 USE_LUA=1 USE_SLZ=1 USE_SYSTEMD=1 USE_OT=1 USE_PROMEX=1
  DEBUG   = -DDEBUG_STRICT -DDEBUG_MEMORY_POOLS

Feature list : -51DEGREES +ACCEPT4 +BACKTRACE -CLOSEFROM +CPU_AFFINITY +CRYPT_H -DEVICEATLAS +DL -ENGINE +EPOLL -EVPORTS +GETADDRINFO -KQUEUE +LIBCRYPT +LINUX_SPLICE +LINUX_TPROXY +LUA -MEMORY_PROFILING +NETFILTER +NS -OBSOLETE_LINKER +OPENSSL +OT -PCRE +PCRE2 +PCRE2_JIT -PCRE_JIT +POLL +PRCTL -PROCCTL +PROMEX -QUIC +RT +SLZ -STATIC_PCRE -STATIC_PCRE2 +SYSTEMD +TFO +THREAD +THREAD_DUMP +TPROXY -WURFL -ZLIB

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

Built with multi-threading support (MAX_THREADS=64, default=16).
Built with OpenSSL version : OpenSSL 3.0.11 19 Sep 2023
Running on OpenSSL version : OpenSSL 3.0.11 19 Sep 2023
OpenSSL library supports TLS extensions : yes
OpenSSL library supports SNI : yes
OpenSSL library supports : TLSv1.0 TLSv1.1 TLSv1.2 TLSv1.3
OpenSSL providers loaded : default
Built with Lua version : Lua 5.3.6
Built with the Prometheus exporter as a service
Built with network namespace support.
Built with OpenTracing support.
Support for malloc_trim() is enabled.
Built with libslz for stateless compression.
Compression algorithms supported : identity("identity"), deflate("deflate"), raw-deflate("deflate"), gzip("gzip")
Built with transparent proxy support using: IP_TRANSPARENT IPV6_TRANSPARENT IP_FREEBIND
Built with PCRE2 version : 10.42 2022-12-11
PCRE2 library supports JIT : yes
Encrypted password support via crypt(3): yes
Built with gcc compiler version 12.2.0

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 multiplexer protocols :
(protocols marked as <default> cannot be specified using 'proto' keyword)
         h2 : mode=HTTP  side=FE|BE  mux=H2    flags=HTX|HOL_RISK|NO_UPG
       fcgi : mode=HTTP  side=BE     mux=FCGI  flags=HTX|HOL_RISK|NO_UPG
  <default> : mode=HTTP  side=FE|BE  mux=H1    flags=HTX
         h1 : mode=HTTP  side=FE|BE  mux=H1    flags=HTX|NO_UPG
  <default> : mode=TCP   side=FE|BE  mux=PASS  flags=
       none : mode=TCP   side=FE|BE  mux=PASS  flags=NO_UPG

Available services : prometheus-exporter
Available filters :
        [CACHE] cache
        [COMP] compression
        [FCGI] fcgi-app
        [  OT] opentracing
        [SPOE] spoe
        [TRACE] trace

Last Outputs and Backtraces

No response

Additional Information

No response

@fabfurnari fabfurnari added status: needs-triage This issue needs to be triaged. type: bug This issue describes a bug. labels May 20, 2024
@wtarreau
Copy link
Member

Thanks for this report! I'm wondering if it's %HPO or if there are other places internally where we're missing the path due to this. For example I'm wondering what %[path] retrieves or what is sent in :path when the request is fowarded over H2. Please note that we've performed some adjustments very recently in this area (3.0-dev). @capflam you may be interested in seeing if your latest updates regarding the path parsing are still compatible with this specific one.

@capflam
Copy link
Member

capflam commented May 24, 2024

indeed, there is no path. %[path] is empty. However, it is not an issue in H2 because the / is added by default when there is no path (or a * for OPTIONS method). I will check if it can be easily fixed.

@capflam capflam added status: reviewed This issue was reviewed. A fix is required. and removed status: needs-triage This issue needs to be triaged. labels May 24, 2024
@wtarreau
Copy link
Member

What I'm particularly interested in is to be certain that the recent restrictions on 3.0 will not break this.

@capflam
Copy link
Member

capflam commented May 24, 2024

I tested on the 3.0-HEAD. So it still works as before: %[path] is empty and the h2 pseudo header is properly filled with a slash. I must check if it may be fixed during the parsing to avoid to adapt the path at each step.

@wtarreau
Copy link
Member

OK thanks! At least it's not rejected as invalid :-)

haproxy-mirror pushed a commit that referenced this issue May 24, 2024
…zation

As stated in RFC3986, for an absolute-form URI, an empty path should be
normalized to a path of "/". This is part of scheme based normalization
rules. This kind of normalization is already performed for default ports. So
we might as well deal with the case of empty path.

The associated reg-tests was updated accordingly.

This patch should fix the issue #2573. It may be backported as far as 2.4 if
necessary.
@capflam
Copy link
Member

capflam commented May 24, 2024

It should be fixed now.

@capflam capflam added status: fixed This issue is a now-fixed bug. 2.4 This issue affects the HAProxy 2.4 stable branch. 2.6 This issue affects the HAProxy 2.6 stable branch. 2.8 This issue affects the HAProxy 2.8 stable branch. 2.9 This issue affects the HAProxy 2.9 stable branch. and removed status: reviewed This issue was reviewed. A fix is required. labels May 24, 2024
@capflam capflam removed the 2.9 This issue affects the HAProxy 2.9 stable branch. label Jun 6, 2024
FireBurn pushed a commit to FireBurn/haproxy that referenced this issue Jun 7, 2024
…zation

As stated in RFC3986, for an absolute-form URI, an empty path should be
normalized to a path of "/". This is part of scheme based normalization
rules. This kind of normalization is already performed for default ports. So
we might as well deal with the case of empty path.

The associated reg-tests was updated accordingly.

This patch should fix the issue haproxy#2573. It may be backported as far as 2.4 if
necessary.

(cherry picked from commit 8d2514e)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
FireBurn pushed a commit to FireBurn/haproxy that referenced this issue Jun 7, 2024
…zation

As stated in RFC3986, for an absolute-form URI, an empty path should be
normalized to a path of "/". This is part of scheme based normalization
rules. This kind of normalization is already performed for default ports. So
we might as well deal with the case of empty path.

The associated reg-tests was updated accordingly.

This patch should fix the issue haproxy#2573. It may be backported as far as 2.4 if
necessary.

(cherry picked from commit 8d2514e)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit eff074a)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
FireBurn pushed a commit to FireBurn/haproxy that referenced this issue Jun 8, 2024
…zation

As stated in RFC3986, for an absolute-form URI, an empty path should be
normalized to a path of "/". This is part of scheme based normalization
rules. This kind of normalization is already performed for default ports. So
we might as well deal with the case of empty path.

The associated reg-tests was updated accordingly.

This patch should fix the issue haproxy#2573. It may be backported as far as 2.4 if
necessary.

(cherry picked from commit 8d2514e)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit eff074a)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
(cherry picked from commit 98b157e)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
@capflam capflam removed 2.4 This issue affects the HAProxy 2.4 stable branch. 2.6 This issue affects the HAProxy 2.6 stable branch. 2.8 This issue affects the HAProxy 2.8 stable branch. labels Jun 12, 2024
@capflam
Copy link
Member

capflam commented Jun 12, 2024

Finally not backported to 2.4.

@capflam capflam closed this as completed Jun 12, 2024
@fabfurnari
Copy link
Author

thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: fixed This issue is a now-fixed bug. type: bug This issue describes a bug.
Projects
None yet
Development

No branches or pull requests

3 participants