Allow DELETE HTTP method for journalist interface #4023

emkll · 2019-01-08T20:29:26Z

Status

Ready for review

Description of Changes

Certain Journalist API operations require DELETE (e.g. delete or unstar a source). This will ensure that Apache allows the DELETE operation for new installs (or existing installs after an Ansible run).

Furthermore, this will disable Apache stripping Etag headers on the journalist interface in order to receive Etag information from the API (containing sha256 sum of file for integrity). Currently all Etag headers should be sha256sum:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 due to a bug tracked in #4032 .

From the test plan below, it is very clear that we need some integration tests in order to validate these configurations (tracked #4030)

Testing

Clean install scenario

Check out this branch
./securedrop-admin install
install completed without error
/etc/apache2/sites-enabled/journalist.conf contains expected changes (or run testinfra suite)
Create admin user for the app
Navigate to the source interface and submit a document/message
Attempt to delete source via API (see below for example commands)
The deletion was successful, DELETE method was allowed by Apache.

Upgrade scenario

Install SecureDrop 0.12.1
Check out this branch
./securedrop-admin install
install completed without error
/etc/apache2/sites-enabled/journalist.conf contains expected changes (or run testinfra suite)
Create admin user for the app
Navigate to the source interface and submit a document/message
Attempt to delete source via API

Example test commands for api (curl)

Ensure aths token is in torrc
Get auth token:

torify curl -X POST \
  http://$JOURNALIST_INTERFACE.onion/api/v1/token \
  -H 'Content-Type: application/json' \
  -d '{
    "username": "$USERNAME", 
    "passphrase": "$PASSPHRASE", 
    "one_time_code": "$TOTP"
}'

Use the above token to get list of sources (to obtain source_uuid):

torify curl -X GET \
  http://$JOURNALIST_INTERFACE.onion/api/v1/sources \
  -H 'Authorization: Bearer $YOUR_TOKEN_GOES_HERE' \
  -H 'Content-Type: application/json'

Get the source based on source_uuid obtained above:

torify curl -X GET \
  http://$JOURNALIST_INTERFACE.onion/api/v1/sources/$SOURCE_UUID \
  -H 'Authorization: Bearer $YOUR_TOKEN_GOES_HERE' \
  -H 'Content-Type: application/json'

Get the download URL of a file from thesource_uuid :

torify curl -O \
  http://$JOURNALIST_INTERFACE.onion/api/v1/sources/$SOURCE_UUID/submissions/$SUBMISSION_UUID/download \
  -H 'Authorization: Bearer $YOUR_TOKEN_GOES_HERE' \
  -H 'Content-Type: application/json'

Get the headers/Etag value :

torify curl -I \
  http://$JOURNALIST_INTERFACE.onion/api/v1/sources/$SOURCE_UUID/submissions/$SUBMISSION_UUID/download \
  -H 'Authorization: Bearer $YOUR_TOKEN_GOES_HERE' \
  -H 'Content-Type: application/json'

Should return the expected Etag value in the headers:

HTTP/1.1 200 OK
Date: Mon, 14 Jan 2019 19:22:02 GMT
Server: Apache
Content-Disposition: attachment; filename=1-better_wrap-msg.gpg
Cache-Control: public, max-age=43200
Expires: Tue, 15 Jan 2019 07:22:02 GMT
X-Frame-Options: DENY
Etag: "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
Cache-Control: no-store
Referrer-Policy: no-referrer
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
X-Download-Options: noopen
X-Content-Security-Policy: default-src 'self'
Content-Security-Policy: default-src 'self'
Last-Modified: Mon, 14 Jan 2019 18:54:00 GMT
Content-Length: 592
Content-Type: application/pgp-encrypted

Delete the source:

curl -X DELETE \
  http://$JOURNALIST_INTERFACE.onion/api/v1/sources/$SOURCE_UUID \
  -H 'Authorization: Bearer $YOUR_TOKEN_GOES_HERE' \
  -H 'Content-Type: application/json'

Ensure the source is deleted:

curl -X GET \
  http://$JOURNALIST_INTERFACE.onion/api/v1/sources \
  -H 'Authorization: Bearer $YOUR_TOKEN_GOES_HERE' \
  -H 'Content-Type: application/json'

Deployment

Due to the risks associated with modifying the Apache configuration and due to this change being opt-in for users of the SecureDrop Workstation, this change will initially require an Ansible run to be applied.

New installs: Via Ansible / securedrop-admin install
Existing installs/upgrades: Via Ansible / securedrop-admin install

Checklist

If you made changes to the system configuration:

Configuration tests pass

If you made non-trivial code changes:

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

redshiftzero · 2019-01-08T22:06:54Z

install_files/ansible-base/roles/app/templates/sites-available/journalist.conf

@@ -53,11 +53,11 @@ ErrorDocument 500 /error
 <Directory /var/www/>
  Options {{ apache_dir_options | default('None') }}
  AllowOverride None
-  <Limit GET POST HEAD>
+  <Limit GET POST HEAD DELETE>


I think we should remove the ErrorDocument directives above as part of this ticket. I'm advocating this because they override the API error handlers we have implemented in the application code. Recall these Apache directives in the journalist config are why we saw redirects in #3977 instead of nice JSON responses. Let me know if you see a reason to keep them in the Apache config?

After taking a look at the removal of the ErrorDocument directives, the default Apache pages for errors do not return the security headers that are configured for the journalist interface (i.e.: x-xss-protection referrer-policy ...) While these error pages are static, I think we should still provide the headers for those pages. However, this meas we will need to provide ErrorDocuments for each error (400, 401, 403, 404 and 500)

codecov-io · 2019-01-10T19:31:09Z

Codecov Report

Merging #4023 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #4023   +/-   ##
=======================================
  Coverage     84.7%   84.7%           
=======================================
  Files           43      43           
  Lines         2765    2765           
  Branches       300     300           
=======================================
  Hits          2342    2342           
  Misses         355     355           
  Partials        68      68

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c877f7...bbff078. Read the comment docs.

conorsch · 2019-01-10T23:15:32Z

Summarizing discussion today among @emkll, @redshiftzero, and myself: outstanding work here prior to merge is:

Adding functional tests to verify the behavior works as intended
Appending torify/curl calls to the manual QA test plan for upcoming releases
Opening a follow-up ticket to track stronger integration testing for these endpoints, since DELETE methods not allowed by apache journalist interface config #3977 will be closed by this PR. Progress on stronger integration testing is blocked by [functional testing] Support tor-browser-selenium tests against app-staging in CI #3661

redshiftzero · 2019-01-11T00:16:43Z

The automated (unit tests since functional tests need to get done in a followup) tests we need for the error handlers are in 9625c4c, one can verify that these tests work by commenting out the registration of the error handlers here and here.

redshiftzero · 2019-01-12T01:03:21Z

While I don't want to increase the scope of this PR further, we should also resolve #3877 with this PR. The reason is that this is another API bug that we have worked around for now on the SDK/client side, but we should resolve while we are requesting a securedrop-admin install run to enable the API.

emkll · 2019-01-15T17:33:03Z

Thanks for the tests @redshiftzero these look good, and thanks for opening the much-needed #4030 .I agree, let's expose Etags for Journalist interface. I have pushed a commit, tested, and updated the test plan accordingly. I've opened #4032 to track an issue with Etags.

This PR should now be ready for re-review.

emkll · 2019-01-18T14:44:18Z

Rebased on latest develop, this is ready for another review.

rmol · 2019-03-19T22:23:45Z

👍 I ran through both scenarios -- fresh install and upgrade (though I started at 0.12.0, not 0.11.0) -- and was able to verify the Etag hashes and delete a source through the API.

One note for anyone else testing this: since this PR was filed, we've started requiring Authorization: Token instead of Authorization: Bearer.

emkll · 2019-03-20T17:16:53Z

rebased on latest develop

redshiftzero · 2019-03-20T21:15:37Z

The testinfra failure here looks legit (test_apache_journalist_interface_vhost)

rmol · 2019-03-21T00:06:17Z

The testinfra failure here looks legit (test_apache_journalist_interface_vhost)

Yep. Just needs a space removed in the expected snippet:

diff --git a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py
index 1249b2c08..cc1a5341a 100644
--- a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py
+++ b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py
@@ -63,7 +63,7 @@ common_apache2_directory_declarations = """
 <Directory {securedrop_code}>
   Options None
   AllowOverride None
-   <Limit GET POST HEAD DELETE>
+  <Limit GET POST HEAD DELETE>
     Order allow,deny
     allow from {apache_allow_from}
   </Limit>

Certain Journalist API operations require DELETE (e.g. delete or unstar a source). This will ensure that Apache allows the DELETE operation for new installs (or existing installs after an Ansible run).

These directives were returning incorrect values for the Journalist API. Errors should be handled in the application to ensure headers are sent. The API should return appropirate error codes, and the web interface should always redirect to the login page.

The status codes in here are to ensure that the cases the ErrorDocument directives previously in the Apache journalist configuration are handled in the application code.

ETags are useful to the journalist API to ensure file integrity, and in the future would allow file download resumption. Disabling ETags is a defense against information leakage https://nvd.nist.gov/vuln/detail/CVE-2003-1418. Since inodes are no longer included by Apache by default, since 2.3.14, that Apache only exposes filesize and MTime by default (information readily available on the Journalist interface, that the Journalist Interface is behind ATHS, it should be safe to re-enable ETags on the journalist interface only.

emkll · 2019-03-21T19:36:51Z

Thanks for the reviews/comments, I've addressed the test failure described above.
@conorsch would you mind doing a visual review of this (specifically removal of the ErrorDocument directives).

conorsch · 2019-03-29T00:54:28Z

Changes look good. Based on the diff of the ErrorDocument removal, I'm inclined to agree with @emkll:

While these error pages are static, I think we should still provide the headers for those pages. However, this meas we will need to provide ErrorDocuments for each error (400, 401, 403, 404 and 500)

Let's revisit the error routes when we add new tests validating that the headers are set, even on error. Some of the functional testing work you've been doing in other contexts is relevant here, @emkll, although we could certainly get away with some barebones curl checks to cover our bases. More important, the ErrorDocument directive will likely never work for our needs, given the Client's expectation of full JSON results.

Since the changes are written affect only the Journalist Interface, no concerns with these changes going in as presented. Doing so will unblock pending work on the Client, and we can shore up the functional testing of the server side components post merge of the long-standing TBB work, when all of our functional tests will be running outside the VMs.

emkll requested review from conorsch, kushaldas and msheiny as code owners January 8, 2019 20:29

redshiftzero reviewed Jan 8, 2019

View reviewed changes

emkll added the PR: Pending additional work label Jan 10, 2019

emkll force-pushed the 3977-allow-delete-for-journalist-interface branch from ec5466e to bbff078 Compare January 10, 2019 19:11

emkll requested a review from heartsucker as a code owner January 10, 2019 23:52

emkll removed the PR: Pending additional work label Jan 15, 2019

emkll force-pushed the 3977-allow-delete-for-journalist-interface branch from 3575957 to ce48126 Compare January 18, 2019 14:43

emkll force-pushed the 3977-allow-delete-for-journalist-interface branch from ce48126 to 3ed7208 Compare March 20, 2019 17:15

emkll and others added 4 commits March 21, 2019 10:01

Allow DELETE HTTP method for journalist interface

3d5934e

Certain Journalist API operations require DELETE (e.g. delete or unstar a source). This will ensure that Apache allows the DELETE operation for new installs (or existing installs after an Ansible run).

tests: Add unit test verifying error handlers are in place

12977cc

The status codes in here are to ensure that the cases the ErrorDocument directives previously in the Apache journalist configuration are handled in the application code.

emkll force-pushed the 3977-allow-delete-for-journalist-interface branch from 3ed7208 to febee2b Compare March 21, 2019 14:29

conorsch approved these changes Mar 29, 2019

View reviewed changes

conorsch merged commit b6aacf9 into develop Mar 29, 2019

conorsch deleted the 3977-allow-delete-for-journalist-interface branch March 29, 2019 23:55

rmol mentioned this pull request May 22, 2019

Release Securedrop 0.13.0 #4439

Closed

17 tasks

redshiftzero mentioned this pull request Jun 6, 2019

Apache is removing ETag headers #3877

Closed

redshiftzero mentioned this pull request Apr 30, 2020

Document playbook run requirement for old installs freedomofpress/securedrop-workstation-docs#39

Closed

zenmonkeykstop mentioned this pull request Feb 18, 2021

Client should probe for valid server responses when started in online mode freedomofpress/securedrop-client#1219

Open

eloquence mentioned this pull request Dec 10, 2021

Large upload failure should result in comprehensible error message #6194

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow DELETE HTTP method for journalist interface #4023

Allow DELETE HTTP method for journalist interface #4023

emkll commented Jan 8, 2019 •

edited

Loading

redshiftzero Jan 8, 2019

emkll Jan 9, 2019

codecov-io commented Jan 10, 2019 •

edited

Loading

conorsch commented Jan 10, 2019

redshiftzero commented Jan 11, 2019

redshiftzero commented Jan 12, 2019

emkll commented Jan 15, 2019

emkll commented Jan 18, 2019

rmol commented Mar 19, 2019

emkll commented Mar 20, 2019

redshiftzero commented Mar 20, 2019

rmol commented Mar 21, 2019

emkll commented Mar 21, 2019

conorsch commented Mar 29, 2019

Allow DELETE HTTP method for journalist interface #4023

Allow DELETE HTTP method for journalist interface #4023

Conversation

emkll commented Jan 8, 2019 • edited Loading

Status

Description of Changes

Testing

Clean install scenario

Upgrade scenario

Example test commands for api (curl)

Deployment

Checklist

If you made changes to the system configuration:

If you made non-trivial code changes:

redshiftzero Jan 8, 2019

Choose a reason for hiding this comment

emkll Jan 9, 2019

Choose a reason for hiding this comment

codecov-io commented Jan 10, 2019 • edited Loading

Codecov Report

conorsch commented Jan 10, 2019

redshiftzero commented Jan 11, 2019

redshiftzero commented Jan 12, 2019

emkll commented Jan 15, 2019

emkll commented Jan 18, 2019

rmol commented Mar 19, 2019

emkll commented Mar 20, 2019

redshiftzero commented Mar 20, 2019

rmol commented Mar 21, 2019

emkll commented Mar 21, 2019

conorsch commented Mar 29, 2019

emkll commented Jan 8, 2019 •

edited

Loading

codecov-io commented Jan 10, 2019 •

edited

Loading