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

[wip] Add HTTP Alternative Service header to source, journalist interfaces when both v2 and v3 onion services are enabled #4715

Closed
wants to merge 3 commits into from

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented Aug 28, 2019

Status

work in progress

Description of Changes

Fixes #4630

Changes proposed in this pull request:

  • When both v2 and v3 are enabled, this PR adds an Alt-Svc header to send user traffic from v2 to v3

Testing

It's not possible as far as I can tell to determine that the Alt-Svc header is being used in Tor Browser (see my thoughts/explanation in #4630 for the full details) but I'll see if I can cook up something better on this for the 1.0.0 QA period.

For now, you can verify that the header is being set by provisioning staging and visiting the v2 onions. Enabling developer tools in Tor Browser you should see:

Screen Shot 2019-08-28 at 4 48 31 PM

To convince yourself that this is correct, you can read the spec or you can check out this site, which nicely shows via the color of the background in the page that is visited when the Alt-Svc is being used. It uses an Alt-Svc from Cloudflare:

Screen Shot 2019-08-28 at 4 49 05 PM

Bonus

I'm marking these as bonus because I think since the reviewer will be inspecting the diff doing this testing as part of 1.0.0 QA is OK:

  1. v2_onion_services=False, v3_onion_services=True -> No Alt-Svc header
  2. v2_onion_services=True, v3_onion_services=False -> No Alt-Svc header

Deployment

This will be ran the next time an administrator runs the Ansible playbook. The

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make -C securedrop test) pass in the 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 - well, I validated the non-bonus steps

@codecov-io
Copy link

codecov-io commented Aug 28, 2019

Codecov Report

Merging #4715 into develop will increase coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4715      +/-   ##
===========================================
+ Coverage    82.38%   82.67%   +0.28%     
===========================================
  Files           46       45       -1     
  Lines         3162     3122      -40     
  Branches       345      338       -7     
===========================================
- Hits          2605     2581      -24     
+ Misses         470      454      -16     
  Partials        87       87
Impacted Files Coverage Δ
securedrop/securedrop/journalist_app/__init__.py 87.87% <0%> (-0.7%) ⬇️
securedrop/securedrop/journalist_app/main.py 75% <0%> (-0.5%) ⬇️
securedrop/securedrop/journalist_app/utils.py 88.7% <0%> (-0.3%) ⬇️
securedrop/securedrop/models.py 89.47% <0%> (-0.03%) ⬇️
...ns/60f41bb14d98_add_session_nonce_to_journalist.py
securedrop/securedrop/journalist_app/api.py 95.23% <0%> (+0.04%) ⬆️
securedrop/securedrop/journalist_app/admin.py 87.95% <0%> (+1.74%) ⬆️

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 1e00822...fedc7cc. Read the comment docs.

@conorsch conorsch moved this from Ready for review to Under Review in SecureDrop Team Board Aug 29, 2019
@redshiftzero redshiftzero changed the title Add HTTP Alternative Service header to source, journalist interfaces when both v2 and v3 onion services are enabled [wip] Add HTTP Alternative Service header to source, journalist interfaces when both v2 and v3 onion services are enabled Aug 29, 2019
@redshiftzero
Copy link
Contributor Author

given how much other stuff needs to go in ahead of this yet for 1.0.0, i'm marking this wip until we can find a way to properly test

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

The headers are indeed served on both Source & Journalist v2 interfaces, which is a good sign. Outstanding questions remain about whether the alternative service is actually working with establishing connections. Some guidance on next steps for more thorough review:

  • GET /.well-known/http-opportunistic HTTP/1.1 request in journalist-access.log ;
  • Use of the h2= protocol id may require enabling HTTP/2 support in Apache
  • Source error logs showing AH00124: Request exceeded the limit of 10 internal redirects due to probable configuration error

Given the difficulty of confirming the functional behavior of the alt-svc header, coupled with the remaining times on the 1.0 milestone still pending merge, let's backburner these changes to prioritize merging other PRs related to v3 functionality. (I'll rebase #4718, for example, to exclude the dependency on this PR.) We can return to this work in the future if we develop a workable test plan for verifying the behavior.

@conorsch conorsch added needs/discussion queued up for discussion at future team meeting. Use judiciously. PR: Pending additional work labels Aug 29, 2019
@redshiftzero
Copy link
Contributor Author

yeah I don't see the internal redirect error on develop in staging so is related to the changes here

@eloquence eloquence moved this from Under Review to In Development in SecureDrop Team Board Aug 29, 2019
@eloquence eloquence removed this from In Development in SecureDrop Team Board Sep 11, 2019
@redshiftzero
Copy link
Contributor Author

closing this for now, see discussion in #4630

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/discussion queued up for discussion at future team meeting. Use judiciously. PR: Pending additional work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v3 onion migration] If v2 and v3 are both used, set Alt-Svc header in Apache configs
3 participants