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.

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

calculated etag as sha256 of file on disk #4314

Merged
merged 12 commits into from
Apr 22, 2019
Merged

calculated etag as sha256 of file on disk #4314

merged 12 commits into from
Apr 22, 2019

Conversation

heartsucker
Copy link
Contributor

Status

Ready for review

Description of Changes

Fixes #4032

Add the column checksum to the submissions and replies tables. Use this value as the ETag header in the API responses.

Testing

CI covers most of this, but there is one part that needs manual review because of the complexity of injecting pytest fixtures across the subprocess boundary.

  1. ./bin/dev-shell
  2. ./bin/run-test -vv tests/test_alembic.py::test_upgrade_with_data[b58139cfdc8c]
  3. cat /tmp/test_rqworker.log
  4. Look for two failures of type OperationalError. The tables are missing because the app context in the alembic migration uses the prod values.

Additionally, the part of the migration is allowed to fail because it is covered by the fact that the columns are nullable and the checksum value is populated when the response is generated if it is missing.

Deployment

This PR has a migration and it should be checked that it is sufficiently robust.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

kushaldas
kushaldas previously approved these changes Apr 15, 2019
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

I have two small comments, otherwise, approved from me.

@@ -160,5 +160,10 @@ def __init__(self):
except AttributeError:
pass

if getattr(self, 'env', 'prod') == 'test':
Copy link
Contributor

Choose a reason for hiding this comment

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

I really wish this env value to be set in the top line in case env is missing in the old config. That way one less getattr call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never set a default value so we're kinda stuck with this for now :/

securedrop/worker.py Outdated Show resolved Hide resolved
emkll
emkll previously requested changes Apr 15, 2019
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Testing in the dev env was successful (etag header return is identical to the sha256sum of the encrypted file). I used the instructions provided in #4023 .

To ensure the headers are properly passed by Apache, also ran the test against staging VMs as follows:

[X] Build deb packages on this branch and provision staging VMs
[ ] Submit a file (:exclamation: see below)
[ ] Create Journalist account, add JI ATHS token to torrc and restart tor
[ ] Retrieve file via API via Journalist interface
[ ] Verify sha256sum of encrypted file is same as etag header

Submitting a file causes a server error, the contents of /var/log/apache2/source-error.log are as follows:

[Mon Apr 15 15:29:07.585403 2019] [wsgi:error] [pid 1496:tid 3761315505920] ERROR:flask.app:Exception on /submit [POST]
[Mon Apr 15 15:29:07.585429 2019] [wsgi:error] [pid 1496:tid 3761315505920] Traceback (most recent call last):
[Mon Apr 15 15:29:07.585433 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 2292, in wsgi_app
[Mon Apr 15 15:29:07.585437 2019] [wsgi:error] [pid 1496:tid 3761315505920]     response = self.full_dispatch_request()
[Mon Apr 15 15:29:07.585446 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1815, in full_dispatch_request
[Mon Apr 15 15:29:07.585450 2019] [wsgi:error] [pid 1496:tid 3761315505920]     rv = self.handle_user_exception(e)
[Mon Apr 15 15:29:07.585453 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1718, in handle_user_exception
[Mon Apr 15 15:29:07.585456 2019] [wsgi:error] [pid 1496:tid 3761315505920]     reraise(exc_type, exc_value, tb)
[Mon Apr 15 15:29:07.585459 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1813, in full_dispatch_request
[Mon Apr 15 15:29:07.585461 2019] [wsgi:error] [pid 1496:tid 3761315505920]     rv = self.dispatch_request()
[Mon Apr 15 15:29:07.585464 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1799, in dispatch_request
[Mon Apr 15 15:29:07.585467 2019] [wsgi:error] [pid 1496:tid 3761315505920]     return self.view_functions[rule.endpoint](**req.view_args)
[Mon Apr 15 15:29:07.585470 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/var/www/securedrop/source_app/decorators.py", line 12, in decorated_function
[Mon Apr 15 15:29:07.585473 2019] [wsgi:error] [pid 1496:tid 3761315505920]     return f(*args, **kwargs)
[Mon Apr 15 15:29:07.585475 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/var/www/securedrop/source_app/main.py", line 208, in submit
[Mon Apr 15 15:29:07.585478 2019] [wsgi:error] [pid 1496:tid 3761315505920]     store.async_add_checksum_for_file(sub)
[Mon Apr 15 15:29:07.585481 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/var/www/securedrop/store.py", line 210, in async_add_checksum_for_file
[Mon Apr 15 15:29:07.585484 2019] [wsgi:error] [pid 1496:tid 3761315505920]     current_app.config['SQLALCHEMY_DATABASE_URI'],
[Mon Apr 15 15:29:07.585486 2019] [wsgi:error] [pid 1496:tid 3761315505920]   File "/var/www/securedrop/worker.py", line 34, in enqueue
[Mon Apr 15 15:29:07.585489 2019] [wsgi:error] [pid 1496:tid 3761315505920]     return (self.__app or current_app).extensions[self.__EXT_NAME].enqueue(*nargs, **kwargs)
[Mon Apr 15 15:29:07.585492 2019] [wsgi:error] [pid 1496:tid 3761315505920] KeyError: 'rq-worker-queue’

@heartsucker
Copy link
Contributor Author

@emkll I'm pretty sure I fixed the bug your reported, but I am in molecule/vagrant hell and can't get things to boot right. I'll see if I can get that sorted out later.

@emkll
Copy link
Contributor

emkll commented Apr 16, 2019

Thanks @heartsucker the issue has been fixed.

Functional testing successful following this plan:

[X] Build deb packages on this branch and provision staging VMs
[x] Submit a file
[x] Create Journalist account, add JI ATHS token to torrc and restart tor
[x] Retrieve file via API via Journalist interface
[x] Verify sha256sum of downloaded file is same as etag header presented

Visual review of the diff looks good to merge from my perspective, but I'd like another developer to perform a visual review of this PR

kushaldas
kushaldas previously approved these changes Apr 17, 2019
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Tested again:

  • Build deb packages on this branch and provision staging VMs
  • Submit a file and a message
  • Create Journalist account, added ATHS token to torrc and restart tor
  • Retrieve file and message via API via Journalist interface
  • Verify sha256sum of downloaded file/message are the same as etag header presented

@redshiftzero redshiftzero mentioned this pull request Apr 17, 2019
1 task
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

lgtm @heartsucker, all comments were addressed after the removal of hasattr, approving based on visual review of the diff. thanks @emkll and @kushaldas for testing and review!

@redshiftzero redshiftzero merged commit 839415f into develop Apr 22, 2019
@redshiftzero redshiftzero deleted the sha256-etag branch April 22, 2019 18:54
@redshiftzero redshiftzero added this to the 0.13.0 milestone May 14, 2019
@rmol rmol mentioned this pull request May 22, 2019
17 tasks
@zenmonkeykstop zenmonkeykstop mentioned this pull request Jun 18, 2019
12 tasks
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.

Etag header set by Journalist API is not sha256sum of file
4 participants