-
Notifications
You must be signed in to change notification settings - Fork 687
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.
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
Disable Source Interface for Trusty when Trusty is end-of-life #4325
Conversation
@@ -18,7 +18,7 @@ def ignore_static(f): | |||
a static resource.""" | |||
@wraps(f) | |||
def decorated_function(*args, **kwargs): | |||
if request.path.startswith('/static'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would moving the org-logo route behind /static make more sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make code changes to create a Blueprint
whose /static
route is set to be behind /static/org
and whose content is exactly the single file that is uploaded. Doing this cleanly is actually just a little tricky, and our current code does not easily support this at the moment.
I think we should not merge this into develop and instead merge it directly into the 0.12 release branch. Adding it to develop just means code we will have to maintain until we completely yank all Trusty everything from the codebase. I think it is appropriate to deprecate Trusty code in develop and make Trusty specific changes just in the 0.12 branch. Isn't that the point of having multiple release branches like this? |
9877614
to
fdbdee9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as advertised. Left some thoughts about how this could be improved a bit.
securedrop/source_app/__init__.py
Outdated
@@ -106,6 +106,11 @@ def handle_csrf_error(e): | |||
for module in [main, info, api]: | |||
app.register_blueprint(module.make_blueprint(config)) # type: ignore | |||
|
|||
# Disables the app if the server is running Trusty past its EOL date. | |||
app.config['XENIAL_VER'] = "16.04" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding these as config options, we could still mock them by making them constants in the disable
module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
securedrop/source_app/disable.py
Outdated
def disable_ui(): | ||
if(platform.linux_distribution()[1] != app.config['XENIAL_VER'] and | ||
date.today() > app.config['TRUSTY_DISABLE_DATE']): | ||
g.locale = i18n.get_locale(app.sdconfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these added here because we haven't yet called the other before_request
handlers? If this was done to follow my suggestion in the original ticket of running this handler before all other handlers, that was to prevent any other code from being hit to minimize code paths on a possibly insecure instance. What we could do is split setup_g
into multiple smaller handlers that each do one thing like init_i18n1
and check_expires
and such. This would make some of the code more reusuable and put less logic into this handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, done
securedrop/tests/test_source.py
Outdated
@@ -21,6 +24,9 @@ | |||
from utils.instrument import InstrumentedApp | |||
|
|||
overly_long_codename = 'a' * (Source.MAX_CODENAME_LEN + 1) | |||
SOURCE_ENDPOINTS = ['main.index', 'main.lookup', 'main.generate', 'main.login', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to add other endpoints called allowed endpoints
for CSS and other static content and check that those endpoints aren't blocked by this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, done
Needed to internationalize the disabling of source interface, to remove logic from the disable class.
We need this for the ignore-static decorator to allow this endpoint to be served when disabling the source interface for EOL Trusty instances.
If the underlying OS is Ubuntu 14.04 (Trusty) and the date for Trusty's end-of-life has passed, we should shortcut the source interface application logic to display an internationalized page indicating that the source interface has been disabled, to preserve the security and anonymity of sources.
fdbdee9
to
d540f6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Source interface is disabled
- Any other URL path (except static assets) returns this new page
- No AppArmor errors in /var/log/syslog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran through the test plan, everything worked as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm based on visual review of the diff
Status
Ready for Review
Description of Changes
Fixes #4305: Disables the source interface for Trusty instances after Ubuntu 14.04 Trusty's EOL date (2019-04-30).
Testing
Target branch
Copy
Test on 0.12.1 (Trusty)
git checkout release/0.12.1
git cherry-pick -x <commit hash(es) in this PR>
make build-debs
molecule converge -s libvirt-staging
/var/www/securedrop/source_app/__init__.py:L111
(changeTRUSTY_DISABLE_DATE
to any past date)Next steps
Deployment
New and existing installs will be upgraded automatically though the securedrop-app-code deb package
Checklist
If you made changes to the server application code:
make ci-lint
) and tests (make -C securedrop test
) pass in the development containerIf you made non-trivial code changes: