Tails

#4 Updated by matsa 2013-11-10 06:12:58

From the old forum thread, tested with Tails 0.21 :

The lockscreen function with gnome-screensaver is bypassable.
Here is how to reproduce:

• Setup an administration password in the greeter
• Install the package gnome-screensaver
• Launch it with sudo -u amnesia gnome-screensaver
• Lock your screen

Now, to unlock the screen without entering password:

• Open a text-console with “Ctrl+Alt+F2”
• Run the command killall –9 gnome-screensaver
• Go back with “Ctrl+Alt+F7”

The screen is unlocked

#9 Updated by mercedes508 2014-08-22 10:42:10

When doing the following:

• Enable persistence.
• Add gnome-screensaver to
  /live/persistence/TailsData_unlocked/live-additional-software.conf
• Restart Tails.
• Set an administrative password.
• Start the screensaver by running:

gnome-screensaver-command —activate

The screen is not locked. No password will be asked to
disable the screensaver.

The administrative passsword should be asked to exit the
screensaver.

This can be achieved by issuing the following command:

gsettings set org.gnome.desktop.lockdown disable-lock-screen false

I recommend adding it in /etc/gdm3/PostLogin/Default when
an administrative password is set. It should have no effect
when gnome-screensaver is not installed.

#11 Updated by micahflee 2014-11-26 18:23:08

anonym wrote:
> With shutdown-helper GNOME Shell extension (Bug #8302) it is trivial to add the Lock Screen button when we’re based on Debian Jessie. However, do we actually want to support screen locking?
>
> Screen locking it’s not a very strong defense for keeping the current session’s data from someone that takes physical control of the computer running Tails. Still, it’s better than nothing in a situation where you want to leave the session running among trusted people, but that you still do not want to have access to the session. Unfortunately users may get a false sense of security from this feature.

I think supporting this feature is very important. I’ve worked in multiple office situations where employees use Tails, and sometimes they need to step out of their office to use the bathroom, or join a brief meeting, etc., but don’t want to shut down the computer because they’re in the middle of work and will be back in a minute. It makes me very concerned that there’s nothing protecting the active session when this happens. I don’t like even walking out of a room to refill my coffee with my screen unlocked. I think a lock screen feature would go a long way to making Tails more secure.

It might make sense if there were some warning or message on the lock screen that informed users that locking the screen is not a strong defense against physical attackers, and that you should shut down the computer instead if you’ll be gone for very long.

#12 Updated by BitingBird 2014-11-27 07:25:19

The warning seems like a good compromise to me.

#13 Updated by intrigeri 2014-11-27 09:31:37

> It might make sense if there were some warning or message on the lock screen that informed users that […]

Good idea, but not sure how well it would work in practice: in my experience, at least GNOME Shell blanks the screen (DPMS or something) immediately after it’s been locked, so users would actually not see this warning before they come back and want to unlock the session. Of course, then they’ve seen it once, and can remember these shortcomings next time. Is that good enough?

#17 Updated by Anonymous 2014-12-03 21:47:19

During the contributor meeting a proposal was made:

instead of using the admin password (that is, really, user password + sudo), a second password would make sense. In order to set this password, we would prompt the user on the first usage of the screen lock feature to set a screen lock password.

This password could also be made persistent, for persistence users. See related tasks.

#27 Updated by elypter 2015-09-16 14:34:13

besides trying to make circumvention as difficult as possible it should also indicate when the system has been unlocked and locked again.
this could be done by:
-showing the time the screen has been locked and how long
-an incerementing number that increases wen the screen is being locked
-a unique randomly generated visually distinctive and easy to remember image. like random-art.org or terragen. if there exist high quality alternatives it could look quite elegant as side effect.

#28 Updated by sajolida 2015-09-17 04:48:22

I’m not sure to understand your proposal. What concrete problem or attack are you trying to prevent with this?

#29 Updated by elypter 2015-09-17 13:47:25

*the victim goes away from his desktop
*the attacker goes to his pc and circumvents the lockscreen and then does his stuff
*then the attacker locks the screen again
*the victim comes back and doesnt notice anything indicating the attack

if every lockscreen looks different though the victim would be able to see that the system has been locked again and knows that he has been spied upon and his system could be compromized.

#30 Updated by elypter 2015-09-17 13:54:35

there could also be an indicator that shows if someone has tried to unlock the system by checking for keyboard, pointing device, camera, microphone, usb or accelerometer activity.

#31 Updated by sajolida 2015-09-18 04:35:18

If the attacker manages to circumvent the screenlocking, in most scenarios the security of Tails is already defeated. Noticing it might not change much. And the attacker might as well be able to circumvent the techniques that you are proposing. So I’m really not convinced it’s worth it both for us in terms of technical work and for the user in terms of added complexity of the interface.

I’d rather work on making the user understand that leaving their computer unattended is very risky and should be with very much care.

Also, if I were an attacker with unlimited technical power, I would instead insert malicious devices in the computer ports (USB, firewire, etc.) with exploits to access memory. And that would be detected by none of your tricks.

#32 Updated by elypter 2015-09-18 10:47:15

sajolida wrote:
> If the attacker manages to circumvent the screenlocking, in most scenarios the security of Tails is already defeated. And the attacker might as well be able to circumvent the techniques that you are proposing.
most of the time the threat model is not an attacker with unlimited technical power who wrote a tool specificly designed to circumvent the lock screen of a niche os. often its someone who looks over the shoulder of someone while hes typing the password or someone who tries to guess the password or someone who knows the password because the user trusts him.

>Noticing it might not change much.
it might change a lot. he would know if he is in serious trouble or not. his life could depend on it.

> I’d rather work on making the user understand that leaving their computer unattended is very risky and should be with very much care.
education and protection do not exclude each other.

> Also, if I were an attacker with unlimited technical power, I would instead insert malicious devices in the computer ports (USB, firewire, etc.) with exploits to access memory. And that would be detected by none of your tricks.
plugging something in will trigger a notice on the lock screen. you could also disable plug and play during the lock.

#33 Updated by intrigeri 2015-11-10 09:58:00

Testing reports on Bug #9408#note-1 might be of some interest here.

#37 Updated by segfault 2016-03-31 18:10:07

> Now I wonder about the integration on the desktop. I think we should aim at triggering your script in the same ways as the screen locker would be triggered in a normal GNOME: at least through Super+L and from an icon in the system menu. Debian has a padlock icon in the system menu, right? Maybe we did something to remove it.
I found what we did to remove the padlock button, it’s the disable-lock-screen = true line in this file:

config/chroot_local-includes/etc/dconf/db/local.d/00_Tails_defaults

You can disable this setting in your Tails session via this command:

gsettings set org.gnome.desktop.lockdown disable-lock-screen false

Triggering the script through Super+L is trivial, since we only have to change the keyboard shortcut. But I did not yet find a way to change the behaviour of the padlock button in the status menu (that’s how the GNOME folks refer to this menu btw.)

Still looking forward to some short feedback if this is going the right direction. Then I would spend some more time on the remaining integration issues.

#43 Updated by segfault 2016-05-09 03:23:19

> So far I’ve discovered these blockers:
>
> 1. The GUI is not translatable.
I will look into this later.

> 2. In the GNOME Session menu, let’s place the new “Lock Screen” button between the Settings and Restart buttons , so the Restart/Shutdown buttons remain the rightmost ones and more easily accessible when in a hurry (i.e. for the emergency shutdown use case).
Right, I thought the same and already did that :) The lock button only appears right if the GNOME Shell plugin was just installed. Just reload the GNOME Shell (Alt F2 -> ‘r’) and the lock button will be left from the restart and shutdown buttons.

> 3. Instead of introducing yet another GNOME Shell plugin, let’s add the Lock Screen button to our existing Emergency Shutdown Helper plugin. Have a look at config/chroot_local-includes/usr/share/gnome-shell/extensions/shutdown-helper@tails.boum.org/extension.js. Some pattern matching and copy-pasting should do it. :)
I already looked at the shutdown-helper extension and copied code from it to the screen lock extension. I’m not sure why merging the plugins would be better than having them separated. The benefit of separate plugins is that we can more easily remove single functionality, if we ever need to do this.

> 4. Check passwd’s man page: “passwd will reject any password which is not suitably complex” => a too short password causes your password setter to silently fail. You need to handle this case, and I think the correct way is not to try to replicate the password complexity check in your tiny application, but to check stderr of passwd on error and see if the specific error string is there. For this reason you’ll want to execute passwd with an English (or “C”) locale too.
Yes, I noticed this too. But I decided to not handle this case but instead allow users to use insecure passwords for their screen lock. After all, the screen lock is not very secure itself, so why should we force users to think up secure passwords and possibly tell them multiple times that their password is not good enough before they can lock their screen?
This check can be disabled by removing the “obscure” option to pam_unix.so in /etc/pam.d/common-password. I propose we do this. Sorry I forgot to mention this before.

#44 Updated by segfault 2016-05-09 03:36:10

> I should also add that it looks nice! :) Btw, I really appreciate that you provided scripts to help setting it up in a live session in absence of a Tails Git branch that I can build.
:)

> So, segfault, do you think you’ll have time to fix these remaining issues in time for Tails 2.4 (especially since you have the Tails server GSoC now, and also (IIRC) said you’d have less time for Tails in general for the next couple weeks)? To be clear, for this to make it into Tails 2.4 we’d need to merge something with no serious issues (i.e. at least all of the above issues resolved, barring perhaps the plugin thing) before 2016-05-25.

If you think removing the secure password check is fine, I will prepare a commit. What do you think would be the right way to do this? A new script in chroot_local-hooks with sed -i 's/pam_unix.so obscure/pam_unix.so/' /etc/pam.d/common-password?

I’m not sure yet what has to be done to make it translatable. I will take a look at other Tails applications to figure this out. If this turns out to be the only remaining blocker, I will take the time to do this in time for 2.4.

#45 Updated by sajolida 2016-05-09 16:46:42

Also make sure that the doc is updated. I don’t mind doing this after the branch is merged for code.

#46 Updated by segfault 2016-05-10 14:38:56

I noticed that the screen lock button reappears right from the shutdown and restart buttons after clicking it. I fixed this with the newest commit in https://gitlab.com/segfault_/tails_screen_locker.

#55 Updated by intrigeri 2016-09-09 14:46:29

anonym wrote:
> If we get some user docs ready in time for 2.6, I’d (probably, see below) merge this. I would even consider merging this in time for 2.6~rc1 without docs if someone promises to deliver the docs before the final 2.6 release. I’m reassigning this ticket to you in hope that you manage something.

(FTR, this did not happen, hence the postponing.)

#56 Updated by anonym 2016-09-27 09:24:13

Your branch doesn’t build, segfault. I’ve fixed the issues on the feature/5684-screen-locker in Tails main Git repo, which I recommend you fetch and look at.

I’m building an image now to test your recent changes.

#58 Updated by intrigeri 2016-09-28 02:43:31

> I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?

+1

#59 Updated by intrigeri 2016-11-02 15:20:02

FWIW the topic branch currently fails to build on Jenkins. Do we need to merge an update from segfault’s branch?

#60 Updated by segfault 2016-11-04 19:49:44

> FWIW the topic branch currently fails to build on Jenkins. Do we need to merge an update from segfault’s branch?
I will look into this soon

#61 Updated by segfault 2016-11-07 02:21:53

The branch builds fine after merging devel.

I didn’t test the image yet though, so I can’t say anything about the two Lock Screen buttons anonym reported.

#62 Updated by segfault 2016-11-07 04:26:17

I can’t reproduce whatever caused anonym seeing two lock screen buttons.

#64 Updated by anonym 2016-11-28 18:35:10

anonym wrote:
> segfault wrote:
> > I can’t reproduce whatever caused anonym seeing two lock screen buttons.
>
> It’s still an issue, see the attached image which shows three lock screen buttons. At least all of them work. :)

During the same session I eventually ended up with four of them. I didn’t do anything I can connect to this happening. Looking at your code and comparing it to the shutdown applet, I see some parts “missing”, e.g. the stuff about this._menuOpenStateChangedId, but I fail to see why it should be related.

#65 Updated by anonym 2016-11-28 18:36:25

I’ve pushed two commit to our repo’s version of the branch:

d52d946 Enter a name into the Icedove account configuration.
19fb574 Make GNOME's default Lock Screen keyboard shortcut use our locker.

Please review’n’merge, segfault! :)

#66 Updated by anonym 2016-11-28 18:41:52

All code looks good, but please move tails_screen_locker from:

/usr/lib/python3.4/dist-packages/

to

/usr/lib/python3/dist-packages/

The former is not used, and let’s try to not (pretend to) depend on specific python versions. An other idea is to move it into Tails’ pythonlib Git submodule in case you see any advantage in it.

So what remain before I can merge this branch is what I just said above + the “multiple screen locker icons” issue + the user docs (Feature #10970).

#67 Updated by segfault 2016-11-30 23:32:23

> I’ve pushed two commit to our repo’s version of the branch:

I can’t find your commits. Did you forget to push? The last commits are from September: https://git-tails.immerda.ch/tails/log/?h=feature/5684-screen-locker

#70 Updated by segfault 2016-12-01 18:25:57

> I also pushed two more commits:
>
> 850afd7 Use the LIVE_USER variable instead of ‘amnesia’.
> f70dae8 Make live-build hook into live-config hook.
>
> These are in fact old commits (before this force-push) that I asked you to cherry-pick/merge into your branch, which you did not. Please try not to lose such feedback in the future — it’s part of the review process!

I did merge these commits into my branch, but in my branch they have the commit ids 0ceea16b and 5cae9c30:
https://gitlab.com/segfault_/tails/commit/0ceea16bbc945d4bc3223a130da82f51d96e2405
https://gitlab.com/segfault_/tails/commit/5cae9c30855a6178616b21f31ee17ffc4edf464e

I don’t know why they have different id’s than yours.

#73 Updated by intrigeri 2016-12-29 11:48:38

If Bug #12098 is fixed before the screen locker is completed, then we might want to re-enable the idle timeout whenever the time is not being synchronized.

#79 Updated by intrigeri 2017-04-15 07:09:34

> I think 3.2 is realistic. I have little free time until summer. I don’t think I will be able to spend time on this in time for 3.0. The little time I can spend on Tails currently I prefer to spend on getting a Tails Server beta release done.

Sounds very reasonable. Thanks for the update!

#83 Updated by intrigeri 2017-11-11 09:35:06

I suspect this should be postponed to the next major release.

#86 Updated by anonym 2017-11-16 10:49:05

segfault wrote:
> I rebased the branch on current stable, resolved the merge conflicts, and replaced the invocation of gnome-screenlocker-command with a dbus call (because gnome-screenlocker-command is not installed in current Tails).
>
> The screen locker now works again in my VM. I am still not able to reproduce the multiple buttons issue anonym experienced. Could you check if you can still reproduce this, anonym?

I’ve only given it a short try so far without reproducing it. To be continued!

Some things I’ve noticed:

• If I start gnome-shell-extension-prefs and disable the “Screen Locker”, the button is not removed. The button is removed as it should when the “Shutdown Helper” is disabled, so something is wrong in your rewrite. Could this be related to the “two buttons” bug?
• When you have locked the screen and are at the lock screen, clicking its GNOME menu still shows the screen lock button, which is a bit confusing and ugly (but not so serious, but still).
• Multiple instances of the password setting prompt can be open at the same time (no big deal).

> Re: #57:
> > Now I get two Lock Screen buttons in the GNOME “session” menu (or whatever it is called). I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?
>
> I am still not convinced that it would be better to merge the screen locker extension with the shutdown-helper extension. They have different purposes, so I think it is better design to keep them separate, so that they can be maintained and debugged separately, and removed separately if one of them becomes obsolete.

I agree that “different purposes” => “different program” is most often the ideal organization, but, as always, blindly following it is not a good idea. The reality here is more complex: there’s an inherent arbitrariness where you draw the line for “different purpose”. I mean, I could argue that the purpose of the unified plugin I propose is to adapt GNOME’s session menu into what Tails needs, i.e. all changes, which is one purpose. Or we could go the opposite way and also split “Shutdown Helper” into “Power off helper” and “Reboot helper”. So, in reality it is a good idea to define the purpose with things like code duplication (which splitting increases, unifying decreases), complexity from interaction (see below), etc, in mind.

> I don’t see how an additional extension adds more complexity than adding the same functionality to an existing extension.

We now have two different entities mutating the same state. That is a much more complex interaction than the same entity doing all the mutating with full control. I mean, it seems there already is (or was) a hard-to-reproduce bug about this, which, from experience, indicates a race or other parallelism (i.e. “program interaction”) issue.

#87 Updated by segfault 2017-11-16 23:39:52

anonym wrote:
> segfault wrote:
> > I rebased the branch on current stable, resolved the merge conflicts, and replaced the invocation of gnome-screenlocker-command with a dbus call (because gnome-screenlocker-command is not installed in current Tails).
> >
> > The screen locker now works again in my VM. I am still not able to reproduce the multiple buttons issue anonym experienced. Could you check if you can still reproduce this, anonym?
>
> I’ve only given it a short try so far without reproducing it. To be continued!
>
> Some things I’ve noticed:
>
> * If I start gnome-shell-extension-prefs and disable the “Screen Locker”, the button is not removed. The button is removed as it should when the “Shutdown Helper” is disabled, so something is wrong in your rewrite. Could this be related to the “two buttons” bug?

This is fixed in my branch now.

> * When you have locked the screen and are at the lock screen, clicking its GNOME menu still shows the screen lock button, which is a bit confusing and ugly (but not so serious, but still).

This is also fixed.

> * Multiple instances of the password setting prompt can be open at the same time (no big deal).

Not sure about this one. Do you think it is worth to create a pid/lock file for this?

> > Re: #57:
> > > Now I get two Lock Screen buttons in the GNOME “session” menu (or whatever it is called). I think this is a good reason to edit the existing Shutdown Helper GNOME extension instead of adding yet another GNOME extension. Less code to maintain, less complexity, less risk of conflicts between the two, which perhaps can explain this problem. Ok?
> >
> > I am still not convinced that it would be better to merge the screen locker extension with the shutdown-helper extension. They have different purposes, so I think it is better design to keep them separate, so that they can be maintained and debugged separately, and removed separately if one of them becomes obsolete.
>
> I agree that “different purposes” => “different program” is most often the ideal organization, but, as always, blindly following it is not a good idea. The reality here is more complex: there’s an inherent arbitrariness where you draw the line for “different purpose”. I mean, I could argue that the purpose of the unified plugin I propose is to adapt GNOME’s session menu into what Tails needs, i.e. all changes, which is one purpose. Or we could go the opposite way and also split “Shutdown Helper” into “Power off helper” and “Reboot helper”. So, in reality it is a good idea to define the purpose with things like code duplication (which splitting increases, unifying decreases), complexity from interaction (see below), etc, in mind.
>
> > I don’t see how an additional extension adds more complexity than adding the same functionality to an existing extension.
>
> We now have two different entities mutating the same state. That is a much more complex interaction than the same entity doing all the mutating with full control. I mean, it seems there already is (or was) a hard-to-reproduce bug about this, which, from experience, indicates a race or other parallelism (i.e. “program interaction”) issue.

Ok, I understand the argument about mutating the same state. So now I did unify the two extensions, and named it “Status Menu Helper”.

#89 Updated by segfault 2017-12-23 14:56:55

anonym wrote:
> I’ve tested a build successfully!
>
> I don’t like that we have moved the lock screen button to the end of the row, where users have been trained for years to find the emergency shutdown button. Please let the Restart and Shutdown buttons remain in the two last positions!

Makes sense, fixed in commit:2d03b1314e380bc4d874108a7499a07755d6b91e.

> If you fix the above problem, I’d like to merge this. I actually don’t think we even have to block on Feature #10970 (user docs) cause this feature is so self-explanatory, and I cannot come up with any negative UX issues that really requires this feature to be documented. Nevertheless, I’ve got the ball rolling on those tickets, so we should get the docs any way.
>
> I also have some nitpicky stuff:
>
> From commit:ad1572c863d20549d6f409b6c300b882111711a3:
> > + “description”: “Adds separate buttons for ‘Restart’, ‘Shutdown’, and ‘Lock Screen, and ’Lock Screen’ to the GNOME user menu, and all actions are immediate without further user verification.”,
>
> * There is a “'Lock Screen, and ” too many. :)
> * The “all actions are immediate” part is not true for ‘Lock screen’.
>
> I suggest: “Adds separate buttons for the ‘Restart’, ‘Shutdown’, and ‘Lock Screen’ actions to the GNOME user menu.”

Oops. Fixed in commit:8d2208617df829448ccfd2706428dd43b2bc514e.

> > + “name”: “Status-Menu-Helper”,
>
> These hyphens must have been added by mistake when you did the (probably automatic) renaming.
>
> GNOME refers to this menu as the “user menu” (soure so I think we should stick with “user menu” (which we actually already have in the meta data (description) of the extension). So it should be “User Menu Helper”. The “Helper” in “Shutdown Helper” made sense when the extension was about helping users to quickly shutdown, but in “User/Status Menu Helper” the “Helper” part feels out of place. I don’t think there is a good name based on functionality any more, so let’s just rename it to what it is: Tails User Menu
>
> What do you think?

I agree that the “Helper” part should be removed. But I think it was only called user menu until 3.10, when they integrated the status icons. Since then it is called “status area” in the docs [1][2], and (more often) “status menu” in commit messages and tickets. So I would argue that we should name it “Tails Status Menu” or maybe “Tails Status Area” instead.

[1] https://help.gnome.org/misc/release-notes/3.10/
[2] https://wiki.gnome.org/Design/OS/SystemStatus

> > > * Multiple instances of the password setting prompt can be open at the same time (no big deal).
> > Not sure about this one. Do you think it is worth to create a pid/lock file for this?
>
> Forget about it. I mostly said it in case it was convenient in Python (for instance, in Ruby (I think it was) there’s a package that you simply import to enforce one instance per user).

Ok. There does not seem to be something like that in Python - OTOH, using a pid file would also not be hard.

#94 Updated by anonym 2018-01-30 17:04:52

bertagaz: FWIW, I have reviewed the branch until commit:8a4f0f046b5432b8bbb3a149c9545a8c418ecaf2 (i.e. the state of feature/5684-screen-locker in Tails main Git repo).

bertagaz wrote:
> I’m having troubles understanding the status of your branch on gitlab. I can’t seem to merge it from/into stable without having to resolve weird conflicts. Would you mind preparing this branch so that it merges correctly? Also if it’s targeting 3.6, this branch should be based on devel. Could be good to rebase it on this one.

FYI, segfault has merged devel so this is now fixed. OTOH, since he didn’t reassign the branch back to you, he might not be done. *flies away*

#95 Updated by segfault 2018-01-30 17:23:56

anonym wrote:
> FYI, segfault has merged devel so this is now fixed. OTOH, since he didn’t reassign the branch back to you, he might not be done. *flies away*

Yes, I merged devel, but I was not finished. My merge somehow reintroduced features/step_definitions/icedove.rb, which I now removed from the commit and force pushed. I think it is ready for review now, but I didn’t test it myself after the merge. I’m currently building it in order to test it later.

@bertagaz: Thanks for working on this. I will reassign this to you after I successfully built and tested the merged branch.

#97 Updated by intrigeri 2018-02-02 05:41:01

> Sorry, my build setup broken and I can’t build any images currently

FWIW I can only build with the noram option currently.

#98 Updated by anonym 2018-02-19 14:36:43

bertagaz, this ticket has mostly been my responsibility, and it only landed on your plate because of my priorities. I feel like it should be on my plate still, which also probably will be more efficient use of our collective time given my previous familiarity with the branch. If you want to give it back to me, please do so ASAP (before 2018-02-22) so I can plan accordingly!

#100 Updated by segfault 2018-02-21 22:41:21

bertagaz wrote:
> Segfault, I force pushed your rebased branch in Tails main repo. I had to fix two issues: the build of this branch was failing due to refresh-translations complaining (should be fixed with commit:3ec58a90424ff2799f0c50066f90f5dcfb31b202)

That seems correct, thanks!

> and for some reasons the rebase you made contained commit:b67a1c3af963255f22e1d447b642551df9b57d18, which is not part of devel, so I’ve reverted it with commit:a8d8f1a8bf6b8221998540443b818b2db375cf6a

That commit was actually pushed by anonym to this branch, and should be there. See Feature #11256.

> Jenkins should start to build and test it soon.
>
> Meanwhile, would you mind having a look at the diff of this branch with devel on immerda’s Tails git repo and tell me if the patch looks like what you wanted? If so, please reassign to me.

The diff contains some unrelated changes, for example the aufs submodule is removed and the kernel version in config/amnesia is changed. I merged devel myself and cherry-picked your fix for the refresh-translations (3ec58a90424ff2799f0c50066f90f5dcfb31b202). Now the diff looks fine.

After building and testing the image, I noticed that I forgot to install a dependency (python3-pydbus), which I did now in commit 47c465117124aaa39369459b118ac08aeaa1499d.

I will build and test again tomorrow.

#101 Updated by bertagaz 2018-02-21 23:20:46

segfault wrote:
> bertagaz wrote:
> > and for some reasons the rebase you made contained commit:b67a1c3af963255f22e1d447b642551df9b57d18, which is not part of devel, so I’ve reverted it with commit:a8d8f1a8bf6b8221998540443b818b2db375cf6a
>
> That commit was actually pushed by anonym to this branch, and should be there. See Feature #11256.

Oops, right, missed that in the comments of this ticket.

> > Meanwhile, would you mind having a look at the diff of this branch with devel on immerda’s Tails git repo and tell me if the patch looks like what you wanted? If so, please reassign to me.
>
> The diff contains some unrelated changes, for example the aufs submodule is removed and the kernel version in config/amnesia is changed. I merged devel myself and cherry-picked your fix for the refresh-translations (3ec58a90424ff2799f0c50066f90f5dcfb31b202). Now the diff looks fine.

Ok, I’ll take your HEAD as the newest one. I was hacking around myself and pushed a commit to fix the dependencies. See below.

> After building and testing the image, I noticed that I forgot to install a dependency (python3-pydbus), which I did now in commit 47c465117124aaa39369459b118ac08aeaa1499d.

To get the screen locker to work, I remember also having to install the python3-pampy package.

That’s what I’ve pushed in commit:ec6b26261b017b1146f541ce82a3b51ed59d6e9c and I’m building now locally. As said above I’ll push your HEAD in the main repo as soon as Jenkins will have build my ISO with python3-pampy.

> I will build and test again tomorrow.

With both this packages installed, my tests of the screen locker are positive. Automatic and triggered locking works nicely. Indeed that means I’ve only tested the use case where the sudo password is set though, as I had to install this packages. :)

I’ll test tomorrow what happens without the sudo password with all deps.

#102 Updated by bertagaz 2018-02-22 13:09:09

bertagaz wrote:
> I’ll test tomorrow what happens without the sudo password with all deps.

So I did some more manual tests with an ISO containing both missing deps I’ve added (python3-pydbus, python3-pampy).

When the administration password is set in the greeter it still works nicely. :)

But when it’s not, the screenlocker password dialog is ineffective. Clicking either “Cancel” or “Ok” does not seem to do anything. I’m not sure how to debug that.

#103 Updated by segfault 2018-02-22 13:21:06

bertagaz wrote:
> To get the screen locker to work, I remember also having to install the python3-pampy package.

Right, pampy was also missing.

> That’s what I’ve pushed in commit:ec6b26261b017b1146f541ce82a3b51ed59d6e9c and I’m building now locally. As said above I’ll push your HEAD in the main repo as soon as Jenkins will have build my ISO with python3-pampy.

I can’t find ec6b26261b017b1146f541ce82a3b51ed59d6e9c on the immerda.ch Tails repo.

> But when it’s not, the screenlocker password dialog is ineffective. Clicking either “Cancel” or “Ok” does not seem to do anything. I’m not sure how to debug that.

Oh, ok, I will test it right now.

#104 Updated by bertagaz 2018-02-22 13:42:31

segfault wrote:
> bertagaz wrote:
> > To get the screen locker to work, I remember also having to install the python3-pampy package.
>
> Right, pampy was also missing.

Ack, pushed commit:76f56ca6809170cf30db90ae13a465a4d020b4fe then.

> > That’s what I’ve pushed in commit:ec6b26261b017b1146f541ce82a3b51ed59d6e9c and I’m building now locally. As said above I’ll push your HEAD in the main repo as soon as Jenkins will have build my ISO with python3-pampy.
>
> I can’t find ec6b26261b017b1146f541ce82a3b51ed59d6e9c on the immerda.ch Tails repo.

Expected, as said I’ve forced pushed your branch later on and removed this commit from there.

#105 Updated by segfault 2018-02-22 14:30:04

bertagaz wrote:
> But when it’s not, the screenlocker password dialog is ineffective. Clicking either “Cancel” or “Ok” does not seem to do anything. I’m not sure how to debug that.

Should be fixed by 77a038e9efe482ca049f5c38437896efd0d2445c. Rebuilding now to test once more. Sorry about the bugs, I didn’t have a working build setup when I implemented this.

#106 Updated by bertagaz 2018-02-22 14:59:45

segfault wrote:
> Should be fixed by 77a038e9efe482ca049f5c38437896efd0d2445c. Rebuilding now to test once more. Sorry about the bugs, I didn’t have a working build setup when I implemented this.

Now the dialog box works and the screen gets locked. yay! I’ve pushed your fix in the main repo branch.

However, when run manually I get that when tails-screen-locker exits:

amnesia@amnesia:~$ LANG=en_US DEBUG=1 /usr/local/bin/tails-screen-locker 
ERROR:root:g-io-error-quark: Timeout was reached (24)
Traceback (most recent call last):
  File "/usr/local/bin/tails-screen-locker", line 34, in lock_screen
    SessionBus().get("org.gnome.ScreenSaver").Lock(timeout=5)
  File "/usr/lib/python3/dist-packages/pydbus/proxy_method.py", line 75, in __call__
    0, timeout_to_glib(timeout), None).unpack()
GLib.GError: g-io-error-quark: Timeout was reached (24)

The screen locking works fine though.

Now that it basically works, I need to get through the whole comments one more time, and finish code reviewing.

#107 Updated by segfault 2018-02-22 16:44:51

> However, when run manually I get that when tails-screen-locker exits:
>

amnesia@amnesia:~$ LANG=en_US DEBUG=1 /usr/local/bin/tails-screen-locker 
ERROR:root:g-io-error-quark: Timeout was reached (24)
Traceback (most recent call last):
  File "/usr/local/bin/tails-screen-locker", line 34, in lock_screen
    SessionBus().get("org.gnome.ScreenSaver").Lock(timeout=5)
  File "/usr/lib/python3/dist-packages/pydbus/proxy_method.py", line 75, in __call__
    0, timeout_to_glib(timeout), None).unpack()
GLib.GError: g-io-error-quark: Timeout was reached (24)

Yeah, the reason why I added this timeout in the first place is that sometimes this dbus call doesn’t return, even though the locking was already done. So without the timeout, the process would just keep on running in the background. I think this way it is fine.

#110 Updated by segfault 2018-02-23 21:44:53

bertagaz wrote:
> bertagaz wrote:
> > segfault wrote:
> > > Yeah, the reason why I added this timeout in the first place is that sometimes this dbus call doesn’t return, even though the locking was already done. So without the timeout, the process would just keep on running in the background. I think this way it is fine.
> >
> > Ok, fair enough.
> >
> > Regarding the comments, seems everything’s fine and there’s no leftovers. Only some code review left!
>
> Done for code reviewing. I’m excited, we’re close releasing this finally!

Yay \o/

> I have one remak though: there are two calls to print() here:
>
> […]
>
> But the script has logging functionnality. Why not rather redirect that prints to the logs?

I don’t think that would make a difference, because I didn’t add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr. But I think it would actually be helpful to have errors logged to the journal, so in addition to your proposal, I added a journal handler in commit 72f96f042e684c55df7509670862092bb5146897. I also catch the timeout exception and log all uncaught exceptions.

#111 Updated by intrigeri 2018-02-24 12:17:23

> I don’t think that would make a difference, because I didn’t add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr.

I’m curious why stderr does not land into the Journal already.

#114 Updated by intrigeri 2018-02-24 15:48:21

> Because journald doesn’t log stderr of all processes, it does that only for services.

I think it depends on how processes are wired (FD passed when starting them). E.g. last time I checked, stdout+stderr of everything started within the GNOME session ended up in the Journal.

#115 Updated by segfault 2018-02-24 15:58:28

bertagaz wrote:
> segfault wrote:
> > intrigeri wrote:
> > > > I don’t think that would make a difference, because I didn’t add a handler for the logging, so the logs are only handled by the default handler, which simply prints them to stderr.
> > >
> > > I’m curious why stderr does not land into the Journal already.
> >
> > Because journald doesn’t log stderr of all processes, it does that only for services.
>
> I’ve tested without the last patch catching up uncaught exceptions, and tails-screen-locker exceptions (Feature #5684#note-107 or introduced fake ones) do ends up in the journal already.

That’s strange, because my tests using *76c2c3936f.iso have a different result. When I add fake exceptions to tails-screen-locker, they are only printed to stderr, but don’t appear in the journal. The only error in the journal related to the screen locking is gdm-password complaining that it can’t find pam_gnome_keyring.so.

#116 Updated by segfault 2018-02-24 16:04:34

intrigeri wrote:
> > Because journald doesn’t log stderr of all processes, it does that only for services.
>
> I think it depends on how processes are wired (FD passed when starting them). E.g. last time I checked, stdout+stderr of everything started within the GNOME session ended up in the Journal.

So if I open two gnome-terminals and in the one I do sudo journalctl -f and in the other one I do echo foo 1>&2, I should see the message printed by journalctl, right? This neither works on my Debian Sid nor Tails 3.5.

#117 Updated by segfault 2018-02-24 16:08:27

segfault wrote:
> intrigeri wrote:
> > > Because journald doesn’t log stderr of all processes, it does that only for services.
> >
> > I think it depends on how processes are wired (FD passed when starting them). E.g. last time I checked, stdout+stderr of everything started within the GNOME session ended up in the Journal.
>
> So if I open two gnome-terminals and in the one I do sudo journalctl -f and in the other one I do echo foo 1>&2, I should see the message printed by journalctl, right? This neither works on my Debian Sid nor Tails 3.5.

Ah, it does work if the script is started by gnome-session!

#118 Updated by segfault 2018-02-24 16:09:52

segfault wrote:
> That’s strange, because my tests using *76c2c3936f.iso have a different result. When I add fake exceptions to tails-screen-locker, they are only printed to stderr, but don’t appear in the journal. The only error in the journal related to the screen locking is gdm-password complaining that it can’t find pam_gnome_keyring.so.

So actually it does log to the journal if I execute the script via the lock button or the keyboard shortcut, and not start it in a terminal.

#125 Updated by segfault 2018-02-28 11:01:18

intrigeri wrote:
> > There is another issue: The .po files don’t contain the screen locker’s strings. I fixed this in 4a17e312ce00983d48bcb0d0ff0ca27f121dc44a and 71ca8b982afcf066cec6a70b1c1906c661fb7512. The refresh-translations doesn’t support Glade files currently, so I decided to remove the screen-locker.ui file and add the widgets in code in the tails-screen-locker script.
>
> Three questions/comments:
>
> * Adding Glade support to refresh-translations seems quite easy and IMO less risky than a code change at this stage of the release cycle.

I agree that it seems quite easy to add Glade support to refresh-translations. But I already wrote and tested the code change, I think now it will be less work to use it.

> Besides, in general I prefer Glade files over hard-coding the GUI in code, because UI designers can much more easily edit Glade files.

At least I learned that I can achieve what I want a lot faster using PyGObject and devhelp than Glade.

> * Have you verified that translations do get applied despite the fact the text domain is “Tails” (gettext.install("Tails")) and not “tails”? If not, let’s discuss this on XMPP today as planned yesterday :)

It does work, but we can still change it for consistency.

> * Do we need commit:72f96f042e684c55df7509670862092bb5146897 in the end? I thought we had decided against it but I didn’t re-read the history of this ticket.

Right, I pushed a0fd727ed22455092028ba8a041f9aaebe2eae0b to revert it.

#132 Updated by sajolida 2018-03-01 14:24:06

Actually, to be consistent with the wording we have to set up an administration password in Tails Greeter we should write:

Set up a password to unlock the screen
======================================

Password:   [          ]
Confirm:    [          ]

[Cancel] [Lock Screen]

#134 Updated by bertagaz 2018-03-01 15:37:43

bertagaz wrote:
> We’re supposed to be frozen, but given that for some unrelated reasons I’m a bit late, I’ll push your new wording.

Updated with abcce04c551650bac9d46d89040be01e12bca78d

#135 Updated by intrigeri 2018-03-02 07:35:27

> We’re supposed to be frozen

Arguably these could be seen as bugfixes that can be applied post-RC :)

#136 Updated by bertagaz 2018-03-02 14:36:26

intrigeri wrote:
> > We’re supposed to be frozen
>
> Arguably these could be seen as bugfixes that can be applied post-RC :)

Sure, but there was still time to push that as I didn’t really start the release process at that moment. So that’s ok :)

#137 Updated by pablonatalino 2018-03-05 23:03:09

bertagaz wrote:
> intrigeri wrote:
> > > We’re supposed to be frozen

This happened sometimes with me, the button light on / off works and a small sound from the cooler seems like the computer is working, but the keyboard, screen, usb and hdmi ports do not work anymore. Anyway tails-screen-locker did me very much ;) :) ;)

I wrote for whisperback but in Portuguese, since the whisperback instrusions were in Portuguese.

tails-screen-locker = ;)

#139 Updated by sajolida 2018-03-15 17:06:02

bertagaz: Why can’t we close this ticket?

#141 Updated by pablonatalino 2018-03-22 05:35:54

The issue of closing the laptop lid also happened to me on windows, so in my case it should be a specific problem of my device