Opened 2 years ago

Closed 7 months ago

#20495 closed Cleanup/optimization (wontfix)

add login failure events to django.security logger

Reported by: ptone Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: apollo13 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#18616 added a signal for failed login attempts - a primary use-case for this was monitoring.

With #19866 we have the introduction of a django.security logger, and it would make more sense to log these events there.

I'd be +0 on also deprecating the signal, they are brand new, and probably don't have much traction. There may be non-monitoring uses of said signal though that I'm not thinking of.

Attachments (2)

logerr.diff (2.9 KB) - added by boylea 2 years ago.
Changes user login failure from signalling event to logging the failure
log_failed_login.diff (4.8 KB) - added by boylea 2 years ago.
Uses warning level for logging user login failures, provides extra context to logger, includes documentation

Download all attachments as: .zip

Change History (19)

Changed 2 years ago by boylea

Changes user login failure from signalling event to logging the failure

comment:1 Changed 2 years ago by boylea

I think the attached patch should work... this is the first time I have worked with the logger (and also my first OS patch!), so let me know if it works. Any feedback on how to do this better appreciated.

comment:2 Changed 2 years ago by ptone

  • Has patch set
  • Needs documentation set
  • Patch needs improvement set

Thanks for the patch!

This patch overall looks good, and needs only a few minor improvements.

deprecation:

We have a procedure we follow anytime we remove a feature, it is documented in a limited way here under "minor releases"

https://docs.djangoproject.com/en/dev/internals/release-process/

We don't have any real contributor documentation about how to actually write the deprecation - but searching for the "DeprecationWarning" in the code will lead you to some examples of the different levels.

As to whether we should deprecate the signal - that can be a harder call. My instinct is to remove it, as I was never really happy with the cleaning of potentially sensitive info we do given that any code can subscribe to a signal, and do potentially dumb things that might expose something by accident:

https://github.com/django/django/commit/7cc4068c4470876c526830778cbdac2fdfd6dc26#L0R38

The reasons in favor of keeping the signal are that there could be times you want to take some action, not just monitor (can't think of any ones that are a good idea though) and the signal captures the cleaned credentials providing more info (and we probably don't want to wholesale log those either).

The compromise is to do keep both (signal and log), but strongly suggest using the logging unless you have some strong need to take action based on the credentials.

I know you are just starting out, but what are your thoughts?

If we deprecate the logging signal, the warning would have to come not when the signal is issued, but when something connects to the signal - not sure how messy that gets to do - off the top of my head probably a custom signal subclass that wraps the 'connect' method.

logging levels:

logging levels across different python projects are an ambiguous mess! My esteemed colleague Mr. Gaynor has argued that there are only two levels - oops and OH NO.

In Django we sort of do this in that we have ERROR level set up to email admins, and is more of OH NO, while a failed login should probably just be a 'warning' level, so that would be a minor change.

Docs
You've added a feature!, we need to make sure people know about it. This kind of change would warrant a small mention in the release notes, and also some brief mention of its availability in the logging docs, or perhaps alternatively/additionally in the Auth docs. I can take a look if you need more pointers. If deprecation of the signal happens, that is an additional entry that goes into the deprecation timeline.

Bravo on including a test.

Trac housekeeping:
We have a bunch of meta-data around the tickets here, and so one thing you can do after submitting a patch is mark the ticket as "has patch"

https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/#has-patch

This helps direct other people who are willing to contribute by reviewing patches.

Thanks again for contributing!

comment:3 Changed 2 years ago by charettes

Concerning the logger, you should use a sublogger of 'django.security' such as 'django.security.FailedLogingAttempt'. Plus you should pass the message format and arguments to the logging method instead of the interpolated message.

I'm not convinced we should deprecate the signal since it was added to allow implementation of security features such as throttling in the first place.

comment:4 Changed 2 years ago by boylea

Thank you for the wonderful feedback. So it seems that including both logging the failed attempt, and signalling it, is the way to go here. If this feature is to be removed, then that can be another ticket. So I will include that change, and fix the use of the logger as suggested. For the release docs, should I just append a section to the latest (1.6.txt) release doc file, or put it under a bullet under the minor features title (or does this matter much)?

comment:5 Changed 2 years ago by ptone

actually, since the security logger is new, there is already a release note for this, probably don't need to call out this part of it in the existing release note. But the other docs will need some changes. I still think pointing out the logger in the signal docs would be good.

Changed 2 years ago by boylea

Uses warning level for logging user login failures, provides extra context to logger, includes documentation

comment:6 Changed 2 years ago by boylea

So, a couple of things here I am not sure about... if the 'username' key is not in the credentials, I add it as None, and include it in the extra content for the logger. I did this so that format strings could refer to it without getting a key error. I included a test for this situation. Also, I added a bit to the topics/logging documentation, but I am not sure if this is where it should go as this file seemed to be less specific, and not including subloggers.

comment:7 Changed 22 months ago by unaizalakain

Instead of:

if 'username' not in credentials:
    credentials['username'] = None

You could use:

credentials.setdefault('username', None)

The patch_logger function's docstring need to be updated because of the new formatstr argument.

The versionaddeds should also be updated to 1.7 (1.6 is feature frozen) and the one in django.security.* section should be placed above the update.

If you have the opportunity, create a PR in github (it makes it easier to comment).

comment:8 Changed 21 months ago by boylea

Thank you for your review. I made those changes and submitted a pull request.

https://github.com/django/django/pull/2013

All tests pass using sqlite

comment:9 Changed 21 months ago by apollo13

  • Cc apollo13 added

I am not sure if it's good idea to log the credentials to prevent leakage. Even though you try to clean them, I'd assume that everything passed to this function is sensitive information.

comment:10 Changed 21 months ago by boylea

Perhaps it is better to err on the side of caution with sensitive info. Do you think just logging a message like, "User login credentials invalid to all supplied authentication backends" would be sufficient?

comment:11 Changed 21 months ago by bouke

The use case for the logger would be to enable some sort of monitoring. Without being passed any information about the request (like credentials or ip addresses), the functionality of the logger would be severely limited.

comment:12 Changed 21 months ago by anonymous

That might be, but putting usernames/passwords in plaintext into a logfile is horrible -- depending on your current logging config this would happen with a change like this.

comment:13 Changed 10 months ago by berkerpeksag

  • Needs documentation unset
  • Patch needs improvement unset

I've opened https://github.com/django/django/pull/3433 to revise https://github.com/django/django/pull/2013.

Changes:

comment:14 Changed 10 months ago by timgraham

Without any details about the credentials, this seems of limited use. As a site owner, what value would I get out of "User login credentials invalid to all supplied authentication backends." messages in my logs? It seems like it would just be noise. Thoughts? If the warning message could be tied to the request so that you could, for example, detect a large number of failed login attempts from a particular IP, that seems potentially useful. It seems like building a tool for site owner's to implement such a policy would be better though rather than monitoring logs to deduce this information. If there's a use case I'm missing, let's expand the docs a bit.

comment:15 Changed 10 months ago by apollo13

It is of limited use, but the main issue is that we can't log credentials by default, this would have to be something you enable explicitly. Take sentry as an example, logging stuff there is often okay cause the logging (as opposed to errors) don't include sensitive data (hopefully). If we now log usernames and passwords that's a completely different story…

comment:16 Changed 10 months ago by timgraham

I guess I don't see the advantage of forcing a particular "not-that-useful" implementation on all users. We already have the signal which users could use to implement their own logging if they like. If someone else like this change though, I wouldn't stand in the way. Hopefully Preston can comment on his original thinking and whether or not the current proposal matches what he had in mind.

comment:17 Changed 7 months ago by timgraham

  • Resolution set to wontfix
  • Status changed from new to closed

Closing in absence of follow up to my last question.

Note: See TracTickets for help on using tickets.
Back to Top