Bug #15583: Replace su by a message to use sudo

Bug #15583

Replace su by a message to use sudo

Added by sajolida 2018-05-05 16:42:37 . Updated 2019-01-30 11:48:45 .

Status:

Resolved

Priority:

Normal

Assignee:

Category:

Target version:

Tails_3.12

Start date:

2018-05-05

Due date:

% Done:

100%

Feature Branch:

feature/15583-replace-su-with-message-to-use-sudo

Type of work:

Code

Blueprint:

Starter:

Affected tool:

Deliverable for:

Description

During the user testing of the Additional Software beta, 3 participants out of 5 tried instinctively su instead of sudo. In Tails the administration password doesn’t work with su and after that they got stuck to various degrees.

The other 2 participants didn’t know enough of the command line to do more than copy pasting things found online (sudo -i and sudo apt-get install mumble).

What about replacing the su binary with a message instructing to use sudo?

Subtasks

Related issues

Related to Tails - ~~Feature #14568~~: Additional Software Packages

Resolved

2013-12-11

2018-06-26

History

#1 Updated by sajolida 2018-05-05 16:42:48

Subject changed from Replace @su@ by a message to use @sudo@ to Replace su by a message to use sudo

#2 Updated by sajolida 2018-06-05 16:04:13

Beware: su might be used by scripts and applications outside of what the user does.

#3 Updated by intrigeri 2018-06-05 16:26:21

> Beware: su might be used by scripts and applications outside of what the user does.

git grep -w su in the Tails Git tree to start with :)

#4 Updated by taowa 2018-08-10 16:15:35

Would a bash alias just for amnesia be sufficient? It’s less drastic than replacing the binary (although it may still break things), and if the only time this is important is when a terminal is open as amnesia, then they will almost definitely be using bash (if they’re not, they should be able to figure out what’s happening).

#5 Updated by intrigeri 2018-08-10 16:33:54

Starter set to Yes

Hi Taowa!

Welcome aboard :)

> Would a bash alias just for amnesia be sufficient?

This looks like an excellent idea to me. We would need to ensure that only interactive bash shells are affected, in order to avoid breaking scripts that have /bin/bash in the shebang and use su. I believe config/chroot_local-includes/etc/profile.d/ would be a fine place to do that — of course the code would need to check the UID to apply this hack only for the amnesia user (alternatively, /etc/skel/.bash_profile would work, but the people will make the file persistent via dotfiles, and next time we update the file their fork will be left behind; we already have this problem for other files, let’s not make it worse).

I think this can be implemented and tested without building an ISO. https://tails.boum.org/contribute/git/ and https://tails.boum.org/contribute/how/code/ are the best starting points we have, and there’s often nice people on our tails-dev XMPP chatroom to answer questions :)

#6 Updated by taowa 2018-08-10 20:36:07

Hi intri!

As far as I can tell (although I may very well be wrong), gnome-terminal isn’t a login shell, which means that anything under profile.d won’t be run (I tested this by adding an amnesia user to my debian install and it wasn’t). If I add if [ $USER = amnesia ]; then alias su='echo "Hello! It is not possible to su on this machine. If you have set an administrative password, please use sudo instead."'; fi [0] to /etc/profile.d/disable-amnesia-su.sh it works when su -l amnesia, but not if I run gnome-terminal from there.

However, I discovered /etc/bash.bashrc, documented here . Its contents are run whenever any interactive bash shell is opened. The problem here being that this file has contents on a default Debian system, making editing the file specifically for Tails liable to break things if the default contents are modified in the future.

[0]: this text is just a draft, and I have no idea how to properly handle i18n…

#7 Updated by Anonymous 2018-08-16 11:05:49

related to ~~Feature #10034~~: Translation web platform added

#8 Updated by Anonymous 2018-08-16 11:06:03

related to deleted (~~~~Feature #10034~~: Translation web platform~~)

#9 Updated by Anonymous 2018-08-16 11:06:13

related to ~~Feature #14568~~: Additional Software Packages added

#10 Updated by groovefest 2018-09-11 22:37:56

Alternative proposal. Create a `su` script in /usr/local/bin that prints the given message. The default $PATH list in Debian is `/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games`. `su` is located in `/bin/`.

This way we avoid any conflict with custom user dotfiles.

#11 Updated by intrigeri 2018-09-19 14:25:06

groovefest wrote:
> Alternative proposal. Create a `su` script in /usr/local/bin that prints the given message.

I think this would affect all scripts that use su and break stuff.

#12 Updated by intrigeri 2018-09-19 14:33:33

Type of work changed from User interface design to Code

> As far as I can tell (although I may very well be wrong), gnome-terminal isn’t a login shell, which means that anything under profile.d won’t be run

Yeah, of course you’re right, sorry I was confused.

> However, I discovered /etc/bash.bashrc, documented here . Its contents are run whenever any interactive bash shell is opened.

Yes, let’s do this.

> The problem here being that this file has contents on a default Debian system, making editing the file specifically for Tails liable to break things if the default contents are modified in the future.

I think it’s fine to append the code we need to that file while building the ISO. I would do that with the config/chroot_local-hooks/ facility.

Once someone volunteers to implement it and gets something working:

ask sajolida, our local UX expert, what the explanatory message should be (so setting “type of work” to code, because I don’t want to ask sajolida to do this work before we know it’s going to be used :)
regarding i18n of shell scripts (painful and ugly but much needed), git grep 'TEXTDOMAIN="tails"' will point to example code we have; then po/POTFILES.in and refresh-translations need to be updated

#13 Updated by f.m3hm00d 2018-11-13 13:38:42

>
> Once someone volunteers to implement it and gets something working:
>
> # ask sajolida, our local UX expert, what the explanatory message should be (so setting “type of work” to code, because I don’t want to ask sajolida to do this work before we know it’s going to be used :)
> # regarding i18n of shell scripts (painful and ugly but much needed), git grep 'TEXTDOMAIN="tails"' will point to example code we have; then po/POTFILES.in and refresh-translations need to be updated

Hi all,

I have a little knowledge of Debian Live-CD mechanism. I am interested in addressing this bug. Based on the above comments, I was thinking of adding an alias for ‘su’ in ‘config/chroot-local_includes/etc/bash.bashrc’ file. Something like:

<code class="python">
alias su="echo Please use 'sudo' instead"
</code>

I hope this alias will achieve the title purpose. Feedback will be highly appreciated :)

Furthermore, @sajolida please suggest the message that should be displayed by the alias.

#14 Updated by sajolida 2018-11-19 19:13:09

Welcome f.m3hm00d and thanks for jumping in on this ticket!

> alias su=“echo Please use ‘sudo’ instead”

Yeah, that’s good! … with a period at the end :)

If you want to do a bit more work, in addition to telling people to use
sudo, the message should ideally also tell them that they have to
configure an administration password when starting Tails; if they haven’t.

We could reuse what we already have on
https://git.tails.boum.org/greeter/tree/data/no-password-lecture.txt

And add a sentence about sudo in it.

That would be:

By default, the administration password is disabled for better security.

In order to perform administration tasks, you need to setup an
administration password when starting Tails.

Then execute commands with sudo.

See the corresponding documentation:
https://tails.boum.org/administration_password/

I’m not sure how you can if an administration password has already been
set… It should work to test for the existence of
/etc/sudoers.d/tails-greeter-no-password-lecture but there’s probably
something smarter to do :)

#15 Updated by f.m3hm00d 2018-11-28 17:41:46

Assignee set to f.m3hm00d

> Welcome f.m3hm00d and thanks for jumping in on this ticket!
Pleasure is mine!

> Yeah, that’s good! … with a period at the end :)
Period added at the end :) Just submitted a patch to tails-dev@boum.org. Please review.

> If you want to do a bit more work, …
Always

> I’m not sure how you can if an administration password has already been
> set… It should work to test for the existence of
> /etc/sudoers.d/tails-greeter-no-password-lecture but there’s probably
> something smarter to do :)

I thought about using something like getent shadow amnesia and checking for the existence of password-hash but shadow is only readable by root. This approach didn’t work.

Thanks for the tails-greeter-no-password-lecture tip. Worked like a charm.

Though /etc/sudoers.d/tails-greeter-no-password-lecture is not world-readable. I had to echo the whole no-password-lecture in the hook.

Regards.

#16 Updated by sajolida 2018-11-29 11:52:38

Target version set to Tails_3.12
QA Check set to Ready for QA

Assigning to 3.12 which is our next major release.

Foundations Team: Tell me if it’s not the way to proceed!

#17 Updated by intrigeri 2018-12-02 21:41:33

Status changed from Confirmed to In Progress
Assignee changed from f.m3hm00d to intrigeri

> Foundations Team: Tell me if it’s not the way to proceed!

It’s the way to proceed except chances are slim that someone picks it up if it remains assigned to the code submitter.

#18 Updated by sajolida 2018-12-04 12:32:44

> […] except chances are slim that someone picks it up if it remains assigned to the code submitter.

Oops, I clearly overlooked this…

#19 Updated by intrigeri 2018-12-07 11:03:41

Assignee changed from intrigeri to f.m3hm00d
QA Check changed from Ready for QA to Dev Needed

> Just submitted a patch to tails-dev@boum.org. Please review.

There we go!

>> I’m not sure how you can if an administration password has already been set… It should work to test for the existence of /etc/sudoers.d/tails-greeter-no-password-lecture but there’s probably something smarter to do :)

> Thanks for the tails-greeter-no-password-lecture tip. Worked like a charm.

I’d rather make this consistent with the other place where we check the same thing: tails-screen-locker checks if the 2nd field in the stdout of passwd --status is “P”, which is the case if, and only if, a password is set. This approach, even if it’s a tad more complicated, feels better to me because the existence of /etc/sudoers.d/tails-greeter-no-password-lecture is a mere implementation detail of Tails Greeter, so relying on it in tails.git increases coupling between these 2 components, turns it into API and makes future changes harder.

Ideally, to avoid code duplication I would do this:

create a /usr/local/lib/tails-is-password-set helper (ideally in Python, worst case in good & robust shell), that returns 0 if and only if a password is set
adjust tails-screen-locker to use that helper; if the helper uses Python, great: tails-screen-locker can import the relevant function and use it; if the helper uses shell, then tails-screen-locker needs to execute that external script and check its exit code
adjust your new code to use that helper

Does this make sense to you?

> Though /etc/sudoers.d/tails-greeter-no-password-lecture is not world-readable. I had to echo the whole no-password-lecture in the hook.

Hold on, I think that’s not relevant: /etc/sudoers.d/tails-greeter-no-password-lecture merely controls the lecture_file sudo parameter but the text you want is in /usr/share/tails-greeter/no-password-lecture.txt, which is world-readable.

Regarding patching /etc/bash.bashrc at ISO build time: I recommended this so I’ll take the blame, but on second thought I realize it makes it harder to write and maintain the code snippets we want to add there, because we’re writing shell scripts as strings in another shell script, and then all kinds of quoting/escaping issues can happen (e.g. I’m not sure everyone who works on this in the future will realize that $NO_PASSWORD_LECTURE_FILE will be expanded at build time; and it’ll get much worse once we add the i18n code). So what about doing this instead:

make the build time hook only patch /etc/bash.bashrc to append just the tiny bits of shell needed to source or execute /etc/bash.bashrc.d/*, e.g. using run-parts
move to config/chroot_local-includes/etc/bash.bashrc.d/replace-su-with-sudo the code you currently append

Apart of that, two other things:

internationalization of the new “su is disabled. Please use sudo instead.” message. I’ve given hints earlier on this ticket wrt. how to do this.
I think the added code should be a no-op unless it’s run by the amnesia user. There’s no reason to break su for the root user, right? :)

#20 Updated by intrigeri 2018-12-07 11:05:32

Feature Branch set to wip/feature/15583-replace-su-with-message-to-use-sudo

Since this won’t be a matter of “send a single patch and we’re done”, let’s please use Git branches. I’ve imported your patch in the wip/feature/15583-replace-su-with-message-to-use-sudo branch, please base future work on this :)

#21 Updated by f.m3hm00d 2018-12-08 18:48:10

> * create a /usr/local/lib/tails-is-password-set helper (ideally in Python, worst case in good & robust shell), that returns 0 if and only if a password is set
> * adjust tails-screen-locker to use that helper; if the helper uses Python, great: tails-screen-locker can import the relevant function and use it; if the helper uses shell, then tails-screen-locker needs to execute that external script and check its exit code
> * adjust your new code to use that helper
>
> Does this make sense to you?

Sounds good. So I created tails-is-password-set which returns 0 if a password is set, 1 otherwise (using passwd —status). The function is named is_password_set. Everything seemed and worked fine.

Then I tried to modify tails-screen-locker to use the new helper. But apparently, its not (easily and elegantly) possible to import a function from another python file if that file’s name doesn’t end with a ‘.py’. Also, the file name shouldn’t include any hyphens. So importing is_password_set from tails-is-password-set doesn’t look very forthcoming.

A possible solution might be to name the helper tails_is_password_set.py and then create a softlink as tails-is-password-set. This way function import (from tails_is_password_set.py) will work fine and tails-is-password-set will remain an executable. Sounds good? Please give go-ahead so I can continue working on this solution.

> Hold on, I think that’s not relevant: /etc/sudoers.d/tails-greeter-no-password-lecture merely controls the lecture_file sudo parameter but the text you want is in /usr/share/tails-greeter/no-password-lecture.txt, which is world-readable.

Indeed.

> * make the build time hook only patch /etc/bash.bashrc to append just the tiny bits of shell needed to source or execute /etc/bash.bashrc.d/*, e.g. using run-parts
> * move to config/chroot_local-includes/etc/bash.bashrc.d/replace-su-with-sudo the code you currently append

Sure.

> Apart of that, two other things:
>
> * internationalization of the new “su is disabled. Please use sudo instead.” message. I’ve given hints earlier on this ticket wrt. how to do this.

I did read your comments earlier :) Perhaps I should get the English version working then I could add support for other localizations?

> * I think the added code should be a no-op unless it’s run by the amnesia user. There’s no reason to break su for the root user, right? :)

Agreed. I’ll modify the code to exempt root.

Regards.

#22 Updated by intrigeri 2018-12-09 09:03:28

Hi!

Meta: when requesting info from me, please set me as the Assignee and set the QA Check field to Info Needed, so it’s clear whose plate the next step is. Thanks in advance!

> Sounds good. So I created tails-is-password-set which returns 0 if a password is set, 1 otherwise (using passwd —status). The function is named is_password_set. Everything seemed and worked fine.

Excellent :)

> Then I tried to modify tails-screen-locker to use the new helper. But apparently, its not (easily and elegantly) possible to import a function from another python file if that file’s name doesn’t end with a ‘.py’. Also, the file name shouldn’t include any hyphens. So importing is_password_set from tails-is-password-set doesn’t look very forthcoming.

> A possible solution might be to name the helper tails_is_password_set.py and then create a softlink as tails-is-password-set. This way function import (from tails_is_password_set.py) will work fine and tails-is-password-set will remain an executable. Sounds good? Please give go-ahead so I can continue working on this solution.

What’s the advantage of adding a symlink? I think I would merely give that file the .py extension and be done with it: it’s a helper, it’s not in the $PATH, we can call it whatever we want.

>> * internationalization of the new “su is disabled. Please use sudo instead.” message. I’ve given hints earlier on this ticket wrt. how to do this.

> I did read your comments earlier :) Perhaps I should get the English version working then I could add support for other localizations?

Sure, this totally makes sense! I just mentioned it to make it easier to track what’s left to do here, without having to read the entire discussion. Also, this requirement partly determines what kind of implementation is a good idea or not (the “shell scripts as strings in another shell script” bits), so IMHO it’s worth keeping it in mind even if that’ll be tackled later.

Thanks for your work!

#23 Updated by f.m3hm00d 2018-12-10 13:03:23

> Meta: when requesting info from me, please set me as the Assignee and set the QA Check field to Info Needed, so it’s clear whose plate the next step is. Thanks in advance!
Noted. I’ll follow these steps in the future :)

> What’s the advantage of adding a symlink? I think I would merely give that file the .py extension and be done with it: it’s a helper, it’s not in the $PATH, we can call it whatever we want.

Importing and executing python functions in bash scripts doesn’t look very elegant (based on some google searches). The symlink might come in handy when its required to use is_password_set functionality from a bash script, as is the case with /etc/bash.bashrc.d/replace-su-with-sudo.

> Thanks for your work!
Thanks for your guidance!

#24 Updated by intrigeri 2018-12-10 13:44:01

>> What’s the advantage of adding a symlink? I think I would merely give that file the .py extension and be done with it: it’s a helper, it’s not in the $PATH, we can call it whatever we want.

> Importing and executing python functions in bash scripts doesn’t look very elegant (based on some google searches). The symlink might come in handy when its required to use is_password_set functionality from a bash script, as is the case with /etc/bash.bashrc.d/replace-su-with-sudo.

I did not mean importing Python from shell scripts. I meant having a Python file that can be executed both as a helper script (by shell scripts) or as a library (by Python programs), using the if __name__ == "__main__": pattern.

#25 Updated by f.m3hm00d 2018-12-11 15:26:42

> I did not mean importing Python from shell scripts. I meant having a Python file that can be executed both as a helper script (by shell scripts) or as a library (by Python programs), using the if __name__ == "__main__": pattern.

Got it.

#26 Updated by f.m3hm00d 2018-12-11 18:02:34

Assignee changed from f.m3hm00d to intrigeri
QA Check changed from Dev Needed to Info Needed

Any tip about adding /usr/share/tails-greeter/no-password-lecture.txt to replace-su-with-sudo.sh script in an “internationalization-friendly” manner? I’m currently using cat to display the lecture when no password is set. But for internationalization, I need to show the lecture using gettext. Can’t figure out an efficient way to do it. Any help would be appreciated!

> * internationalization of the new “su is disabled. Please use sudo instead.” message. I’ve given hints earlier on this ticket wrt. how to do this.

I modified replace-su-with-sudo.sh to use gettext to show this message. Updated po/POTFILES.in and refresh_translations, as you pointed out. Seems to be working fine.

Once I find some smart way to display the no-password-lecture.txt using gettext and test it locally with Tails ISO, I’ll put in a merge request.

Thanks!

#27 Updated by intrigeri 2018-12-17 16:13:54

Assignee changed from intrigeri to f.m3hm00d
QA Check changed from Info Needed to Dev Needed

>> Hold on, I think that’s not relevant: /etc/sudoers.d/tails-greeter-no-password-lecture merely controls the lecture_file sudo parameter but the text you want is in /usr/share/tails-greeter/no-password-lecture.txt, which is world-readable.

> Any tip about adding /usr/share/tails-greeter/no-password-lecture.txt to replace-su-with-sudo.sh script in an “internationalization-friendly” manner? I’m currently using cat to display the lecture when no password is set. But for internationalization, I need to show the lecture using gettext. Can’t figure out an efficient way to do it. Any help would be appreciated!

I say let’s not block on this: we’ve never localized this message so far. If you’re interested in fixing this, please file a dedicated ticket. IMO that’s low priority though.

#28 Updated by f.m3hm00d 2018-12-24 18:39:00

Assignee deleted (~~f.m3hm00d~~)
QA Check changed from Dev Needed to Ready for QA

> I say let’s not block on this: we’ve never localized this message so far. If you’re interested in fixing this, please file a dedicated ticket. IMO that’s low priority though.

I think I’ve completed the required work for this feature (except translation of no-password-lecture).

I don’t have commit rights to the tails repo. Please find my commits at [1]. Please review and advise.

[1] https://gitlab.com/m3hm00d/tails/tree/wip/feature/15583-replace-su-with-message-to-use-sudo

#29 Updated by intrigeri 2019-01-02 13:24:43

Assignee set to intrigeri

#30 Updated by intrigeri 2019-01-02 21:31:48

Assignee changed from intrigeri to f.m3hm00d
QA Check changed from Ready for QA to Dev Needed

> Please review and advise.

Here we go! I have a bunch of comments and recommendations but I think they should all be rather easy to address. I think we can make it in time for the 3.12 freeze (likely around January 15).

51-update-bash.bashrc:
- Please add set -u.
- The “Update /etc/bash.bashrc at runtime” comment does not reflect correctly what this hook does: /etc/bash.bashrc is modified at build time. What about “Make /etc/bash.bashrc source /etc/bash.bashrc.d/*” instead?
- The OPTS_DIR variable name is somewhat unfortunate given its content is not a directory. We don’t really need a variable here anyway, so perhaps just hard-code what you want below?
replace-su-with-sudo.sh
- Instead of linking to the ticket, please briefly sum up why we do that, to save some time to the next person who’ll work on this (possibly offline :)
- Please quote $USER and more generally, please run shellcheck on any shell script you write for Tails :)
- return does not make much sense outside of a function, does it? If this works anyway, please add a comment to explain why.
- Please write if tails_is_password_set.py; then which makes the control flow clearer.
- "`gettext \"su is disabled. Please use sudo instead.\"`" feels… weird. Missing echo?
tails_is_password_set.py
- I’m not convinced this should be in the $PATH. Usually we put this sort of things in config/chroot_local-includes/usr/local/lib/.

Also, to ease my next (and likely last) round of review, please make it explicit what scenarios you’ve tested and how. Thanks in advance!

#31 Updated by f.m3hm00d 2019-01-03 15:07:58

> Here we go! I have a bunch of comments and recommendations but I think they should all be rather easy to address. I think we can make it in time for the 3.12 freeze (likely around January 15).

Thanks a lot for your thorough review and valuable suggestions!

> * 51-update-bash.bashrc:
> Please add set -u.
Done.
> The “Update /etc/bash.bashrc at runtime” comment does not reflect correctly what this hook does: /etc/bash.bashrc is modified at build time. What about “Make /etc/bash.bashrc source /etc/bash.bashrc.d/*” instead?
Done. Comment changed.
> The OPTS_DIR variable name is somewhat unfortunate given its content is not a directory. We don’t really need a variable here anyway, so perhaps just hard-code what you want below?
Done. Hard-coded the directory path.
> * replace-su-with-sudo.sh
> Instead of linking to the ticket, please briefly sum up why we do that, to save some time to the next person who’ll work on this (possibly offline :)
Done. I added some details as you suggested. Link to the ticket is still there so the next person doesn’t have to search it.
> Please quote $USER and more generally, please run shellcheck on any shell script you write for Tails :)
Done. Thanks for the advice.
> return does not make much sense outside of a function, does it? If this works anyway, please add a comment to explain why.
The objective is to stop the script execution if the user is root. A quick look on man return tells that it can also be used to return from dot scripts. I’m not well versed in Bash. But return gets the job done here: continue execution of script if user != root, quit otherwise. I added a comment to clarify this.
> Please write if tails_is_password_set.py; then which makes the control flow clearer.
Done.
> "`gettext \"su is disabled. Please use sudo instead.\"`" feels… weird. Missing echo?
Done. Updated as: echo "`gettext \"su is disabled. Please use sudo instead.\"`". Works fine.
> * tails_is_password_set.py
> I’m not convinced this should be in the $PATH. Usually we put this sort of things in config/chroot_local-includes/usr/local/lib/.

The problem here is that the function is_password_set needs to be called from tails-screen-locker. Calling a python function from another python script can be easy if the callee script is placed in python’s sys.path. But since /usr/local/lib isn’t included in sys.path, I had to take care of certain things, such as:

Script needs to be in the same directory as the calling script
Script name must not contain any hyphens
Script name ends in ‘.py’

Due to these limitations, I had to name it tails_is_password_set.py, and place it in /usr/local/bin. Please let me know if I overlooked something and if there’s some way to place tails_is_password_set.py in /usr/local/lib, as you suggest, and keep it callable from tails-screen-locker.

> Also, to ease my next (and likely last) round of review, please make it explicit what scenarios you’ve tested and how. Thanks in advance!

Testing:

51-update-bash.bashrc:
I ran the script on my (non-debian) host machine. This script made the required changes to ‘/etc/bash.bashrc’, by sourcing ‘/etc/bash.bashrc.d/*’. There isn’t much else to test. I think it works fine.
replace-su-with-sudo.sh:
Building a custom Tails ISO using my git repo was not working for some reason so then I booted a live Tails system and placed the scripts where they’re supposed to be. The scenarios I tested were:
- amnesia receives the use sudo instead message when ‘su’ is run.
- root can use ‘su’ without any hindrance.
- Created a new user testuser. Played around with this account by locking it, setting password, removing password, etc. no-password-lecture is shown to this user when his password is not set and ‘su’ is executed. use-sudo-instead message is shown when his password is set and ‘su’ is executed. Just as programmed.

I’ve pushed changes to my gitlab repo [1]. Please review and advise.

[1] https://gitlab.com/m3hm00d/tails/tree/wip/feature/15583-replace-su-with-message-to-use-sudo

#32 Updated by f.m3hm00d 2019-01-03 15:09:44

Assignee changed from f.m3hm00d to intrigeri
QA Check changed from Dev Needed to Ready for QA

#33 Updated by hefee 2019-01-06 19:01:28

Assignee changed from intrigeri to f.m3hm00d
QA Check changed from Ready for QA to Dev Needed

Okay another round of reviews:

It proberly getting into shape :D

51-update-bash.bashrc:
> The OPTS_DIR variable name is somewhat unfortunate given its content is not a directory. We don’t really need a variable here anyway, so perhaps just hard-code what you want below?
I have more in mind to make it even simpler, as OPTS_FILE is only used once:
remove this line entirely OPTS_FILE='/etc/bash.bashrc'
and change the line to cat <<EOF >> /etc/bash.bashrc

Add add an comment inside bash.bashrc to the added snippset, so a user looking to bash.bashrc knows, that this snippset was added by this script.

replace-su-with-sudo.sh:

/usr/share/tails-greeter/no-password-lecture.txt is not translated to the users language right now. (But don’t care about this now, this is the state for years.)

tails_is_password_set.py:

missing “import sys” and replace exit(0) with sys.exit(0), there is no global exit function.

>The problem here is that the function is_password_set needs to be called from tails-screen-locker. Calling a python function from another python script can be easy if the callee script is placed in python’s sys.path. But since /usr/local/lib isn’t included in sys.path, I had to take care of certain things, such as:

That is all true. But /usr/local/lib/python3/dists-packages/ is in sys.path.
So place the tails_is_password_set.py to /usr/local/lib/python3/dists-packages/tails.py. Than you can use it with from tails import is_password_set in tails-screen-locker

an move the main function of tails_is_password_set.py to /usr/local/bin/replace-su-with-sudo.

And I would move the output part of replace-su-with-sudo.sh to /usr/local/bin/replace-su-with-sudo. In the end replace-su-with-sudo.sh would looks like (without comments):

[ "$USER" == root ] && return
su () {
    /usr/local/bin/replace-su-with-sudo
}

Sorry a lot of fiddle ring around, that it is even harder to describe what I have in mind. So if there is any question regarding my review, feel free to ask!
Or if you get stuck somewhere - I’m happy to help.

#34 Updated by f.m3hm00d 2019-01-07 08:06:26

> It proberly getting into shape :D
Yayy! :D
> I have more in mind to make it even simpler, as OPTS_FILE is only used once:
> remove this line entirely OPTS_FILE='/etc/bash.bashrc'
> and change the line to cat <<EOF >> /etc/bash.bashrc
Done.
> * Add add an comment inside bash.bashrc to the added snippset, so a user looking to bash.bashrc knows, that this snippset was added by this script.
Done.
> tails_is_password_set.py:
> * missing “import sys” and replace exit(0) with sys.exit(0), there is no global exit function.
Done.
> So place the tails_is_password_set.py to /usr/local/lib/python3/dists-packages/tails.py. Than you can use it with from tails import is_password_set in tails-screen-locker
Done. Thanks for the tip!
> an move the main function of tails_is_password_set.py to /usr/local/bin/replace-su-with-sudo.
I am not sure I understand what you mean here. I suppose you are asking me to create a new python script /usr/local/bin/replace-su-with-sudo? If I were to move def main() from tails_is_password_set.py to /usr/local/bin/replace-su-with-sudo, then I’d also have to modify the __name__ function in tails_is_password_set.py from main() to is_password_set(). Please clarify.

> And I would move the output part of replace-su-with-sudo.sh to /usr/local/bin/replace-su-with-sudo. In the end replace-su-with-sudo.sh would looks like (without comments):
On the basis of the supposition that replace-su-with-sudo is a python script, I would have to python-ify (is that a word? It should be) the bash code. I’d also have to move and python-ify the localization code.

Making these changes is doable but testing the various changes takes lots of time, serious attention and efforts. With regard to the previous two suggestions, please discuss/finalize the location/content of involved files so that I could make the changes.

> it is even harder to describe what I have in mind
Tell me about it :D

Lots of thanks for your time!

#35 Updated by f.m3hm00d 2019-01-07 08:07:13

Assignee changed from f.m3hm00d to hefee
QA Check changed from Dev Needed to Info Needed

#36 Updated by hefee 2019-01-08 02:06:17

You haven’t pushed your branch yet again, so I can’t check what you have done already. But you had questions:

> […]
> > So place the tails_is_password_set.py to /usr/local/lib/python3/dists-packages/tails.py. Than you can use it with from tails import is_password_set in tails-screen-locker
> Done. Thanks for the tip!
> > an move the main function of tails_is_password_set.py to /usr/local/bin/replace-su-with-sudo.
> I am not sure I understand what you mean here. I suppose you are asking me to create a new python script /usr/local/bin/replace-su-with-sudo?

right

If I were to move def main() from tails_is_password_set.py to /usr/local/bin/replace-su-with-sudo, then I’d also have to modify the __name__ function in tails_is_password_set.py from main() to is_password_set(). Please clarify.

No, as you don’t need the main method in tails.py anymore, as tails.py is a library it is not meant to be used as script.

> > And I would move the output part of replace-su-with-sudo.sh to /usr/local/bin/replace-su-with-sudo. In the end replace-su-with-sudo.sh would looks like (without comments):
> On the basis of the supposition that replace-su-with-sudo is a python script, I would have to python-ify (is that a word? It should be) the bash code. I’d also have to move and python-ify the localization code.

exacty python-ify is the goal to reach, as python is easier to extent and have a better security in place like for bash scripts. That’s why Tails prefer to get new code written in python. I’m sorry, that it means that you have to do additional work to do.

Okay I’ll outline the script, to get you started:

/usr/local/bin/replace-su-with-sudo:

<code class="python">
def main() -> None:
    if is_password_set():
        print(_('"su is disabled. Please use sudo instead."'))
        exit(0)
    else:
        print(open("/share/tails-greeter/no-password-lecture.txt").read())
        exit(1)

if __name__ == "__main__":
    # Initialize translations
    translation = gettext.translation("tails", fallback=True)
    _ = translation.gettext

    main()
</code>

> > it is even harder to describe what I have in mind
> Tell me about it :D

I hope this helps you understanding, what I have in mind. As I read your answer you are not satisfied by rewriting the script in Python. So feel free to ask for help, if you get stuck.

#37 Updated by hefee 2019-01-08 02:09:47

Assignee changed from hefee to f.m3hm00d
QA Check changed from Info Needed to Dev Needed

#38 Updated by f.m3hm00d 2019-01-08 14:56:53

Assignee changed from f.m3hm00d to hefee
QA Check changed from Dev Needed to Ready for QA

> No, as you don’t need the main method in tails.py anymore, as tails.py is a library it is not meant to be used as script.
Earlier the plan was to program it as a script as well as a library. Hence the confusion. Thanks for the clarification!

> I hope this helps you understanding, what I have in mind. As I read your answer you are not satisfied by rewriting the script in Python. So feel free to ask for help, if you get stuck.
That was tremendously useful!

> > tails_is_password_set.py:
> > * missing “import sys” and replace exit(0) with sys.exit(0), there is no global exit function.
> Done.

Now that tails_is_password_set.py has nothing except the is_password_set function, I think this recommendation is no longer applicable.

Thanks for the detailed instructions and feedback. I have made all the suggested changes (except the one about import sys), and pushed them to my repo [1]. The commit message details the changes made to each of the file. I hope you’ll find those comments useful.

Testing:

I copied over the modified files to a live Tails system for testing as described at [2]. Things work as expected. The only thing I couldn’t test is the translation part. I made the required changes in POTFILES.in and refresh-translations, but I’m not so sure about how to use gettext with python. I simply copied the code from the outline you provided.

Please review and advise. Thanks for your time!

[1] https://gitlab.com/m3hm00d/tails/tree/wip/feature/15583-replace-su-with-message-to-use-sudo
[2] https://redmine.tails.boum.org/code/issues/15583/#note-31

#39 Updated by hefee 2019-01-09 00:13:55

Some small last things:

rename tails_is_password_set.py -> tails.py, yes there is only one function inside this file. But tails.py is actually make up to adding more functions/classes around tails.
add a comment on top telling that it coming from tails.git
replace-su-with-sudo:
- add comment on the top, what this script does and where it comes from.
- import sys and use sys.exit there is no exit method in Python. The whole magic you are using is and it is python3 interpreter specific:
https://docs.python.org/3/library/constants.html?highlight=exit#exit
The script will blow up if used with ipython3 or other python interpreter. That’s why using sys.exit is recommended.

- Instead of using from lib import func better use import lib that keeps namespaces separated and it is easier to spot, where you have to search for this function.

 
   import tails
[...]
  if tails.is_password_set():

Please also change that for tails-screen-locker.

> I copied over the modified files to a live Tails system for testing as described at [2]. Things work as expected. The only thing I couldn’t test is the translation part. I made the required changes in POTFILES.in and refresh-translations, but I’m not so sure about how to use gettext with python. I simply copied the code from the outline you provided.

You normally just run ./refresh-translations and get updated PO files. Can you past the error here?

#40 Updated by f.m3hm00d 2019-01-09 13:14:31

> * rename tails_is_password_set.py -> tails.py, yes there is only one function inside this file. But tails.py is actually make up to adding more functions/classes around tails.
> * add a comment on top telling that it coming from tails.git
> * replace-su-with-sudo:
> - add comment on the top, what this script does and where it comes from.
> - import sys and use sys.exit there is no exit method in Python. The whole magic you are using is and it is python3 interpreter specific:
> https://docs.python.org/3/library/constants.html?highlight=exit#exit
> The script will blow up if used with ipython3 or other python interpreter. That’s why using sys.exit is recommended.
>
> - Instead of using from lib import func better use import lib that keeps namespaces separated and it is easier to spot, where you have to search for this function.
>
> […]
>
> Please also change that for tails-screen-locker.
Done and committed. Thanks for the helpful suggestions!

During testing, I noticed that the use sudo instead message produced by ‘su’ was enclosed in double quotes:

<code class="text">
amnesia@amnesia:~$ su
"su is disabled. Please use sudo instead."
</code>

So I removed the innermost double quotes from the following line in replace-su-with-sudo:

<code class="python">
        print(_('"su is disabled. Please use sudo instead."'))
</code>

The output message is now quotes free.

> You normally just run ./refresh-translations and get updated PO files. Can you past the error here?
I ran ./refresh-translations. No errors were produced. PO files were updated successfully. I added these changes as a separate commit.

Please review and advise.

#41 Updated by hefee 2019-01-09 21:43:57

Assignee changed from hefee to intrigeri
QA Check changed from Ready for QA to Pass
Feature Branch changed from wip/feature/15583-replace-su-with-message-to-use-sudo to hefee/feature/15583-replace-su-with-message-to-use-sudo

f.m3hm00d I’m happy about the current state of your patch and good comments you added.
I push your branch to hefee/feature/15583-replace-su-with-message-to-use-sudo, so CI will built it.
I reverted the PO files update, as it produces merge conflicts with devel. Those need to be updated, when merging to devel.
I pull and merged to devel into my branch.

intrigeri: After CI built the branch, merge the branch and update the PO files by running ./refresh-translations after merge.

#42 Updated by intrigeri 2019-01-10 07:25:53

QA Check changed from Pass to Ready for QA

(My understanding is that the test suite was not run yet ⇒ review not completed ⇒ setting back to “Ready for QA” accordingly.)

Thanks!

#43 Updated by f.m3hm00d 2019-01-11 13:47:58

> (My understanding is that the test suite was not run yet ⇒ review not completed ⇒ setting back to “Ready for QA” accordingly.)

Please guide me about how to proceed forward? Those tests need to be carried out by me or some reviewer?

Thanks!

#44 Updated by intrigeri 2019-01-11 16:32:31

> Please guide me about how to proceed forward? Those tests need to be carried out by me or some reviewer?

I’ll do it (as per the Assignee field): I don’t know if anyone has built an ISO and run our test suite, and in any case our review process requires to do that :)

#45 Updated by intrigeri 2019-01-11 17:36:34

Assignee changed from intrigeri to f.m3hm00d
% Done changed from 0 to 60
QA Check changed from Ready for QA to Dev Needed

No obvious breakage in the (partial) test suite run on Jenkins, woohoo! :)

A few comments:

It feels wrong to introduce a second Tails Python library. I realize you may not be aware that we already had one though. I suggest adding this code to submodules/pythonlib/tailslib/adminpassword.py.
Please keep PYTHON_PROGS sorted.
Please update the PO files (now that the branch is up-to-date wrt. devel so this can be done safely), otherwise the ISO will never build reproducibly in our CI, and then we can’t tell before merging whether it will break reproducible builds.

Thanks in advance!

Also, hefee: did you build from this branch and test both the “password is set” and “password is not set” situations?

#46 Updated by f.m3hm00d 2019-01-11 19:11:26

Assignee changed from f.m3hm00d to intrigeri
% Done changed from 60 to 0
QA Check changed from Dev Needed to Ready for QA

#47 Updated by f.m3hm00d 2019-01-11 19:12:51

% Done changed from 0 to 60

> No obvious breakage in the (partial) test suite run on Jenkins, woohoo! :)
:D

> * It feels wrong to introduce a second Tails Python library. I realize you may not be aware that we already had one though. I suggest adding this code to submodules/pythonlib/tailslib/adminpassword.py.
I moved the tails.py file to the location you specified, and made appropriate changes in tails-screen-locker and replace-su-with-sudo. I hope that’s what you wanted.

> * Please keep PYTHON_PROGS sorted.
> * Please update the PO files (now that the branch is up-to-date wrt. devel so this can be done safely), otherwise the ISO will never build reproducibly in our CI, and then we can’t tell before merging whether it will break reproducible builds.
Done.

I made these changes on top of hefee’s feature 15583 branch. The changes can be viewed at [1].

Please review and advise.

[1] https://gitlab.com/m3hm00d/tails/tree/hefee/feature/15583-replace-su-with-message-to-use-sudo

#48 Updated by intrigeri 2019-01-12 08:48:40

Assignee changed from intrigeri to f.m3hm00d
QA Check changed from Ready for QA to Dev Needed

>> * It feels wrong to introduce a second Tails Python library. I realize you may not be aware that we already had one though. I suggest adding this code to submodules/pythonlib/tailslib/adminpassword.py.
> I moved the tails.py file to the location you specified, and made appropriate changes in tails-screen-locker and replace-su-with-sudo. I hope that’s what you wanted.

Thank you. Something seems wrong though: it seems that you’re dropping the submodules/pythonlib submodule, which will break stuff. I think you need to do this:

fork this submodule
add the new adminpassword.py to your fork of that submodule
update your branch (in tails.git) so the pointer to this submodule points to your updated version

Cheers!

#49 Updated by hefee 2019-01-13 11:57:56

Hey,

> Also, hefee: did you build from this branch and test both the “password is set” and “password is not set” situations?

As I had not enough i-net the last days, I couldn’t build a ISO to test stuff on the ISO. I just tested parts of it locally.

#50 Updated by f.m3hm00d 2019-01-13 12:07:41

> Thank you. Something seems wrong though: it seems that you’re dropping the submodules/pythonlib submodule, which will break stuff.
It seems pythonlib is not a regular directory? I made another clone of hefee’s feature branch. Making changes inside this directory shows no changes when git status is executed. Issuing git status after copying over the tails.py file into this directory again reports pythonlib as deleted.

> # fork this submodule
> # add the new adminpassword.py to your fork of that submodule
> # update your branch (in tails.git) so the pointer to this submodule points to your updated version
I’m sorry but my rather limited knowledge of git is keeping me from understanding and following these steps.

#51 Updated by intrigeri 2019-01-13 12:35:27

> It seems pythonlib is not a regular directory?

Indeed, it’s a Git submodule.

>> # fork this submodule
>> # add the new adminpassword.py to your fork of that submodule
>> # update your branch (in tails.git) so the pointer to this submodule points to your updated version
> I’m sorry but my rather limited knowledge of git is keeping me from understanding and following these steps.

I think you should probably start with the gitsubmodules(7) manpage or (probably better) the submodules chapter of the “Pro Git” (free online) book.
I understand the learning curve is a bit steep here but given what I’ve seen on this ticket, I’m very confident you’ll be able to learn it pretty fast! :)

#52 Updated by f.m3hm00d 2019-01-13 16:32:34

> Indeed, it’s a Git submodule.
I didn’t see that coming :p

> I think you should probably start with the gitsubmodules(7) manpage or (probably better) the submodules chapter of the “Pro Git” (free online) book.
> I understand the learning curve is a bit steep here but given what I’ve seen on this ticket, I’m very confident you’ll be able to learn it pretty fast! :)
Thanks for that excellent guidance!

I cloned the pythonlib submodule, copied over the tails.py library, and pushed the changes to [2].

Then I updated my tails.git repo [1] to reflect the changes made in tails.py and .gitmodules. I hope that’s what you wanted.

Please note that the changes I made in pythonlib submodule were committed to feature/15583-replace-su-with-message-to-use-sudo branch. However, the pythonlib submodule in .gitmodules file points to https://gitlab.com/m3hm00d/pythonlib. I assume tails.git will fetch the master branch of pythonlib submodule, while I committed changes to features/15583... branch.

Please review and advise.

[1] https://gitlab.com/m3hm00d/tails/tree/hefee/feature/15583-replace-su-with-message-to-use-sudo
[2] https://gitlab.com/m3hm00d/pythonlib/tree/feature/15583-replace-su-with-message-to-use-sudo

#53 Updated by f.m3hm00d 2019-01-13 16:33:36

Assignee changed from f.m3hm00d to intrigeri
QA Check changed from Dev Needed to Ready for QA

#54 Updated by intrigeri 2019-01-14 17:07:28

Feature Branch changed from hefee/feature/15583-replace-su-with-message-to-use-sudo to feature/15583-replace-su-with-message-to-use-sudo

> Thanks for that excellent guidance!

Thank you for being ready to learn! :)

> I cloned the pythonlib submodule, copied over the tails.py library, and pushed the changes to [2].

> Then I updated my tails.git repo [1] to reflect the changes made in tails.py and .gitmodules. I hope that’s what you wanted.

Looks great from a Git repo perspective. I’ve pushed your pythonlib branch to the official repo of this submodule and reset .gitmodules accordingly: commit:8c798291258d4281e91d81fab11606050c5e17b0

> Please note that the changes I made in pythonlib submodule were committed to feature/15583-replace-su-with-message-to-use-sudo branch. However, the pythonlib submodule in .gitmodules file points to https://gitlab.com/m3hm00d/pythonlib. I assume tails.git will fetch the master branch of pythonlib submodule, while I committed changes to features/15583... branch.

Actually, the parent repo has no clue what branch it tracks: it encodes the actual commit you want. To do so you need to commit submodules/pythonlib in the parent repo (tails.git), i.e. update the pointer. I did that too: commit:aba1e089c299e50f73839a7c2aa07f2dec988d18. Please take a look at these commits, this should help you understand how to deal with submodules :)

I’ll now build and test stuff manually, while Jenkins builds and runs the test suite.

#55 Updated by intrigeri 2019-01-14 20:05:34

% Done changed from 60 to 70

Added a fix: commit:15e4d615f868ba06ed14b1dc4ed9c27685d6b77c (otherwise the library would not be found, I guess). Tested by hand, works fine! In case you missed them, see my previous comments.

Now waiting for test suite results. If Jenkins is happy, I’m happy, and then I’ll merge!

#56 Updated by intrigeri 2019-01-15 07:19:51

Assignee changed from intrigeri to f.m3hm00d
QA Check changed from Ready for QA to Dev Needed

Test suite passe!

Now, one last thing: I think we should only set up this machinery for $LIVE_USERNAME aka. amnesia, just to be safe (it does not make sense for other users so let’s not take any risk). Do you have time to do this in time for the freeze? (https://tails.boum.org/contribute/calendar/) If not, I can try to do it myself.

#57 Updated by f.m3hm00d 2019-01-15 13:03:03

Assignee changed from f.m3hm00d to intrigeri
QA Check changed from Dev Needed to Ready for QA

> Test suite passe!
Phew. Atlast! I have a newfound respect for Linux developers :D

And thanks for making those other changes!

> Now, one last thing: I think we should only set up this machinery for $LIVE_USERNAME aka. amnesia, just to be safe (it does not make sense for other users so let’s not take any risk). Do you have time to do this in time for the freeze? (https://tails.boum.org/contribute/calendar/) If not, I can try to do it myself.
I made the change on top of the latest feature/15583-replace-su-with-message-to-use-sudo branch. I hope this is what you wanted [1].

Please review and advise.
[1] https://gitlab.com/m3hm00d/tails/commit/425f38de99b2163aff290aeab6f0e01a070043d4

#58 Updated by intrigeri 2019-01-16 07:58:19

Assignee changed from intrigeri to hefee

>> Now, one last thing: I think we should only set up this machinery for $LIVE_USERNAME aka. amnesia, just to be safe (it does not make sense for other users so let’s not take any risk). Do you have time to do this in time for the freeze? (https://tails.boum.org/contribute/calendar/) If not, I can try to do it myself.
> I made the change on top of the latest feature/15583-replace-su-with-message-to-use-sudo branch. I hope this is what you wanted [1].

> Please review and advise.

The freeze is now very very close so I fixed myself (commit:d92bfe9f7f3b0db0461b13685771942a13333b2d, commit:4d3c1121beaca93223af03dc3e1eadbdf791746b) the remaining 2 issues I’ve spotted in your last change (commit:425f38de99b2163aff290aeab6f0e01a070043d4). Will now build, test manually, wait for CI results, and try to find a reviewer: there’s now too much of my own changes on this branch for me to feel comfortable merging it myself.

hefee, can you take this review?

I’ll still do the tests described above and will report here, but there’s probably no need for the reviewer to block on this before they start their part of the work :)

#59 Updated by intrigeri 2019-01-16 08:32:03

> Will now build, test manually

Manual testing went fine. Methodology:

start Tails, don’t set any admin password in the Greeter but enable Tor bridges (/usr/local/sbin/tails-tor-launcher uses pkexec)
Tor Launcher starts as expected after login
try to use su and sudo in a terminal: got the expected message
try to start “Root Terminal” from the Activities overview (this uses pkexec): fails as expected
start Tails, set an admin password in the Greeter and enable Tor bridges
Tor Launcher starts as expected after login
try to use su in a terminal: I’m told to use sudo
use sudo in a terminal: works as expected
start “Root Terminal” from the Activities overview: starts as expected

#60 Updated by anonym 2019-01-16 14:45:11

Status changed from In Progress to Fix committed
% Done changed from 70 to 100

Applied in changeset commit:tails|8da89ef2b69654f0680fb16f932ea2b074d6494d.

#61 Updated by anonym 2019-01-16 14:54:12

Assignee deleted (~~hefee~~)
QA Check changed from Ready for QA to Pass

I stole the last part of this review from hefee just to increase chances for inclusion before the 3.12 feature freeze.

The code looks great! I avoided the long history and just reviewed the diff, so in that sense I reviewed all code and not just intrigeri’s last few commits. I only pushed a trivial whitespace fix (commit:e8d2a644a2b11a0f1ef99f91980aafc13cbcadda) which doesn’t require review. I did some testing of my own (but I mostly rely on intrigeri’s testing) and found nothing odd so it looks ready to ship.

I’ve merged the branch so this feature will first appear in Tails 3.12 (nitpicking: actually it will first appear in the release candidate we’ll release by the end of the weekend)!

Congrats for your first contribution, f.m3hm00d! Would you like to be credited in the Tails 3.12 changelog, and if so which nick/name do you prefer?

#62 Updated by f.m3hm00d 2019-01-16 15:07:18

>actually it will first appear in the release candidate we’ll release by the end of the weekend)!
I’ll be waiting :D

> Congrats for your first contribution, f.m3hm00d!
Thanks. But I think the real credit goes to you guys for being helpful beyond my imagination!
> Would you like to be credited in the Tails 3.12 changelog, and if so which nick/name do you prefer?
Yes, please. My current nick (f.m3hm00d) would do.

#63 Updated by anonym 2019-01-30 11:48:45

Status changed from Fix committed to Resolved