Skip to content

Failed authentications are logged to level Error (#31968) #31983

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
wants to merge 1 commit into from
Closed

Failed authentications are logged to level Error (#31968) #31983

wants to merge 1 commit into from

Conversation

itzonban
Copy link
Contributor

@itzonban itzonban commented Sep 5, 2024

PR for issue #31968.

Failed authentications are now logged to level Error instead of Info.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 5, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Sep 5, 2024
@delvh
Copy link
Member

delvh commented Sep 5, 2024

But… it isn't an error.
I somewhat agree with increasing the level to Warning, but no higher than that.
I'm even torn if we shouldn't leave it as Info since especially on public instances your logs will get spammed with unnecessary logs you cannot change.

@itzonban
Copy link
Contributor Author

itzonban commented Sep 5, 2024

But… it isn't an error. I somewhat agree with increasing the level to Warning, but no higher than that. I'm even torn if we shouldn't leave it as Info since especially on public instances your logs will get spammed with unnecessary logs you cannot change.

I don't know how useful it is to argue about semantics in this case... Just read your post in the issue how you define error - I can see your point, that a failed login could just be an Info but on the other hand it is a failed login process in a code block which deals explicitly with errors... that's why I changed it to Error.

If you can live with raising it to Warning that would be also fine for me... my issue is, that on our productive system the log has a lot of unnecessary Info logs I don't need BUT I need to see the failed logins for fail2ban, therefore I would like to be able to raise the log level.

@lunny
Copy link
Member

lunny commented Sep 6, 2024

I agree those are not errors. Errors should be a system problem, but when inputting the wrong password, the system is right but the man inputting the password may made a mistaken. Or it's an attack. But whatever which one it is, it's not an error.

@itzonban
Copy link
Contributor Author

itzonban commented Sep 6, 2024

I agree those are not errors. Errors should be a system problem, but when inputting the wrong password, the system is right but the man inputting the password may made a mistaken. Or it's an attack. But whatever which one it is, it's not an error.

How about the the alternative setting it to Warning?

@itzonban itzonban closed this Sep 9, 2024
@itzonban itzonban deleted the write_login_error_to_loglevel_error branch September 9, 2024 19:03
lunny pushed a commit that referenced this pull request 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`.
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants