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

Add a Directive to get a Proxied Client's IP Address (Closes #341) #342

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thekief
Copy link

@thekief thekief commented Feb 13, 2025

As written in #341, this patch adds functionality to obtain a client's IP address, if the request has been proxied using the Proxy Protocol.

Besides the check, if the option is enabled, I added a check if the Proxy Protocol is actually being used.

@thekief thekief changed the title Add a Directive to Proxied Client's IP Address (Closes #341) Add a Directive to get a Proxied Client's IP Address (Closes #341) Feb 13, 2025
@thekief
Copy link
Author

thekief commented Feb 13, 2025

Please let me know, if the whitespace changes in the readme should be removed. I made the changes using VS Codium and didn't know that it forces a newline encoding.

@airween
Copy link
Member

airween commented Feb 17, 2025

Please let me know, if the whitespace changes in the readme should be removed.

No worries, that's in a good place.

Anyway, thanks for the PR - I'm thinking about we should add new test cases for each new PR (which adds a new feature - like this). I mean please take a look at the regression test file of this repository, how could we make a config where we can see that the modified code does what the author wants. And this is important, because if someone in the future sends a new PR, we must be sure this behavior does not change. Do you have any idea?

@thekief
Copy link
Author

thekief commented Feb 19, 2025

Yeah, I have an idea for a test case. curl not only supports the Proxy Protocol 1, but also the option to set the client's IP address 2.

If that's alright with you, I would add 2 new endpoints to the test configuration:

  • one that returns the client's ip with the new directive enabled
  • another one hat returns the client's ip with the new directive disabled

What do you think of that?

Footnotes

  1. https://curl.se/docs/manpage.html#:~:text=also%20-x%2C%20--proxy.-,--haproxy-protocol,-(HTTP)%20Send%20a

  2. https://curl.se/docs/manpage.html#:~:text=--haproxy-clientip%20%3Cip%3E

@airween
Copy link
Member

airween commented Feb 19, 2025

Hi @thekief,

I tried to check your patch, but may be I'm doing something wrong, I can't see the client's IP in my log.

Here is my config:

server {
    listen 8088;
    server_name _;

    root /var/www/html;
    index index.html;

    modsecurity on;
    modsecurity_rules_file /etc/nginx/modsecurity_includes.conf;
    modsecurity_proxy_protocol_ip on;

    error_log /var/log/nginx/backend_error.log info;
    access_log /var/log/nginx/backend_acces.log;

    location / {
        try_files $uri $uri/ =404;
    }
}

server {
    listen          8080;
    server_name     proxytest;

    modsecurity off;
    location / {
        proxy_read_timeout 300;
        proxy_connect_timeout 300;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header X-Forwarded-For $remote_addr;
        proxy_set_header Host $host;
        proxy_set_header X-Forwarded-Proto $scheme;
        proxy_pass http://127.0.0.1:8088;
        proxy_http_version 1.1;
        proxy_set_header Upgrade $http_upgrade;
        proxy_buffering off;
        proxy_redirect http:// https://;
    }
}

As you can see I created a simple vhost for static files, there I turned on modsecurity, load rules and turned on your new feature. I also set a separated logs (both access and error).

Below that I created a proxy which sends all requests to that site.

Here is how I checked that:

curl --interface 172.35.40.12 -H "Host: proxytest" "http://localhost:8080/?q=/bin/bash"

where 172.35.40.12 is the IP of my physical interface, but the request sent to localhost (with that IP) with HTTP header Host: proxytest (see above the proxy config).

With this request I get this line in my error.log:

2025/02/19 21:04:29 [info] 255143#255143: *3 ModSecurity: Warning. Matched "Operator `ValidateByteRange' with parameter `38,44-46,48-58,61,65-90,95,97-122' against variable `ARGS:q' (Value: `/bin/bash' ) [file "/home/airween/src/coreruleset/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf"] [line "1723"] [id "920273"] [rev ""] [msg "Invalid character in request (outside of very strict set)"] [data "ARGS:q=/bin/bash"] [severity "2"] [ver "OWASP_CRS/4.12.0-dev"] [maturity "0"] [accuracy "0"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-protocol"] [tag "paranoia-level/4"] [tag "OWASP_CRS"] [tag "capec/1000/210/272"] [hostname "127.0.0.1"] [uri "/"] [unique_id "173999546951.849862"] [ref "o0,1o4,1v8,9t:urlDecodeUni"], client: 127.0.0.1, server: _, request: "GET /?q=/bin/bash HTTP/1.1", host: "proxytest"

Where should I see my client IP instead of 127.0.0.1 (which is the proxy's IP)?

Or may be I misunderstand something.

@thekief
Copy link
Author

thekief commented Feb 19, 2025

To clear up a small misunderstanding, the "Proxy Protocol" I am talking about, is specified in the listen-directive 1.

I will add an example configuration tomorrow.

Footnotes

  1. https://nginx.org/en/docs/http/ngx_http_core_module.html#:~:text=%20the%20proxy_protocol%20parameter%20(1.5.12)%20allows%20specifying%20that%20all%20connections%20accepted%20on%20this%20port%20should%20use%20the%20proxy%20protocol.%20

@airween
Copy link
Member

airween commented Feb 19, 2025

To clear up a small misunderstanding, the "Proxy Protocol" I am talking about, is specified in the listen-directive 1.

I will add an example configuration tomorrow.

Thanks - then I can help you to create test cases. And of course, examples are always helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants