Add --trusted-ip to log real client IP from X-Forwarded-For

[Jipok]

This option is very useful when working behind a reverse proxy.

Perhaps it should be enabled by default for the 127.0.0.1 address. But I'm not sure about that.

[Jipok]

Not exactly what was proposed in 38, but solves the same problem.

[g-rden]

I would suggest handling the string stuff like here

darkhttpd/darkhttpd.c

Lines 2202 to 2204 in d1e9e2d

	/* strip out query params */
	if ((end = strchr(conn->url, '?')) != NULL)
	*end = '\0';

Ending up with something like this

    const char *client_ip;
    char *log_ip = NULL;

...

    client_ip = get_address_text(&conn->client);
    /* Determine which IP to log */
    if (trusted_ip != NULL && conn->forwarded_for != NULL) {
        if (strcmp(client_ip, trusted_ip) == 0) {
            /* X-Forwarded-For can be a comma separated list.
               We want the first IP (the client), not the whole string. */
            char *end;
            if ((end = strchr(conn->forwarded_for, ',')) != NULL)
                *end = '\0';

            log_ip = xmalloc(strlen(conn->forwarded_for) * 3 + 1);
            logencode(conn->forwarded_for, log_ip);
        }
    }

Then log 'log_ip' and free if allocated.

Otherwise lgtm, but reverse proxy isn't my area.

[Jipok]

Done

[emikulic]

If you extract the parsing into a helper function, it'll be easier to write a unit-test...

[Jipok]

Well, I did it. Although I don't really like code bloat.

[emikulic]

Would you like to also write the unit test? ;)

[hhartzer]

I think this is a nice commit and it's definitely useful for some people. I don't mean to nitpick, but I feel that using getaddrinfo() might make for a more robust setup. Both for IP matching (make sure the trusted IP is the one connecting) and making sure the IP in the header is valid.

One case where this might be handy, is if say someone does --trusted-ip 192.168.001.001, or 2001:db8::1 and the expanded/compressed forms are compared, and don't match when they should. You can also potentially normalize IPs sent by the X-Forwarded-For header.

Doesn't mean this necessarily has to happen. Just sharing my 2 cents worth.

[Jipok]

Would you like to also write the unit test? ;)

I refactored the implementation and removed the helper function. But I wrote (using LLM, although I checked) tests for the logs. I think this is better. In any case, you can discard the commit and use the old one.

I feel that using getaddrinfo() might make for a more robust setup

I agree that strictly compliant IP parsing/canonicalization is more robust, but given darkhttpd's philosophy of being a simple, single-threaded, minimal server, pulling in full resolver logic for logging seems like overkill.
However, I will switch to strcasecmp to correctly handle IPv6 case sensitivity discrepancies.

[Jipok]

I should also note that darkhttpd truncates URL parameters, and only the "file name" appears in the logs. For a static server, parameters are generally unnecessary, but it's worth noting that other web servers don't behave this way. Ideally, the full URL should be displayed in the logs.