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

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

Open
si14 opened this issue Aug 4, 2021 · 5 comments
Open

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

si14 opened this issue Aug 4, 2021 · 5 comments
Labels
bug Something isn't working pinned The issue must not be marked as stale

Comments

@si14
Copy link

si14 commented Aug 4, 2021

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:

Screenshot 2021-08-04 at 20 14 57

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 #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
Copy link

stale bot commented Oct 3, 2021

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.

@stale stale bot added the wontfix This will not be worked on label Oct 3, 2021
@stale stale bot closed this as completed Oct 10, 2021
@si14
Copy link
Author

si14 commented Dec 15, 2021

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

@dunglas dunglas reopened this Dec 15, 2021
@stale stale bot removed the wontfix This will not be worked on label Dec 15, 2021
@dunglas
Copy link
Owner

dunglas commented Dec 15, 2021

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?

@dunglas dunglas added the pinned The issue must not be marked as stale label Dec 15, 2021
@SherinBloemendaal
Copy link

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?

@dunglas dunglas added the bug Something isn't working label May 17, 2023
aegypius added a commit to aegypius/mercure that referenced this issue Jun 1, 2023
dunglas added a commit that referenced this issue Jun 15, 2023
)

* feat(hub): add metadata to new subscriber log

Ref: #546

* feat(hub): display payload in new subscriber log only in debug mode

* test(hub): add a test with debug mode...

... to ensure payload is properly logged

* test(hub): make sure payload is present in debug mode only

* style(hub): fix golangci issues

- use gofumpt instead of go fmt

- declare testSubscribeLogs as an helper

* test: update tests to chez payload content

* style: add a blank line before return

* test(hub): debug subscription test

* test: unmarshal some json to mimic jwt payload content

* fix(hub): add payload if log level is debug

* test(hub): use assert.Equal instead of assert.True

* test: try again with a structure

* Revert "test: try again with a structure"

This reverts commit e4b283f.

* test: get rid of json.Unmarshal by declaring the resulting structure directly

* minor cleanup

---------

Co-authored-by: Kévin Dunglas <[email protected]>
@bobvandevijver
Copy link

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.

useForwardedHeadersHandlers = handlers.ProxyHeaders(compressHandler)

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pinned The issue must not be marked as stale
Projects
None yet
Development

No branches or pull requests

4 participants