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

feat: enable Tor's proof-of-work defense on the Source Interface #7175

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

cfm
Copy link
Member

@cfm cfm commented Jun 5, 2024

Status

Ready for review

Description of Changes

Closes #6933 by:

  1. adding a securedrop-admin sdconfig prompt for enabling Tor's proof-of-work defense on the Source Interface (i.e., the only unauthenticated onion service a SecureDrop installation hosts); and
  2. making it so.

Testing

On this branch:

  1. securedrop-admin sdconfig, accepting the new default:

    On the Source Interface, enable Tor's proof-of-work defense against denial-of-service attacks?: yes
  2. securedrop-admin install

  3. ssh app sudo cat /etc/tor/torrc shows HiddenServicePoWDefensesEnabled 1

  4. onionprobe -e <your Source Interface>.onion includes1 output like:

    2024-06-11 22:07:39,747 INFO: Proof of Work (PoW) params found in the descriptor
    2024-06-11 22:07:39,747 INFO: PoW v1 set with effort 0, expiration 2024-06-12T05:54:47 and seed Avq2iMON/49Vevxb+q479hUReRYKGAyw1fLScS/uKdE=
  5. securedrop-admin sdconfig again, but turn the proof-of-work defense off

  6. securedrop-admin install again

  7. ssh app sudo cat /etc/tor/torrc does NOT show HiddenServicePoWDefensesEnabled 1

  8. onionprobe -e <your Source Interface>.onion does NOT include PoW output includes Proof of Work (PoW) params not found2

Deployment

See discussion in this thread on half-opt-out: This change will default to enabled on subsequent securedrop-admin {sdconfig,install} runs, but it won't be applied automatically on server-side update.

Checklist

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:

#7175 (comment)

If you made non-trivial code changes:

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

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

Deferred to freedomofpress/securedrop-docs#568.

Footnotes

  1. You'll need onionprobe v1.2.0 directly from Tor, not from the Debian bookworm package.

  2. https://github.com/freedomofpress/securedrop/pull/7175#issuecomment-2163949743

@cfm cfm added the needs/discussion queued up for discussion at future team meeting. Use judiciously. label Jun 5, 2024
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Adding in the PoW tuning directives now might save us doing another release down the line. Otherwise this is straightforward and a good first step.

admin/securedrop_admin/__init__.py Outdated Show resolved Hide resolved
admin/securedrop_admin/__init__.py Show resolved Hide resolved
@rocodes
Copy link
Contributor

rocodes commented Jun 5, 2024

My opinion on the questions posed:

  • Opt-out (default enabled), pending upstream confirmation and assuming we've had any remaining questions answered by Tor team (re performance concerns) would be what I'd suggest.
  • Timeout configuration: if there's any possibility of a footgun, we should validate inputs if we even surface that level of configuration to the end user (re maintainability/support, I'm of half a mind to say "nope, that's buried in site-specific if you absolutely think you want to tweak things you shouldn't be tweaking", but I think that would depend on what Tor says about those values. If there are good reasons for average users to adjust these values I'd change my mind)
  • Testing/failure modes: good q / open q.

@zenmonkeykstop
Copy link
Contributor

Opt-out would be nice but means changing the tor config via a deb postint (likely securedrop-app-code as we only need to update app) - that's high-impact if it goes awry for users relying on Tor for access to the server.

Ideally site admins should also be monitoring Tor and adjusting those values constantly in response to a targeted DoS - I don't think we can get the monitoring setup in place in a reasonable timeframe, but giving admins the ability to tweak without futzing would at least be something.

@legoktm
Copy link
Member

legoktm commented Jun 11, 2024

I think this is a reasonable approach given the various considerations and the amount of time we want to spend on it, thanks for working on it :)

@cfm cfm marked this pull request as ready for review June 11, 2024 22:57
@cfm cfm requested a review from a team as a code owner June 11, 2024 22:57
@cfm
Copy link
Member Author

cfm commented Jun 11, 2024

(Marking "ready for review" just to enable notifications from GitHub to Slack. Not quite ready for review.)

After reviewing Tor's example configuration and playing with a self-contained example onion service, I don't think we should expose sdconfig knobs for HiddenServicePoWQueue{Rate,Burst} at all, for two reasons. First, Tor will always be the bottleneck for a SecureDrop instance; we have no other performance or scaling knob that would suggest "lower or higher values depending on the overall capacity of your system to handle requests and with the current load of your system".1 Second:

The service starts with a default suggested effort value of 0, which keeps the PoW defenses dormant until it notices signs of overload.

The system is dynamic: as time goes by, the server regularly updates the suggested effort based on its load.2

This feedback loop is going to be more responsive than any administrator's fine-tuning, and we have no precedent for exposing knobs (or footguns, per @rocodes) like these in sdconfig for any other SecureDrop component.

However, I'll write a test plan here and for v2.9.0 QA that includes monitoring Tor's actual performance with HiddenServicePoWDefensesEnabled via MetricsPort.

Thanks for your other feedback, @zenmonkeykstop, in #7175 (review), which I'll address now. And thanks for reminding me that this is "one-half opt-in": it'll only take effect the next time an administrator runs a full securedrop-admin {sdconfig,install}.

Footnotes

  1. https://community.torproject.org/onion-services/ecosystem/technology/pow/#example-configuration

  2. https://community.torproject.org/onion-services/ecosystem/technology/pow/#how-does-it-work-in-simple-terms

@cfm
Copy link
Member Author

cfm commented Jun 12, 2024

Feedback addressed. Tests and test plan forthcoming.

@cfm cfm self-assigned this Jun 12, 2024
@cfm cfm removed the needs/discussion queued up for discussion at future team meeting. Use judiciously. label Jun 12, 2024
@nathandyer
Copy link
Contributor

nathandyer commented Jun 12, 2024

Just stepped through the test plan for this branch, and am happy to report that everything worked beautifully!

Just a couple small caveats to note, regarding deviations from expected output and a possible stumbling point for any additional testers:

  1. onionprobe is not installed by default in Tails. After sudo apt install onionprobe I was able to run it, but it did not work. It kept giving a CRITICAL: Error initializing Tor error after a 90 second timeout. I discovered this is a bookworm-related bug, and is fixed in the 1.1.2 release (the version available in Tails 6.3 is 1.0.0).

    To work around this, I installed the 1.2.0 version available in sid on my daily driver machine, since the onionprobe step didn't strictly need to be performed from my Admin Workstation.

  2. 8 [x] onionprobe -e <your Source Interface>.onion does NOT include PoW output

    is not accurate to what I saw; instead, it reported back:

    2024-06-12 17:33:39,984 INFO: Proof of Work (PoW) params not found in the descriptor
    

    which I do believe is what we would expect (and want) to see there

@cfm
Copy link
Member Author

cfm commented Jun 12, 2024

Thanks for testing, @nathandyer! I've addressed your feedback and updated the test plan with your clarification of the expected output, which I agree still passes as intended.

@cfm
Copy link
Member Author

cfm commented Jun 13, 2024

Rebased from develop after #7179. I've updated the admin test suite for the addition of the securedrop_app_pow_on_source_interface site-specific variable. We apparently don't verify the onion-service configuration in testinfra, so I've not added anything there. CI lint is currently failing on a flaky Ubuntu mirror, but I'll kick it when CircleCI lets me.

@cfm
Copy link
Member Author

cfm commented Jun 13, 2024

@zenmonkeykstop, with @nathandyer's review today and tests passing, this is now ready for your review and merge tomorrow morning.

@zenmonkeykstop zenmonkeykstop dismissed their stale review June 13, 2024 15:48

Decided against adding the additional directives for now.

…app_pow_on_source_interface=True

Otherwise a "securedrop-admin install" without a prior "securedrop-admin
sdconfig" will default to enabling this feature.  Let's not surprise an
administrator who likely intended to enforce the existing configuration.

Co-authored-by: Kevin O'Gorman <[email protected]>
@zenmonkeykstop zenmonkeykstop merged commit ec11875 into develop Jun 13, 2024
17 checks passed
@zenmonkeykstop zenmonkeykstop mentioned this pull request Jun 13, 2024
3 tasks
@cfm cfm mentioned this pull request Jun 18, 2024
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enable Tor Proof-of-Work defenses for Onion Services
5 participants