fossar / selfoss

multipurpose rss reader, live stream, mashup, aggregation web application
https://selfoss.aditu.de
GNU General Public License v3.0
2.35k stars 343 forks source link

Higher loglevel for authentication errors #1463

Open heull001 opened 10 months ago

heull001 commented 10 months ago

Wrong username or password is currently logged on DEBUG-level. I think this information is more important, so should be logged on a higher level. I think NOTICE could be okay for this.

This would allow to use tools like fail2ban without creating gigantic logfiles.

jtojnar commented 10 months ago

Thanks for reporting, could you describe your use case a bit more?

I would expect that for it to be useful for fail2ban, we would need to log more info (e.g. an IP address), so just raising the level would not help.

Maybe it would be better to match web server logs which should already contain the response code, something like:

127.0.0.1 - - [15/Aug/2023:22:00:30 +0200] "POST /selfoss/login HTTP/1.1" 400 52

Unfortunately, the login endpoint always returns 200 OK status, and we cannot change it without breaking backwards compatibility.

So there is currently no nice way to distinguish failures just from that log. (Technically, you could rely on the fact that responses containing wrong username/password have size 52 bytes, while ones without an error field are 16 bytes long. But there is no guarantee that the response sizes will remain the same.)

Ideally, we would create a new API endpoint for signing in that uses separate response codes but that will require more thought (e.g. which response code to use, should we support HTTP authentication…)

Or you could change the following line to jsonError risking clients potentially crashing when incorrect credentials are entered:

https://github.com/fossar/selfoss/blob/cdc7f3e71a979a15575ff10e09673b4ae514c7f4/src/controllers/Authentication.php#L56

heull001 commented 10 months ago

I would expect that for it to be useful for fail2ban, we would need to log more info (e.g. an IP address), so just raising the level would not help.

Ah yes, my fail, it would need the IP-adress too ...