Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Improved lock screen support #2706

Open
thatguystone opened this issue Feb 2, 2021 · 39 comments
Open

Improved lock screen support #2706

thatguystone opened this issue Feb 2, 2021 · 39 comments

Comments

@thatguystone
Copy link

thatguystone commented Feb 2, 2021

I'm looking for some feedback and ideas about the current state of lock screens under wlroots.

Right now, it looks like lock screens are implemented the same way they were under X11, specifically: fullscreen window on top of everything else and input inhibit. While this works, it has a number of problems, and I'm hoping that it can be improved on.

Problems

  1. If anything causes the lock screen process to exit, the screen becomes unlocked. This issue continues to pop up (eg. xscreensaver, swaylock), and likely can't be solved (thanks, oomk) without compositor support.

  2. Programs can unintentionally end up on the lock screen. Programs that want to end up on the lock screen have no way to ensure they do.

  3. Race conditions: when resuming from sleep or messing with dpms, the desktop sometimes shows briefly before the lock screen does.

Protocol design goals

  1. Provide a mechanism to request the compositor "lock" / "unlock". The lock state should persist even if the requesting client dies.

  2. Provide clients with lock screen state: enum {UNLOCKED,LOCKING,LOCKED,UNLOCKING}

    • UNLOCKED: the unlocked desktop is visible

    • LOCKING: the compositor has received a LOCK request, but the unlocked desktop is still visible

    • LOCKED: the unlocked desktop is not visible

    • UNLOCKING: the locked desktop is still visible

  3. Allow clients to configure which surfaces are visible when locked and unlocked. By default, surfaces are only visible when unlocked.

Client usage

  • A wallpaper process could display different images when locked / unlocked (similar to how Android and iOS do it).

  • A volume/brightness OSD could ensure it appears when locked and unlocked.

  • A notification daemon might want to hide notification content and only display icons with numbers next to them when locked.

  • A panel could hide all workspace buttons/menus/trays but continue to show current time.

  • A lock screen process (eg. swaylock --daemonize) could wait for a LOCKED event, ensuring that the desktop is not visible once the parent exits.

Implementation

I haven't worked much with wayland, but here's what I'm thinking, and I'd appreciate any feedback.

This should probably be a new protocol. It looks like it would fit nicely into layer_shell, but with kwin implementing layer_shell, I doubt it would be a good idea to force a lockscreen on them since they already handle locking their own way.

I'm not entirely sure of the best way to communicate to clients which surfaces the compositor is capable of rendering on a lock screen.


wlroots has migrated to gitlab.freedesktop.org. This issue has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/2706

@ifreund
Copy link
Member

ifreund commented Feb 2, 2021

Agree about problems 1 and 2. Problem 3 affects all layer shell clients that wish to render on all outputs including new ones and will be fixed by swaywm/wlr-protocols#86 however when I (or someone else) gets around to implementing it.

  1. Provide a mechanism to request the compositor "lock" / "unlock". The lock state should persist even if the requesting client dies.

Why should this be exposed by the protocol? All compositors have some version of compositor keybinds/compositor commands.

  1. Provide clients with lock screen state: enum {UNLOCKED,LOCKING,LOCKED,UNLOCKING}

This seems unnecessary and likely racy to use unless implemented very carefully. I'm not convinced clients need to know if the screen is locked.

  1. Allow clients to configure which surfaces are visible when locked and unlocked. By default, surfaces are only visible when unlocked.

Again, I'm not sure if this feature is worth it. Maybe that's just because I personally don't want anything shown above my lockscreen.

My long-term plan for river is to implement the authentication part of locking entirely in the compositor, hiding everything while locked. This solves all the problems listed above but does not allow for customizable lockscreen graphics. My plan for that is to design and implement a screensaver protocol which allows a single client to provide graphics for the lockscreen as well as display a gui for the unlocking process if it so chooses.

@valpackett
Copy link
Contributor

valpackett commented Feb 2, 2021

I think if we want to make out-of-process lockscreens good, the most important thing to have is a "hardcore mode" flag for wlr-input-inhibitor which would tell the compositor to NOT unlock when the locking client dies, only unlocking if there's an explicit destructor call on the inhibitor object.

To ensure a good experience (i.e. to avoid getting completely locked out of the system), it shouldn't just be a simple flag, there should be an option to respawn the lockscreen in case of its sudden death, i.e.

  • lockscreen says get_inhibitor { respawn = "/usr/local/bin/my-cool-lockscreen --flags --whatever" }
  • lockscreen dies (OOM, segfault, whatever)
  • compositor runs the passed respawn command to start the lockscreen again, without unlocking in between the old and new lockscreen process
    • maybe simply by associating the first get_inhibitor from the child with the leftover inhibitor from the dead lockscreen process
      • of course the association would work by spawning the new lockscreen with a WAYLAND_SOCKET fd
    • or do a whole token passing thing?

The second thing is, yes, requesting exclusive display in addition to exclusive input. i.e. "render-inhibit"

I'm not sure if this feature is worth it. Maybe that's just because I personally don't want anything shown above my lockscreen.

I generally don't want "anything" too… except an on-screen keyboard that could be used for entering a password.

@ifreund
Copy link
Member

ifreund commented Feb 2, 2021

I think if we want to make out-of-process lockscreens good, the most important thing to have is a "hardcore mode" flag for wlr-input-inhibitor which would tell the compositor to NOT unlock when the locking client dies, only unlocking if there's an explicit destructor call on the inhibitor object.

I think it is clear that preforming all input handling and authentication inside the compositor would be simpler (and therefore more secure) than any solution that relies on a client to determine when the screen is locked/unlocked. This would also allow entirely dropping wlr-input-inhibitor which I see as a great thing.

It should be noted that hikari already implements this architecture, albeit without a way to arbitrarily customize the lockscreen UI and screensaver. Furthermore hikari allows marking any view as "public" which causes it to be drawn above the locksceen while locked. As you may have guessed, this means that hikari does not support wlr-input-inhibitor.

The second thing is, yes, requesting exclusive display in addition to exclusive input. i.e. "render-inhibit"

I don't see a strong reason for clients to be able to arbitrarily request either. Instead, the compositor may simply expose a keybinding or other compositor-specific command to trigger input/render inhibit and keep both active until the user has entered a valid password. Is there any real gain to be had by giving this power to clients that would outweigh the complexity?

I generally don't want "anything" too… except an on-screen keyboard that could be used for entering a password.

That's a very good point which IMO makes it clear that we do in fact need some mechanism to display select clients above the lockscreen (well, assuming you don't want to implement the on-screen-keyboard in the compositor). The hands-off approach to this in protocol design would be to simply state that the compositor may choose to display any client above the lockscreen at its discretion. Alternatively, we could have a new layer-shell layer or similar construct to allow clients to place themselves above the lockscreen. I'm not a huge fan of leaking into the layer-shell protocol though. If we want to allow clients to request that they be displayed above the lockscreen while locked, a request (which the compositor is free to deny) for a specific surface to be displayed above as proposed in the original post might be the most reasonable direction to take.

Relevant discussion in #river on freenode: https://freenode.irclog.whitequark.org/river/2021-02-02#1612266901-1612270334

@agx
Copy link
Contributor

agx commented Feb 2, 2021

Again, I'm not sure if this feature is worth it. Maybe that's just because I personally don't want anything shown above my lockscreen.

Lockscreens can be inherently complex requiring all kinds of input (touch, keyboard) and binding to lots of services (see e.g. phones with weather information and music players) - those shouldn't be in the process space of the compositor.

I've looked at something similar recently and think one way would be to have the lockscreen client use a separate socket and the compositor disabled output and input when no client is bound to that socket.

@valpackett
Copy link
Contributor

In Wayfire, we currently have a silly namespace based decision process for placing things above the lockscreen.

preforming all input handling and authentication inside the compositor would be simpler

But still having the visual interaction in the process? That split seems way more complicated than keeping everything together in either the compositor or the client. How do you even synchronize all that without having a very complex custom protocol? i.e. if the compositor gets the password, how would the client know to render the dots/asterisks in the text field? What if there are other methods of authentication a DE wants to implement? (While using a generic "build-any-DE-on-top-of-this" compositor like Wayfire)


Another benefit of having everything in the client is that the memory space that was used for authentication gets destroyed, but that's advanced paranoia territory.

@ifreund
Copy link
Member

ifreund commented Feb 2, 2021

In Wayfire, we currently have a silly namespace based decision process for placing things above the lockscreen.

That would fall into the "hands-off" approach I mentioned above, leaving it up the the compositor to decide based on arbitrary criteria. I think this would not be a totally unreasonable path to take.

But still having the visual interaction in the process? That split seems way more complicated than keeping everything together in either the compositor or the client. How do you even synchronize all that without having a very complex custom protocol? i.e. if the compositor gets the password, how would the client know to render the dots/asterisks in the text field? What if there are other methods of authentication a DE wants to implement? (While using a generic "build-any-DE-on-top-of-this" compositor like Wayfire)

I think such a protocol would only require 6 events informing the client of password entry state.

  1. character entered (what character specifically is not sent)
  2. character deleted
  3. password cleared
  4. password submitted
  5. password rejected
  6. password accepted (this means the screensaver client should close)

As there is no back and forth communication with the client here and the client is free to display this state however it wants, I don't see this as a great source of complexity.

Other authentication mechanisms (a fingerprint reader or similar I suppose) could use the same password submitted/reject/accepted events. You are right that this would require implementation of alternative authentication in the compositor though.

Another benefit of having everything in the client is that the memory space that was used for authentication gets destroyed, but that's advanced paranoia territory.

Nothing stops the compositor from doing the authentication in a separate process if it really cares that much.

@thatguystone
Copy link
Author

thatguystone commented Feb 2, 2021

  1. Provide a mechanism to request the compositor "lock" / "unlock". The lock state should persist even if the requesting client dies.

Why should this be exposed by the protocol? All compositors have some version of compositor keybinds/compositor commands.

That was vague of me, sorry. I don't mean that the compositor is responsible for executing a lock screen. I mean that the compositor, once it receives a lock request, would enter a sort of "lockdown" mode where it doesn't render or send any input events to surfaces that are not marked as safe-for-lockscreen.

This is similar to how kwin does it.

Problem 3 affects all layer shell clients that wish to render on all outputs including new ones and will be fixed by swaywm/wlr-protocols#86 however when I (or someone else) gets around to implementing it.

For lockscreens, I think this protocol could actually partially address that. While there could still be a few blank frames shown before clients get surfaces submitted, there would be no risk of any private windows being shown.

  1. Provide clients with lock screen state: enum {UNLOCKED,LOCKING,LOCKED,UNLOCKING}

This seems unnecessary and likely racy to use unless implemented very carefully. I'm not convinced clients need to know if the screen is locked.

I think that this covers a couple possible cases.

  1. Clients might want to change what they render depending on lock state. For example, a notification daemon, once it receives LOCKING/LOCKED, could render its "locked" state, flag the surface as safe, and commit.

  2. A locker wants to postpone system sleep until it can confirm the screen is locked. This problem actually exists with swaylock today; at swaylock/main.c:1222, it exits before it has confirmed that the windows are displayed, so it's possible for the unlocked screen to flash on resume.

  3. An on-screen keyboard might want to disable certain functionality when displayed on a lockscreen, eg. copy/paste.

UNLOCKED and UNLOCKING might be overkill, but I included them for completeness.

  1. Allow clients to configure which surfaces are visible when locked and unlocked. By default, surfaces are only visible when unlocked.

Again, I'm not sure if this feature is worth it. Maybe that's just because I personally don't want anything shown above my lockscreen.

That's what I think is nice about this point: it allows the user to decide what is actually displayed. Each process could have its own rules for if it should display when locked. This is similar to how hikari allows marking any view as "public".

Personally, I want my volume and brightness OSDs to display above the lockscreen. It would also be nice if my notification daemon displayed only the icons of pending notifications on my lockscreen so that I could quickly check if anything is waiting for me as I walk by my desk without unlocking my screen.

My long-term plan for river is to implement the authentication part of locking entirely in the compositor, hiding everything while locked. This solves all the problems listed above but does not allow for customizable lockscreen graphics. My plan for that is to design and implement a screensaver protocol which allows a single client to provide graphics for the lockscreen as well as display a gui for the unlocking process if it so chooses.

This protocol is aiming in that direction, except it allows for more customization.

@thatguystone
Copy link
Author

In Wayfire, we currently have a silly namespace based decision process for placing things above the lockscreen.

I think that with my proposed "lockdown" mode, this could also be addressed. Rather than using namespaces, the compositor could choose to only allow layer_shell surfaces to render when lockdown'd. For example, a locker's surface could then be at LAYER_BOTTOM, an on-screen keyboard could be at LAYER_OVERLAY.

@emersion
Copy link
Member

emersion commented Feb 8, 2021

I think a lockscreen-specific protocol would definitely be desirable. I think it'd make more sense to handle authentication in the client:

  • This feels like embedding a client within the compositor.
  • Systems without PAM require root privileges to check credentials, this is better done in an isolated, small client process (like swaylock does).
  • I'd rather not depend on PAM in my compositor.

@thatguystone
Copy link
Author

I think it'd make more sense to handle authentication in the client:

  • This feels like embedding a client within the compositor.
  • Systems without PAM require root privileges to check credentials, this is better done in an isolated, small client process (like swaylock does).
  • I'd rather not depend on PAM in my compositor.

I agree. I think "lockscreen protocol" might be the wrong name for this since lockscreens already exist and have specific functionality. Perhaps "lockdown protocol" would be better, mainly to emphasize that the compositor is only responsible for limiting which surfaces are shown and informing clients of lockdown, not for lockscreen/auth/etc.

@WhyNotHugo
Copy link

The compositor could enter lock-screen mode: in this mode it runs a configurable client, and only renders surfaces for that client. This client would, typically, be an unlocker configured by the user (and can use PAM or whatever each implementation wants).

I'm not a fan of exposing screen lock state to all regular clients too though.

@Leon-Plickat
Copy link

Coming across this issue again, one question that crossed my mind is where will the locking be initiated? Will the lockscreen client initiate the lockdown with a request? Or will that be serverside, meaning the server initiates locking and then sends the lockscreen client some sort of "we are locked now, please spawn some surfaces" event? Should that client be spawned by the server as part of the locking process?

If the server decides to restart a crashed lockscreen client, the second approach would mean that restart is no different from initial start from the clients perspective, which I think simplifies it a bit.

@WhyNotHugo
Copy link

It's likely simpler and safer to have the server to initiate the locking and start the client. This can guarantee that the server knows how to restart the screenlock client if it ever dies unexpectedly.

An approach that makes sense to me is:

  • compositor has a pre-configured command indicating how to start the screenlock client.
  • compositor is sent a request indicating it to lock down.
  • compositor starts the screenlock.
    • client uses a priviledged API indicating that it is a screenlocker client and its surface DOES render during lockscreen.
    • all other surfaces don't render (e.g.: treat them as you would when they're in a different workspace)
    • if the client does unexpectedly, the compositor will restart it.
  • when the locker deems that it's okay to unlock (e.g.: when the user has provided a password), it will send a request to the compositor saying so.

I do think the only unclear thing is: what happens if the client is broken and dies immediately after starting. Does the locking get aborted? This seems sane it it's as soon as the user locked the screen.

I do think a downside of this approach is that the compositor has to track the lifecycle of the screenlocker, and I'm not sure what the sentiment is on having this kind of responsibility.

@danieldg
Copy link
Contributor

Thinking about this from a combined security and minimal-functionality-in-the-compositor perspective, I can see two important parts of a screenlocker:

  1. Actually locking the screen - hiding normal windows, intercepting input
  2. Showing something useful (unlock prompt, dots for the password, clock, background, etc) and handling unlock

Functionality (1) needs to work even when everything else is crashing (or getting killed by OOM killer, or not forking due to PID limits, etc) - so I think it belongs in the compositor. The goal of the lockscreen protocol and configuration in the compositor is so that this fallback is never seen by the user in normal cases - only in all the edge cases that today cause lockscreens to "fail insecure".

Functionality (2) can be handled by any number of programs, but in the absence of a need for premature complexity, a single program is probably best - it could farm out sub-windows to other programs if needed, after all. It can also fork other programs if needed to do password checking. If this program crashes, it can be restarted without any problems (other than perhaps annoying the user if it happens too often) - and if it fails to start, then it's up to the compositor to decide if it should retry forever (likely with some delay between tries to avoid 100% CPU use) or wait for the user to log in on another terminal to fix the problem and trigger a reload (via something like swaymsg or kill -USR1).

This split would suggest a near-trivial lock protocol: one request (unlock) and maybe one event (lock) that could even be optional if this protocol is only present on the locker. The normal protocols for input and output would still apply (though perhaps layer-shell would be required for visible top-levels).

Regarding aborting locking if the program fails - it may be sane to abort a lock if the lock request was triggered interactively, but that's not a good idea if the lock was due to a timer or an event like a laptop lid close (pre-sleep).

@mstoeckl
Copy link
Contributor

mstoeckl commented Jun 8, 2021

This split would suggest a near-trivial lock protocol:

I made a somewhat buggy protoype for how a lock screen/compositor could do this; see the safe-screenlock branches for sway and swaylock, which currently point to commits mstoeckl/sway@8bb8e7a and mstoeckl/swaylock-plugin@a770e39 . To use, build both sway and swaylock from the mstoeckl/safe-screenlock branch, and add the following to your config file.

bindsym Ctrl+Shift+U lock_screen --fail-unlocked swaylock --color='#ff00ff'
bindsym Ctrl+Shift+L lock_screen --fail-locked swaylock --color='#00ff00'

It adds a lock_screen command to sway, which runs the screen locker program via WAYLAND_SOCKET and provides it an ext_unlocker_v1 protocol object. Until this specific program closes its connection or sends the ext_unlocker_v1.unlock() request, it will be the only client rendered.

If --fail-unlocked is set, the program being killed, aborting, or even closing without connecting unlocks the screen; if --fail-locked is set, any of these events will make sway permanently lock itself.

@danieldg
Copy link
Contributor

danieldg commented Jun 9, 2021

As far as a demo goes, this looks pretty much exactly what I pictured. Two things I noticed:

  • You could easily allow recovery from the permanent lock scenario by adding a --locked keybinding to run lock_screen again - just have the code set permanent_lock = false instead of erroring if it is set in cmd_lock_screen.
  • Input interception isn't implemented yet, that would also be required to be secure

@mstoeckl
Copy link
Contributor

mstoeckl commented Jun 9, 2021

You could easily allow recovery from the permanent lock scenario by adding a --locked keybinding to run lock_screen again - just have the code set permanent_lock = false instead of erroring if it is set in cmd_lock_screen.

Another option would be to replace --fail-locked with a --fail-retry flag, that has the compositor rerun the lock command a short wait after the previous screen locker client crashes.

Input interception isn't implemented yet, that would also be required to be secure

I have yet to figure out how to do this properly, and don't know if I'll have time to, so feel free to continue where I've left off.

Other things that would need to be done to make the prototype mergeable:

  • Submitting ext_unlocker_v1 to wayland-protocols
  • Fixing damage tracking -- for example, when the lock screen exits, the entire screen needs to be redrawn. This problem is easier to see with an unlocker client that doesn't draw any surfaces; see attached timed-unlock.c.

@thatguystone
Copy link
Author

thatguystone commented Jun 10, 2021

I'm not sure that this is heading in the right direction. Introducing a special, compositor-defined and controlled way to launch a client in order for the client to function correctly feels wrong. Right now, if I launch swaylock, the screen locks; I don't have to configure the compositor or dig through docs. In the long run, this path feels like it will lead to incompatibility and confusion.

While I agree that it's important for a lockscreen client to be restarted if it crashes, that feels like a separate issue. It would be interesting to experiment with compositor-supported process management, eg. as an alternative to sway's exec, and support launching the lockscreen client to launch through that.

I also think it's important that other clients (eg. volume OSD, on-screen keyboard, etc) be able to show while locked. Restricting the lockscreen to a single client feels overly intrusive; in my opinion, what displays should be up to user configuration, not compositor restrictions. This way, the compositor is client agnostic; there's probably a lockscreen running, but that's configured elsewhere; the compositor only needs to render things it knows to be safe.

@danieldg
Copy link
Contributor

While I agree that it's important for a lockscreen client to be restarted if it crashes, that feels like a separate issue.

I agree that it's not really sway's job to restart the locker on crash. I also don't think sway should imitate systemd and start handling process management - there are existing tools that work well for that. You can already add "systemd-run --scope" to any "exec" lines to get some process management, if you want it.

I'm not sure that this is heading in the right direction.

This one-request protocol does fix the immediate issue, but after re-reading the other points in the original request I agree that a more complex protocol has advantages and should really be the long-term goal. However, I'd also like it if we didn't have to debate how exactly to handle the most complex future cases before fixing the problem.

One solution is to start with a minimal protocol as v1, and then make a v2 that supports features like notifying other clients of lock/unlock actions and having other windows visible on the lockscreen. For v1, just adding a lock request would allow the main protocol to be exposed to all clients - allowing both easier launching outside the compositor and adding notification events in the future.

Or maybe it's just as easy to write out a proposal of the full protocol and get comments on it without trimming anything. I'd personally be happy seeing either.

@mstoeckl
Copy link
Contributor

mstoeckl commented Jun 10, 2021

I'm not sure that this is heading in the right direction. Introducing a special, compositor-defined and controlled way to launch a client in order for the client to function correctly feels wrong. Right now, if I launch swaylock, the screen locks; I don't have to configure the compositor or dig through docs. In the long run, this path feels like it will lead to incompatibility and confusion.

Giving any client the ability to lock the screen is itself a minor security problem, since then an untrusted client could lock the screen and never release it. There've been some attempts on controlling which clients can use which protocols over the last three years -- most recently, see swaywm/sway#5675 and swaywm/wlr-protocols#27 . The approach I prefer is to have the compositor launch programs needing extra privileges, because while it is mildly annoying to prefix commands with e.g. swaymsg exec --something., this method seems to be secure. Another direction is to try to identify the connecting program, and give permissions based on its identity, but as discussed in https://gitlab.freedesktop.org/wayland/weston/-/issues/206 , determining identity is tricky.

[thatguystone] I also think it's important that other clients (eg. volume OSD, on-screen keyboard, etc) be able to show while locked

[danieldg] One solution is to start with a minimal protocol as v1, and then make a v2 that supports features like notifying other clients of lock/unlock actions and having other windows visible on the lockscreen

Having other windows visible above the lockscreen is a matter of compositor policy, and probably doesn't need an extra protocol. The only reason I didn't change the prototype to allow this is because I don't know how displaying things above the lock screen works in current Sway.

Also, notifying clients of lock/unlock actions probably might work best as an extension of whatever mechanism clients use to determine if the user is idle; there is no need to combine it with the ability to unlock the screen.

@danieldg
Copy link
Contributor

I believe idle detection is done through the system dbus (logind) which also has a "LockedHint" property, so that may suffice for notifying clients that are willing to subscribe to that mechanism. Having a privileged lock protocol aligns well with the other privileged protocols (note that current screenlockers show you can make just as much of a mess using layer-shell and input-inhibit as with a proposed locking protocol - with perhaps the exception that after killing the client, you need to run an actual locker to unlock the screen).

While the exact mechanism may be compositor-specific, adding an additional request to layer-shell that gives approval to display on the lockscreen would retain compatibility with kwin and guide compositors that it's OK to display the window on a locked screen. Clients that want to hide/show content could monitor logind and change the setting back to false on unlock (prior to showing full details).

@thatguystone
Copy link
Author

Giving any client the ability to lock the screen is itself a minor security problem, since then an untrusted client could lock the screen and never release it.

I assumed locking/unlocking permissions would be controlled by the compositor. I see files in /etc/sway/security.d/* that seem to reference such permissions, but looking through sway source, I don't see the files actually used. Hmm.

Having other windows visible above the lockscreen is a matter of compositor policy, and probably doesn't need an extra protocol.

I think that it's up to the clients to determine if they're safe to display while locked. There's no real way for a compositor to figure out, for example, if a notification daemon is safe to display. Such a daemon might want to redact all messages before it even considers itself safe, too.

Also, notifying clients of lock/unlock actions probably might work best as an extension of whatever mechanism clients use to determine if the user is idle; there is no need to combine it with the ability to unlock the screen.

The compositor only plays a small part in determining idle state, and it's not typically involved in any idle decisions/actions like locking.

@thatguystone
Copy link
Author

I believe idle detection is done through the system dbus (logind) which also has a "LockedHint" property

That property can be modified by any user process at-will. It's really a hint, and probably isn't reliable.

Also, that assumes that users are using logind. There are quite a few who don't.

Having a privileged lock protocol aligns well with the other privileged protocols

Do you have any examples? I thought there were some, but I can't seem to find them.

While the exact mechanism may be compositor-specific, adding an additional request to layer-shell

I raised this question in my original post. Should every shell protocol really add lock information to it? That feels like the lock protocol is leaking into others. But on the other hand, if it's part of the lock protocol, then you end up in a situation where a client might send try to flag an unsupported surface.

@danieldg
Copy link
Contributor

Also, that assumes that users are using logind. There are quite a few who don't.

This is true, but at some point the state of the system isn't the compositor's job to manage. Locking the screen could be argued to be on either side of that line, but I don't think it's obviously something the wayland protocol should be in charge of.

Having a privileged lock protocol aligns well with the other privileged protocols
Do you have any examples? I thought there were some, but I can't seem to find them.

There's a list of protocols that I found contain security-relevant requests at swaywm/sway@97d3d64#diff-f6cbe9a2900bd874a6dc4319d4c9ef0a993df77da8e65db944c8524d12e0879b - that list was actually taken from some older issues. If you look at the protocols themselves, it's generally pretty clear how they can be used to mess up the user interface or grab data from other clients.

So far, there aren't many protocols that provide both useful non-security-relevant requests and useful security-relevant ones, though layer-shell kinda qualifies there. If that becomes common, then the security model based on filtering globals will need to become more complicated (and it's likely "Permission Denied" errors/results are going to need to be added).

While the exact mechanism may be compositor-specific, adding an additional request to layer-shell
I raised this question in my original post. Should every shell protocol really add lock information to it? That feels like the lock protocol is leaking into others. But on the other hand, if it's part of the lock protocol, then you end up in a situation where a client might send try to flag an unsupported surface.

Layer shell is intended for "non-regular" windows like taskbars, which are the kind of windows that would make sense to appear on a lockscreen. I don't think it makes sense to add a set_sensitivity() request to regular shell protocols like xdg-shell.

@kennylevinsen
Copy link
Member

kennylevinsen commented Jun 10, 2021

I believe idle detection is done through the system dbus (logind) which also has a "LockedHint" property, so that may suffice for notifying clients that are willing to subscribe to that mechanism.

No, a Wayland idle protocol is used, which causes swayidle to start running its configured commands.

swayidle can then optionally update logind's idle info when we idle/resume, but it is not part of how idle actions normally execute.

We should not assume systemd, nor should we rely on dbus for something like this.

@WhyNotHugo
Copy link

While I agree that restating the locker is a bit out-of-scope, the problem is that the locker needs to somehow be restarted if it crashes.

Otherwise, if it crashes, the user is left with a system they cannot unlock (and would also lose any data of running programs). It might be fixable by just indicating that it's up to the user to configure something that restarts it (e.g.: systemd restart-on-failure, or whatever they prefer).

@mstoeckl
Copy link
Contributor

mstoeckl commented Jun 12, 2021

FYI, I've updated my lock screen prototype* again, fixing the damage and input interception problems, adding global filters, and drawing a message when the lock screen crashes. As advised in #2706 (comment) , running lock_screen now replaces the earlier lock screen, if one exists, so that you can reload a crashed lockscreen with a --locked-type keybinding,

*On branches mstoeckl/sway/safe-screenlock and mstoeckl/swaylock/safe-screenlock

Please try it out and let me know what you think. Even if this approach doesn't get merged, having a nice prototype makes it easier for people to design alternative lock screen improvements.

@danieldg
Copy link
Contributor

danieldg commented Jun 13, 2021

Based on the code @mstoeckl wrote, I made a version that uses a protocol that can be made visible to all clients (or only to privileged clients, if you support such a thing). The one-client limitations are still present in the implementation, but it is no longer encoded into the protocol. See danieldg/swaylock@d07f07f and danieldg/sway@674bec3.

I think https://github.com/danieldg/sway/blob/safe-locking/protocols/wp-screenlocker-unstable-v1.xml should give most of the capabilities that were referenced in the original issue or brought up in discussion; comments would be appreciated, especially regarding if frame callbacks are sufficient for everything I claim they are. The protocol assumes a (not-yet-written) extension to layer_shell that allows setting a layer_surface to be visible on locked, unlocked, or both desktops.

@thatguystone
Copy link
Author

thatguystone commented Jun 15, 2021

regarding if frame callbacks are sufficient for everything I claim they are

From my understanding, frame callbacks can't be used to determine visibility. The lack of any way to determine visibility reliably was the driving force behind Protocol design goals #2.

The protocol assumes a (not-yet-written) extension to layer_shell that allows setting a layer_surface to be visible on locked, unlocked, or both desktops.

This still bothers me.

  1. This could cause long-term compatibility problems. For example, kwin currently implements layer_shell and has its own in-compositor lockscreen. Should layer_shell really be making assumptions about which lockscreen protocol is in use and its capabilities?

  2. Should input_method_unstable_v1 (get_input_panel_surface) also be extended to be lockscreen aware? And all future protocols that have unique surface roles?

  3. For clients that have separate locked and unlocked states, it would be good for them to know current lock state so that they don't have to render 2 separate surfaces all the time. While this could be added to layer_shell, it feels like a lot of leakage and assumptions.

This really looks like something that should be contained in the protocol itself.

@thatguystone
Copy link
Author

https://github.com/danieldg/sway/blob/safe-locking/protocols/wp-screenlocker-unstable-v1.xml

<event name="lock_abandoned">: I'm curious who this is intended for. If there's a process that watches for this event, wouldn't that same process know if the lockscreen crashed since it'll probably be its parent anyway?

<request name="lock">: I like the idea of limiting to a single locker client.

@danieldg
Copy link
Contributor

regarding if frame callbacks are sufficient for everything I claim they are

From my understanding, frame callbacks can't be used to determine visibility. The lack of any way to determine visibility reliably was the driving force behind Protocol design goals #2.

If you want to support compositors that do locking animations, you'll need to render both surfaces for the duration of that animation. I envisioned frame callbacks being used like this: once the lock animation completes, the surface on the unlocked screen won't get frame updates any more (because it's not visible) and so you won't be using resources to render it. Similarly, after unlocking, the locked surface won't get frame updates and you aren't rendering it either.

1. This could cause long-term compatibility problems. For example, kwin currently implements layer_shell and has its own in-compositor lockscreen. Should layer_shell really be making assumptions about which lockscreen protocol is in use and its capabilities?

A compositor that doesn't support this could just ignore it, like sway ignores maximize and minimize requests from xdg_shell.

2. Should input_method_unstable_v1 (get_input_panel_surface) also be extended to be lockscreen aware? And all future protocols that have unique surface roles?

I think that protocol appears to be bound to a parent surface, so I would think it would inherit its "locksreenness" from the parent surface. In general, only surfaces that need to appear on a lockscreen would need to be able to specify that fact.

Also, the more unique surface roles you have appearing on the lock screen, the more likely one of them exposes something like keyboard auto-completion that isn't supposed to be functional on a locked desktop. The list of on-locked-screen clients should really be small and they should be aware that they are on the lockscreen (and probably should explicitly opt-in to this).

Changing layer-shell isn't the only way to solve the problem; the lockscreen protocol could itself be used to mark surfaces as show-on-lock or show-on-both.

3. For clients that have separate locked and unlocked states, it would be good for them to know current lock state so that they don't have to render 2 separate surfaces all the time. While this could be added to layer_shell, it feels like a lot of leakage and assumptions.

But the lock state is already communicated? No, I don't think that we should add locked/unlocked events to layer_shell, that would be far outside its scope.

<event name="lock_abandoned">: I'm curious who this is intended for. If there's a process that watches for this event, wouldn't that same process know if the lockscreen crashed since it'll probably be its parent anyway?

This depends on how you set up locking; in some cases, this event might not be needed. I envisioned a sway keybinding that does something like swaylock -f && rtcwake -m mem, and then another daemon (maybe swayidle, maybe a session-management script) that just watches for the lock_abandoned event and then re-launches swaylock if needed. The forking behavior of swaylock -f here is being used to delay the S3 sleep until the screen is visibly locked, but it prevents using waitpid to easily catch a crashing swaylock.

<request name="lock">: I like the idea of limiting to a single locker client.

I can't tell if this is an agreement or a disagreement with the proposal. Note that making a lock request doesn't require the compositor to honor the request - if you want to limit locking to a single client, just reject all requests not by that client. But this behavior is up to the compositor.

@thatguystone
Copy link
Author

regarding if frame callbacks are sufficient for everything I claim they are

From my understanding, frame callbacks can't be used to determine visibility. The lack of any way to determine visibility reliably was the driving force behind Protocol design goals #2.

If you want to support compositors that do locking animations, you'll need to render both surfaces for the duration of that animation. I envisioned frame callbacks being used like this: once the lock animation completes, the surface on the unlocked screen won't get frame updates any more (because it's not visible) and so you won't be using resources to render it. Similarly, after unlocking, the locked surface won't get frame updates and you aren't rendering it either.

That's true for animation. But once the animation completes, it would be nice for clients to know the final un/locked state so that they can free/destroy un-needed resources, like a GtkWindow.

<request name="lock">: I like the idea of limiting to a single locker client.

I can't tell if this is an agreement or a disagreement with the proposal. Note that making a lock request doesn't require the compositor to honor the request - if you want to limit locking to a single client, just reject all requests not by that client. But this behavior is up to the compositor.

Yeah, I suppose a compositor could decide to allow multiple clients. I was just thinking about the time I accidentally started 2 lockscreens in X, and it was a total mess.

@danieldg
Copy link
Contributor

That's true for animation. But once the animation completes, it would be nice for clients to know the final un/locked state so that they can free/destroy un-needed resources, like a GtkWindow.

Ah, I hadn't really considered how toolkits would use up resources when thinking of this.

So, another alternative to the 4-state method: have the compositor close all lock-only windows after an unlock completes, similar to how it closes popups. The "lock desktop" would be transient and re-constructed every time you lock. Since I doubt any client would want to close its "unlocked" windows during the fully-locked state, that would really be all you need here.

It looks like layer_shell already has a Close event, so that seems to be doable with no changes. It might also make it easier to work with toolkits because you don't need to track all your lock-only windows to close them on the fully-unlocked event.

Yeah, I suppose a compositor could decide to allow multiple clients. I was just thinking about the time I accidentally started 2 lockscreens in X, and it was a total mess.

My sway prototype will reject the second of 2 simultaneous lockers. mstoeckl's prototype rejected the first, I think. Either way, it's quite possible to avoid the mess (and I think you can easily get a similar mess today with layer-shell lockers if they don't mutex on input grabs).

@nyancow
Copy link

nyancow commented Aug 1, 2021

Is this issue being seriously considered by any of the Sway devs? I would definitely be willing to donate/sponsor/patreon to see this happen.

@danieldg
Copy link
Contributor

danieldg commented Aug 1, 2021

FYI: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/100 has a bit more discussion. I haven't really looked at how wlroots deals with surfaces enough to wire up everything more properly than the prototype, so anyone interested in working on this further should feel free to do so.

@thatguystone
Copy link
Author

So, another alternative to the 4-state method: have the compositor close all lock-only windows after an unlock completes, similar to how it closes popups. The "lock desktop" would be transient and re-constructed every time you lock. Since I doubt any client would want to close its "unlocked" windows during the fully-locked state, that would really be all you need here.

I think a better approach to this would be, based on the current protocol: change zwp_screenlocker_v1.get_visibility to zwp_screenlocker_v1.get_surface, keep set_visibility, and add a not-needed type of event that hints for the client to clean up.

Having the compositor close all windows after unlock completes could lead to race conditions, eg: client uses the same surface for locked-only and unlocked-only drawing, but the compositor decided to kill the locked-only before the client could update it.

A zwp_screenlocker_surface_v1 interface would also make the protocol more extensible for future changes.

@danieldg
Copy link
Contributor

danieldg commented Aug 3, 2021

Note that in wayland protocols, get_*_surface is normally reserved for setting the (unique) role of a surface. While a lockscreen surface role might be doable, I think layer_shell is already pretty good at this and I see no reason to duplicate that protocol.

@thatguystone
Copy link
Author

Yeah, I noticed that name collision, just wasn't sure of a better name. It obviously shouldn't create a role; it should just allow for setting some extra lockscreen-specific, role-independent features.

@danieldg
Copy link
Contributor

danieldg commented Aug 3, 2021

Ah, so it's actually just a name change to decouple it from visibility. Seems reasonable, though finding a good name for it might be tricky.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.