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

Restart Apache during installation when onion service version configuration changes #5726

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Jan 20, 2021

Status

Ready for review

Description of Changes

Fixes #5715.

If the configuration of v2 and v3 services is modified, Apache needs to be restarted for the app config to reflect the changes, so that migration warnings are accurate.

Testing

  • check out develop
  • enable both v2 and v3 onion services by running securedrop-admin sdconfig or editing site-specific
  • securedrop-admin install
  • create admin user, visit JI, confirm v2 warning is shown
  • disable v2, reinstall
  • visit JI and confirm warning is still shown
  • git checkout -b fix-5715 origin/fix-5715
  • reenable v2, reinstall
  • visit JI and confirm v2 warning is shown
  • disable v2, reinstall
  • visit JI and confirm v2 warning is no longer shown

Deployment

No special considerations.

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 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

@emkll emkll self-assigned this Jan 20, 2021
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

While this will work on existing installs, this may not work on clean/new installs: the tor role is run before the app role on the app server, and Apache2 is installed/configured as part of the app role. In order to resolve, we could either:

  • detect if the service exists and conditionally restart the apache2 service

or, since this is likely going to be a one-time operation:

  • address this issue through documentation, since admins will likely be using the documentation to guide them through this process.

@rmol
Copy link
Contributor Author

rmol commented Jan 20, 2021

Drat, right, I'll add a check for the presence of the Apache package.

If the configuration of v2 and v3 services is modified, Apache needs
to be restarted for the app config to reflect the changes, so that
migration warnings are accurate.
@rmol rmol changed the title Restart Apache when restarting Tor during installation Restart Apache during installation when onion service version configuration changes Jan 20, 2021
@rmol
Copy link
Contributor Author

rmol commented Jan 20, 2021

Better still, just notify restart in the app server playbook, where the source URL files are managed. I tested a fresh install with both v2+v3 installed, then removed v2, and everything worked as expected.

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

Thanks @rmol for the changes, went through the test plan and confirm the banner disappeared on a simple page refresh.

It looks like you are also covering the downgrade to v2 scenarios which aren't strictly required, but fine to merge since this logic will soon be purged .

when: v2_onion_services

- name: Remove source v2 onion service info if not enabled
file:
path: /var/lib/securedrop/source_v2_url
state: absent
notify:
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@@ -23,12 +23,16 @@
mode: "0644"
content: |
{{ v2_onion_url_lookup_result.stdout|default('') }}
notify:
Copy link
Contributor

Choose a reason for hiding this comment

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

Making a note to remove this logic as part of #5731 as is it outside of the tor hidden service role

@emkll emkll merged commit 6606ce3 into develop Jan 20, 2021
@emkll emkll deleted the fix-5715 branch January 20, 2021 23:22
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.

V2 warning still showing even with only v3 enabled service
2 participants