Project

General

Profile

Bug #7755

Migrate WhisperBack to Python 3

Added by intrigeri about 5 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Elevated
Assignee:
-
Category:
-
Target version:
Start date:
09/13/2014
Due date:
% Done:

100%

Feature Branch:
whisperback:feature/python3
Type of work:
Code
Blueprint:
Starter:
Affected tool:
WhisperBack

Description

python-gnupginterface won't be part of Jessie, so we need to migrate it to some other GnuPG Python bindings, such as python-gnupg (or isis' fork, that has been ITP'd, or python3-gnupg.


Subtasks

Feature #7891: Migrate WhisperBack to python3/GIResolved

Feature #7892: Migrate WhisperBack to SSLResolved

Feature #7893: Test updated WhisperBackResolved

Feature #7894: Migrate WhisperBack to python-gnupgResolved


Related issues

Related to Tails - Bug #9412: Add SOCKS support to WhisperBack Resolved 05/16/2015 07/15/2015
Related to Tails - Bug #10577: WhisperBack does not start on Jessie with French locales Resolved 11/18/2015
Duplicated by Tails - Feature #7805: Migrate WhisperBack from python-gnupginterface to python-gnupg Duplicate 08/23/2014
Blocks Tails - Feature #7756: Reintroduce WhisperBack in Jessie ISOs Resolved 01/18/2015
Blocked by Tails - Feature #8513: Implement PGP/MIME in WhisperBack Resolved 01/02/2015

Associated revisions

Revision a716e575
Added by intrigeri over 4 years ago

Merge branch 'feature/7756-reintroduce-whisperback' into feature/jessie

The resulting WhisperBack fails to start under torsocks, but let's merge
that anyhow, and I'll file a ticket for that other problem.

Closes: #8724, #7756, #7755

History

#1 Updated by intrigeri about 5 years ago

  • Parent task set to #7756

#2 Updated by BitingBird about 5 years ago

  • Duplicated by Feature #7805: Migrate WhisperBack from python-gnupginterface to python-gnupg added

#3 Updated by intrigeri about 5 years ago

  • Subject changed from WhisperBack needs to be migrated away from python-gnupginterface to Migrate WhisperBack away from python-gnupginterface

#4 Updated by intrigeri about 5 years ago

  • Subject changed from Migrate WhisperBack away from python-gnupginterface to Migrate WhisperBack to Python 3

This task now has various subtasks that apparently have absolutely nothing to do with python-gnupginterface, so I'm retitling it to better match how it's used. Alan, if I missed something, feel free to correct me.

#5 Updated by intrigeri about 5 years ago

  • Status changed from Confirmed to In Progress

#6 Updated by alant over 4 years ago

  • Feature Branch set to whisperback:feature/python3

#7 Updated by BitingBird over 4 years ago

  • Parent task deleted (#7756)

#8 Updated by BitingBird over 4 years ago

  • Blocks Feature #7756: Reintroduce WhisperBack in Jessie ISOs added

#9 Updated by BitingBird over 4 years ago

Trying to set this ticket as a child from #5958 fails with "Parent task is invalid". The message doesn't say why, so if somebody can debug that...

#10 Updated by BitingBird over 4 years ago

  • Affected tool set to WhisperBack

#11 Updated by alant over 4 years ago

  • Related to Feature #6183: Use multiple PGP/MIME attachments for text snippets and OpenPGP public key included in Whisperback reports added

#12 Updated by alant over 4 years ago

  • Related to deleted (Feature #6183: Use multiple PGP/MIME attachments for text snippets and OpenPGP public key included in Whisperback reports)

#13 Updated by alant over 4 years ago

We new use PGP/MIME (see #8513). The migration should be completed as whisperback is now able to send bugreports from Tails/Jessie.

#14 Updated by alant over 4 years ago

  • Blocked by Feature #8513: Implement PGP/MIME in WhisperBack added

#15 Updated by intrigeri over 4 years ago

What are the next steps here? Trying to guess:

  • get the updated code reviewed
  • update the packaging
  • build a Jessie-specific package

Right? Anything else?

#16 Updated by alant over 4 years ago

  • Assignee deleted (alant)
  • QA Check set to Ready for QA

This branch is ready for review. I don't know wether we should merge it in a new "jessie" branch in whisperback repository or consider it to be the jessie branch once reviewed.

#17 Updated by intrigeri over 4 years ago

  • Assignee set to intrigeri

#18 Updated by alant over 4 years ago

  • Assignee deleted (intrigeri)

- get the updated code reviewed

Yes, there is an email about that on tails-dev.

- update the packaging

This is done in the branch.

- build a Jessie-specific package

This is 7756 and its childern. Please see there.

#19 Updated by intrigeri over 4 years ago

- get the updated code reviewed

Yes, there is an email about that on tails-dev.

That's why I've assigned this ticket to me.

#20 Updated by intrigeri over 4 years ago

  • Assignee set to intrigeri

#21 Updated by alant over 4 years ago

Please consider also reviewing a forgotten commit I just pushed: c3a4096. Sorry for the inconvenience.

#22 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to alant
  • QA Check changed from Ready for QA to Dev Needed

Good job! Here's an initial code review. I haven't tested it yet.

  • I see gnupg.GPG(keyring=keyring): does this imply --no-default-keyring?
  • Any reason to set X-Python3-Version: >= 3.2 and not 3.4? Jessie has 3.4, so I suspect this code wasn't tested on 3.2, so maybe better require 3.4 right away :)
  • Typo in "a list of recepients" (s/recepients/recipients/)
  • Typo in "message to be send" (s/send/sent)
  • I see a few set_charset('us-ascii') and can't be bothered to understand if it will apply to decrypted content as well: was the PGP/MIME code tested with non-US-ascii cleartext?
  • Please include the copyright and license information for the code taken from http://www.physics.drexel.edu/~wking/code/python/send_pgp_mime, and then add it to debian/copyright too.

Perhaps more importantly, I see:

+    context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
+    context.load_verify_locations(cafile=tls_cafile)
+    context.verify_mode = ssl.CERT_REQUIRED
+    # disable compression to prevent CRIME attacks (OpenSSL 1.0+)
+    # this is copied from python's SSL module source
+    context.options |= ssl.OP_NO_COMPRESSION

Why do we bother doing this by hand, while ssl.create_default_context does the right thing wrt. supported protocols, CERT_REQUIRED (once purpose is set to SERVER_AUTH), and also configures a sane set of allowed ciphers?

My understanding is that using create_default_context gives us required protocol and ciphers upgrades for free, as opposed to:

  • protocols: having to maintain the list ourselves, which we'll forget too often, and even if we don't we can make mistakes: e.g. the current code does not enable TLS 1.1 and 1.2, for some reason;
  • ciphers: relying on SSLContext's undocumented defaults, that may be weaker than what create_default_context does (I suspect these defaults are just OpenSSL's ones).

#23 Updated by alant over 4 years ago

  • I see gnupg.GPG(keyring=keyring): does this imply --no-default-keyring?

keyring (defaults to None)
If specified, the value is used as the name of the keyring file. The default keyring is not used. A list of paths to keyring files can also be specified.

(https://pythonhosted.org/python-gnupg/#getting-started)

  • Any reason to set X-Python3-Version: >= 3.2 and not 3.4? Jessie has 3.4, so I suspect this code wasn't tested on 3.2, so maybe better require 3.4 right away :)

No, I pasted that from debian wiki I think. Fixed

  • Typo in "a list of recepients" (s/recepients/recipients/)

Fixed.

  • Typo in "message to be send" (s/send/sent)

Fixed.

  • I see a few set_charset('us-ascii') and can't be bothered to understand if it will apply to decrypted content as well: was the PGP/MIME code tested with non-US-ascii cleartext?

This comes from http://www.physics.drexel.edu/~wking/code/python/send_pgp_mime, which seems to handle other charsets. I didn't test though.

Added to the header of encryption.py.

Perhaps more importantly, I see:

+    context = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
+    context.load_verify_locations(cafile=tls_cafile)
+    context.verify_mode = ssl.CERT_REQUIRED
+    # disable compression to prevent CRIME attacks (OpenSSL 1.0+)
+    # this is copied from python's SSL module source
+    context.options |= ssl.OP_NO_COMPRESSION

Why do we bother doing this by hand, while ssl.create_default_context does the right thing wrt. supported protocols, CERT_REQUIRED (once purpose is set to SERVER_AUTH), and also configures a sane set of allowed ciphers?

Right, that's now the right way to do it. Fixed.

#24 Updated by alant over 4 years ago

  • Assignee changed from alant to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

Please check debian package build. Thanks!

#25 Updated by intrigeri over 4 years ago

Please check debian package build.

What do you mean exactly?

Also, please address the two remaining concerns from my previous review (updating debian/copyright, and actually testing encoding support) before asking for another one, thanks :)

#26 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to alant
  • QA Check changed from Ready for QA to Info Needed

#27 Updated by alant over 4 years ago

  • Assignee changed from alant to intrigeri

intrigeri wrote:

Please check debian package build.

What do you mean exactly?

I'm not currently able to build the package. It would be very nice if you could handle this part. Else, I can do it, but not shortly.

Also, please address the two remaining concerns from my previous review (updating debian/copyright,

I think this one was addressed already. What is missing?

and actually testing encoding support) before asking for another one, thanks :)

I send a bug reports with a lot of accents and recieved it well. I don't know how to test better, but I'm up to hear from you what else you'd like me to test.

#28 Updated by intrigeri over 4 years ago

I'm not currently able to build the package. It would be very nice if you could handle this part. Else, I can do it, but not shortly.

I'm happy to do it, but I doubt I'll have time to handle that shortly -- who knows. In any case I'll first do a review of the current state, so it's fine to leave the ticket assigned to me.

Also, please address the two remaining concerns from my previous review (updating debian/copyright,

I think this one was addressed already.

You wrote that you added it to encryption.py, without mentioning debian/copyright, and I didn't notice that it also had been added there in a commit titled "Bump python version to the one actually targeted" that includes two changes that are fully unrelated to its explicit purpose.

What is missing?

Apparently nothing :)

and actually testing encoding support) before asking for another one, thanks :)

I send a bug reports with a lot of accents and recieved it well.

Thanks, that's good enough for me! :)

#29 Updated by BitingBird over 4 years ago

  • QA Check deleted (Info Needed)

#30 Updated by intrigeri over 4 years ago

  • QA Check set to Ready for QA

#31 Updated by intrigeri over 4 years ago

Code review passes. Will now deal with the Debian packaging, then will test it.

#32 Updated by intrigeri over 4 years ago

  • Status changed from In Progress to Resolved

#33 Updated by intrigeri over 4 years ago

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

Pass, except it doesn't start under torsocks => merged to ease future work on this anyway.

#34 Updated by intrigeri over 4 years ago

  • Related to Bug #9412: Add SOCKS support to WhisperBack added

#38 Updated by romeopapa about 4 years ago

On the latest jessie build, I'm getting the following error when trying to send a report:

Exception in thread Thread-6:
Traceback (most recent call last):
File "/usr/lib/python3.4/threading.py", line 920, in _bootstrap_inner
self.run()
File "/usr/lib/python3.4/threading.py", line 868, in run
self._target(*self._args, **self._kwargs)
File "/usr/lib/python3/dist-packages/whisperBack/whisperback.py", line 193, in save_exception
func(*args)
File "/usr/lib/python3/dist-packages/whisperBack/mail.py", line 48, in send_message_tls
ssl_context = ssl.create_defaut_context(purpose=ssl.Purpose.SERVER_AUTH,
AttributeError: 'module' object has no attribute 'create_defaut_context'

Python3 on latest jessie build is Python 3.4.2

#39 Updated by intrigeri about 4 years ago

romeopapa wrote:

On the latest jessie build, I'm getting the following error when trying to send a report:

Confirmed. That's a typo, hopefully fixed (108fdd1), untested, pushed to the topic branch. This makes me "slightly" doubtful that this code path was ever tested..

#40 Updated by intrigeri about 4 years ago

intrigeri wrote:

romeopapa wrote:

On the latest jessie build, I'm getting the following error when trying to send a report:

Confirmed. That's a typo, hopefully fixed (108fdd1), untested, pushed to the topic branch. This makes me "slightly" doubtful that this code path was ever tested..

Uploaded 1.6.29+jessie.1 to the feature-jessie APT suite => next builds of feature/jessie should have the fix.

#41 Updated by romeopapa about 4 years ago

OK seems to work, I've been able to successfully send a whisperback report. Nice catch on the typo, I really hadn't noticed it!

#42 Updated by alant about 4 years ago

Worked for me too.

#43 Updated by intrigeri almost 4 years ago

  • Related to Bug #10577: WhisperBack does not start on Jessie with French locales added

Also available in: Atom PDF