Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve default configuration and reporting of Bandit email hijacking #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+30 −5
Open

Improve default configuration and reporting of Bandit email hijacking #20

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55da623
Print name to real Bandit settings module
jmurty Jul 24, 2019
f3c7594
Raise exception if BANDIT_EMAIL is unset or empty
jmurty Jul 24, 2019
8f3f7fe
Print emails automatically whitelisted by Bandit
jmurty Jul 24, 2019
0f7d0ff
Avoid potentially risky import of django.conf.settings
jmurty Jul 24, 2019
a22cb40
Drop use of locals()
jmurty Jul 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions ixc_django_docker/settings/email_bandit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import os

from django.conf import settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmurty is this necessary, or wise, considering that this file is itself part of the settings?

If the ADMINS and SERVER_EMAIL settings are already defined in our base settings file, we can just access it immediately. But since we only need these settings to print them as help text, it might be better to put those print statements (and import django.conf.settings) in https://github.com/ixc/ixc-django-docker/blob/master/ixc_django_docker/bandit.py instead?

In that module, the settings will be finalised and any other subsequent adjustments to those settings (e.g. in develop.py or project settings which haven't been imported yet) will be available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable, I'll move them @mrmachine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, except it doesn't look like the ixc_django_docker.bandit module is loaded at application start-up. It may be lazy-loaded and not actually called unless/until a site tries to actually send an email, which is far too late to be applying the sanity-checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrmachine Since moving the print and exception lines to ixc_django_docker leaves them unrun until too late, I have pushed a change to load settings from locals() instead of doing the risky import of django.conf.settings

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmurty I think we don't need to use locals(). Those two variables are always set in ixc_django_docker/settings/base.py. I don't think we need to support attempts to use individual optional split settings modules without also using the base split settings modules.

It's probably ok 99% of the time where it is, but there is still a possibility for the actual value used by email-bandit to differ from the value we report in the settings module, if local settings or some other settings module alters those values afterwards. We could add ixc_django_docker as an INSTALLED_APP and create a AppConfig.setup() method to report any settings sanity check data at startup?

Or just leave it where it is as it is an edge case, but we can drop the locals().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrmachine I have dropped the use of locals(). I'm not sure why you're opposed to them (?) but it should be fine either way.

I don't think it is worth adding a whole new ixc_django_docker app just to review, report on, and sanity-check settings. That's more complication than it's worth IMO

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmurty I am not against using locals(). You just mentioned in your commit message that this usage was a hack, and I was pointing out that either way it's unnecessary (as is the NameError check) because we can assume that the email_bandit.py split settings module will only ever be applied on top of the base.py split settings module, which will always defines both of these two variables (SERVER_EMAIL and ADMINS). The POST_OFFICE variable would still need to use locals() or NameError check, though.



# When loaded by django-split-settings __name__ gives us the *includer* file's
# name, not the name of this *included* file.
REAL_MODULE_NAME = ".".join([__package__, "email_bandit"])


# Hijack django-post-office backend if project is using that lib...
if 'POST_OFFICE' in locals():
HIJACKED_EMAIL_BACKEND = POST_OFFICE['BACKENDS']['default']
Expand All @@ -22,7 +30,7 @@
]
else:
BANDIT_EMAIL = None
print("%s: BANDIT_EMAIL = %r" % (__name__, BANDIT_EMAIL))
print("%s: BANDIT_EMAIL = %r" % (REAL_MODULE_NAME, BANDIT_EMAIL))

# Whitelist outgoing emails to these specific addresses or domains to let
# them through, instead of redirecting them to the BANDIT_EMAIL address.
Expand All @@ -34,9 +42,28 @@
for wl in os.environ['BANDIT_WHITELIST'].split(',')
if wl.strip()
]
print("%s: BANDIT_WHITELIST = %r" % (__name__, BANDIT_WHITELIST))
else:
print("%s: BANDIT_WHITELIST is not set" % __name__)
BANDIT_WHITELIST = []
print("%s: BANDIT_WHITELIST = %r" % (REAL_MODULE_NAME, BANDIT_WHITELIST))

# Print the additional emails whitelisted by Bandit by default, to make it
# clearer that this is what Bandit does. See logic in
# `bandit.backends.base:HijackBackendMixin.send_messages()`
admin_emails = [email for name, email in getattr(settings, 'ADMINS', [])]
server_email = getattr(settings, 'SERVER_EMAIL', 'root@localhost')
extra_whitelisted = admin_emails + [server_email]
print(
"%s: Emails automatically whitelisted by Bandit, from `settings.ADMINS` and"
" `settings.SERVER_EMAIL` = %r" % (REAL_MODULE_NAME, extra_whitelisted)
)

# Ensure that BANDIT_EMAIL is set appropriately: it is always required and
# must contain at least one value
if not BANDIT_EMAIL:
raise ValueError(
"BANDIT_EMAIL environment variable must be set with at least one"
" email address. If you do not want to hijack email, remove"
" 'email_bandit.py' from the BASE_SETTINGS environment variable")

# Make it clear that emails have been hijacked and from which site.
# NOTE: This only applies to emails sent with admin-specific methods:
Expand Down