Provide a callback for failed login attempts

[GoogleCodeExporter]

I use log_cmd() to log all activities on the server. Of special interest are 
failed login attempts.

However, by the time log_cmd() is called after a failed PASS command, the 
self.username attribute is cleared.

I would like a way to receive this username.

One way I can think of is to modify on_login() so that it receives a True/False 
flag indicating login failure or success. Since it already receives the 
username, this would allow logging of failed login attempts (along with the 
attempted username).

Original issue reported on code.google.com by btimby@gmail.com on 7 Dec 2011 at 4:26

[GoogleCodeExporter]

Here is a patch illustrating the change.

Original comment by btimby@gmail.com on 7 Dec 2011 at 4:39

Attachments:

• on_login-success.patch

[GoogleCodeExporter]

Another option would be to include the respcode/respstr similar to how 
log_cmd() works. Success or failure can be determined by the respcode.

Also, it might be useful to provide the username to the log_cmd() function, and 
just move the logging there. I can envision a use-case where the PASS command 
should be logged with the respcode/respstr and username along with all other 
commands (instead of handling as a special case).

In fact, thinking about this, it makes more sense to somehow send the username 
to log_cmd() instead of sending a success flag to on_login(). That is assuming 
log_cmd() is used for logging FTP commands and on_login() is intended for 
something else (creating a home directory or something).

I am fine either way, the point is that the username is useful information when 
logging FTP commands, but is unavailable for the PASS command since it is 
cleared.

Also, I noticed that if the PASS command is given without an argument, the 
username IS NOT cleared. If the wrong password is provided, the username IS 
cleared. I don't know if this behaviour is intended or not. It seems to me that 
a PASS command without a password argument should be treated as a failed login 
and clear the username.

Original comment by btimby@gmail.com on 7 Dec 2011 at 4:11

[GoogleCodeExporter]

After further discussion, I think the way to move forward on this one is to 
provide a new on_failed_login() method which will receive the username as a 
parameter. This will augment the current on_login(), which will continue to be 
called only for successful logins.

The call to on_failed_login() should be made within the auth_failed() callback 
that is invoked by ftp_PASS() for failed login attempts.

I will provide a new patch for this strategy.

Original comment by btimby@gmail.com on 21 Dec 2011 at 6:49

[GoogleCodeExporter]

Here is a patch implementing this. made a few small changes to support this.

1. I moved the clearing of self.username after CallLater() so it is available 
when the callback is set up.

2. I added username as a kwarg to the auth_failed() callback function.

3. I added FTPHandler.on_failed_login() as well as a call to it in 
auth_failed().

Original comment by btimby@gmail.com on 21 Dec 2011 at 7:01

Attachments:

• on_failed_login.patch

[GoogleCodeExporter]

Checked in as r939.
Differences with your patch:
- "password" is passed as a second argument
- method name is "on_login_failed" rather than "on_failed_login": it is more 
consistent with "on_login", which already exists

Original comment by g.rodola on 22 Dec 2011 at 10:44

• Changed title: Provide a callback for failed login attempts
• Changed state: FixedInSVN
• Added labels: Milestone-0.6.1, Usability, Version-0.6.0

[GoogleCodeExporter]

Is it a good idea to provide the user's password to a callback? I guess a 
malicious piece of code could probably gain access to the login method to read 
passwords anyway, but I am thinking that providing passwords to a callback 
increases attack vectors. Just a thought I had when reviewing this issue. 

Original comment by jlo...@gmail.com on 22 Dec 2011 at 10:50

[GoogleCodeExporter]

Original comment by g.rodola on 3 Jan 2012 at 11:40

• Added labels: Milestone-0.7.0

[GoogleCodeExporter]

0.7.0 is out. Closing this out as definitively fixed.

Original comment by g.rodola on 25 Jan 2012 at 7:24

• Changed state: Fixed