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

Fix gpg issue with importing public key #252

Merged
merged 1 commit into from
Mar 1, 2019
Merged

Conversation

sssoleileraaa
Copy link
Contributor

Fixes #251

@sssoleileraaa sssoleileraaa marked this pull request as ready for review February 21, 2019 22:22
@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Feb 22, 2019

When you run the client in the development environment, it either starts a new gpg-agent process or uses one that is already running and connected to the same home-dir. If you delete the home-dir while the gpg-agent is still running then next time you try to start the client, you'll run into #251 because the agent won't be able to find private-keys-v1.d.

Either a developer needs to know that they should kill the gpg-agent after they delete the agent's home-dir (which will be located here: <sdc-home>/gpg) or we would have to somehow detect that the running agent's home-dir no longer exists and either kill it or point it to a new home-dir for new key creation.

The confusing thing about this whole issue is that <sdc-home>/gpg is recreated when the client starts, and the gpg-agent tries to generate a new key pair but says it can't import the private key. You would think we would be able to tell the gpg-agent, hey look it's your home-dir again, regenerate a new key-pair.

@redshiftzero - what do you think is reasonable to do in this case?

@sssoleileraaa sssoleileraaa changed the title Fix gpg issue with importing public key [WIP] Fix gpg issue with importing public key Feb 22, 2019
@redshiftzero
Copy link
Contributor

update: the underlying issue appears to only be happening on debian (see details in #251). we chatted about this PR and decided that just killing the gpg-agent process for the sdc-home directory after we close the client (only in the dev env, i.e. in run.sh) is the best path forward (bonus: this will also close #215)

@sssoleileraaa
Copy link
Contributor Author

@redshiftzero so the code is ready for final review. it now:

TODO: Create a separate issue for killing the gpg-agent that is run from the client code since that will most likely also be used in production

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.

nice! couple minor suggestions inline

run.sh Outdated
if [ -d "$SDC_HOME" ]; then
SDC_HOME=${SDC_HOME}
else
SDC_HOME="/tmp/sdc-home"
Copy link
Contributor

Choose a reason for hiding this comment

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

note this is not platform independent (mac doesn't use /tmp for tempdirs), so I think we want to keep the mktemp -d logic

run.sh Outdated

echo "Running app with home directory: $SDC_HOME"
echo ""

# make sure a ew gpg-agent is used for each run
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ew -> new

run.sh Outdated

echo "Running app with home directory: $SDC_HOME"
echo ""

# make sure a ew gpg-agent is used for each run
PID=$(ps -ef | grep gpg-agent | grep "$GPG_HOME" | grep -v grep | awk '{print $2}')
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! 😎

what about using this in a bash trap (so that we can use mktemp above), i.e. here's a diff doing that based on your commit here:

diff --git a/run.sh b/run.sh
index 28fc9e3..e72b344 100755
--- a/run.sh
+++ b/run.sh
@@ -15,28 +15,25 @@ while [ -n "$1" ]; do
   shift
 done

-if [ -d "$SDC_HOME" ]; then
-  SDC_HOME=${SDC_HOME}
-else
-  SDC_HOME="/tmp/sdc-home"
-  mkdir -p "SDC_HOME"
-  chmod 0700 "$SDC_HOME"
-fi
+SDC_HOME=${SDC_HOME:-$(mktemp -d)}
 export SDC_HOME

 GPG_HOME="$SDC_HOME/gpg"
-mkdir -p "$GPG_HOME"
+mkdir -p "$SDC_HOME" "$GPG_HOME"
 chmod 0700 "$GPG_HOME"

+function cleanup {
+  # make sure a new gpg-agent is used for each run
+  PID=$(ps -ef | grep gpg-agent | grep "$GPG_HOME" | grep -v grep | awk '{print $2}')
+  if [ "$PID" ]; then
+    kill $PID
+  fi
+}
+trap cleanup EXIT
+
 echo "Running app with home directory: $SDC_HOME"
 echo ""

-# make sure a ew gpg-agent is used for each run
-PID=$(ps -ef | grep gpg-agent | grep "$GPG_HOME" | grep -v grep | awk '{print $2}')
-if [ "$PID" ]; then
-  kill "$PID"
-fi
-
 gpg --homedir "$GPG_HOME" --allow-secret-key-import --import tests/files/securedrop.gpg.asc &

 # create the database and config for local testing
@@ -44,4 +41,6 @@ gpg --homedir "$GPG_HOME" --allow-secret-key-import --import tests/files/secured

 wait

-exec python -m securedrop_client --sdc-home "$SDC_HOME" --no-proxy $@
+exec python -m securedrop_client --sdc-home "$SDC_HOME" --no-proxy $@ &
+
+wait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

making the client a bg process and waiting at the end did the trick! 💯

@eloquence eloquence changed the title [WIP] Fix gpg issue with importing public key Fix gpg issue with importing public key Feb 28, 2019
@sssoleileraaa sssoleileraaa force-pushed the kill-gpg-agent branch 2 times, most recently from b69aaad to 1950a9c Compare February 28, 2019 22:43
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.

beautiful! works as advertised, thanks @creviera

@redshiftzero redshiftzero merged commit 2692a69 into master Mar 1, 2019
@redshiftzero redshiftzero deleted the kill-gpg-agent branch March 1, 2019 01:05
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.

2 participants