Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: machined, talosctl: enable listing labels #8556

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dsseng
Copy link
Member

@dsseng dsseng commented Apr 7, 2024

Pull Request

What? (description)

Extended attributes are useful for SELinux labels and more

Tested in Docker on a SELinux-enforcing host (openSUSE)

image

Why? (reasoning)

Fixes #1542

Acceptance

Please use the following checklist:

  • you linked an issue (if applicable)
  • you included tests (if applicable)
  • you ran conformance (make conformance)
  • you formatted your code (make fmt)
  • you linted your code (make lint)
  • you generated documentation (make docs)
  • you ran unit-tests (make unit-tests)

See make help for a description of the available targets.

@dsseng

This comment was marked as resolved.

@dsseng dsseng requested a review from smira April 22, 2024 16:06
Copy link
Member

@smira smira left a comment

Choose a reason for hiding this comment

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

is there anything meaningful in xattrs except for SELinux labels for us?

In other words, should we limit it to SELinux labels only?

@dsseng
Copy link
Member Author

dsseng commented Apr 22, 2024

IMA/EVM and AppArmor use them as well. IMA/EVM has been requested afaik, and we should consider it someday

@smira
Copy link
Member

smira commented Apr 22, 2024

IMA/EVM and AppArmor use them as well. IMA/EVM has been requested afaik, and we should consider it someday

So my 2 cents:

  • it'd be easier to just have list -l show SELinux labels as a column vs. a separate output for xattrs
  • how resource intensive is reading xattrs for every file even if we never use them?

@dsseng
Copy link
Member Author

dsseng commented Apr 22, 2024

it'd be easier to just have list -l show SELinux labels as a column vs. a separate output for xattrs

Well, if we only consider SELinux, yes. We could also just avoid listing those and directly get security.selinux

how resource intensive is reading xattrs for every file even if we never use them?

According to source there should be 1 syscall to list and 1 syscall per xattr to read. Anyway these API calls are not expected to be ran frequently.

@smira
Copy link
Member

smira commented Apr 22, 2024

it also allocates a buffer for each xattr read.

what I mean is that make xattr reading optional, enable only if we list with --long, and for now in talosctl show just selinux label to make it less of a mess

@dsseng
Copy link
Member Author

dsseng commented Apr 22, 2024

Alright, will do

@dsseng
Copy link
Member Author

dsseng commented Apr 24, 2024

Formatting seems a bit off (` added to make sure those aren't extra long strings).

image

@dsseng dsseng changed the title feat: machined, talosctl: enable listing xattrs feat: machined, talosctl: enable listing labels Apr 24, 2024
@dsseng
Copy link
Member Author

dsseng commented Apr 24, 2024

Will reword after review when merging

api/machine/machine.proto Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
api/machine/machine.proto Outdated Show resolved Hide resolved
This will be useful for debugging SELinux implementation. Make API report other xattrs for further development like IMA/EVM

Signed-off-by: Dmitry Sharshakov <dmitry.sharshakov@siderolabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add extended attributes support in ls
4 participants