Skip to content

Homed bulk directories v2 - #30840

Merged
bluca merged 9 commits into
systemd:mainfrom
AdrianVovk:homed-bulk-v2
Feb 19, 2024
Merged

Homed bulk directories v2#30840
bluca merged 9 commits into
systemd:mainfrom
AdrianVovk:homed-bulk-v2

Conversation

@AdrianVovk

@AdrianVovk AdrianVovk commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

This is a reimagined approach for #30646, as I described in this comment

Depends on #31004

Fixes: #18323

@AdrianVovk

AdrianVovk commented Jan 11, 2024

Copy link
Copy Markdown
Contributor Author

Unforeseen issue I ran into w/ this approach: signatures

Right now the code works something like this. If a caller wants to change the bulk directory, they add bulkDirectory to the regular section. This is passed through homed into homework. homework then calls user_record_steal_bulk_dir, which extracts the value of bulkDirectory from the record (making sure to purge it from the JSON). homework then does the bulk directory replacement/reconciliation magic. Finally, homework returns the new JSON (now lacking bulkDirectory, with a possibly updated binding and so on) back up to homed, which writes it to disk.

So here's the problem: Before handing the JSON record into homework, homed signs it first. This signature happens before homework strips the bulkDirectory out of the record. So when homework does strip out the field, the record no longer matches the signature.

This all also brings in a natural follow-up question: should the contents of the bulk directory be signed into the user record?

Couple ideas on how to solve this:

  • Exclude bulkDirectory from the signature
  • Make clients pass an updated bulkDirectory into homed through the status section
  • Make a new section transient that behaves like secret but is for non-sensitive information
  • Sign the record after homework is done with it, to allow homework to manipulate the record however it wishes (I think this is personally my favorite solution)

Comment thread src/home/user-record-sign.c Fixed
Comment thread test/units/testsuite-46.sh Fixed
Comment thread test/units/testsuite-46.sh Fixed
Comment thread test/units/testsuite-46.sh Fixed
Comment thread test/units/testsuite-46.sh Fixed
Comment thread test/units/testsuite-46.sh Fixed
Comment thread test/units/testsuite-46.sh Fixed
Comment thread test/units/testsuite-46.sh Fixed
Comment thread test/units/testsuite-46.sh Fixed
@poettering

Copy link
Copy Markdown
Member

So for various fields we allow overriding via the binding section.

I think a scheme where we have one of these dirs declared in the regular user record, and then overriden with the copied out version in the binding section does make sense to me.

thta would would mean clients would always have to check the binding first, and only when not set use the original one. but we do something similar with homeDirectory itself already, it's automatically resolved when parsing.

@poettering

Copy link
Copy Markdown
Member

btw, should we really name this "bulk"? i.e. is that really descriptive enough? it's just saying something about the size. But I think its more important to say something about the public exposure of the data contained in there. Hence maybe publicResourceDirectory?

@poettering

Copy link
Copy Markdown
Member

I'd probably not try to hide the updating of the bulkdir in the updating of the record, but keep that a separaze operation, where you just pass in an fd to a temporary dir with the new bulkdir, and homed does the replacement.

@poettering

Copy link
Copy Markdown
Member

I am still not onboard with moving the identity file down one level. This just opens uip the door to various vulnerabilities with symlinks.

i.e. people could then replace .identity/ with a symlink to anything they like and a simple O_NOFOLLOW when opening the identity file won't suffice anymore to avoid being tricked. i.e. the tough bit here is that privileged code is going to access this, and we should not make it easy for an evil user to trick priv code to read stuff it shouldn't read, and trusting it because the object's perms actually indicate its owend by root. but in reality somebody just redirected things via symlinks.

Comment thread src/home/homework-directory.c
Comment thread src/shared/group-record.c
Comment thread src/basic/fd-util.h
Comment thread docs/USER_RECORD.md
@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 Feb 13, 2024
@poettering

Copy link
Copy Markdown
Member

please rebase

@poettering poettering added needs-rebase and removed please-review PR is ready for (re-)review by a maintainer labels Feb 14, 2024
@github-actions github-actions Bot added please-review PR is ready for (re-)review by a maintainer and removed needs-rebase labels Feb 14, 2024
Comment thread src/basic/fd-util.c
Comment thread src/basic/fd-util.c Outdated
Comment thread src/shared/user-record-show.c Outdated
Comment thread src/home/homework-blob.c Outdated
Comment thread src/home/homectl.c
@poettering

Copy link
Copy Markdown
Member

i don't like the passing down of the flags field still

@poettering poettering left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's get this baby landed.

Comment thread src/home/homework-blob.c Outdated
Comment thread src/home/homework-blob.c Outdated
Comment thread src/home/homectl.c
@poettering

Copy link
Copy Markdown
Member

please fix those error return things, and we are good to go.

Comment thread docs/USER_RECORD_BLOB_DIRS.md
@AdrianVovk

Copy link
Copy Markdown
Contributor Author

CI failures appear unrelated:

  • mkosi / ci (fedora, rawhide) (pull_request):
Transaction failed: Signature verification failed.
PGP check for package "filesystem-3.18-8.fc40.x86_64" (/var/cache/libdnf5/fedora-306b6523e9c8dc02/packages/filesystem-3.18-8.fc40.x86_64.rpm) from repo "fedora" has failed: Import of the key didn't help, wrong key?
  • All of the rest:
<snip>
TEST-75-RESOLVED:                   FAIL     ( 66 s)
<snip>
TOTAL FAILURES: 1 OF 64

@bluca

bluca commented Feb 19, 2024

Copy link
Copy Markdown
Member

too many failures for comfort, I've hit the rebase button for another run

This is useful for situations where an array of FDs is to be passed into
a child process (i.e. by passing it through safe_fork). This function
can be called in the child (before calling exec) to pack the FDs to all
be next to each-other starting from SD_LISTEN_FDS_START (i.e. 3)
We're documenting the behavior of blob directories here. These docs
refer to things that aren't yet implemented at the time of the commit, but will be later in the same PR.
These fields are used to connect a JSON user record to its blob
directory, and to include the directory's contents in the record's
signature
This ensures that a user-specific blob directory exists in
/var/cache/systemd/homed for as long as the user exists, and gets
deleted if the user gets deleted.

It also advertises this blob directory via the user record, so that
clients can find and use it.
Whenever the host & embedded records are reconciled, the host & embedded
blob directories are now reconciled too in the same direction.
Reconciling the blob directories serves exactly the same purpose as
reconciling the user records, and thus should behave in the same way.
Introduces new extended variants of the various incarnations of
Create and Update, which take a map of filenames to FDs. This map is
then used to populate the bulk directory.

FDs are used to prevent the client from abusing homed's blob directory
permissions (everything is made world-readable by homed) to open files
that they normally aren't allowed to open. Passing along an FD ensures
that the client has read access to the file it wants homed to make
world-readable.

Internally, homework uses the map to overwrite the system blob dir.
Later, homework's existing blob dir reconciliation logic will propagate
the new contents from the system blob dir into the embedded blob
dir
This makes it possible to edit blob directories using homectl. The
following syntax is available:

* `--blob-directory=/path/somewhere`: Replaces the entire blob directory
with the contents of /path/somewhere

* `--blob-directory=foobar=/path/somewhere`: Replaces just the file
foobar in the blob directory with the contents of /path/somewhere

* `--blob-directory=foobar=`: Deletes the file foobar from the blob
directory

* `--blob-directory=`: Resets all previous flags

* `--avatar=`, etc: Shortcuts for `--blob-directory=FILENAME=` for the
known files in the blob directory
@AdrianVovk

Copy link
Copy Markdown
Contributor Author

🎉 Thank you!

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.

RFE userdb/homed: user entries should provide avatar/face field

4 participants