-
-
Notifications
You must be signed in to change notification settings - Fork 660
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
Add option to show security attribute and improve extended support #855
Conversation
The code looks good. How does it work in modes other than long? (grid, grid long, other?) Also, if you rebase on master it should fix the failing CI check. |
2c0faa1
to
b13ffe3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here’s a superficial review of your code (at least for now), since I don’t know much about xattr or SELinux.
3875166
to
2b6f6dc
Compare
0b06299
to
948c2ad
Compare
948c2ad
to
df09f1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK second review! I don’t have any knowledge of SELinux, do you have any resources about it so I could check that out and understand better what you’re doing in the code?
Also note to myself if you don’t know/want to do that: --context
/-Z
should be added to zsh and fish completion files.
SELinux is a Linux Security Module (like AppArmor) providing mandatory access control.
|
I decided to try this patch on one of my systems, there seems to be a special case for
|
I can confirm that b4504f9 is working fantastically on my SELinux enabled systems. |
02c47bb
to
c167adb
Compare
Did you take a look at the unit tests error? Sorry for the long time between reviews, I just don’t have much time these days. |
1b27f27
to
12bb4f3
Compare
12bb4f3
to
b1ca452
Compare
b1ca452
to
192040e
Compare
192040e
to
07b6716
Compare
07b6716
to
b48f210
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last review for me, afterward I’d like to merge this, but the Vagrant tests aren’t up-to-date.
Do you think you could either add this to this PR, or do a PR later to add them? Right now, there are tests checking the output of exa with xattrs
in xtests/attributes.toml
and xtests/details-view-permissions.toml
.
It’s fine if you can’t, but I honestly would prefer not to merge anything that breaks existing tests (in the future, these tests will be part of the CI).
@@ -63,6 +65,9 @@ pub struct File<'dir> { | |||
/// directory’s children, and are in fact added specifically by exa; this | |||
/// means that they should be skipped when recursing. | |||
pub is_all_all: bool, | |||
|
|||
/// The extended attributes of this file. | |||
pub extended_attributes: Vec<Attribute>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not quite sure why we need to do that? We didn’t need to do that before for xattrs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause currently the extended attributes are retrieved from the filesystem at the point when the output is generated (potentially even in another process), now the attributes are used in more places, e.g. whether to show the extended attribute hint (@
), so the extended attributes are gathered at scan time, e.g. strace(1) output:
old:
statx(AT_FDCWD, "/datadrive/", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=STATX_ATTR_MOUNT_ROOT, stx_mode=S_IFDIR|0755, stx_size=4096, ...}) = 0
openat(AT_FDCWD, "/datadrive/", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
newfstatat(3, "", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_EMPTY_PATH) = 0
getdents64(3, 0x56db6bc84130 /* 5 entries */, 32768) = 136
getdents64(3, 0x56db6bc84130 /* 0 entries */, 32768) = 0
close(3) = 0
statx(AT_FDCWD, "/datadrive/vm", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0755, stx_size=4096, ...}) = 0
statx(AT_FDCWD, "/datadrive/lost+found", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0700, stx_size=16384, ...}) = 0
statx(AT_FDCWD, "/datadrive/christian", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0755, stx_size=4096, ...}) = 0
[later in separate process]
listxattr("/datadrive/lost+found", NULL, 0) = 17
listxattr("/datadrive/lost+found", "security.selinux\0", 17) = 17
getxattr("/datadrive/lost+found", "security.selinux", NULL, 0) = 27
new:
statx(AT_FDCWD, "/datadrive/", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=STATX_ATTR_MOUNT_ROOT, stx_mode=S_IFDIR|0755, stx_size=4096, ...}) = 0
llistxattr("/datadrive/", NULL, 0) = 17
llistxattr("/datadrive/", "security.selinux\0", 17) = 17
lgetxattr("/datadrive/", "security.selinux", NULL, 0) = 27
lgetxattr("/datadrive/", "security.selinux", "system_u:object_r:var_t:s0", 27) = 27
openat(AT_FDCWD, "/datadrive/", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
newfstatat(3, "", {st_mode=S_IFDIR|0755, st_size=4096, ...}, AT_EMPTY_PATH) = 0
getdents64(3, 0x598099e93220 /* 5 entries */, 32768) = 136
getdents64(3, 0x598099e93220 /* 0 entries */, 32768) = 0
close(3) = 0
statx(AT_FDCWD, "/datadrive/vm", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0755, stx_size=4096, ...}) = 0
llistxattr("/datadrive/vm", NULL, 0) = 17
llistxattr("/datadrive/vm", "security.selinux\0", 17) = 17
lgetxattr("/datadrive/vm", "security.selinux", NULL, 0) = 37
lgetxattr("/datadrive/vm", "security.selinux", "system_u:object_r:libvirt_state_"..., 37) = 37
statx(AT_FDCWD, "/datadrive/lost+found", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0700, stx_size=16384, ...}) = 0
llistxattr("/datadrive/lost+found", NULL, 0) = 17
llistxattr("/datadrive/lost+found", "security.selinux\0", 17) = 17
lgetxattr("/datadrive/lost+found", "security.selinux", NULL, 0) = 27
lgetxattr("/datadrive/lost+found", "security.selinux", "system_u:object_r:var_t:s0", 27) = 27
statx(AT_FDCWD, "/datadrive/christian", AT_STATX_SYNC_AS_STAT|AT_SYMLINK_NOFOLLOW, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFDIR|0755, stx_size=4096, ...}) = 0
llistxattr("/datadrive/christian", NULL, 0) = 17
llistxattr("/datadrive/christian", "security.selinux\0", 17) = 17
lgetxattr("/datadrive/christian", "security.selinux", NULL, 0) = 27
lgetxattr("/datadrive/christian", "security.selinux", "system_u:object_r:var_t:s0", 27) = 27
b48f210
to
8f52a85
Compare
Thanks for reviewing.
I briefly tried to run a vagrant virtual machine with a libvirt backend, but it didn't seem to work out of the box. For me it would be easiest if the integration tests are enabled in the GitHub CI. |
8f52a85
to
8605532
Compare
I didn’t know Vagrant could use libvirt; I’ve successfully used it with Virtualbox. But these tests should obviously be in the CI in the mid/longterm (I had COVID so couldn’t really progress much on that front, anyway). |
8605532
to
59493ac
Compare
59493ac
to
e49acdb
Compare
Hello! What's the status on this? Would love to see this merged. Is there any way I can help? |
e49acdb
to
1d15fba
Compare
I would also love to see this merged if there are no blockers. |
Add a command line option -Z/--context to show the security context of objects, similar to ls(1). Show the actual extended attribute values on -@/--extended, instead of just their length. In case of a symbolic link, show the extended attributes of the symbolic link itself, not the target. This matches the behavior of ls(1) and is more intuitive. Closes: #254
1d15fba
to
afeac47
Compare
Closing this since exa is unmaintained (see #1243), and this has been done in the active fork eza. Thanks anyway! |
Add a command line option
-Z/--context
to show the security context of objects, similar to ls(1).Show the actual extended attribute values on
-@/--extended
, instead of just their length.In case of a symlink, show the extended attributes of the symlink itself, not the target.
This matches the behavior of ls(1) and is more intuitive.
TODO:
macos
(should compile)In the future one might want to add support for SMACK security labels (
"security.SMACK64"
).Closes: #254 #613