chcon: added implementation and integration tests

[koutheir]

The ToDo list was updated to mark chcon as done.

Building and testing chcon requires enabling the feat_selinux feature. If make is used for building, then please specify SELINUX_ENABLED=1 if building and testing on a system where SELinux is not enabled.

Closes #1041

[jhscheer]

I think some of the failing checks are due to the test environment lacking the packages libselinux1 and libselinux1-dev.

[jhscheer]

Running tests without root I get:

thread 'test_chcon::user_role_range_type' panicked at 'assertion failed: `(left == right)`
  left: `Some("unconfined_u:object_r:user_tmp_t:s0")`,
 right: `None`', tests/by-util/test_chcon.rs:336:5

Running with root privileges I also get:

run: target/debug/coreutils chcon --verbose --reference=/dev/null /dev/null
thread 'test_chcon::reference_errors' panicked at 'Command was expected to fail.
stdout = 
 stderr = chcon: Changing security context of: /dev/null
', tests/common/util.rs:167:13

Running with root privileges and --include-ignored all previously ignored tests fail, most of them with this panic:

---- test_chcon::user_change stdout ----                                                                                                      
current_directory_resolved:                                                                                                                        
touch: /tmp/.tmp4IdUx8/a.tmp                                                                                                                       
thread 'test_chcon::user_change' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invali
d argument" }', tests/by-util/test_chcon.rs:394:75

[jhscheer]

I saw that you're the author of selinux-sys and used it for this PR.

Since this crate uses raw Rust bindings for libselinux it allows direct access to functions and other symbols in the library via unsafe, but having to use unsafe blocks everywhere in application code is not very ergonomic.
The better approach for uutils/coreutils would be an additional intermediate crate with a library to wrap those functions and make them safe for normal use.

In addition to other applications directly related to selinux, i.e. runcon, there are other uutils, e.g. ls, cp, stat, etc. that are waiting for selinux support for their -Z flag. I also know of other projects, e.g. exa or fd that would benefit from a safe selinux library.

Would you be interested in also writing the selinux-rs crate?

[koutheir]

I think some of the failing checks are due to the test environment lacking the packages libselinux1 and libselinux1-dev.

Can you provide some pointers on how to fix that in the CI build?

[koutheir]

Running tests without root I get:

thread 'test_chcon::user_role_range_type' panicked at 'assertion failed: `(left == right)`
  left: `Some("unconfined_u:object_r:user_tmp_t:s0")`,
 right: `None`', tests/by-util/test_chcon.rs:336:5

Running with root privileges I also get:

run: target/debug/coreutils chcon --verbose --reference=/dev/null /dev/null
thread 'test_chcon::reference_errors' panicked at 'Command was expected to fail.
stdout = 
 stderr = chcon: Changing security context of: /dev/null
', tests/common/util.rs:167:13

Running with root privileges and --include-ignored all previously ignored tests fail, most of them with this panic:

---- test_chcon::user_change stdout ----                                                                                                      
current_directory_resolved:                                                                                                                        
touch: /tmp/.tmp4IdUx8/a.tmp                                                                                                                       
thread 'test_chcon::user_change' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invali
d argument" }', tests/by-util/test_chcon.rs:394:75

Interesting, so SELinux is actually enabled and enforcing on some CI build server. That is something I didn't expect. I'll fix that. Good catch!

[koutheir]

I saw that you're the author of selinux-sys and used it for this PR.

I made it to support this pull request.

Since this crate uses raw Rust bindings for libselinux it allows direct access to functions and other symbols in the library via unsafe, but having to use unsafe blocks everywhere in application code is not very ergonomic.

I agree, and I dislike unsafe.

The better approach for uutils/coreutils would be an additional intermediate crate with a library to wrap those functions and make them safe for normal use.

True. However, my goal was to enable a fast and reasonable contribution to this project. Providing an ergonomic wrapper is a more involved work which covers way more than the API used by chcon.

In addition to other applications directly related to selinux, i.e. runcon, there are other uutils, e.g. ls, cp, stat, etc. that are waiting for selinux support for their -Z flag. I also know of other projects, e.g. exa or fd that would benefit from a safe selinux library.

Thank you for this resume of potential dependencies. You're right, such a crate is needed in more than a few places.

Would you be interested in also writing the selinux-rs crate?

I'm interested in writing it. But when I started exploring the SELinux API/headers, I found out that roughly 90% of the SELinux API is irrelevant to chcon, so it would be an overkill for contributing the chcon rewrite.

I suggest we integrate this pull request depending on the selinux-sys crate, while I start developing an ergonomic selinux crate which should cover a reasonable chunk of the SELinux API. Once the latter crate is good enough for the needs of chcon, I will update the code of chcon in order to use the new ergonomic crate, thus reducing the amount of unsafe code in chcon. Once that crate later covers more APIs, I would also consider using it from other projects that need access to SELinux.

[jhscheer]

I think some of the failing checks are due to the test environment lacking the packages libselinux1 and libselinux1-dev.

Can you provide some pointers on how to fix that in the CI build?

Someone with access needs to run apt install <packagename>
Also note that the package names are just a guess, on my local fedora it was dnf install libselinux-devel

[koutheir]

Running with root privileges and --include-ignored all previously ignored tests fail, most of them with this panic:

---- test_chcon::user_change stdout ----                                                                                                      
current_directory_resolved:                                                                                                                        
touch: /tmp/.tmp4IdUx8/a.tmp                                                                                                                       
thread 'test_chcon::user_change' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }', tests/by-util/test_chcon.rs:394:75

I didn't expect this behavior, and I don't know how to explain it as of now. I'll need to debug this. Could you provide more information on the host running the test, so that I try to reproduce the issue locally?

[jhscheer]

Running with root privileges and --include-ignored all previously ignored tests fail, most of them with this panic:

---- test_chcon::user_change stdout ----                                                                                                      
current_directory_resolved:                                                                                                                        
touch: /tmp/.tmp4IdUx8/a.tmp                                                                                                                       
thread 'test_chcon::user_change' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }', tests/by-util/test_chcon.rs:394:75

I didn't expect this behavior, and I don't know how to explain it as of now. I'll need to debug this. Could you provide more information on the host running the test, so that I try to reproduce the issue locally?

Fedora 5.11.11-200.fc33.x86_64
Let me know if I can assist somehow.

[koutheir]

I'll try to run it in a VM. LXC containers require configuring the kernel to use SELinux inside them. Did you perform explicit steps to enable SELinux, or was it enabled by default?

[sylvestre]

Kudos for the https://github.com/koutheir/selinux-sys project :)

[koutheir]

I fixed the integration tests. Most of the tests are still ignored by default. On systems without SELinux configured, the ignored tests need to run as root in order to have a chance to succeed. On systems with SELinux configured (e.g., Fedora 33), the ignored tests run fine on a normal user.

[koutheir]

As you know, SELinux is Linux-specific, so the command and tests should not be built for other systems. What should I modify in order to achieve this?

[sylvestre]

Do like:

coreutils/tests/by-util/test_who.rs

Line 4 in 4873c8a

#[cfg(target_os = "linux")]

[koutheir]

I need a way to avoid building the chcon binary altogether when building for non-Linux systems. Conditional compilation only allows dropping sections of code, not a whole sub-project.

[jhscheer]

I need a way to avoid building the chcon binary altogether when building for non-Linux systems. Conditional compilation only allows dropping sections of code, not a whole sub-project.

In cargo.toml you put chcon in feat_require_unix. Maybe it's better to create a new feature for every uutil related to selinx. Similar to the feat_require_unix_utmpx feature.
Then it should be possible to run cargo build, e.g. cargo build --release --features unix_require_selinx

[koutheir]

Why not make a feat_require_linux instead? This would make linux a platform of its own (just like MacOS), instead of making it a special unix.

[jhscheer]

Why not make a feat_require_linux instead? This would make linux a platform of its own (just like MacOS), instead of making it a special unix.

You asked for a way to be able to separately build chcon. As far as I can tell, that can be achieved by not putting chcon in feat_require_unix. The rest was just an example to make it clearer what I mean.

I looked at the features some more. Based on what is already there it would make sense to have:

linux = [ "feat_os_linux" ]
feat_os_linux = [
    "feat_require_linux_selinux",
]
feat_require_linux_selinux = [
    "chcon",
]

(Also TIL that currently, macos is feat_os_macos and that's just feat_os_unix.)

[sylvestre]

great start, not reviewed for now but some first comments

  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Failure { command: "\"pkg-config\" \"--libs\" \"--cflags\" \"libselinux\"", output: Output { status: ExitStatus(ExitStatus(256)), stdout: "", stderr: "Package libselinux was not found in the pkg-config search path.\nPerhaps you should add the directory containing `libselinux.pc\'\nto the PKG_CONFIG_PATH environment variable\nNo package \'libselinux\' found\n" } }', /cargo/registry/src/github.com-1ecc6299db9ec823/selinux-sys-0.3.1/build.rs:18:61

you should install the missing package, no ?
should be done in
https://github.com/uutils/coreutils/blob/master/.github/workflows/CICD.yml

[koutheir]

you should install the missing package, no ?
should be done in
https://github.com/uutils/coreutils/blob/master/.github/workflows/CICD.yml

Thank you for the hint. I added the necessary apt-get install libselinux1-dev. However, it looks like pkg-config fails to find the libselinux.pc, even though the libselinux1-dev package, as reported by Ubuntu [1, 2], contains that file.

References:

• [1] File list of libselinux1-dev in Ubuntu 20.04 LTS on amd64.
• [2] File list of libselinux1-dev in Ubuntu 18.04 LTS on amd64.

I'll try the operation in a container to fix the issue.

[koutheir]

The problem seems to be the implicit docker images that the cross tool uses. These ones don't have libselinux1-dev installed. Any ideas how to customize these images in order to install the missing package?

[sylvestre]

@koutheir don't know, sorry :) did you look at the github action doc? thanks

[koutheir]

It seems pkg-config does not support cross-compilation, and that's what the selinux-sys crate relies on to locate libselinux. I'll try to modify the selinux-sys crate in order to use another method to locate the library.

[koutheir]

Given that testing is driven by the cross tool, and that the selinux-sys crate requires the libselinux1-dev package to be installed on the testing container, the testing scenarios need to create one custom container for each tested platform, based on the cross base containers rustembedded/cross:*. These custom containers would then be used by cross in order to build and run tests in a way that allows building the selinux-sys crate.

I experimented doing this for a few platforms, and it seems to work fine. For example, targeting the aarch64-unknown-linux-gnu, here is what I did:

• Create the file Cross.toml with contents like this:

[build.env]
passthrough = [
    "RUST_BACKTRACE",
    # ...
]

[target.aarch64-unknown-linux-gnu]
image = "my/cross-with-selinux:aarch64-unknown-linux-gnu-0.2.1"
# ...

• Create the file docker/aarch64-unknown-linux-gnu/Dockerfile with contents like this:

FROM rustembedded/cross:aarch64-unknown-linux-gnu-0.2.1

RUN dpkg --add-architecture arm64 && \
    apt-get update && \
    apt-get upgrade --assume-yes && \
    apt-get install --assume-yes --no-install-recommends build-essential clang-8 libclang-8-dev llvm-8-dev libselinux1-dev:arm64 && \
    ln -s /usr/bin/clang-8 /usr/local/bin/clang

• Create a custom container where libselinux1-dev is installed:

$ docker build -t 'my/cross-with-selinux:aarch64-unknown-linux-gnu-0.2.1' 'docker/aarch64-unknown-linux-gnu'

• Execute tests as follows:

$ env RUST_BACKTRACE=1 cross test --target aarch64-unknown-linux-gnu

This works for some target platforms, and doesn't for others, for different reasons related to the configuration of cross base containers.

I have no idea how complicated this is within the build/test architecture of this project. Can someone help with this?

The reason why the selinux-sys crate requires the libselinux1-dev package to be installed on the testing container, instead of simply building libselinux statically from sources, is that SELinux is a technology tightly integrated into the system, so that building a different version/variant of libselinux and using it on a system configured with a different version may lead to all kinds of hard-to-debug problems.

[sylvestre]

@koutheir are you still interested by finishing this PR? thanks

[koutheir]

@koutheir are you still interested by finishing this PR? thanks

If no one can help me resolve the infrastructure issues this PR faces, then I cannot proceed alone.

[tertsdiepraam]

@sylvestre Can we install libselinux-dev on the Linux containers? More SELinux requests are popping up (#2404, #1809), so it might be a nice time to do this?

[koutheir]

@sylvestre Can we install libselinux-dev on the Linux containers? More SELinux requests are popping up (#2404, #1809), so it might be a nice time to do this?

SELinux needs to be supported and configured on a kernel level. AFAIK, running it inside containers is not a supported configuration. I personally use QEMU to run the tests of a crate I'm currently developing exposing the libselinux API.

[tertsdiepraam]

Ah, I see. That makes it more complicated. Is there maybe some way to mock it?

[koutheir]

With the state of SELinux distributed over the kernel, the file system, and the libselinux memory, I suspect it would be difficult to faithfully mock its APIs. If you have some easier approach, then I'm open to try it.

[sylvestre]

I updated the PR to reflect recent changes to this repository. It's been 3 months since I created the PR. Is the feature I'm adding still interesting? Or should I abort this request?

Yeah, it is clearly super interesting and I have been monitoring your PR. However, we cannot land it until it is green.
Maybe we should limit the feature of this program to make it run in the CI?

[koutheir]

Yeah, it is clearly super interesting and I have been monitoring your PR.

Thank you for confirming this work is still useful to someone.

However, we cannot land it until it is green.

I understand, and I was trying for some time to get it to work. However, that's proving difficult due to the nature of libselinux, and its dependency, not only on the Linux kernel configuration, but also on the file system features, and the container configuration.

Maybe we should limit the feature of this program to make it run in the CI?

I was trying to be faithful to the original GNU implementation of chcon, and I didn't want to introduce artificial limitations of this project's implementation due to limitations of the CI servers. Given your comment, it looks like I was following a different approach than yours.

I can go back and try to introduce a very limited version of chcon (initially, probably just an empty shell which throws unimplemented!() everywhere) and then add features progressively until something useful comes out of it, or the same blocking issues come back again, leaving the shell mostly empty... I'm not convinced this is the best approach, but if that's the direction of this project, then I can probably do something about it. Notice that going with this approach will disallow me from depending on the selinux crate and reusing its ergonomic bindings, which will result in more unsafe code than strictly necessary.

@sylvestre, please confirm that this approach makes sense to you.

[tertsdiepraam]

Maybe if we can't run the tests on the CI in the near future, we should just skip the tests on the CI and merge this? It's not ideal, but at least there are tests we can run locally

[jhscheer]

Maybe we should limit the feature of this program to make it run in the CI?

I can go back and try to introduce a very limited version of chcon

I don't like the idea of a watered down version specifically tailored to run on the docker image.

Maybe if we can't run the tests on the CI in the near future, we should just skip the tests on the CI and merge this? It's not ideal, but at least there are tests we can run locally

That's also what I was thinking, however the problem is that (at the moment) this can't be build inside a docker image.
I think, until we have a solution for CICD, we should should simply decouple this and the other Security Context features (-Z) from the CICD and introduce a new feature gate (similar to what is used for musl, utmpx, etc).
That's a quick solution and less difficult than further tinkering with the docker images.

[koutheir]

I think, until we have a solution for CICD, we should should simply decouple this and the other Security Context features (-Z) from the CICD and introduce a new feature gate (similar to what is used for musl, utmpx, etc).

@jhscheer, could you elaborate on how to implement this solution?

[koutheir]

Is it possible to raise the minimum supported Rust version? Or do we have to do whatever it takes to keep the current MSRV?

[koutheir]

The approach used to collect coverage information hits this bug #78011 due to setting RUSTFLAGS=... -Cpanic=abort. The recommended workaround is currently using a dedicated coverage profile which specifies rustc flags via Cargo.toml instead of RUSTFLAGS environment variable.

This bug is hit due to the usage of bindgen by the selinux-sys crate. This bug causes the compiled build script of selinux-sys to crash with a SIGABRT. Postmortem debugging reveals that the issue happens in bindgen due to usage of proc_macro2 which calls catch_unwind() and causes the whole binary to crash.

[koutheir]

The problem with defining a dedicated coverage profile in Cargo.toml is that cargo complains when building with the stable toolchain. This means we can neither use RUSTFLAGS (crashes) nor a dedicated coverage profile (nightly-only).

We should probably consider a different way to build coverage information. Please consider the approach used by my selinux crate to generate coverage information.

[Funky185540]

Hello there!

If I follow this conversation correctly, @koutheir implemented the selinux crate (cool stuff!) specifically to allow handling of SELinux through rust, but it can't be used because it breaks the CI which runs in a docker container. Is that correct?

I was interested in picking up on #2404 in the near future as SELinux is part of a few of the utils. But without a solution to this discussion I'm afraid there isn't much of a point to that. Is there a chance I could help here?

I think, until we have a solution for CICD, we should should simply decouple this and the other Security Context features (-Z) from the CICD and introduce a new feature gate (similar to what is used for musl, utmpx, etc).
That's a quick solution and less difficult than further tinkering with the docker images.

Is the "decoupling" from CI a separate step from the feature gate, or do these imply each other? I.e. couldn't one add a feature "selinux" and then have the CI build and test explicitly without that for now?

Oh and since there's talk about "workarounds" and "quick solutions" here I wonder if there's some place where one could document all these sorts of things? Something like a documentation or a project-wide TODO, or should we just open issues for this sort of thing?

[jhscheer]

If I follow this conversation correctly, @koutheir implemented the selinux crate (cool stuff!) specifically to allow handling of SELinux through rust, but it can't be used because it breaks the CI which runs in a docker container. Is that correct?

From everything I read so far it is not possible to have SELinux support for CICD github actions.
The containers would need to run on a Kernel that supports SELinux, this is not the case for Ubuntu out of the box. (There are threads asking github for VMs with Fedora, etc. instead of Ubuntu, but the official response from github is that they won't provide it and suggest to use a self hosted runner instead.)
Even if we achieve to build the selinux crate in the CI (potential solution outlined here), we couldn't run and test the selinux features in the CI container.

I think, until we have a solution for CICD, we should should simply decouple this and the other Security Context features (-Z) from the CICD and introduce a new feature gate (similar to what is used for musl, utmpx, etc).

could you elaborate on how to implement this solution?

see below

Is the "decoupling" from CI a separate step from the feature gate, or do these imply each other? I.e. couldn't one add a feature "selinux" and then have the CI build and test explicitly without that for now?

That is precisely what I had in mind and I implemented this here last night for uu_id.

Now id can be compiled and tested with and without the selinux crate like this:

cargo run --features feat_selinux -- id -Z
cargo run --features "id" -- id
cargo build -p uu_id --no-default-features --features feat_selinux
cargo build -p uu_id --no-default-features
cargo test test_id --features feat_selinux
cargo test test_id --features id --no-default-features

The feat_selinux approach can be adopted in this PR as well as other uutils when adding selinux support (-Z, etc).

[Funky185540]

@jhscheer and @koutheir

I think that this deserves a Wiki page so other contributors can find it and the coding is somewhat uniform across utilities, what do you think?

I've prepared something, maybe you want to add to it? (If it is of interest to the project)

selinux-wikipage.md

[koutheir]

I think that this deserves a Wiki page so other contributors can find it and the coding is somewhat uniform across utilities, what do you think?

I agree. I'll contribute to the wiki page to document my experience with integrating SELinux, in the hope it eases the life of future contributors.

I've prepared something, maybe you want to add to it? (If it is of interest to the project)

Here is my take on it: selinux-wikipage.md

[tertsdiepraam]

That wikipage looks good to me. With approval from @sylvestre, I'll add it to the wiki. Too bad there is no PR-like workflow for the wiki...

[sylvestre]

1.53.0 is a significant bump. there isn't older version that we could use?

[jhscheer]

1.53.0 is a significant bump. there isn't older version that we could use?

I'm aware of the following dependencies (for compiling selinux-sys 0.4.2):

1. Traits on larger arrays from rust 1.47.0
2. std::slice::strip_prefix from rust 1.51.0

I'm in favour for a bump to 1.47, I think a workaround for strip_prefix is reasonable.

[sylvestre]

if we can update it to 1.47, i would accept this PR now :)

[koutheir]

1.53.0 is a significant bump. there isn't older version that we could use?

I was attempting to get the CI to o work somehow, and that 1.53.0 was not a specific choice; it simply represented latest stable. I'll revert the MSRV to 1.47.0.

I'm waiting for any other PR to handle the issue of depending on SELinux and still please the CI servers somehow. Once that is done, I'll modify this PR to handle the issue in a similar fashion. Then you might consider reviewing this PR again.

[sylvestre]

@koutheir Other PR approved! :)

[koutheir]

@sylvestre, could you please review?

I executed the tests for chcon on both RockyLinux and Fedora virtual machines, and they all pass.

[koutheir]

FYI, I'm done developing this PR. I'm only continuing to rebase it over master to simplify merging once the PR is approved. I hope this does not cause any confusion.

[koutheir]

Manual testing successful:

$ cargo test --features feat_selinux test_chcon
    Finished test [unoptimized + debuginfo] target(s) in 0.36s
     Running unittests (target/debug/deps/coreutils-020f2123956982ed)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/tests.rs (target/debug/deps/tests-c40d15ceacb2151f)

running 18 tests
test test_chcon::range_change ... ok
test test_chcon::role_change ... ok
test test_chcon::help ... ok
test test_chcon::reference_errors ... ok
test test_chcon::type_change ... ok
test test_chcon::user_role_range_type ... ok
test test_chcon::user_change ... ok
test test_chcon::valid_context ... ok
test test_chcon::valid_context_directory_recursive ... ok
test test_chcon::recursive_errors ... ok
test test_chcon::valid_context_directory ... ok
test test_chcon::valid_context_directory_recursive_follow_all_symlinks ... ok
test test_chcon::valid_context_directory_recursive_follow_args_dir_symlinks ... ok
test test_chcon::valid_context_on_broken_symlink ... ok
test test_chcon::valid_context_on_valid_symlink ... ok
test test_chcon::valid_reference ... ok
test test_chcon::valid_context_with_prior_xattributes ... ok
test test_chcon::version ... ok

test result: ok. 18 passed; 0 failed; 0 ignored; 0 measured; 1303 filtered out; finished in 0.05s

[koutheir]

Now that this PR is merged, I'll try to reduce the amount of unsafe code in this implementation.

[sylvestre]

bravo for your hard work on this btw :)

[koutheir]

@sylvestre, @Funky185540, can someone insert the document selinux-wikipage.md in the Wiki? I don't know if I'm allowed to do that.

[koutheir]

This implementation continues in #2563.

[sylvestre]

@koutheir https://github.com/uutils/coreutils/wiki/Supporting-SELinux-in-the-coreutils added, sorry for the latency