Log IP on SSH authentication failure for Built-in SSH server #13150
Conversation
elesiuta
force-pushed
the
ssh_log_ip
branch
2 times, most recently
from
6f437f8
to
b436c7d
Compare
GiteaBot
added
the
lgtm/need 1
Codecov Report
@@ Coverage Diff @@
## master #13150 +/- ##
==========================================
- Coverage 42.27% 42.23% -0.05%
==========================================
Files 708 708
Lines 77191 77196 +5
==========================================
- Hits 32634 32604 -30
- Misses 39198 39230 +32
- Partials 5359 5362 +3
Continue to review full report at Codecov.
|
techknowlogick
added
the
type/enhancement
|
Yes, please update docs in this PR |
lafriks
added
the
topic/security
also match failed authentication over command line
modules/ssh/ssh.go
Outdated
| @@ -124,11 +124,11 @@ func sessionHandler(session ssh.Session) { | |||
| // Wait for the command to exit and log any errors we get | |||
| err = cmd.Wait() | |||
| if err != nil { | |||
| log.Error("SSH: Wait: %v", err) | |||
| log.Error("SSH: Wait: %v Failed authentication attempt from %s", err, session.RemoteAddr()) | |||
There was a problem hiding this comment.
Are you sure that every error from cmd wait will be due to an authentication failure?
There was a problem hiding this comment.
Nope, maybe a check if err is exit status 1 but then err could also be other things and still be an authentication failure and I don't want to miss those. I'm not too familiar with the code base yet and only started using gitea this week (if my log file was longer, finding other cases might be easier).
Another option is prepend it with 'Possible' or omit 'Failed authentication attempt' entirely, leaving just the IP and leave it to the user to decide whether to include this in fail2ban.
modules/ssh/ssh.go
Outdated
| } | ||
|
|
||
| if err := session.Exit(getExitStatusFromError(err)); err != nil { | ||
| log.Error("Session failed to exit. %s", err) | ||
| log.Error("Session failed to exit. %s Failed authentication attempt from %s", err, session.RemoteAddr()) |
There was a problem hiding this comment.
I'm even less sure about this one, wasn't able to find an input that triggers this in my testing.
|
Ok so I think this is all in the wrong place. By the time the session handler is involved they have authenticated with the server as using a public key. You presumably want to catch the people who are failing to authenticate. In terms of errors from the session handler - those are more authorization errors and you'd be better off logging them in routers/private/serv.go |
Thank you! You're right, that place is much better. I've updated it to reflect this, and also added one more location to log the remote IP address, when trying to clone over HTTP for a user that does not exist. |
GiteaBot
added
lgtm/done
|
🚀 |
targets #13094
Line 127 logs the IP address when connecting with a key from a user that does not have permission for the repo they're trying to clone or the repo does not exist.
Line 188 logs the IP address when connecting with a key not recognized by the server.
I'm not sure which cases Line 131 will be hit (if any and if Line 127 already covers this), but including it seemed prudent.
The regex from this guide could also be updated to omit the 'for' so it matches this along with failed logins, should I include that in this pull request or make a new one? It could also be updated to match "invalid credentials from" in the log when cloning over HTTP.