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

Clean up cleanup in run.sh #949

Merged
merged 1 commit into from
Mar 20, 2020
Merged

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Mar 18, 2020

Description

The cleanup function was busier than it needed to be, and because of the exec of the client, was never actually being invoked. This change:

  • reworks cleanup to use gpgconf --kill to stop the agent
  • invokes it explicitly before the client is run
  • removes the exec so that trap cleanup EXIT works and cleanup happens after the client exits

Making sure gpg-agent is stopped should prevent any problems it might have if GNUPGHOME is pulled from under its feet between client runs.

This should make run.sh more robust when developers are mucking about with SDC_HOME.

We might consider adding similar agent cleanup in production, but since SDC_HOME should be consistent and in normal usage should never be removed, the problems we've seen with the GPG agent should not arise in production use.

Fixes #896. Possibly.

Test Plan

Follow any of the STR in #896, if they've worked for you. See if the GPG errors still rear their ugly little heads.

If you're testing in Qubes, please check both normal run.sh usage and with --no-proxy to try to mimic what happens in non-Qubes developer environments.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

@ntoll
Copy link
Contributor

ntoll commented Mar 18, 2020

Love the branch name... ;-)

@ntoll
Copy link
Contributor

ntoll commented Mar 18, 2020

OK... I can confirm this works for me.

sssoleileraaa
sssoleileraaa previously approved these changes Mar 20, 2020
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

lgtm and indeed fixes #896

The cleanup function was busier than it needed to be, and because of
the exec of the client, was never actually being invoked. This change:

- reworks cleanup to use "gpgconf --kill" to stop the agent
- invokes it explicitly before the client is run
- removes the exec so that "trap cleanup EXIT" works and cleanup
  happens after the client exits

Making sure gpg-agent is stopped should prevent any problems it might
have if GNUPGHOME is pulled from under its feet between client runs.

This should make run.sh more robust when developers are mucking about
with SDC_HOME.

We might consider adding similar agent cleanup in production, but
since SDC_HOME should be consistent and in normal usage should never
be removed, the problems we've seen with the GPG agent should not
arise in production use.
@rmol rmol force-pushed the all-my-sins-will-be-warshed-away branch from 653f465 to c1404b2 Compare March 20, 2020 17:19
@sssoleileraaa sssoleileraaa self-requested a review March 20, 2020 17:26
@sssoleileraaa sssoleileraaa merged commit 5436f22 into master Mar 20, 2020
@sssoleileraaa sssoleileraaa deleted the all-my-sins-will-be-warshed-away branch March 20, 2020 17:51
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.

Messages get stuck in skeleton mode when sdc-home is ~/.securedrop_client
3 participants