many: reintroduce fdstore helpers - #16784
Conversation
ZeyadYasser
added
Needs Samuele review
There was a problem hiding this comment.
Pull request overview
This PR reintroduces systemd file-descriptor-store (fdstore) helpers and updates activation socket handling to avoid treating all passed FDs as activation sockets, enabling future work to keep sensitive data (e.g., recovery keys/passphrases) across snapd restarts without persisting it to disk.
Changes:
- Add Linux/non-Linux split implementations for
systemd.SdNotify, and introduceSdNotifyWithFdson Linux. - Introduce
systemd/fdstorepackage to manage named FDs from systemd fdstore and expose activation socket FDs. - Replace
go-systemd/activationusage innetutilwithfdstore.ActivationSocketFds, and dropgo-systemddependency from Go module and packaging manifests.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| systemd/sdnotify.go | Removed monolithic sdnotify implementation (replaced by per-OS files). |
| systemd/sdnotify_linux.go | New Linux implementation of SdNotify and SdNotifyWithFds. |
| systemd/sdnotify_other.go | New non-Linux stub returning unsupported error. |
| systemd/sdnotify_linux_test.go | New Linux-only tests, including FD-passing coverage. |
| systemd/sdnotify_test.go | Removed prior cross-platform sdnotify tests (replaced by linux-only tests). |
| systemd/fdstore/fdstore.go | New fdstore helper package for named FDs and activation socket filtering. |
| systemd/fdstore/fdstore_test.go | New tests covering init/prune/add/remove/socket-fd behavior. |
| systemd/fdstore/export_test.go | New test-only hooks/mocks for fdstore internals. |
| netutil/activation.go | Switch activation listener discovery from go-systemd to fdstore.ActivationSocketFds. |
| packaging/fedora/snapd.spec | Remove go-systemd activation dependency from RPM spec. |
| packaging/debian-sid/control | Remove go-systemd dependency from Debian control file. |
| go.mod / go.sum | Drop github.com/coreos/go-systemd module dependency. |
You can also share your feedback on Copilot code review. Take the survey.
| for name, fds := range socketFds { | ||
| for _, fd := range fds { | ||
| f := os.NewFile(uintptr(fd), name) | ||
| ln, err := net.FileListener(f) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| addr := ln.Addr().String() | ||
| lns[addr] = ln | ||
| } |
There was a problem hiding this comment.
this isn't necessarily wrong, net.FileListener() does a dup inside. I hadn't realized this before, but we likely have an ownership issue in fdstore.Get() and ActivationSocketFds(). i'll leave a comment there
| ch := make(chan string) | ||
| go func() { | ||
| var buf [128]byte | ||
| n, err := conn.Read(buf[:]) | ||
| c.Assert(err, IsNil) | ||
| ch <- string(buf[:n]) | ||
| }() | ||
|
|
||
| err = systemd.SdNotify("something") | ||
| c.Assert(err, IsNil) | ||
| c.Check(<-ch, Equals, "something") |
| _, err = unix.Seek(msgfds[1], 0, 0) | ||
| panicOnErr(err) | ||
| _, err = unix.Write(msgfds[1], []byte("hello-from-the-other-side-2")) | ||
| panicOnErr(err) |
| if shouldRemove { | ||
| logger.Noticef("removing unexpected fdstore entry %q", name) | ||
| if err := removeUnlocked(name); err != nil { | ||
| logger.Noticef("internal error: cannot remove fdstore entry %q: %v\n", name, err) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16784 +/- ##
==========================================
+ Coverage 79.04% 79.20% +0.15%
==========================================
Files 1370 1379 +9
Lines 190784 193253 +2469
Branches 2465 2466 +1
==========================================
+ Hits 150810 153067 +2257
- Misses 30888 30992 +104
- Partials 9086 9194 +108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Mon Jun 8 19:23:57 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
| FdNameMemfdSecretState FdName = "memfd-secret-state" | ||
| ) | ||
|
|
||
| var knownFdNames = map[FdName]bool{ |
There was a problem hiding this comment.
Maybe some explanation is needed here. Is this all the names that are not sockets that we just maintain within snapd? And in that case, will we need some way to register extra names here in the future?
There was a problem hiding this comment.
Is this all the names that are not sockets that we just maintain within snapd?
Yes, I will add a comment to make it clear.
And in that case, will we need some way to register extra names here in the future?
I don't expect the list to grow that much, which is why I went with centralized hand written list.
There was a problem hiding this comment.
Yep, this is fine. But if we do grow it we should start registering them in init().
| return removeUnlocked(name) | ||
| } | ||
|
|
||
| func removeUnlocked(name FdName) (err error) { |
There was a problem hiding this comment.
"Unlocked"? What is unlocked?
There was a problem hiding this comment.
should have been "Locked" but as in caller should hold the mutex lock, it's a mistake.
I will just name it remove and have the doc comment explicitly make it clear that the caller should hold the lock
| if name.isSocket() { | ||
| // Activation socket file descriptors should be accessed | ||
| // through ActivationSocketFds. | ||
| return -1 |
There was a problem hiding this comment.
Should there be an error return instead?
| if err != nil { | ||
| return err | ||
| } | ||
| defer conn.Close() |
There was a problem hiding this comment.
I wonder if we should just keep it open. Just make sure to have O_CLOEXEC.
There was a problem hiding this comment.
This was copy-pasted from here, I will add a todo so the PR doesn't grow further
Lines 34 to 59 in 17804c7
| if len(fds) != 1 { | ||
| return -1 | ||
| } | ||
| return fds[0] |
There was a problem hiding this comment.
As mentioned earlier, I think there's a potential ownership issue. Assume that the caller takes the Fd, calls os.NewFile() and proceeds to call Close(). At this point the fd is closed and should Get() be called again it returns an integer, but the value does not correspond to a valid fd anymore. It feels like that when someone Get()s the fd, they really get it and fdstore should stop tracking it, thus the actual fd is "consumed" by the caller. This applies to ActivationSocketFds() as well.
There was a problem hiding this comment.
Unless we dup(), but I don't think there's any scenario in which this would be useful.
There was a problem hiding this comment.
Also note that the caller doesn't even need to call Close() on the file, it's enough to stop referencing the *os.File for the object to be collected and finalizer (which closes the fd) be called.
There was a problem hiding this comment.
Good point, I left a comment/question in the removal code. I wonder if on removal consumed fds should be closed implicitly vs making it the burden of the consumer. I am leaning towards the latter and closing fds on removal only if they are not consumed.
| return -1, fmt.Errorf("%s: found more than one matching file descriptors", errPrefix) | ||
| } | ||
|
|
||
| consumed[name] = true |
There was a problem hiding this comment.
do we need to keep track of what was consumed/removed?
| if name.isSocket() { | ||
| // Activation socket file descriptors should be accessed | ||
| // through ActivationSocketFds. | ||
| return -1, fmt.Errorf("%s: socket found, use ActivationSocketFds instead", errPrefix) |
There was a problem hiding this comment.
just an API misuse, let's make it known:
| return -1, fmt.Errorf("%s: socket found, use ActivationSocketFds instead", errPrefix) | |
| return -1, fmt.Errorf("internal error: %s: socket found, use ActivationSocketFds instead", errPrefix) |
| lns = make(map[string]net.Listener, len(socketFds)) | ||
| for name, fds := range socketFds { | ||
| for _, fd := range fds { | ||
| f := os.NewFile(uintptr(fd), name) |
There was a problem hiding this comment.
maybe the API should really return *os.File instead of the OS abstraction which is an int value
| // ActivationSocketFds returns activation socket file descriptors | ||
| // that were passed from systemd. Only sockets whose name has a | ||
| // ".socket" suffix are returned. | ||
| func ActivationSocketFds() (socketFds map[string][]int) { |
There was a problem hiding this comment.
| func ActivationSocketFds() (socketFds map[string][]int) { | |
| func ActivationSocketFds() (socketFds map[string][]*os.File) { |
There was a problem hiding this comment.
Actually this could return []net.Listener instead of files and then avoid the problem with dup entirely
| // matching file descriptors are found or if the passed name corresponds | ||
| // to a socket (i.e. ends in ".socket"). To get activation sockets use | ||
| // fdstore.ActivationSocketFds() instead. | ||
| func Get(name FdName) (fd int, err error) { |
There was a problem hiding this comment.
| func Get(name FdName) (fd int, err error) { | |
| func Get(name FdName) (f *os.File, err error) { |
| // | ||
| // - The file descriptors can be retrieved by calling Get(). | ||
| // - Only a single file descriptor can associated with a FdName. | ||
| func Add(name FdName, fd int) error { |
There was a problem hiding this comment.
| func Add(name FdName, fd int) error { | |
| func Add(name FdName, f *os.File) error { |
| // matching file descriptors are found or if the passed name corresponds | ||
| // to a socket (i.e. ends in ".socket"). To get activation sockets use | ||
| // fdstore.ActivationSocketFds() instead. | ||
| func Get(name FdName) (fd int, err error) { |
There was a problem hiding this comment.
Discussed with @ZeyadYasser how to tweak the API such that we would not have to deal with potential fd invalidation if the caller misuse the os.File objects they receive. One of the options is to have Add and Get (by extension ActivationSocketFds too) duplicate the fd before passing it to the caller.
| // ActivationSocketFds returns activation socket file descriptors | ||
| // that were passed from systemd. Only sockets whose name has a | ||
| // ".socket" suffix are returned. | ||
| func ActivationSocketFds() (socketFds map[string][]int) { |
There was a problem hiding this comment.
Actually this could return []net.Listener instead of files and then avoid the problem with dup entirely
bboozzoo
left a comment
bboozzoo
left a comment
There was a problem hiding this comment.
just some smaller tweaks
| // | ||
| // https://www.freedesktop.org/software/systemd/man/latest/sd_pid_notify_with_fds.html#FDNAME=%E2%80%A6 | ||
| if err := systemd.EnsureAtLeast(236); err != nil { | ||
| return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) |
There was a problem hiding this comment.
this could use a distinct error, ErrUnsupported or something along those lines
| // - The file descriptors can be retrieved by calling Get(). | ||
| // - Only a single file descriptor can associated with a FdName. | ||
| // | ||
| // It is the caller's responsibility to close f when finished. |
There was a problem hiding this comment.
| // It is the caller's responsibility to close f when finished. | |
| // Maintains a copy of the underlying file descriptor internally. It is the caller's | |
| // responsibility to close f when finished. |
| // Note: Removing the all references of os.File will impicitly | ||
| // close opened fds by finalizer for os.File so no need to | ||
| // explicitly call close. |
There was a problem hiding this comment.
though we could call Close(), couldn't we?
There was a problem hiding this comment.
we could, I think this was left over from when fdstore didn't hold a copy and instead shared the raw fds with consumers
| return nil | ||
| } | ||
|
|
||
| // Get retrieves file descriptor passed from systemd by its name. |
There was a problem hiding this comment.
| // Get retrieves file descriptor passed from systemd by its name. | |
| // Get retrieves a duplicate of file descriptor passed from systemd by its name. |
| // to a socket (i.e. ends in ".socket"). To get activation sockets use | ||
| // fdstore.ActivationListeners() instead. | ||
| // | ||
| // It is the caller's responsibility to close f when finished. |
There was a problem hiding this comment.
The fdstore holds a copy of the file descriptor, the caller needs to call Remove() on top of closing
all privately held references in order to release all resources associated with a given fd.
This commit reintroduces reverted PR canonical#16119 * systemd: add fdstore helpers The initial use case of using systemd's fdstore is to keep sensitive data that cannot be persisted to disk like recovery keys and passphrases while having them survive snapd restarts to increase the robustness of FDE operation. * netutil: only use activation sockets passed from systemd In upcoming work, more fds will be passed from systemd on startup, previously the helper from go-system considered all passed fds as activation fds which will not be true in the future. * packaging: remove go-systemd dependency This was the only usage of go-systemd, so I am removing its dependency as well. --------- Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
a23b454 to
b02ed00
Compare
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
b02ed00 to
6c74eb0
Compare
| # Should this be enabled to survive soft reboots and service stop/start | ||
| # starting with systmed v254+? | ||
| #FileDescriptorStorePreserve=yes |
There was a problem hiding this comment.
I think systemd ignores what it does not know. So we could set it. But I would do it only if we it was tested. Maybe with a spread test even. Otherwise I would not even have it as a comment.
| logger.Noticef("unexpected fdstore entry %q found: %v", name, err) | ||
| shouldRemove = true | ||
| } | ||
| // Only activation sockets can be associated with multiple fds. |
There was a problem hiding this comment.
I suppose this is a restriction for us, not for systemd. Could you maybe rephrase to make it less misleading?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| unixCloseOnExec(duplicatedFd) |
There was a problem hiding this comment.
Maybe add todo here too about check for error.
There was a problem hiding this comment.
unix.CloseOnExec does not return an error, maybe I should switch to the raw fnctl?
| return | ||
| } | ||
|
|
||
| // Make sure initialization only happens once, only here. |
There was a problem hiding this comment.
btw could we use sync.Once?
There was a problem hiding this comment.
It is good to always unset those to avoid anything else parsing them, it is to avoid other parts of the code base of parsing them afterwards.
| osGetenv = os.Getenv | ||
| osUnsetenv = os.Unsetenv | ||
| osLookupEnv = os.LookupEnv |
There was a problem hiding this comment.
we're not running unit tests within a package in parallel, so you may as well mock specific things in the environment like we do elsewhere
|
|
||
| // Get retrieves a duplicate of the file descriptor passed from systemd by | ||
| // its name. close-on-exec is set on the returned file descriptor. An error | ||
| // is returned if no matching file descriptor is found, if more than one |
There was a problem hiding this comment.
| // is returned if no matching file descriptor is found, if more than one | |
| // matching ErrNotFound is returned if no matching file descriptor is found. |
|
|
||
| // Get retrieves a duplicate of the file descriptor passed from systemd by | ||
| // its name. close-on-exec is set on the returned file descriptor. An error | ||
| // is returned if no matching file descriptor is found, if more than one |
There was a problem hiding this comment.
aren't we already dropping extraneous named file descriptors?
There was a problem hiding this comment.
yes, updated comment
| } else if len(fds) > 1 { | ||
| return nil, fmt.Errorf("%s: found more than one matching file descriptors", errPrefix) | ||
| } |
There was a problem hiding this comment.
isn't this already prevented in initFdstore?
There was a problem hiding this comment.
yes, I added the check just in case. I will drop it as it can never happen
| duplicatedFd, err := unixDup(int(f.Fd())) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| unixCloseOnExec(duplicatedFd) |
There was a problem hiding this comment.
maybe this and the other instance of the same code in Get() could be moved to a helper?
| if err != nil { | ||
| return nil, err | ||
| } | ||
| unixCloseOnExec(duplicatedFd) |
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| const sd_LISTEN_FDS_START = 3 |
There was a problem hiding this comment.
this should have a doc comment, also maybe it should be moved closer to where it is used
| for i := 0; i < nfds; i++ { | ||
| fd := sd_LISTEN_FDS_START + i | ||
| name := FdName(names[i]) | ||
| fdstore[name] = append(fdstore[name], os.NewFile(uintptr(fd), string(name))) |
There was a problem hiding this comment.
maybe we should have a comment already here about support many for the sockets case
There was a problem hiding this comment.
| // assigned by setting `FileDescriptorName=` on the socket unit. | ||
| // | ||
| // `FileDescriptorName=` was added in systemd version 227. | ||
| for name, fds := range fdstore { |
There was a problem hiding this comment.
Side observation, but the order in which listeners are returned isn't deterministic. I don't think it's an issue though, but maybe worth a doc comment.
| // remove file descriptors from systemd given their name. | ||
| // | ||
| // Caller must hold the fdstore lock. | ||
| func remove(name FdName) (err error) { |
There was a problem hiding this comment.
| func remove(name FdName) (err error) { | |
| func remove(name FdName) error { |
| // | ||
| // Maintains a copy of the underlying file descriptor internally. It | ||
| // is the caller's responsibility to close f when finished. | ||
| func Add(name FdName, f *os.File) (retErr error) { |
There was a problem hiding this comment.
| func Add(name FdName, f *os.File) (retErr error) { | |
| func Add(name FdName, f *os.File) error { |
| // returned. | ||
| // | ||
| // It is the caller's responsibility to close returned listeners when finished. | ||
| func ActivationListeners() (listeners []net.Listener, retErr error) { |
There was a problem hiding this comment.
| func ActivationListeners() (listeners []net.Listener, retErr error) { | |
| func ActivationListeners() (listeners []net.Listener, err error) { |
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
thank you, couple more comments
|
|
||
| state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) | ||
| if err := sdNotifyWithFds(state, duplicatedFile); err != nil { | ||
| duplicatedFile.Close() // clean up the duplicated fd |
There was a problem hiding this comment.
should this use osFileClose and be tested more directly?
| ) | ||
|
|
||
| // Note: os.File is used to wrap raw fds so that the | ||
| // underlying fds are impicitly closed by finalizer |
There was a problem hiding this comment.
| // underlying fds are impicitly closed by finalizer | |
| // underlying fds are implicitly closed by finalizer |
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
ZeyadYasser
added
Auto rerun spread
This commit reintroduces reverted PR #16119
systemd: add fdstore helpers
The initial use case of using systemd's fdstore is to keep sensitive data that cannot be persisted to disk like recovery keys and passphrases while having them survive snapd restarts to increase the robustness of FDE operation.
netutil: only use activation sockets passed from systemd
In upcoming work, more fds will be passed from systemd on startup, previously the helper from go-system considered all passed fds as activation fds which will not be true in the future.
packaging: remove go-systemd dependency
This was the only usage of go-systemd, so I am removing its dependency as well.
JIRA: SNAPDENG-36595