Skip to content

Log failed LDAP authentication attempts#256

Open
JensH0rnung wants to merge 6 commits into
seibert-media:mainfrom
JensH0rnung:feat/log-failed-ldap-logins
Open

Log failed LDAP authentication attempts#256
JensH0rnung wants to merge 6 commits into
seibert-media:mainfrom
JensH0rnung:feat/log-failed-ldap-logins

Conversation

@JensH0rnung

Copy link
Copy Markdown
Contributor
  • Log failed LDAP authentication attempts for more helpful debugging
  • Preserves existing logging behavior while showing certain problems previously hidden at Debug-Level

Comment thread teamvault/apps/settings/config.py Outdated
'django_auth_ldap': {
'handlers': ['console'],
'level': level,
'level': 'DEBUG',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to hardcode this to DEBUG

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logger needs to be at DEBUG because that's the level django_auth_ldap uses for auth failures internally:
except self.AuthenticationFailed as e:
logger.debug("Authentication failed for %s: %s", self._username, e)
~ .venv/lib/python3.12/site-packages/django_auth_ldap/backend.py:379

I did not find a way around this if we want to surface these messages in production.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could for example add a config option like log_auth_failures which sets the LOGGING['loggers']['django_auth_ldap']['level'] to DEBUG if set (similar to insecure_debug_mode)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added in this commit:
7d8fa7e

Can be enabled via teamvault.cfg, could look like:
# enable DEBUG-level logging for failed LDAP authentication attempts including failure reason log_auth_failures_mode = enabled

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something to point out in the docs, out-of-scope here, but maybe worth keeping in mind for the docs refactor.
But we could (and maybe should) add this to the default config (default disabled?) in teamvault/apps/settings/config.py:447

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we should point this out, I added a reminder in our ticket.

Added it do the default config as well (disabled), thank you for mentioning:
aa388da

Comment thread teamvault/apps/settings/config.py Outdated
return (
'Authentication failed for' in record.getMessage()
or 'Rejecting empty password for' in record.getMessage()
or record.levelno >= logging.INFO

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also take a min_loglevel as a dict config argument for the init of this class, and then compare against that, instead of just hardcoding to >= INFO. Otherwise this might surface INFO log records that were previously suppressed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition, done:
5a59944

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants