Send replies #67

heartsucker · 2018-10-25T07:44:44Z

Fixes #16
Fixes #73

Doesn't implement DB functionality yet.

redshiftzero · 2018-10-26T23:30:19Z

so, check out commit 41870a7, in there I've shown an example of what I would expect the structure to be in terms of the callbacks / API functionality. if you agree this is reasonable, feel free to cherry-pick into here

note: the reason in that diff I remove your ConversationView.add_reply call in ReplyBoxWidget.send_reply is because imho we should have Window.show_conversation_for just represent the state of the current conversation (I added the files part of this logic over in a PR here) and then call that whenever we want to refresh the conversation view (there are other ways to do this, which may be better or worse, so let me know if you disagree with this pattern)

ntoll · 2018-10-29T13:50:29Z

+1 on Window.show_conversation_for comment.

ntoll · 2018-10-29T13:58:07Z

This looks good. Perhaps when clicking send, the conversation view should scroll down to the bottom so you see the latest entry.

redshiftzero

hey @heartsucker, this is looking good so far, I dropped some comments inline (note this review from just from looking at the diff, i haven't tested this)

redshiftzero · 2018-11-01T23:41:24Z

securedrop_client/logic.py

+                if source.key and source.key.get('type', None) == 'PGP':
+                    pub_key = source.key.get('public', None)
+                    if pub_key:
+                        self.gpg.import_key(source.uuid, pub_key)


since we're doing this each sync, i was curious how fast this is: pgp key imports on my system took 0.16s for a new key, and about half that time to attempt to import a key that was already in the keyring. so if a news organization has 100 sources, that's 8 seconds just for attempting key imports for sources that already have keys in the workstation. this is not a blocker for initial merge, just flagging this for the future when we do a bit more benchmarking of the time to sync.

more generally, i think that storage.update_sources is a more natural place for this gpg key import such that this source update logic is all together - what do you think? (indeed this means passing self.gpg in storage.update_sources, which we're also doing with self.session - i think that's a fine pattern)

I think we shouldn't rely on the client always having the same key as the server because at some point in the future, we could rotate keys. It would be nice to do this pretty and async, but for now this is fine. I can look in to moving the logic to the other function.

redshiftzero · 2018-11-01T23:46:02Z

securedrop_client/logic.py

+
+    def on_reply_timeout(self) -> None:
+        self.set_status("Connection to server timed out, please try again.")
+        self.call_reset()


note this'll need a rebase on latest master (self.call_reset is no longer called directly in the callbacks after the threading cleanup in #85)

redshiftzero · 2018-11-01T23:46:20Z

securedrop_client/logic.py

+                      source_uuid, message)
+
+    def on_reply_to_source(self, result) -> None:
+        self.call_reset()


until we have the UUID of the created reply in the response body, we should probably just sync the API here

redshiftzero · 2018-11-01T23:48:25Z

securedrop_client/gpg.py

+            raise RuntimeError('Local source not found: {}'
+                               .format(source_uuid))
+
+        out = self.gpg.encrypt(message, local_source.fingerprint)


here we'll also want to encrypt to the submission key of the instance

in terms of how to actually figure out what key that is in the keyring, there are a couple approaches (maybe more):

we have the submission private key in dom0 (from there we use salt to drop the private key in the gpg VM, which the VM the SVS runs in uses for gpg actions via qubes' split-gpg). as part of the provisioning process, we could use salt to drop the submission key fingerprint in the config file (or in a file on disk e.g. ~/public_key_fingerprint that the client can use to populate the config file).

use the fact that the public key fingerprint associated with the only private key in the keyring that is not an autogenerated source key, is the submission key (a safe assumption for the alpha, but not a great assumption for the beta)

i think the former is a better approach, what do you think? that would mean for the purposes of this PR, we'd need to add a submission key fingerprint to the config file (and for the client dev env we should add the key associated with the securedrop dev env), file a followup in the workstation repo to add the salt logic.

There was nothing that seemed to reference this key anywhere, so I left that out for now.

Assuming we have a known path for the key, we can always reload it at boot time to store a reference to the fingerprint. Reloading a key will always provide a fingerprint.

>>> res = g.import_keys(k) >>> res.fingerprints ['0CEC936888A60171461174C5C0A2586F09D77C82'] >>> res = g.import_keys(k) >>> res.fingerprints ['0CEC936888A60171461174C5C0A2586F09D77C82']

I don't want to put it in the config because if a key gets rotated, we have to remember to rotate the config, and that sounds like a place where errors could happen.

heartsucker · 2018-11-07T21:30:58Z

hey @redshiftzero I'm trying to merge your decrypt_submission function here into the GpgHelper, and I realized we have a MessageSync object that creates its own session. Is there a reason to have multiple sessions?

Also, my idea is that this needs to have a reference to the GpgHelper to do the decryption itself, so these need to be injected / passed to each other via __init__() or setup(). Does that sound reasonable?

redshiftzero · 2018-11-08T01:32:15Z

I'm trying to merge your decrypt_submission function here into the GpgHelper, and I realized we have a MessageSync object that creates its own session. Is there a reason to have multiple sessions?

hey so, we can pass Client.session in logic.py to MessageSync and ReplySync when we create those threads in Client.start_message_thread and Client.start_reply_thread

Also, my idea is that this needs to have a reference to the GpgHelper to do the decryption itself, so these need to be injected / passed to each other via init() or setup(). Does that sound reasonable?

if by "this" in your first sentence you mean the MessageSync and ReplySync objects then yep, that's a solid plan

joshuathayer · 2018-11-10T00:08:09Z

securedrop_client/message_sync.py



 logger = logging.getLogger(__name__)


 class APISyncObject(QObject):

-    def __init__(self, api, home, is_qubes):
+    def __init__(self, api, session, gpg):


I think we got in to the pattern of creating new sqlite Sessions in these objects because of threading issues: you can't use a session created in, say, thread A from a different thread, B. Since these "sync objects" tend to run in their own threads, I think it makes sense to build their sessions in the objects' __init__ fns.

In fact, now that I can test this, I think we're already getting bit by this problem. I can move the session-building back in to init.

Yeah that was what I was posting about in gitter. However, this does still create a problem that if we pass a reference to this object, it could still be tainted. I think I'm going to roll this back for now so that each has its own session.

joshuathayer · 2018-11-10T00:57:25Z

securedrop_client/crypto.py

+
+    def __init__(self, gpg_home: str, session, is_qubes: bool) -> None:
+        if is_qubes:  # pragma: no cover
+            gpg_binary = "qubes-gpg-client"


Strangely, upon testing, pretty_bad_protocol doesn't play well with qubes-gpg-client, but does work well with qubes-gpg-client-wrapper. I'll update the binary name, here.

so unfortunately we tested this with qubes-gpg-client-wrapper and found that python-gnupg actually does not work at all with that as a binary, so we'll need to implement our own (ideally minimal) gpg wrapper. we're going to take a crack at doing this at the hackathon this weekend

joshuathayer · 2018-11-10T01:34:40Z

securedrop_client/crypto.py

+        local_source = self.session.query(Source) \
+            .filter_by(uuid=source_uuid).one()
+
+        out = self.gpg.encrypt(message, local_source.fingerprint)


Per irl conversation with @redshiftzero, we want to encrypt to both the source key and the journalist key. We decided on an interim solution for figuring out what fingerprint to use here: upon startup, we'll try to get the fingerprint of the journalist key from sd-gpg (via pretty_bad_protocol.client.list_keys()) then add that fingerprint to the self.gpg.encrypt() call here.

redshiftzero · 2018-11-10T02:22:40Z

securedrop_client/crypto.py

+
+        local_source.fingerprint = fingerprints.pop()
+        self.session.add(local_source)
+        self.session.commit()


one thought: I feel like it makes sense for this function to return the fingerprint and then over where this is called we make the database change (ideally this would only be crypto operations and interactions with the gpg binary)

Yeah can do.

joshuathayer · 2018-11-12T21:20:42Z

So, I got this to the point where things seem functional in the client, at least running with a local SD docker instance. It needs more manual testing, some code cleanup, and new unit tests. I think I might be putting this aside for a bit to do other work in preparation for the audit, but that's the status if other folks want to pick this up.

heartsucker · 2018-11-20T16:30:37Z

This has gotten so out of sync that I'm going to close it and open a new one.

securedrop-proxy 0.2.0

Check extract path and ensure minimal perms

heartsucker force-pushed the send-replies branch 2 times, most recently from 2fc3ec9 to 31ac240 Compare November 1, 2018 16:47

redshiftzero reviewed Nov 2, 2018

View reviewed changes

heartsucker force-pushed the send-replies branch 3 times, most recently from 1c932ef to 4933789 Compare November 5, 2018 19:56

heartsucker changed the title ~~GUI functionality for sending replies~~ Send replies Nov 5, 2018

redshiftzero mentioned this pull request Nov 6, 2018

use python-gnupg instead of subprocessing out to gpg binary #110

Closed

heartsucker force-pushed the send-replies branch 3 times, most recently from 4b40a62 to ad7b3f5 Compare November 7, 2018 16:23

heartsucker added 13 commits November 9, 2018 11:34

added reply box to conversation

6c7690c

mock controller for test

2eb687d

tests for sending reply

a83dca8

added pretty_bad_protocol to Pipfile

be60cd5

gpg basics

df925bb

wired reply button to api

3672716

gpg tests

aaa973c

remove unnecessary mocks

31486ff

change internal organization

fe39998

fix tests

fab65ab

logic tests for reply sending

cdcc74d

merge gpg/crypto code

330eff6

update tests to accomodate crypto/gpg changes

ec0f5c0

heartsucker force-pushed the send-replies branch from 7efb000 to ec0f5c0 Compare November 9, 2018 10:56

joshuathayer suggested changes Nov 10, 2018

View reviewed changes

redshiftzero reviewed Nov 10, 2018

View reviewed changes

joshuathayer added 5 commits November 10, 2018 13:57

wip gpg wrapper

f182f76

wip, replies getting encrypted

b4ceec9

wip self-encrypted replies not yet working

4c6e0f4

adds dev key to new private ring at startup

0b84eeb

remove pretty bad protocol

af48de7

Removes pretty-bad-protocol from Pipfile

b1bd9b7

redshiftzero mentioned this pull request Nov 13, 2018

Refresh conversation view #142

Merged

heartsucker closed this Nov 20, 2018

redshiftzero mentioned this pull request Nov 29, 2018

Added more functions to the GPG helper #190

Merged

legoktm pushed a commit that referenced this pull request Dec 11, 2023

Merge pull request #67 from freedomofpress/release/0.2.0

3a79ea4

securedrop-proxy 0.2.0

legoktm pushed a commit that referenced this pull request Dec 11, 2023

Merge pull request #67 from freedomofpress/check-extract-path

3491a30

Check extract path and ensure minimal perms

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send replies #67

Send replies #67

heartsucker commented Oct 25, 2018 •

edited

Loading

redshiftzero commented Oct 26, 2018

ntoll commented Oct 29, 2018

ntoll commented Oct 29, 2018

redshiftzero left a comment

redshiftzero Nov 1, 2018

heartsucker Nov 2, 2018

redshiftzero Nov 1, 2018

redshiftzero Nov 1, 2018

redshiftzero Nov 1, 2018

heartsucker Nov 2, 2018

heartsucker commented Nov 7, 2018

redshiftzero commented Nov 8, 2018

joshuathayer Nov 10, 2018

heartsucker Nov 10, 2018

joshuathayer Nov 10, 2018

redshiftzero Nov 10, 2018

joshuathayer Nov 10, 2018

redshiftzero Nov 10, 2018

heartsucker Nov 10, 2018

joshuathayer commented Nov 12, 2018

heartsucker commented Nov 20, 2018

Send replies #67

Send replies #67

Conversation

heartsucker commented Oct 25, 2018 • edited Loading

redshiftzero commented Oct 26, 2018

ntoll commented Oct 29, 2018

ntoll commented Oct 29, 2018

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heartsucker commented Nov 7, 2018

redshiftzero commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuathayer commented Nov 12, 2018

heartsucker commented Nov 20, 2018

heartsucker commented Oct 25, 2018 •

edited

Loading