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

Tor Apt Repo mirror #2113

Closed
wants to merge 8 commits into from
Closed

Tor Apt Repo mirror #2113

wants to merge 8 commits into from

Conversation

msheiny
Copy link
Contributor

@msheiny msheiny commented Aug 15, 2017

Status

Ready for review 👍

Description of Changes

Addresses #2106 , in response to the big ol' #2105 issue of 2017.

Changes proposed in this pull request:

Switches tor apt repo to point from http://deb.torproject.org/torproject.org to http://tor-apt.ops.freedom.press. Which is a snapshot in time of the upstream tor repo.

Testing

This switches the network location of the tor packages pulled in by the securedrop servers. So the big part in testing is to ensure that:

  • tor doesnt break
  • securedrop-app deb packages correctly switch the repo to point to our mirror

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances. --- the securedrop-app deb package contains basic bash logic run on updates that attempts to modify the tor apt repo location. This needs to be tested more before this PR is pulled in.
  2. New installs. --- Do the tor packages get installed and does securedrop onion services come up 👍

Checklist

If you made changes to the app code:

  • Unit and functional tests pass on the development VM

If you made changes to the system configuration:

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.

A test needs updating (comment inline), otherwise 👍 from me

@@ -9,7 +9,7 @@ def test_tor_apt_repo(File):
The version of Tor in the Trusty repos is not up to date.
"""
f = File('/etc/apt/sources.list.d/deb_torproject_org_torproject_org.list')
repo_regex = re.escape('deb http://deb.torproject.org/torproject.org trusty main')
repo_regex = re.escape('deb http://tor-apt.ops.freedom.press trusty main')
assert f.contains(repo_regex)

Copy link
Contributor

Choose a reason for hiding this comment

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

test_cron_apt_repo_list in testinfra/common/test_cron_apt.py also needs updating

@msheiny msheiny force-pushed the TorAptRepoMirror branch 2 times, most recently from c81f904 to 99162fb Compare August 16, 2017 20:01
@msheiny
Copy link
Contributor Author

msheiny commented Aug 16, 2017

rebased on develop and attempting to address broken testinfra test -- waiting for CI

@msheiny
Copy link
Contributor Author

msheiny commented Aug 16, 2017

Sweet CI is passing -- the only situation I havent really tested is the upgrade path (taking the debs)... where is #1689 when we need it!!?

if [ -f "/etc/apt/$file" ]; then
sed -i 's/deb.torproject.org\/torproject.org/tor-apt.ops.freedom.press/g' "/etc/apt/$file"
fi
done
Copy link
Contributor

Choose a reason for hiding this comment

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

This strategy will only update the apt config on the App Server, not on the Mon Server. Definitely it's critical that we minimize breakage for the App Server interfaces, but we need a way to land config changes across the board. Debian configuration packages come to mind: https://debathena.mit.edu/config-packages/

@@ -10,8 +10,7 @@ def test_tor_apt_repo(File):
The version of Tor in the Trusty repos is not up to date.
"""
f = File('/etc/apt/sources.list.d/deb_torproject_org_torproject_org.list')
repo_regex = re.escape('deb http://deb.torproject.org/torproject.org '
'trusty main')
repo_regex = re.escape('deb http://tor-apt.ops.freedom.press trusty main')
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to test for the absence of the old repo URL, otherwise the change is moot.

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.

Good first pass, but we're only touching the Application Server via these automatic updates. That doesn't seem good enough to me.

Also requesting changes as an intentional blocker until we have more robust testing on the new backend service, otherwise we're trading one potential point of failure for another.

@msheiny msheiny added this to the 0.4.4 milestone Sep 14, 2017
@conorsch conorsch removed the blocked label Sep 28, 2017
@conorsch conorsch force-pushed the TorAptRepoMirror branch 2 times, most recently from a6883e3 to 02f83f9 Compare September 28, 2017 00:33
@conorsch conorsch dismissed stale reviews from redshiftzero and themself September 28, 2017 00:33

Edited PR to implement requested changes

@conorsch
Copy link
Contributor

Created new deb package securedrop-config to handle the repo swap. Deliberately dismissed old reviews since the diff has changed.

We'll need vigorous QA regarding upgrades to make sure the path is smooth. Reminder to @redshiftzero and @msheiny that #1689 remains unresolved.

@conorsch conorsch force-pushed the TorAptRepoMirror branch 2 times, most recently from 12f2d31 to 2d3ba28 Compare September 28, 2017 00:52
@conorsch conorsch requested a review from a user October 2, 2017 17:01
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

A minor leftover and two nits, otherwise looks great :-)

Homepage: https://securedrop.org
Package: securedrop-config
Version: 0.1.0+0.4.3
Architecture: amd64
Copy link

Choose a reason for hiding this comment

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

It should be all instead of amd64 because the package is not architecture dependent.

https://www.debian.org/doc/debian-policy/index.html#architecture

assert tor_gpg_pub_key_info in c.stdout


@pytest.mark.parametrize('filename', [
Copy link

Choose a reason for hiding this comment

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

this is probably a leftover of a previous implementation ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dachary Can you clarify what "this" refers to? The test beginning on L36 of testinfra/common/test_tor_mirror.py checks for absence of the official Tor apt repository, thereby complementing the tests that check for presence of the FPF Tor mirror.

Copy link

Choose a reason for hiding this comment

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

the filename parameter is not used by the test below and it will fail testinfra

@@ -6,5 +6,5 @@ Homepage: https://securedrop.org
Package: securedrop-keyring
Version: 0.1.0+0.4.3
Architecture: amd64
Depends: gnupg
Depends: gnupg,securedrop-config
Copy link

Choose a reason for hiding this comment

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

securedrop-config should be a direct dependency of securedrop-ossec-* and securedrop-app-code

@ghost
Copy link

ghost commented Oct 3, 2017

./testinfra/test.py app-staging locally fails with

INFO:testinfra:RUN CommandResult(command="grep -riP 'deb\\.torproject\\.org' /etc/apt*", exit_status=0, stdout=u'Binary file /etc/apt/trusted.gpg~ matches\nBinary file /etc/apt/trusted.gpg.d/deb.torproject.org-keyring.gpg~ matches\nBinary file /etc/apt/trusted.gpg.d/deb.torproject.org-keyring.gpg matches', stderr=u'')

@conorsch
Copy link
Contributor

conorsch commented Oct 3, 2017

@dachary Addressed most of the changes you requested, but I could use some clarification on one of the comments: #2113 (comment)

"""
f = File('/etc/apt/sources.list.d/tor_apt_ops_apt_freedom_press.list')

regex = ('^deb \[arch=amd64\] http:\/\/tor-apt\.ops\.freedom\.press '
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs updating since c4159dd

@conorsch
Copy link
Contributor

conorsch commented Oct 3, 2017

Great review, @dachary and @redshiftzero. I'm going to rebase this on top of latest develop and clean up a bit. Will ping you both when it's ready for re-review.

This addresses issue #2106 - mitigate side-effects from a tor package
breaking securedrop instances in the field.

The actual logic that creates our mirror is in source control in another repo.
msheiny and others added 7 commits October 3, 2017 10:56
We don't have the mirror reflected in the config yet, but these tests
describe the system state we want. Time for some TDD!
New Debian package designed for shipping unattended upgrades that affect
system state (rather than simply installing new software). We're
breaking some of the Debian packaging rules, specifically by touching
contents inside `/etc/apt/`, but SecureDrop is not an official Debian
package, and we're highly opinionated about what gets to run on
SecureDrop servers, so I'm comfortable with the lintian errors for now.

Creates postinst for securedrop-config package

Running the sed replace actions via postinst, so that current official
Tor apt repo settings are replaced with the FPF mirror.
Removes postinst tor repo logic from the `securedrop-app-code` package, since
that would only have affected the Application Server.
Reusing the generic-pkg role to build the new `securedrop-config`
package along with the others.

Sets securedrop-config as dependency of securedrop-keyring

Adds local pkg vars for securedrop-keyring. Since `securedrop-keyring`
depends on `securedrop-config`, the latter must be installed first, so
the order of items the local packages list var is important.

Runs the usual collection of tests over the new deb package, as well.

Includes entry in the `update_version.sh` script so that the config
package will have updates made to it, as well.
The package isn't architecture-dependent, so don't lie about it.
The SecureDrop install scripts only setup "amd64" package access
via the FPF tor repository. Will shake out any inconsistencies in
pre-release QA with release candidate debs.

Pointed out by @dachary during review. Further reading at:
https://www.debian.org/doc/debian-policy/index.html#architecture

Updates associated config tests, as well.
Previously tried to piggyback on securedrop-keyring, setting the
securedrop-config package as a dependency thereof. It's more explicit to
list securedrop-config as a dependency of packages required for the
Application and Monitor Server roles:

  * securedrop-app-code
  * securedrop-ossec-agent
  * securedrop-ossec-server

Therefore, removed the dependency in securedrop-keyring.
The parametrization was improperly implemented, causing the test suite
to fail. Rather than recursive grep across all of `/etc/apt`—which will
cause false positives in the tor keyring under
/etc/apt/trusted.gpg.d—let's look only at the two rationale locations,
and recursively search through /etc/apt/sources.list.d (since multiple
Ansible versions could have written to different paths).
@conorsch
Copy link
Contributor

conorsch commented Oct 6, 2017

During yesterday's SecureDrop Engineering meeting, the team decided the S3 bucket mirror strategy is insufficient, given that it requires substantial manual maintenance, and creates scheduling problems for QA, via Valid-Until. @msheiny is working on more comprehension solution that involves using the SecureDrop Release Signing Key to re-sign Tor packages, for distribution to SecureDrop servers.

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.

3 participants