Does the proxy_ip_parser not work with IPv6 addresses properly?

[victor-sudakov]

Dear Colleagues,

I have the proxy_ip_parser enabled in roadrunner 2.12.3

http:
  address: 0.0.0.0:8060
  middleware: ["http_metrics", "headers", "gzip", "proxy_ip_parser"]

I have noticed a problem: when the X-Real-IP header contains an IPv6 address, roadrunner logs this IPv6 address to its log correctly but does not set the $_SERVER['REMOTE_ADDR'] variable for the PHP workers, they receive the variable as empty. When however the X-Real-IP header contains an IPv4 address, the $_SERVER['REMOTE_ADDR'] variable as seen from the PHP workers has the expected value.

Is it a possible bug or some misconfiguration on my part?

A sample log entry with IPv6:

2023-03-27T03:14:53.913Z	INFO	http        	http log	{"status": 200, "method": "HEAD", "URI": "/YYY/XXXXXXXXXXXXX", "remote_address": "2a01:111:f400:fe5a::100", "read_bytes": 0, "write_bytes": 0, "start": "2023-03-27T03:14:53.432Z", "elapsed": "481.145642ms"}

[rustatian]

Hey @victor-sudakov 👋🏻
I double checked the tests, it seems that the RR is passing the value correctly to the PHP. I'm not sure what is $_SERVER, since the RR passes data to the PHP worker via JSON. It may be that the PHP worker is setting this value incorrectly, you can verify this by checking the RR's payload in the PHP worker.

[vladimir-vv]

Hello @rustatian

We use RR to launch Laravel app.
"laravel/framework", "version": "v9.52.4"
"spiral/roadrunner-laravel", "version": "v5.11.1"

I added error_log() into "vendor/spiral/roadrunner-http/src/HttpWorker.php"
in function waitRequest() to dump payload.

Result with IPv4:

{
  "remoteAddr": "X.X.196.87",
  "protocol": "HTTP/1.1",
  "method": "GET",
  "uri": "http://SITE.com/",
  "headers": {
    "Cf-Connecting-Ip": [
      "X.X.196.87"
    ],
    "X-Scheme": [
      "https"
    ],
    "User-Agent": [
      "curl/7.87.0"
    ],
    "X-Request-Id": [
      "899100e44f1f6d7da07580165192b934"
    ],
    "X-Real-Ip": [
      "X.X.196.87"
    ],
    "X-Forwarded-For": [
      "X.X.196.87"
    ],
    "X-Forwarded-Scheme": [
      "https"
    ],
    "Cf-Visitor": [
      "{\"scheme\":\"https\"}"
    ],
    "X-Forwarded-Host": [
      "SITE.com"
    ],
    "X-Forwarded-Proto": [
      "https"
    ],
    "X-Original-Forwarded-For": [
      "X.X.196.87"
    ],
    "Accept": [
      "*/*"
    ],
    "Cf-Ipcountry": [
      "RU"
    ],
    "X-Forwarded-Port": [
      "443"
    ],
    "Accept-Encoding": [
      "gzip"
    ],
    "Cf-Ray": [
      "7aed6de6aba12de4-DME"
    ],
    "Cdn-Loop": [
      "cloudflare"
    ]
  },
  "cookies": {},
  "rawQuery": "",
  "parsed": false,
  "uploads": null,
  "attributes": {}
}

Result with IPv6:

{
  "remoteAddr": "",
  "protocol": "HTTP/1.1",
  "method": "GET",
  "uri": "http://SITE.com/",
  "headers": {
    "Cf-Connecting-Ip": [
      "2001:19f0:5001:21dc::10"
    ],
    "X-Scheme": [
      "https"
    ],
    "User-Agent": [
      "curl/7.87.0"
    ],
    "X-Request-Id": [
      "2af82c002779440ff68431b5e9ac3fd7"
    ],
    "X-Real-Ip": [
      "2001:19f0:5001:21dc::10"
    ],
    "X-Forwarded-For": [
      "2001:19f0:5001:21dc::10"
    ],
    "X-Forwarded-Scheme": [
      "https"
    ],
    "Cf-Visitor": [
      "{\"scheme\":\"https\"}"
    ],
    "X-Forwarded-Host": [
      "SITE.com"
    ],
    "X-Forwarded-Proto": [
      "https"
    ],
    "X-Original-Forwarded-For": [
      "2001:19f0:5001:21dc::10"
    ],
    "Accept": [
      "*/*"
    ],
    "Cf-Ipcountry": [
      "NL"
    ],
    "X-Forwarded-Port": [
      "443"
    ],
    "Accept-Encoding": [
      "gzip"
    ],
    "Cf-Ray": [
      "7aed6717b818b926-AMS"
    ],
    "Cdn-Loop": [
      "cloudflare"
    ]
  },
  "cookies": {},
  "rawQuery": "",
  "parsed": false,
  "uploads": null,
  "attributes": {}
}

As you see "remoteAddr" field is empty in case of IPv6.
If you need any additional information we are ready to provide it.

[rustatian]

The function SplitHostPort already checks this case with the square brackets.

From an algorithmic point of view, this is basically the same: O(n) for the strings.Contains + O(1) for checking string[0] (SplitHostPort also checks the string[0] index) = O(n).

From a latency perspective, this is about 0.1% of PHP time. For example, IPC communication costs us 100ms, but adding net.ParseIP adds 1ms. If we optimize this to 0.5ms, we would still have 100.5ms instead of 101ms 😃.

I will release the final release candidate this Thursday so you can test the changes. Thanks a lot!

[vladimir-vv]

Sorry if I'm bothering you

func FetchIP(pair string) string {
	if !strings.ContainsRune(pair, ':') {
		return pair
	}
	addr, _, err := net.SplitHostPort(pair)
	if err == nil {
		return addr
	}
	// will be here in case if **pair** is just a IPv6 address without brackets
	return pair
}

If we suppose that pair is ~ golang RemoteAddr than it will be enough to return pair in case if SplitHostPort return error
What do you think?
;-)

[rustatian]

NP, you're not bothering me, feel free to ask questions/ping me 😃

I think that we should additionally check the provided IP address. In case if user uses only http plugin w/o a middleware, SplitHostPort is sufficient because it on a par with the RemoteAddr. So, any RemoteAddr would be parsed succesfully with the SplitHostPort. On the other hand, since proxy_ip_parser rewrites this address (RemoteAddr), we need to additionally verify it, since my super XFF/CF 🥲 parser might fail haha))

Just FYI, RoadRunner is not a replacement for the web server. Since we have some of its basic functionality, I don't intend to replace NGINX or other web servers with RoadRunner, which is an application server.

[rustatian]

@vladimir-vv BTW2: If you wanted to test this change -> all binaries that passed the tests might be found here: https://github.com/roadrunner-server/roadrunner/actions/runs/4552603327 (in the CI artifacts)

[vladimir-vv]

Looks like its working! 👍
Thank you!

[victor-sudakov]

Thank you @vladimir-vv for isolating the issue and suggesting a fix, and @rustatian for implementing it! Now this RR middleware is useful behind CloudFlare with IPv6.

[victor-sudakov]

RoadRunner is not a replacement for the web server. Since we have some of its basic functionality, I don't intend to replace NGINX or other web servers with RoadRunner, which is an application server.

Sure, RoadRunner works very well behind a Kubernetes ingress. It's ideal to serve Laravel apps.