Skip to content

Failed authentications should be logged to log.Error instead of log.Info #31968

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

Closed
itzonban opened this issue Sep 3, 2024 · 4 comments
Closed
Labels
good first issue Likely to be an easy fix type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@itzonban
Copy link
Contributor

itzonban commented Sep 3, 2024

Feature Description

Hi,

it would be great if failed authentications would be logged to ERROR level instead of INFO.

routers/web/auth/auth.go:231:SignInPost() [I] Failed authentication attempt for username from 1.1.1.1:44774: user does not exist [uid: 0, name: username]

It is in fact an error and the INFO level output can still be a bit much in production, therefore it would be preferable to be able to lower the level to "warn" and still catch the failed logins for fail2ban etc.

If I see it correctly the change need to be done in routers/web/auth/auth.go in lines 227-252.
What do you think?

Best regards
Daniel

Screenshots

No response

@itzonban itzonban added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Sep 3, 2024
@techknowlogick techknowlogick added the good first issue Likely to be an easy fix label Sep 3, 2024
@yp05327
Copy link
Contributor

yp05327 commented Sep 4, 2024

Can you send a PR?

@itzonban
Copy link
Contributor Author

itzonban commented Sep 5, 2024

I just did

@delvh
Copy link
Member

delvh commented Sep 5, 2024

It is in fact an error

Precisely not.
An error is something Gitea did itself that failed.
But Gitea did not do anything here, other than handle the request (successfully!).
So, yes, I can agree with a change to WARNING, but the ERROR level seems overinflated.
Especially on larger instances, it will pollute your log with actions you cannot do anything against.

@itzonban
Copy link
Contributor Author

itzonban commented Sep 5, 2024

It is in fact an error

Precisely not. An error is something Gitea did itself that failed. But Gitea did not do anything here, other than handle the request (successfully!). So, yes, I can agree with a change to WARNING, but the ERROR level seems overinflated. Especially on larger instances, it will pollute your log with actions you cannot do anything against.

Ok, I see your point in how you define "error" in this case.
I will create another PR for WARNING tomorrow... if you are still fine with it.. shall I just close the old PR or can you delete it?

lunny pushed a commit that referenced this issue Sep 11, 2024
PR for issue #31968 
Replaces PR #31983 to comply with gitea's error definition

Failed authentications are now logged to level `Warning` instead of
`Info`.
@KN4CK3R KN4CK3R closed this as completed Sep 18, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Likely to be an easy fix type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests

5 participants