Skip to content

Move handling of secure locking into logind#31796

Closed
AdrianVovk wants to merge 14 commits into
systemd:mainfrom
AdrianVovk:homed-lock-plumbing
Closed

Move handling of secure locking into logind#31796
AdrianVovk wants to merge 14 commits into
systemd:mainfrom
AdrianVovk:homed-lock-plumbing

Conversation

@AdrianVovk

@AdrianVovk AdrianVovk commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

Depends on #31841

I'd like to get this into v256, to make sure that we can use this in GNOME 47 (releasing September 2024)

Here's a quick summary / todo-list of the changes:

  • Introduced a new logind SecureLock()API to loosen the coupling between homed and other components (systemd-sleep, the desktop environment, etc)
  • Introduced functionality that warns clients about an imminent SecureLock(), and lets them delay it for a little while. It's pretty much the same thing as logind's delay inhibitor + PrepareForSleep() mechanism. This lets session services prepare for a secure lock by wiping secrets out of memory (think: your ssh-agent), closing sensitive FDs, and unsubscribing from the bus (since they'll be frozen)
  • Made homed a backend for the SecureLock() API
  • Moved homed's "please_suspend" logic into logind, again to loosen the coupling between desktop environment and homed
  • Deprecated the various features in homed

@AdrianVovk AdrianVovk force-pushed the homed-lock-plumbing branch 3 times, most recently from dfd1e89 to 268fc1c Compare March 16, 2024 18:25
@github-actions github-actions Bot added the sleep label Mar 16, 2024
@AdrianVovk AdrianVovk force-pushed the homed-lock-plumbing branch from bf083f8 to 93f3297 Compare March 18, 2024 19:21
@AdrianVovk AdrianVovk force-pushed the homed-lock-plumbing branch from 93f3297 to 719adca Compare March 18, 2024 19:32
@AdrianVovk AdrianVovk changed the title homed: Add some more plumbing Move handling of secure locking into logind Mar 18, 2024
@YHNdnzj

YHNdnzj commented Mar 18, 2024

Copy link
Copy Markdown
Member

Honestly, I'd prefer this to stay in homed... This is more home-area-related than login-related after all.

@AdrianVovk

Copy link
Copy Markdown
Contributor Author

What happens when sssd or whatever active directory thing wants to provide a more secure lockdown mode where your AD credentials are wiped from memory, similar to homed's lock?

@AdrianVovk

Copy link
Copy Markdown
Contributor Author

Don't get me wrong, I'd love to do less work and put this into homed directly. Originally this PR was just about adding the DelayLock+PrepareForLock mechanism directly into homed. The problem is that nobody will be OK calling into that: each new service that manages user records will need to have its own bespoke functionality for this and desktop environments will need to support each one manually. If I put this into homed, desktop environments, ssh-agent, gpg-agent, the secret service / keyring, etc support it once and that's that.

I don't really see why SecureLock is any different from homed's existing session Lock

Homed's existing Lock function is a weird mix of high-level and low-level. On one hand, it'll have to respect the delay inhibitors I've added. On the other hand, it doesn't respect the fact that your desktop environment might not actually support secure locking. Putting it into logind cleans this logic up: logind does the high-level stuff (inhibitors, checking if it's actually supported by all the running sessions), and homed does the low-level stuff (locking up your home directory)

@AdrianVovk AdrianVovk force-pushed the homed-lock-plumbing branch from 719adca to 521958d Compare March 22, 2024 22:44
@YHNdnzj

YHNdnzj commented Mar 23, 2024

Copy link
Copy Markdown
Member

Generalizing a concept is a good thing, but for this I really think the backend that actually gets used matters to clients. Here you somehow define the semantics of SecureLock. What if some backend provides more functionility, some of which requires extra response from clients? I think eventually you have to communicate to the backend?

Plus, as @poettering has also brought up a few times, most people use the secure lock through sleep, and we already freeze the user sessions in systemd-sleep.

If this in the end has to go into logind, please don't add a completely new concept for it. Call it LockWithFlags and have PrepareForLockWithMetadata just like PrepareForShutdownWithMetadata?

And note that logind is already the high-level interface of systemd-sleep. Hence there's no point to first invoke sd-sleep and then make it call into logind again later. (Even the session freezer should be moved into logind in this case)

@YHNdnzj

YHNdnzj commented Mar 23, 2024

Copy link
Copy Markdown
Member

I mean, if you really want to generalize it, do it fully wrt sd-sleep and generic session locks. The current implementation still looks like a "mixed mess" and IMO is even worse than status quo.

@AdrianVovk

Copy link
Copy Markdown
Contributor Author

Generalizing a concept is a good thing, but for this I really think the backend that actually gets used matters to clients.

I disagree that the backend matters to clients.

For apps taking the delay inhibitors: They're going to get frozen and encrypted. That's all they need to know. What actually happens is irrelevant to the apps wanting to be notified. We even list out the things the apps should do in response: scrub sensitive data out of memory, prepare to be frozen. That's all they need to know, the rest is an irrelevant implementation detail.

For other apps listening to the signal (notably, GDM): again, doesn't matter. SecureLock means "make a secure lock screen that's running outside of the user's session appear on screen". So GDM puts itself on screen. That requires no knowledge of what the backend is doing either.

And for clients actually requesting a secure lock: it's a "hey OS, I think now would be a great time to enter a secure lock state if you know how to do that, thanks" call. I don't see why the client cares about exactly what the OS does in response here either

I actually can't think of any use case where a client should know or care about the backend... Can you give any examples?

Here you somehow define the semantics of SecureLock. What if some backend provides more functionility, some of which requires extra response from clients? I think eventually you have to communicate to the backend?

Well then clients will have bug reports opened up against them along the lines of "when secure locking happens with X backend, app fails to do Y in preparation causing Z to happen". The maintainers of the app will then decide if it's worth fixing for the app and that's that.

But anyway it's all best effort. Notifying apps that they should prepare is a courtesy. If they fail to heed our notification they'll get locked and frozen anyways.

Plus, as @poettering has also brought up a few times, most people use the secure lock through sleep, and we already freeze the user sessions in systemd-sleep.

As I have brought up every time this point is brought up: secure lock has to work outside of sleep for lots of reasons. One of which is switching users (when you switch users the one you're leaving should be secure locked by the DE). Another is advanced locking behavior UX (i.e. you're on a Linux phone, and shouldn't secure lock on every suspend and should instead secure lock after 24 hours of inactivity). A third is having a button or key combination to secure-lock-now, which pretty much does what it says on the tin (again, for Linux phones you're not secure locking on suspend)

If we can secure lock without sleeping we cannot rely on sleep's freeze to preserve the integrity of the user session, we must do our own

If this in the end has to go into logind, please don't add a completely new concept for it. Call it LockWithFlags and have PrepareForLockWithMetadata just like PrepareForShutdownWithMetadata?

It has to be a new concept because unlike the existing lock mechanism it is done per user not per session. We can't reuse the existing per-session API.

The existing lock mechanism is also completely asynchronous, but we need something synchronous.

Also, unlike regular locking, secure locking must be done in 2 stages. First we warn all the clients, then once all the clients tell us they're ready (or we time out) we lock them and freeze them.

We can't reuse logind's existing concept for inhibitors because they inhibit system-wide things (suspend, power off, etc) and have no concept of inhibiting something for a session or user. If I'm an app running under user X, I don't want to be able to inhibit secure locking for user Y.

And note that logind is already the high-level interface of systemd-sleep. Hence there's no point to first invoke sd-sleep and then make it call into logind again later.

Sure I was thinking of moving the SecureLockUsers step out of systemd-sleep and into logind. I'll do that.

(Even the session freezer should be moved into logind in this case)

Sorry wdym? There's a couple of freezers involved here: the one freezing the session during a secure lock, and another freezing the entire users.slice during sleep. Which are you referring to?

If you mean the one freezing the session during secure lock, then NACK. This must be managed by the backend because the backend is the only one that knows about when it's safe to release the freezer. Plus we have no concept of unlocking from a secure lock (which is fine - that's handled through PAM) but that means we don't know when to unfreeze the session.

I also don't know the architecture of the backend - maybe it has parts running inside the session itself and it can't freeze itself and must do something else to freeze the rest of the session. That'd be a terrible design decision on the backend's part but it's possible - we've seen code like that happen.

If you mean freezing the entire users.slice during sleep, sure I could probably do that from logind if you want me to.

I mean, if you really want to generalize it, do it fully wrt sd-sleep and generic session locks

Not sure what you mean here.

sd-sleep is already loosely coupled to logind, you could replace the service and be done with it.

Session locks are loosely coupled in logind too - all that Lock()/Unlock() does in logind is emit a signal and that's it. Frankly not a fan of that API but not much I can do about it. I would have made it synchronous, same way I'm doing with secure locking.

The current implementation still looks like a "mixed mess" and IMO is even worse than status quo.

Don't forget that the status quo completely lacks features that this implementation has, like warning clients about impending secure lock and letting them delay, and notifying GDM that it needs to put itself on screen or else the user will be stuck looking at a frozen session. When I added this same functionality into homed directly, it became a similarly "mixed mess" of event sources.

This needs to be somewhat generic (i.e. be in logind) for things like your ssh-agent, random apps, etc to be convinced to use it. If it's specific to homed I think the sell is a lot harder

I don't disagree that having logind handle the high-level inhibitor and etc logic, then telling a low level backend to lock now isn't the prettiest solution. But I think it's the best we can get.

@AdrianVovk AdrianVovk force-pushed the homed-lock-plumbing branch 4 times, most recently from 24be0db to a16313b Compare March 26, 2024 23:30
@AdrianVovk

AdrianVovk commented Mar 28, 2024

Copy link
Copy Markdown
Contributor Author
Program to test secure lock notifications
from gi.repository import GLib, Gio
import time
import os
from random import randint

bus = Gio.bus_get_sync(Gio.BusType.SYSTEM)

(path,) = bus.call_sync("org.freedesktop.login1", "/org/freedesktop/login1", "org.freedesktop.login1.Manager", "GetUser", GLib.Variant("(u)", [os.getuid()]), None, Gio.DBusCallFlags.NONE, -1)
proxy = Gio.DBusProxy.new_sync(bus, Gio.DBusProxyFlags.NONE, None, "org.freedesktop.login1", path, "org.freedesktop.login1.User")

fd = None

def close(fd):
    os.close(fd)
    return None

def sleep(sec, msg):
    for i in range(0, int(sec / 0.1)):
        time.sleep(0.1)
        print(['|', '/', '—', '\\'][i%4], msg, end='\r', flush=True)
    print("\33[2K\r", end='', flush=True)

def delay(fd):
    fd = proxy.call_with_unix_fd_list_sync("DelaySecureLock", None, Gio.DBusCallFlags.NONE, -1, None).out_fd_list.get(0)
    print("Acquired delay FD")
    return fd

def signal(proxy, sender, signal, params):
    global fd

    if signal != "PrepareForSecureLock":
        return
    (locking,) = params
    
    if locking:
        print("Prepare for secure lock!")
        sleep(randint(2, 5), "Pretending to do work...")
        print("Done with work! Allowing secure lock to continue.", flush=True)
        fd = close(fd)
        sleep(2, "Waiting for secure lock to take effect...")

    else:
        print("Recover the FD")
        fd = delay(fd)

proxy.connect("g-signal", signal)

print("Getting FD for first time")
fd = delay(fd)

GLib.MainLoop().run()

@AdrianVovk AdrianVovk force-pushed the homed-lock-plumbing branch 6 times, most recently from 58cd370 to 7a80114 Compare March 29, 2024 02:35
@AdrianVovk

Copy link
Copy Markdown
Contributor Author

Writing tests for this is unfortunately rather difficult, because we need:

  • A running logind session that we need to be able to spin up and then let it run in the background while we interact with it. Plus we need to be able to do this w/ control over whether or not the PAM session supports secure locking
  • clients that listen for a message of impending secure-lock, then delay it somehow, and failing the test if it doesn't work (no idea how to do this one)
  • the ability to repeatedly trigger a suspend (and test that it correctly activates a secure lock, or not) from within the test.

We can probably get away with not testing this so rigorously. Plus, if it breaks, it would be very noticeable from the behavior of the desktop environment. On the other hand, I guess this is at least somewhat security sensitive and having CI testing for everything is great.

I think I'll skip writing tests for this for now, but I'm very open to input regarding how to reasonably test this

@github-actions github-actions Bot removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label May 1, 2024
@AdrianVovk AdrianVovk added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels May 1, 2024
AdrianVovk added 11 commits May 1, 2024 19:06
This is a special lock operation that wipes sensitive data out of
memory, makes files inaccessible, and freezes the user session (i.e.
homed's lock feature).

This is implemented in a backend-agnostic manner, to loosen the coupling
between systemd components and to allow desktop environments to support
this feature no matter what service is managing the user.

This impl. also supports warning clients about an imminent secure lock,
and letting them delay the lock for a short while so that they can
prepare themselves. This will allow session services to additionally
wipe their own secrets from memory to further improve the security of a
secure lock.
Securely locks all the users that logind is tracking
To expose the secure-locking behavior via the CLI, so that it can be
conveniently activated.
Previously, Operation supported replying to pending DBus calls. Now, it
also supports replying to pending Varlink calls. This allows us to
initiate operations via Varlink, which will be used later to implement
the secure-lock backend
This implements the io.systemd.SecureLockBackend Varlink service in
homed for logind's SecureLock() functionality
Some sessions are going to be incompatible w/ secure locking (i.e. if
the desktop environment doesn't have a reauthentication mechanism, or if
logged in on a TTY). Let's completely disable the functionality if any
session belonging to the user reports that it doesn't support secure
locking
Before, we had logind handling delay inhibitors, then starting sd-sleep,
which would call back into homed to handle secure locking.

Now that we've moved secure locking into logind, logind would start
sd-sleep and sd-sleep would call back into logind. Instead of
implementing it like this, we just make logind handle secure locking
itself before it starts sd-sleep. This is better layering and makes it
easier to reason about what's happening immediately before a suspend.
Since it has moved into logind. We mark LockAllHomes() as deprecated and
calling it just proxies through into logind's equivalent API
This was moved into logind, and does nothing anyways since
LockAllHomes() was stubbed out
If a user is inactive, that means that the user's sessions are
inaccessible from any seat. This will happen, for instance, if the
person at the seat presses the "Switch Users" button in their desktop
environment. In this case, we should secure-lock the now inaccessible
user.
@AdrianVovk AdrianVovk force-pushed the homed-lock-plumbing branch from 836d12c to a00e406 Compare May 1, 2024 23:06
@github-actions github-actions Bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 1, 2024
@AdrianVovk

Copy link
Copy Markdown
Contributor Author

So I've mostly reworked it, however there's still some buggy behavior to iron out & testing to be done. So not yet ready to be looked at

  • Need to test delay inhibitors
  • Need to test inactive inhibitors
  • Need to test suspend inhibitors (real hardware?)
Post-rework script to test the secure lock notification
from gi.repository import GLib, Gio
import time
import os
from random import randint

bus = Gio.bus_get_sync(Gio.BusType.SYSTEM)

(path,) = bus.call_sync("org.freedesktop.login1", "/org/freedesktop/login1", "org.freedesktop.login1.Manager", "GetUser", GLib.Variant("(u)", [os.getuid()]), None, Gio.DBusCallFlags.NONE, -1)
proxy = Gio.DBusProxy.new_sync(bus, Gio.DBusProxyFlags.NONE, None, "org.freedesktop.login1", path, "org.freedesktop.login1.User")

fd = None

def close(fd):
    os.close(fd)
    return None

def sleep(sec, msg):
    for i in range(0, int(sec / 0.1)):
        time.sleep(0.1)
        print(['|', '/', '—', '\\'][i%4], msg, end='\r', flush=True)
    print("\33[2K\r", end='', flush=True)

INHIBIT_SECURE_LOCK = 1<<0
INHIBIT_SUSPEND_SECURE_LOCK = 1<<1
INHIBIT_INACTIVE_SECURE_LOCK = 1<<2

def delay(fd):
    args = GLib.Variant("(tssb)", [ INHIBIT_SECURE_LOCK, "WhoWhy", "WhyWho", True ])
    fd = proxy.call_with_unix_fd_list_sync("Inhibit", args, Gio.DBusCallFlags.NONE, -1, None).out_fd_list.get(0)
    print("Acquired delay FD")
    return fd

def signal(proxy, sender, signal, params):
    global fd
    
    if signal == "PrepareForSecureLock":
        print("Prepare for secure lock!")
        sleep(randint(2, 5), "Pretending to do work...")
        print("Done with work! Allowing secure lock to continue.", flush=True)
        fd = close(fd)
        sleep(2, "Waiting for secure lock to take effect...")
    elif signal == "SecureUnlocked":
        print("Recover the FD")
        fd = delay(fd)

proxy.connect("g-signal", signal)

print("Getting FD for first time")
fd = delay(fd)

GLib.MainLoop().run()

@AdrianVovk AdrianVovk removed the please-review PR is ready for (re-)review by a maintainer label May 1, 2024
@bluca bluca added needs-rebase ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Sep 3, 2024
@bluca bluca modified the milestones: v257, v258 Nov 12, 2024
@poettering

Copy link
Copy Markdown
Member

Needs a major rebase. Dropping from milestone for now.

@poettering poettering removed this from the v258 milestone Jun 5, 2025
@AdrianVovk

Copy link
Copy Markdown
Contributor Author

Superseded by #41633

@AdrianVovk AdrianVovk closed this Apr 16, 2026
@github-actions github-actions Bot removed needs-rebase ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants