[Whonix-devel] misc qubes-whonix 11 code review

Thu Jun 4 14:48:26 CEST 2015

On 06/04/2015 07:41 AM, Patrick Schleizer wrote:
> Hi Jason!
>
> Was looking at your progress:
> https://github.com/nrgaway/qubes-whonix/tree/Whonix11

Thanks, I haven't even have time to review the code yet :)  It still
needs some modifications for the whonix--tor-enable{disable} and I might
be able to eliminate the qubes-whonixsetup script completely; just takes
time to test.  Today, I will be creating a pre-configuration manager
states (profile configurations) to allow Whonix to be installed with Tor
disabled and possibly create a new package for salt version 2015.5
(current is 2014.7)

In future, I do find it useful, and maybe less confusing to provide
comments on github.  You are able to comment at each line of code.  Not
sure if you are interested in doing this?

Also, I will include Joanna on this reply, but not future ones.  I have
been taught not to include her in developer speak types of conversations
since she already receives way too much email.

Even Marek would not need to be included until its ready for merge or we
have some question that needs his input.

>
>>     # Make sure we remove whonixsetup.done if Tor is not enabled
>>     # to allow choice of repo and prevent whonixcheck errors
>>     grep "^DisableNetwork 0$" /etc/tor/torrc || {
>>         sudo rm -f var/cache/whonix-setup-wizard/status-files/whonixsetup.done
>>     }
> Unless I am missing something, this won't work. There is a bug. 'var' ->
> '/var'

Yes, it appears it would not work.  I need to re-add code to
qubes-whonixsetup as well to display whonix-setup-manager too.  It'd in
the NOTES (TODO) in root directory.

> Drop sudo? Runs as root anyhow?

Yes

> Also not sure it's needed at all, because if
> - /var/cache/whonix-setup-wizard/status-files/whonixsetup.done exists +
> - /etc/tor/torrc contains 'DisableNetwork 0',
> then on Whonix-Gateway by Whonix / whonix-setup-wizard default, the
> following is started anyhow:
> 'whonix-setup-wizard setup'

I don't understand this point.  The current whonix-setup-wizard is
over-ridden and runs manually in qubes-whonixsetup when needed.  This is
something that may be able to be re-enabled due to recent improvements
of whonix-setup-wizard.  As you can already see I have managed to remove
most manual depends within that file.

>>     # Repository setup should only be run in template
>>     if [ ! -e /var/cache/whonix-setup-wizard/status-files/whonix_repository.done ]; then
>>         sudo touch /var/cache/whonix-setup-wizard/status-files/whonix_repository.done
>>     fi
> Drop sudo? Runs as root anyhow?
>
> Any reason for not
> '/var/cache/whonix-setup-wizard/status-files/whonix_repository.done' ->
> '/var/cache/whonix-setup-wizard/status-files/whonix_repository.skip'?

I can't remember off hand.  At some point I had issues with whonixsetup
not running unless certain status-files existed.

> Also rather than 'touch
> /var/cache/whonix-setup-wizard/status-files/whonix_repository.skip' I
> think it would be best just ship the file
> var/cache/whonix-setup-wizard/status-files/whonix_repository.skip in the
> package.
>
> From usr/lib/qubes-whonix/qubes-whonixsetup you would still be able to
> run /usr/bin/whonix-setup-wizard repository as you're doing now. [No
> modification of usr/lib/qubes-whonix/qubes-whonixsetup required for this.]

There is a good reason for using 'touch' :)  If I provide that in the
package, the setting gets written to the TemplateVM and then the
TemplateVM will have the setting which means a user would not be
prompted to enable repo ever.  This setting is only set in the AppVM and
when set only persists in the AppVM and therefore can not be set in the
package.

>> usr/lib/qubes-whonix/init/qubes-whonix-sysinit
> Drop sudo? Runs as root anyhow?
>
>> subprocess.call(['systemctl', 'reload', 'tor'])
> 'reload' -> 'restart'? I guess that would be better due to this
> non-systemd-related(!) issue with 'reload' in upstream Tor:
> https://trac.torproject.org/projects/tor/ticket/16161

I was also thinking about this.  This function will never be called
unless the script is run manually since it gets called before the
multiuser target now.

>> /usr/lib/qubes-whonix/init/qubes-whonix-sysinit
> Contains two times sudo. Drop sudo? Runs as root anyhow?
>
>> license headers
> Not all scripts contain license headers.

I will clean that up when ready to merge into master.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0x77DC3687.asc
Type: application/pgp-keys
Size: 6049 bytes
Desc: not available
URL: <http://www.whonix.org/pipermail/whonix-devel/attachments/20150604/01467e0f/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://www.whonix.org/pipermail/whonix-devel/attachments/20150604/01467e0f/attachment.sig>