Use of x-forwarder-for and metadata in "New subscriber" logs

[si14]

First of all, thanks for this awesome piece of software!

We've deployed the latest Mercure and it works great. However, there are two tiny ops snags in its logging as you can see on the screenshot:

Firstly, it uses IP of our LB, not the one from X-Forwarder-For. There is a very old issue about this that eventually got fixed https://github.com/dunglas/mercure/issues/114 , but I guess the fix was lost during the big rewrite.

Secondly, do you think it's possible to log JWT payload if it's present? According to the spec, we can grab it by subscribing to the subscription topic, but that would require a separate service just to log those payloads. "New subscriber" log entries are already there, so if payload can be logged as well it would be perfect.

Hopefully I don't miss anything from the docs that makes those points moot.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[si14]

I think the bot was too eager to close it :(

[dunglas]

Hi @si14,

Regarding X-Forwarded-For, I think that this should be handled at Caddy's level. It's probably already to do this kind of rewrite (but I'm not sure if it's a good idea).

Regarding logging JWT payload, indeed it's a good idea, especially now that Caddy will allow to easily filter and redact log fields. Do you want to try to open a PR?

[SherinBloemendaal]

How should i use it in Caddy? I've read something about the reverse_proxy directive that has a trusted_proxies option but i can't figure out how to use that in the mercure directive. What should i use for Caddy?

[bobvandevijver]

Just hit this as well.

@dunglas I believe the issue here is that the remote_addr is being logged, which comes from the net library where it is defined as just the address of the connection, without parsing anything.

I have configured the following for the global mercure options:

servers {
  trusted_proxies static private_ranges
}

This will log the actual client_ip when a connection is closed, where you can also see that the remote_ip is unchanged.

before:
"logger":"http.log.access.log0","msg":"handled request","request":{"remote_ip":"127.0.0.1","remote_port":"57950","client_ip":"127.0.0.1"
after:
"logger":"http.log.access.log0","msg":"handled request","request":{"remote_ip":"127.0.0.1","remote_port":"37192","client_ip":"130.89.x.y"

I am however not seeing the client_ip being available in the Request struct, which explains why it is not visible in the "New subscriber" and "LocalSubscriber disconnected" log lines.

I tried setting the use_forwarded_headers true directive for the mercure server as well, but that does not seem to have any effect. It seems to be handled in code, but I believe that to be the legacy server meaning @si14 was on the right track.

https://github.com/dunglas/mercure/blob/0e9cc025495dfef52e0fc2fb72ef99279a6434b7/handler.go#L253

This means that the legacy server did have the logic we need for correct proxy header handling, as that update the RemoteAddr property in the request. [StackOverflow] seems to agree that a handler is needed for this: 1, 2 & 3.

So, the actual question here is: can we get the option and handler implementation back?