Feature: Log failed login attempts

[shamoon]

Proposed change

I think from a security standpoint this is a good idea to work with e.g. fail2ban or crowdsec even though reverse proxies might make this a little less critical. I tested this locally / remotely with a RP, e.g.

[2023-01-05 21:31:40,257] [INFO] [paperless.auth] Login failed for user `admin` from IP `x.x.x.x`

Fixes #407
Fixes #1456

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Other (please explain)

Checklist:

• I have read & agree with the contributing guidelines.
• If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
• If applicable, I have checked that all tests pass, see documentation.
• I have run all pre-commit hooks, see documentation.
• I have made corresponding changes to the documentation as needed.
• I have checked my modifications for any breaking changes.

[coveralls]

Pull Request Test Coverage Report for Build 3894439824

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 60 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.09%) to 92.546%

---

Files with Coverage Reduction	New Missed Lines	%
documents/views.py	20	95.27%
paperless/settings.py	40	83.78%

Totals
Change from base Build 3884791465:	-0.09%
Covered Lines:	5438
Relevant Lines:	5876

---

💛 - Coveralls

[tribut]

This seems insecure. When we trust x-forwarded-for, we would have to have some sort of list of trusted proxies. Otherwise an attacker could blacklist arbitrary IPs by including them in the x-forwarded-for header, leading to a DoS situation.

[shamoon]

Thanks @tribut , wasn’t aware of that. Can you explain how one might do that (in a way that can’t also be spoofed)?

[tribut]

Normally, you require a list of trusted proxies as a setting and then the code looks something like this terrible pseudocode (sorry, I'm on mobile rn):

ip = remote_addr
if remote_addr in config['trusted_proxies']:
  if 'x-forwarded-for' in headers:
    ip = headers['x-forwarded-for']

This implies that when trusted proxies are not configured, the header is always ignored.

[vanto]

Didn't think of this scenario either. You may consider to delegate the problem eg to https://github.com/un33k/django-ipware with trusted proxy IPs. Yet another dependency though.

[stumpylog]

My own 2 cents on this is users should use a reverse proxy for integrating security related things like fail2ban or crowdsec. Tools like Traefik already handle access logs and integrate with other tools, while having handled the odd cases like trusted proxies, etc.

[shamoon]

My own 2 cents on this is users should use a reverse proxy for integrating security related things like fail2ban or crowdsec. Tools like Traefik already handle access logs and integrate with other tools, while having handled the odd cases like trusted proxies, etc.

I agree except that at the moment I dont think that works unless you have auth integrated with your RP and are passing that same auth to ngx. Django gives status 200 on failed logins so Im not sure theres an obvious way that those tools would 'know' there was a failed login attempt. Please someone correct me if Im wrong.

django-ipware looks like a good solution to me but as evidenced by the above Im not an expert here. In general I'm happy to close this if others are not in favor.

[shamoon]

Ok I've updated this to use django-ipware which has support for trusted proxies, and a new (optional) setting PAPERLESS_TRUSTED_PROXIES which I think achieves this. Again, if consensus is to not merge its NBD to me.

[shamoon]

@tribut any other thoughts?

[codecov]

Codecov Report

Merging #2359 (07ec6ff) into dev (9893ae9) will increase coverage by 0.26%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##              dev    #2359      +/-   ##
==========================================
+ Coverage   92.53%   92.79%   +0.26%     
==========================================
  Files         145      147       +2     
  Lines        6306     6361      +55     
==========================================
+ Hits         5835     5903      +68     
+ Misses        471      458      -13     

Flag	Coverage Δ
backend	92.79% <96.00%> (+0.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/paperless/settings.py	83.96% <75.00%> (-0.14%)	⬇️
src/paperless/apps.py	100.00% <100.00%> (ø)
src/paperless/signals.py	100.00% <100.00%> (ø)
src/documents/serialisers.py	91.99% <0.00%> (+1.01%)	⬆️
src/documents/permissions.py	71.69% <0.00%> (+22.64%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.