Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add IP Addresses to Synapse log files #12014

Closed
SPiRiT369 opened this issue Feb 16, 2022 · 15 comments
Closed

Add IP Addresses to Synapse log files #12014

SPiRiT369 opened this issue Feb 16, 2022 · 15 comments

Comments

@SPiRiT369
Copy link

Logging clients IP Addresses is essential for security.

I use fail2ban to automatically block IP Addresses when I detect clients trying to bruteforce my server (e.g. guess mail server passwords).

With Synapse I can't do that because the log file records do not include source IP Address. For example:
2022-02-16 19:33:23,003 - synapse.handlers.auth - 1387 - WARNING - POST-4233 - Failed password login for user <user>

I think that it will be very useful (and also essential) to include IP addresses in the log file, at least in security-related events such as registrations and authentication failures.

Hiding IP Addresses from Synapse logs does not improve privacy, as the address is visible in the webserver's logs and also stored in the database. So I don't see any benefit of hiding it.

I am aware that ratelimiting is an option, but it's a different solution.

@DMRobertson
Copy link
Contributor

Synapse includes IP addresses and other data in the Processing lines as it finishes handling a given request. Look for lines matching the regex POST-4233.*Processed request.

@SPiRiT369
Copy link
Author

This is not good enough because:

  • I only log WARNING+ events. "Processed request" lines are only in INFO logs, which I don't want to enable.
  • The IP address should be in the same log line as "Failed password login for user " because that's how I can catch such events automatically.

@dklimpel
Copy link
Contributor

You can additionally configure your fail2ban to block IPs with error 403 at /_matrix/client/r0/login on your reverse proxy.

@SPiRiT369
Copy link
Author

Yes @dklimpel - if 403 is raised on auth failure only then this should work. But it is still something that should go thru synapse log. I want to have separate rule for synapse instead of modifying the webserver rule.

@dklimpel
Copy link
Contributor

My thought would be security and request blocking as early as possible.

It would probably make more sense to document all relevant log entries.
E.g.

  • Invalid username
  • Wrong password
  • Rate limiting

This is related to:

@erikjohnston
Copy link
Member

We're uncomfortable making it so that we change this particular line or make it log IP addresses for all lines by default. We would accept a patch for passing in the IP address as a optional field so that admins can configure their logging to include the IP address.

@richvdh
Copy link
Member

richvdh commented Feb 17, 2022

another option might be to make use of a structured log format (see #8683)

@erikjohnston
Copy link
Member

We're uncomfortable making it so that we change this particular line or make it log IP addresses for all lines by default. We would accept a patch for passing in the IP address as a optional field so that admins can configure their logging to include the IP address.

In fact, do we already support this? https://github.com/matrix-org/synapse/blob/develop/synapse/logging/context.py#L605

In which case adding ip_address the logger formatter might Just Work?

@SPiRiT369
Copy link
Author

SPiRiT369 commented Feb 17, 2022

In fact, do we already support this? https://github.com/matrix-org/synapse/blob/develop/synapse/logging/context.py#L605
In which case adding ip_address the logger formatter might Just Work?

How can I do it? I've tried with %(ip_address)s, and it didn't work (caused python error).

@erikjohnston
Copy link
Member

Oh, hmm, probably because not every log line has an IP address associated with it. If you're using py3.10 then I think you can use the new defaults param on the in built log formatter:

formatters:
    precise:
        (): logging.Formatter
        fmt: '%(asctime)s - %(name)s - %(lineno)d - %(levelname)s - %(request)s - %(ip_address)s - %(message)s'
        defaults:
            ip_address: "n/a"

Otherwise you could create a custom formatter class that handles ip_address not always being defined.

On the Synapse side: not sure if we should give defaults to the extra params?

@SPiRiT369
Copy link
Author

Oh wow, @erikjohnston, you solved the problem! 👍
Thank you very much for this solution and for your help!
Is that ok if I close this issue or should you do that as the one who provided solution?

@clokep
Copy link
Member

clokep commented Feb 18, 2022

I think we can go ahead and close it since we came to a solution. 👍

@clokep clokep closed this as completed Feb 18, 2022
@SPiRiT369
Copy link
Author

I've just noticed that although it DOES solve the problem, other events cause python errors. So I need to look further on this :(

@SPiRiT369
Copy link
Author

SPiRiT369 commented Mar 11, 2022

@clokep is it possible to re-open this case, as the previous solution is faulty? :/ This "feature" is a must.

@DMRobertson
Copy link
Contributor

DMRobertson commented Mar 17, 2022

I've just noticed that although it DOES solve the problem, other events cause python errors. So I need to look further on this :(

@clokep is it possible to re-open this case, as the previous solution is faulty? :/ This "feature" is a must.

@SPiRiT369 I think it'd be best if you can open a new issue, containing

  • the logging config which causes the problem,
  • the python errors you're seeing, and
  • instructions on how to reproduce them.

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

No branches or pull requests

6 participants