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

Disable v2 onion addresses on restore on Focal #5677

Merged
merged 3 commits into from
Feb 24, 2021

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Dec 17, 2020

Status

Ready for review

Description of Changes

Fixes #5676 , towards #5731

We filter out any v2 onion address related line from /etc/tor/torrc
and also the directories from /var/lib/tor/services. This will
happen only on Focal. On Xenial, everything stays the same.

Testing

  • Checkout this branch in Tails
  • Restore a v2 + v3 backup into a Xenial prod, nothing should change
  • Create a Focal prod vm following steps at Prod vms on focal #5669 with both v2+v3
  • Restore the same backup from tails to the Focal VM.

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make 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

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

If you added or updated a code dependency:

Choose one of the following:

  • I have performed a diff review and pasted the contents to the packaging wiki
  • I would like someone else to do the diff review

@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2020

This pull request introduces 3 alerts when merging c98ad06 into 2e513b1 - view on LGTM.com

new alerts:

  • 3 for Variable defined multiple times

@kushaldas kushaldas force-pushed the disable_v2_on_restore_on_focal branch from c98ad06 to fafb675 Compare December 17, 2020 17:06
@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2020

This pull request introduces 3 alerts when merging fafb675 into 2e513b1 - view on LGTM.com

new alerts:

  • 3 for Variable defined multiple times

@rmol rmol self-assigned this Dec 17, 2020
@codecov-io
Copy link

codecov-io commented Dec 17, 2020

Codecov Report

Merging #5677 (fafb675) into develop (2e513b1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5677   +/-   ##
========================================
  Coverage    85.38%   85.38%           
========================================
  Files           51       51           
  Lines         3709     3709           
  Branches       464      464           
========================================
  Hits          3167     3167           
  Misses         441      441           
  Partials       101      101           

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 2e513b1...fafb675. Read the comment docs.

@eloquence eloquence changed the title Fixes #5676 disable v2 onion addresses on restore on Focal Disable v2 onion addresses on restore on Focal Jan 5, 2021
@eloquence eloquence added this to the 1.8.0 milestone Jan 21, 2021
@eloquence
Copy link
Member

(Marked as release blocker for 1.8.0.)

@kushaldas
Copy link
Contributor Author

Rebased today.

@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2021

This pull request introduces 3 alerts when merging 31f6091 into 65e8d6b - view on LGTM.com

new alerts:

  • 3 for Variable defined multiple times

rmol
rmol previously requested changes Feb 11, 2021
Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

This left Xenial services untouched. On Focal it removed all v2 services but ssh. Seems like that should go as well.

@kushaldas
Copy link
Contributor Author

I started working on the updating of the PR, and now wondering about the scenario of v2 ssh. My update will assume that v3 ssh is already setup.

@kushaldas kushaldas force-pushed the disable_v2_on_restore_on_focal branch from 31f6091 to e5fa74b Compare February 17, 2021 12:49
@kushaldas
Copy link
Contributor Author

Rebased against develop.

@lgtm-com
Copy link

lgtm-com bot commented Feb 17, 2021

This pull request introduces 5 alerts when merging e5fa74b into f3e51b4 - view on LGTM.com

new alerts:

  • 5 for Variable defined multiple times

@emkll emkll force-pushed the disable_v2_on_restore_on_focal branch from e5fa74b to 1066cd0 Compare February 18, 2021 18:15
@lgtm-com
Copy link

lgtm-com bot commented Feb 18, 2021

This pull request introduces 5 alerts when merging 1066cd0 into 88ac049 - view on LGTM.com

new alerts:

  • 5 for Variable defined multiple times

@emkll emkll unassigned rmol Feb 22, 2021
@emkll emkll self-assigned this Feb 22, 2021
@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Feb 22, 2021

Tried using this to restore a v2+v3 Xenial backup onto a v3-only Focal system. It fails at the compare_torrc.py step, as the configurations are obviously different, but this scenario should probably be supported.

My expectation would be that the migration states should go as follows:

backup \ target v2 Xenial v2+v3 Xenial v3 Xenial or Focal
v2 Yes No (restore then migrate) yes with --skip-tor
v2+v3 No Yes yes but v3-only
v3 No Yes but v3-only Yes

(implicit above is that there are no valid Focal targets with v2)

  • The v2 backup row is covered with current code.
  • The v2+v3 backup row is not covered, needs changes to compare_torrc.py logic and logic to apply only the v3 config.
  • the v3 backup row is not covered, needs same changes as v2+v3

In all cases the docs need refreshing (see freedomofpress/securedrop-docs#133 ).

Implementation notes

  • We can probably get away without the script to clean up the servers' torrc files, as the install playbook is run as part of migrations and it should ensure that torrc is correct (but does it? goood question).
  • There is existing logic to skip Tor configs altogether using the exclude field:
    exclude: "var/lib/tor,etc/tor/torrc"
    - a similar approach could be used to selectively exclude v2 configurations This could be cleaner than removing v2 files after copying them to the server.
  • Changes to compare_torrc.py (or a different approach) would be required for the case where v2+v3 backups are applied to a v3 target - instead of erroring out it could indicate that v3 was available on the server, and allow the restore to proceed with the v3 info from the backup.

@lgtm-com
Copy link

lgtm-com bot commented Feb 23, 2021

This pull request introduces 5 alerts when merging 05d83c7 into da9dbf2 - view on LGTM.com

new alerts:

  • 5 for Variable defined multiple times

kushaldas and others added 3 commits February 24, 2021 10:59
We filter out any v2 onion address related line from /etc/tor/torrc
and also the directories from /var/lib/tor/services. This will
happen only on Focal. On Xenial, everything stays the same.
@kushaldas kushaldas force-pushed the disable_v2_on_restore_on_focal branch from 05d83c7 to 8835810 Compare February 24, 2021 05:36
@lgtm-com
Copy link

lgtm-com bot commented Feb 24, 2021

This pull request introduces 5 alerts when merging 8835810 into 23bf5f8 - view on LGTM.com

new alerts:

  • 5 for Variable defined multiple times

@kushaldas
Copy link
Contributor Author

I tested with v2/v3 and only v3 restore. Also fixed the merge conflict in CI. Now, need at least more person to make sure that I did not loose anything after merge conflict. Maybe @zenmonkeykstop

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.

Conflict resolution looks good based on visual review. Approving.

@rmol rmol dismissed their stale review February 24, 2021 16:35

ssh has been addressed

@zenmonkeykstop zenmonkeykstop merged commit f856c5b into develop Feb 24, 2021
@zenmonkeykstop zenmonkeykstop deleted the disable_v2_on_restore_on_focal branch July 21, 2021 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable v2 onion addresses while restoring from backup on Focal
6 participants