Tails

#2 Updated by intrigeri 2015-05-16 10:16:29

Perhaps the right way to fix that would be to give WhisperBack native SOCKS5 support?

#4 Updated by intrigeri 2015-06-16 00:54:07

> That looks quite easy to do, so let’s do this way!

Woohoo :)

#8 Updated by alant 2015-08-03 16:11:37

> What’s your ETA on this one?

I wasn’t aware of a deadline but I can very well do that before october 15th. Can you take care of the packaging?

#9 Updated by intrigeri 2015-08-04 02:35:44

> I can very well do that before october 15th.

Excellent. Even better if it’s done earlier, e.g. by the end of August or the end of September.

> Can you take care of the packaging?

Sure!

#12 Updated by romeopapa 2015-08-11 08:18:44

To note, i was also going to supply a patch for tails/feature/jessie but intrigeri’s commit 72647c7b2c674c7488a32d8942562d1d692b9d94 (Replace patches that wrapped apps with torsocks with dynamic patching with a hook.) has made that unnecessary.

#14 Updated by romeopapa 2015-08-11 15:31:42

Sure, I can make the SOCKS posrt configurable.

> E.g. in Tails we’re going to use something else than 9050

• Should it be a constant that could be overloaded (in order of priority) by: a command line argument and an environmnent variable and a config file somewhere?
• or were you thinking about a GUI element?

#15 Updated by romeopapa 2015-08-11 17:25:36

> : read from the ENV something like WHISPERBACK_SOCKS_HOST and WHISPERBACK_SOCKS_PORT, but default to 127.0.0.1 and 9050 if not present

OK, going for environment variables WHISPERBACK_SOCKS_HOST and WHISPERBACK_SOCKS_PORT.

#18 Updated by intrigeri 2015-08-12 02:01:27

> Sure, I can make the SOCKS posrt configurable.

:)

>> E.g. in Tails we’re going to use something else than 9050

> * Should it be a constant that could be overloaded (in order of priority) by: a command line argument and an environmnent variable and a config file somewhere?

See the “CONFIGURATION” section in README, in order to make this new setting consistent with how the rest of WhisperBack is configured :)

> * or were you thinking about a GUI element?

No.

#19 Updated by romeopapa 2015-08-12 12:48:06

> See the “CONFIGURATION” section in README, in order to make this new setting consistent with how the rest of WhisperBack is configured :)

Should be good, followed the implementation. I believe task is complete.

#22 Updated by alant 2015-08-17 05:53:50

Hi romeopapa

Thanks a lot for your patches! I tested them and they work perfectly. Find below a few comments before I merge them.

• commit 6d195d8:
  • merge 660a2c4 (do not import os)

• commit a8cfaea: please rewrite it to:
  • not import os
  • add default values for socks_host and socks_port to the definition of send_message_tls in mail.py
  • do not fix whitespace errors in unrelated commit (# Load the python configuration file "config.py" from different locations)
  • do not move # XXX: Add sanity checks because it still applies to the old lines
  • validateHostnameOrIPv4: see next commit. It would be even more clean if you added all the sanity checks in commit 8257943
  • please add the parameters you add to the “SMTP” section of doc/config.py.sample
  • fixup for docstring It doesn't take any argument. should be in a separate commit in another branch (or be dropped).

• commit 660a2c4
  • meld into 6d195d8

• commit 8257943
  • param subject The topic of the feedback; def execute_threaded(self, func, args, progress_callback=None, and next changes; param progress_callback: do not fix whitespace errors in unrelated commit
  • put validateHostnameOrIPv4 and validatePortValue in utils.py, where other validation functions are, and rename them validate_hostname_or_ipv4 and validate_port (according to project coding style ane PEP8).

Congrats again for your contribution!

#23 Updated by romeopapa 2015-08-18 02:46:04

alant, thanks for the review

Having fun rebasing with git, will post cleaned up patches when I’m done.

#27 Updated by alant 2015-08-18 10:14:20

romeopapa wrote:
> There we go.
>
Great! I still have a few issues though.

The change in debian/control in 0b4d804 should be in 229e894 which is the commit adding the dependancy (or in a specific commit if you prefern but definately not in “The SOCKS host and port can be configured in whisperback’s config.py”

> You might want to have a look at the validate_port and validate_hostname_or_ipv4 functions. They return True/False (like the others in utils.py) but I don’t use thos values when calling the functions.

Your current proposal doesn’t make sense, because the return False statement cannot be reached.

Also, you call the validation functions with the wrong numbers of arguments:

<code class="python">
whisperBack.utils.validate_hostname_or_ipv4(self.smtp_host, "smtp_host")
</code>

<code class="python">
def validate_hostname_or_ipv4(self, hostname_or_ipv4, valuename):
</code>

whisperBack.utils is a module, not an object, so there is nothing to reference as self. You are writing pure functions here, not object methods (that’s actually why they are best in utils.py).

Finally, I’m not sure of the interest of passing valuename here (see below).

I think there is a choice that needs to be made here:

• either you use validate_ functions that raise exceptions, as in your 1st patch, and don’t set a return value
• or you use is_valid_ functions that return a boolean, as the others in util.py do, and then raise a whisperBack.exceptions.MisconfigurationException in __check_conf, e.g.:

<code class="python">
if not self.socks_host:
    raise whisperBack.exceptions.MisconfigurationException('socks_host')
elif not whisperBack.utils.is_valid_hostname_or_ipv4(self.socks_host):
    raise ValueError("socks_host is not a correct hostname or IPv4 address")
</code>

I tend to perfer the 2nd proposal which seems me more coherent with the current code, at the price of having less clear description of the cause of the lack of correctness.

To end with, no need to create patches and attach them here, pushing to your git repository is enough (for others that might want to follow the discussion, you can add an URL to its web interface).

Thanks by advance for your patience.

#28 Updated by romeopapa 2015-08-18 17:40:47

Firstly, I might have rushed things a little. Maybe if I actually tried to run the code I wrote, there’d be less things to change. Sorry about that. Secondly, thanks for putting things so eloquently and taking the time to walk me through all of this, I’m learning some good things!

> there is a choice that needs to be made here:
> * either you use validate_ functions that raise exceptions, as in your 1st patch, and don’t set a return value
> * or you use is_valid_ functions that return a boolean, as the others in util.py do, and then raise a whisperBack.exceptions.MisconfigurationException in __check_conf
>
> I tend to perfer the 2nd proposal which seems me more coherent with the current code, at the price of having less clear description of the cause of the lack of correctness.

Going with #2.

OK, I’ve repushed to https://gitlab.com/romeopapa/whisperback/commits/feature/python3-9412-add-socks-support-to-whisperback
All the clean and fixed commits are in there. I have actually tested the code this time ;)

#30 Updated by romeopapa 2015-08-20 05:37:30

> Don’t use a generic except but only catch the expected exception; in our case except ValueError: (https://www.python.org/dev/peps/pep-0008/#programming-recommendations).

The excerpt in question: “A bare except: clause will catch SystemExit and KeyboardInterrupt exceptions, making it harder to interrupt a program with Control-C, and can disguise other problems.”

I’ve ammended the last commit and repushed to https://gitlab.com/romeopapa/whisperback/commits/feature/python3-9412-add-socks-support-to-whisperback

#32 Updated by romeopapa 2015-08-22 08:49:30

intrigeri wrote:
> Cool! Please update the “QA Check” field accordingly next time: https://tails.boum.org/contribute/merge_policy/#submit

Hi intrigeri.. In case this message was directed towards me: I don’t think my user romeopapa on the redmine has the right to do that.. I think I can only add notes (and attachments) to a ticket, not much more (or perhaps I’m not allowed because I haven’t created THIS ticket).

Cheers!

#33 Updated by sajolida 2015-08-23 01:40:35

romeopapa: I granted you the “Contributor” role now. You should be able to do more stuff.

#40 Updated by alant 2015-09-14 13:09:10

Applied in changeset commit:c85411bb7f6a749a5c7607cb3309600428542f75.

#41 Updated by alant 2015-09-14 14:03:28

Applied in changeset commit:a25da197d39225a44e3c5cbd1855a9c9789b7023.

#42 Updated by intrigeri 2015-09-16 23:59:40

Looks good, thanks! Sent a report with that WhisperBack and asked frontdesk to forward it to me.

Two small notes, though:

• I find it sad that the versioning scheme for Jessie-specific releases was suddenly changed with no obvious reason, but well — I’ll try to remember it and not to mess up by assuming it’s still like it used to be.
• Pushed a commit on top to remove trailing whitespace introduced by the branch you merged — please make sure your Git prevents you from merging such stuff in the future.