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

Log file should [optionally] include IP addresses for failed login attempts #2824

Closed
3 tasks done
Aesir opened this issue Mar 16, 2021 · 11 comments
Closed
3 tasks done
Assignees
Milestone

Comments

@Aesir
Copy link

Aesir commented Mar 16, 2021

  • I am running the latest version - Version: v0.105.2
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

Problem Description

There are currently no overall limits or rate limits for failed login attempts, allowing someone to attempt to brute-force attack the server. There are standard fixes for this issue, e.g. fail2ban, but they require a timestamp and IP address of failed login attempts to be logged to a file. Currently, we can set log_file in AdguardHome.yaml to get most of this but even setting verbose to true does not give us the necessary IP address to ban.

Proposed Solution

Obviously, there are a lot of privacy-minded people using Adguard Home solely on private networks and VPNs, so not everyone would want an IP address of any sort logged. The ideal solution would be to make it so you could only log IPs on failed login attempts. Since these sorts of solutions tend to be behind reverse proxies it would be really nice to be able to say which header (e.g. X-Client-IP, X-Real-IP) to log the IP from as well. This is extremely necessary for those of us running on a VPS who don't want to bring our dns down to expose ports every time we want to check the stats or make a filter change.

Alternatives Considered

Adguard Home could implement its own rate limiting and banning. This wouldn't have the benefit of banning from other things running on the same server and would require a lot more work for the developers as well as configuration for users.

Additional Information

Currently, the log lines look like this:
2021/03/16 07:59:47 1#204 [info] Auth: invalid user name or password: name="admin"

@ameshkov
Copy link
Member

Tbh, I don't see any issues with logging IP addresses of failed attempts by default without any additional settings. Regardless of how privacy-minded people use AG, this may be a security issue.

Also, I guess it's time to have some protection against brute-force attacks: #2826

@Aesir
Copy link
Author

Aesir commented Mar 16, 2021

Tbh, I don't see any issues with logging IP addresses of failed attempts by default without any additional settings. Regardless of how privacy-minded people use AG, this may be a security issue.

If the user mistypes their password, their IP would end up in the log files. I agree that it's probably overboard for the vast majority of people but there are definitely people who are, and have reason to be, that paranoid.

@ameshkov
Copy link
Member

Well, it's their own instance after all.

@ameshkov
Copy link
Member

I guess we could just write this info in the debug level log, and keep it simple in INFO.

@Aesir
Copy link
Author

Aesir commented Mar 16, 2021

Something like the log level would probably work. I fully expected to see it when I turned verbose logging on.

@ainar-g ainar-g self-assigned this Apr 6, 2021
adguard pushed a commit that referenced this issue Apr 6, 2021
Updates #2824.

Squashed commit of the following:

commit 4457725
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 6 14:23:12 2021 +0300

    home: imp docs, spacing

commit 7392cba
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 6 14:10:12 2021 +0300

    home: print client ip after failed logins
@ainar-g
Copy link
Contributor

ainar-g commented Apr 6, 2021

Should be fixed on the edge channel as of snapshot 8746005. The message that is shown in the logs will look like:

2021/04/06 12:00:00 144103#134 [info] auth: failed to login user "john.doe" from ip "192.168.12.34"

Can you please check if our solution fixes the issue for you?

@Aesir
Copy link
Author

Aesir commented Apr 8, 2021

Sorry, I recently had a death in the family and won't be at my workstation for a week or two so I can't test it. The log line you posted looks perfect though.

@ainar-g
Copy link
Contributor

ainar-g commented Apr 9, 2021

Our condolences to you and your family.

I hope it's okay for me to close the issue for now. Please reopen if you find any issues with the implementation.

@ainar-g ainar-g closed this as completed Apr 9, 2021
@de-es
Copy link

de-es commented Feb 11, 2023

This feature seems to have been removed with d317e19

Was this intentional?

@ainar-g
Copy link
Contributor

ainar-g commented Feb 13, 2023

@de-es, we did not intend to remove any information form the log. Can you provide an example of a message that lacks information?

@de-es
Copy link

de-es commented Feb 13, 2023

I was hoping to get the ip address on a failed login attempt but it just states

[error] POST mydomain.com /control/login: invalid username or password

It is working though for a successful attempt:

[info] auth: user "admin" successfully logged in from ip 1.2.3.4

v0.108.0-b.26

heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
Updates AdguardTeam#2824.

Squashed commit of the following:

commit 4457725
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 6 14:23:12 2021 +0300

    home: imp docs, spacing

commit 7392cba
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Tue Apr 6 14:10:12 2021 +0300

    home: print client ip after failed logins
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants