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

Fixes #3772, allows Authorization header to pass #3774

Conversation

kushaldas
Copy link
Contributor

Status

Ready for review.

Description of Changes

Fixes #3772

If deploying to Apache using mod_wsgi, the authorization header is not passed
through to a WSGI application by default, as it is assumed that
authentication will be handled by Apache, rather than at an application
level.

See http://www.django-rest-framework.org/api-guide/authentication/#apache-mod_wsgi-specific-configuration
for more details.

Testing

Make sure you have Tor service in your host and also added the hidservauth token in the torrc file to access the journalist interface. Also run the create-dev-data.py in the server to add some test data and the journalist account.

To execute the following script, create a python3 virtualenv, and then install the following 3 packages

pip install requests[socks] requests pyotp pysocks

Then add and execute the following script, after adding the right onion address for the server.

import requests
import pyotp
import json


def main():
    proxies = {"http": "socks5h://127.0.0.1:9050", "https": "socks5h://127.0.0.1:9050"}
    totp = pyotp.TOTP("JHCOGO7VCER3EJ4L")
    user_data = {
        "username": "journalist",
        "passphrase": "correct horse battery staple profanity oil chewy",
        "one_time_code": str(totp.now()),
    }

    r = requests.post(
        "http://SERVER.onion/api/v1/token",
        data=json.dumps(user_data),
        proxies=proxies,
    )

    token = r.json()
    print(token)
    a = {
        "Authorization": "token " + token["token"],
        "Content-Type": "application/json",
        "Accept": "application/json",
    }
    r = requests.get("http://SERVER.onion/api/v1/sources", headers=a, proxies=proxies)
    print(r.text)


if __name__ == "__main__":
    main()

Deployment

Any special considerations for deployment? Consider both:

1. Upgrading existing production instances.

@conorsch @redshiftzero @msheiny ^^ any tips on update of the existing instances?

2. New installs.

This should be fine.

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 changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

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

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

@conorsch
Copy link
Contributor

conorsch commented Sep 4, 2018

Our best bet for getting the change to land in prod as part of the nightly update is to use the postinst script for securedrop-app-code to insert the relevant line. That's a bit brittle, so we should inspect the history of the template in version control, and identify a stable pattern to target for the insertion.

@msheiny
Copy link
Contributor

msheiny commented Sep 4, 2018

Nothing to add here on-top of @conorsch 's suggestion - I really don't enjoy that as a long-term solution we really need #3136 but obv. in the vein of pressing forward lets add more tech debt!! 🎉

@conorsch
Copy link
Contributor

conorsch commented Sep 4, 2018

Appended a commit to add a postinst implementation. Suggested test plan to verify automatic updates:

  1. Use Linux workstation (for upgrade scenario).
  2. Run make build-debs on this feature branch to build debs.
  3. Run molecule converge -s upgrade and confirm no errors.
  4. Run molecule sideseffect -s upgrade and confirm no errors.
  5. Inspect /etc/apache2/sites-available/journalist.conf on the app server and confirm it includes the WSGIPassAuthorization line added by this PR.
  6. Use the posted by @kushaldas above to verify that the header has taken effect, and the API works.

@zenmonkeykstop, can you take this for a lap?

kushaldas and others added 2 commits September 4, 2018 11:31
If deploying to Apache using mod_wsgi, the authorization header is not passed
through to a WSGI application by default, as it is assumed that
authentication will be handled by Apache, rather than at an application
level.

See http://www.django-rest-framework.org/api-guide/authentication/#apache-mod_wsgi-specific-configuration
for more details.
In order to ensure config updates during scheduled nightly upgrades for
0.9.0, we must patch the Apache config in-place. The approach uses an
in-place substitution on the journalist vhost config file, checking
first for the presence of the line, and skipping the substitution is
it's already found.
@conorsch conorsch force-pushed the open_the_gate_for_authorization_header branch from 855dd91 to f3e81ca Compare September 4, 2018 18:31
@emkll emkll mentioned this pull request Sep 4, 2018
22 tasks
@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Sep 4, 2018

Ran through upgrade scenario from 0.8.0 to locally-built 0.9.0 debs, no errors.
Added test data and set up Tor proxy locally, ran test script. Response as expected.

LGTM!

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.

Great catch @kushaldas, thanks for the migration @conorsch and QA report @zenmonkeykstop - approving

@conorsch
Copy link
Contributor

conorsch commented Sep 4, 2018

CI failing with:

Makefile:20: recipe for target 'ci-go' failed
make: *** [ci-go] Terminated
Too long with no output (exceeded 10m0s)

That's #3779, so I'll rebase this branch, since the fix has already been merged into develop.

@conorsch
Copy link
Contributor

conorsch commented Sep 4, 2018

I'll rebase this branch

The commits here have already been cherry-picked with -x into the release branch as #3781. Rather than break those commit references, I'll wait till CI quiets down, then kick off this job again to see if we can get it into develop.

@conorsch
Copy link
Contributor

conorsch commented Sep 5, 2018

CI passed. These changes made it into rc4 earlier today as part of #3781. Merging into develop to stay in sync.

@conorsch conorsch merged commit eaa2d45 into freedomofpress:develop Sep 5, 2018
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.

5 participants