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

core: wrap and intercept groupadd calls in scriptlets #3778

Merged
merged 1 commit into from
Jul 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rust/src/builtins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@

pub(crate) mod apply_live;
pub(crate) mod compose;
pub mod scriptlet_intercept;
pub mod usroverlay;
162 changes: 162 additions & 0 deletions rust/src/builtins/scriptlet_intercept/groupadd.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
//! CLI handler for intercepted `groupadd`.

// SPDX-License-Identifier: Apache-2.0 OR MIT

use anyhow::{anyhow, Result};
use cap_std::fs::{Dir, Permissions};
use cap_std_ext::prelude::CapStdExtDirExt;
use clap::{Arg, Command};
use std::io::Write;
use std::os::unix::prelude::PermissionsExt;

/// Entrypoint for (the rpm-ostree implementation of) `groupadd`.
pub(crate) fn entrypoint(args: &[&str]) -> Result<()> {
fail::fail_point!("intercept_groupadd_ok", |_| Ok(()));

// This parses the same CLI surface as the real `groupadd`,
// but in the end we only extract the group name and (if
// present) the static GID.
let matches = cli_cmd().get_matches_from(args);
let gid = matches
.value_of("gid")
.map(|s| s.parse::<u32>())
.transpose()?;
let groupname = matches
.value_of("groupname")
.ok_or_else(|| anyhow!("missing required groupname argument"))?;
if !matches.is_present("system") {
eprintln!("Trying to create non-system group '{groupname}'; this will become an error in the future.");
}

let rootdir = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
generate_sysusers_fragment(&rootdir, groupname, gid)?;

Ok(())
}

/// CLI parser, matches <https://linux.die.net/man/8/groupadd>.
fn cli_cmd() -> Command<'static> {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, shouldn't we error out if any of the switches we don't actually support are used? And I guess one way to do that is to just drop those switches from the clap definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely yes. I'll do that in a followup PR together with enforcing the --system flag, as soon as I've verified that all packages in the FCOS base image are fine.

let name = "groupadd";
Command::new(name)
.bin_name(name)
.about("create a new group")
.arg(Arg::new("force").short('f').long("force"))
.arg(
Arg::new("help")
.short('h')
.long("help")
.action(clap::ArgAction::Help),
)
.arg(Arg::new("gid").short('g').long("gid").takes_value(true))
.arg(Arg::new("key").short('K').long("key").takes_value(true))
.arg(Arg::new("allow_duplicates").short('o').long("non-unique"))
.arg(
Arg::new("password")
.short('p')
.long("password")
.takes_value(true),
)
.arg(Arg::new("system").short('r').long("system"))
Copy link
Member

Choose a reason for hiding this comment

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

I think we should either:

  • Error if this flag isn't passed
  • No-op if this flag isn't passed

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I think longer term we'd want to error out.
I'm not sure if we currently have scriptlets not using -r, so in the short term I'd just log a warning and proceed with the logic, so that we can adjust affected packages (if any).

.arg(
Arg::new("chroot_dir")
.short('R')
.long("root")
.takes_value(true),
)
.arg(
Arg::new("prefix_dir")
.short('P')
.long("prefix")
.takes_value(true),
)
.arg(Arg::new("users").short('U').long("users").takes_value(true))
.arg(Arg::new("groupname").required(true))
}

/// Write a sysusers.d configuration fragment for the given group.
///
/// This returns whether a new fragment has been actually written
/// to disk.
fn generate_sysusers_fragment(rootdir: &Dir, groupname: &str, gid: Option<u32>) -> Result<bool> {
static SYSUSERS_DIR: &str = "usr/lib/sysusers.d";

// The filename of the configuration fragment is in fact a public
// API, because users may have masked it in /etc. Do not change this.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Why would someone do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be one approach for handling users/groups mismatches like coreos/fedora-coreos-tracker#1201 (either as a transitory measure or a permanent override).

let filename = format!("30-pkg-group-{groupname}.conf");
Copy link
Member

Choose a reason for hiding this comment

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

For tmpfiles snippets we derive the filename from the package name - I think we could do that here by passing it through the environment.

I don't have a strong opinion on that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered doing that, I explored the pkgname-through-env approach and it's indeed feasible.

I'm still oscillating on the file-naming decision though.
My current stance is that having {groupname} in the file helps with idempotence, as a single package may be doing multiple groupadd. Current naming convention avoid interferences and allows for atomic-replaces without parsing/merging.
One third option I considered is the {pkgname}-{groupname} format, but I didn't see many additional benefits in doing that.

I'm open to more brainstorming on this topic. /cc @jlebon.

Copy link
Member

Choose a reason for hiding this comment

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

My current stance is that having {groupname} in the file helps with idempotence, as a single package may be doing multiple groupadd.

Makes sense to me!

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, looking at /usr/lib/sysusers.d in my local FSB and FCOS, I don't see any config using priority prefixes in their names (except Zincati). So even with 30-, we'd be at the top of the list.

One third option I considered is the {pkgname}-{groupname} format, but I didn't see many additional benefits in doing that.

That'd be my vote. I think the additional benefit of being able to tell which package we derived from is nice (given that we don't currently include the package name in the generated fragment either). It would also better conform to the guideline established in sysusers.d(5):

Each configuration file shall be named in the style of package.conf or package-part.conf. The second variant should be used when it is desirable to make it easy to override just this part of configuration.

(Though it's a bit fuzzy here because we're doing it for them, hehe, but it's in the same spirit I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it looks like there is consensus about adding the package name in the filename. Sorry I didn't see the last comment here before rebasing, I'll wire the pkgname in a followup.

Regarding the numerical prefix and suggested naming pattern, there has been some friction reported upstream already: systemd/systemd#21155

In our case, I already know we'd need to properly priority-sort several sources with (possibly conflicting) definitions:

While we aren't strictly following the suggested pattern from the docs, the numerical prefix at least allows predictable sorting/overriding which is helpful in the above case of multiple/conflicting sources of truth.


rootdir.create_dir_all(SYSUSERS_DIR)?;
let conf_dir = rootdir.open_dir(SYSUSERS_DIR)?;
if conf_dir.exists(&filename) {
return Ok(false);
}

let gid_value = gid
.map(|id| id.to_string())
.unwrap_or_else(|| "-".to_string());
conf_dir.atomic_replace_with(&filename, |fragment| -> Result<()> {
let perms = Permissions::from_mode(0o644);
fragment.get_mut().as_file_mut().set_permissions(perms)?;

fragment.write_all(b"# Generated by rpm-ostree\n")?;
let entry = format!("g {groupname} {gid_value}\n");
fragment.write_all(entry.as_bytes())?;

Ok(())
})?;

Ok(true)
}

#[cfg(test)]
mod test {
use super::*;
use std::io::Read;

#[test]
fn test_clap_cmd() {
cli_cmd().debug_assert();

let cmd = cli_cmd();
let static_gid = ["/usr/sbin/groupadd", "-g", "23", "squid"];
let matches = cmd.try_get_matches_from(static_gid).unwrap();
assert_eq!(matches.value_of("gid"), Some("23"));
assert_eq!(matches.value_of("groupname"), Some("squid"));

let cmd = cli_cmd();
let dynamic_gid = ["/usr/sbin/groupadd", "-r", "chrony"];
let matches = cmd.try_get_matches_from(dynamic_gid).unwrap();
assert!(matches.contains_id("system"));
assert_eq!(matches.value_of("gid"), None);
assert_eq!(matches.value_of("groupname"), Some("chrony"));

let err_cases = [vec!["/usr/sbin/groupadd"]];
for input in err_cases {
let cmd = cli_cmd();
cmd.try_get_matches_from(input).unwrap_err();
}
}

#[test]
fn test_fragment_generation() {
let tmpdir = cap_tempfile::tempdir(cap_tempfile::ambient_authority()).unwrap();

let groups = [
("foo", Some(42), true, "42"),
("foo", None, false, "42"),
("bar", None, true, "-"),
];
for entry in groups {
let generated = generate_sysusers_fragment(&tmpdir, entry.0, entry.1).unwrap();
assert_eq!(generated, entry.2, "{:?}", entry);

let path = format!("usr/lib/sysusers.d/30-pkg-group-{}.conf", entry.0);
assert!(tmpdir.is_file(&path));

let mut fragment = tmpdir.open(&path).unwrap();
let mut content = String::new();
fragment.read_to_string(&mut content).unwrap();
let expected = format!("# Generated by rpm-ostree\ng {} {}\n", entry.0, entry.3);
assert_eq!(content, expected)
}
}
}
49 changes: 49 additions & 0 deletions rust/src/builtins/scriptlet_intercept/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
//! CLI handler for `rpm-ostree scriplet-intercept`.

// SPDX-License-Identifier: Apache-2.0 OR MIT

mod groupadd;
use anyhow::{bail, Result};

/// Entrypoint for `rpm-ostree scriplet-intercept`.
pub fn entrypoint(args: &[&str]) -> Result<()> {
// Here we expect arguments that look like
// `rpm-ostree scriptlet-intercept <command> -- <rest>`
if args.len() < 4 || args[3] != "--" {
bail!("Invalid arguments");
}

let orig_command = args[2];
let rest = &args[4..];
match orig_command {
"groupadd" => groupadd::entrypoint(rest),
x => bail!("Unable to intercept command '{}'", x),
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_entrypoint_args() {
// Short-circuit groupadd logic, this test is only meant to check CLI parsing.
let _guard = fail::FailScenario::setup();
fail::cfg("intercept_groupadd_ok", "return").unwrap();

let err_cases = [
vec![],
vec!["rpm-ostree", "install"],
vec!["rpm-ostree", "scriptlet-intercept", "groupadd"],
vec!["rpm-ostree", "scriptlet-intercept", "foo", "--"],
];
for input in &err_cases {
entrypoint(input).unwrap_err();
}

let ok_cases = [vec!["rpm-ostree", "scriptlet-intercept", "groupadd", "--"]];
for input in &ok_cases {
entrypoint(input).unwrap();
}
}
}
38 changes: 32 additions & 6 deletions rust/src/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ const SSS_CACHE_PATH: &str = "usr/sbin/sss_cache";
const SYSTEMCTL_PATH: &str = "usr/bin/systemctl";
const SYSTEMCTL_WRAPPER: &[u8] = include_bytes!("../../src/libpriv/systemctl-wrapper.sh");

// Intercept `groupadd` for automatic sysusers.d fragment generation.
const GROUPADD_PATH: &str = "usr/sbin/groupadd";
const GROUPADD_WRAPPER: &[u8] = include_bytes!("../../src/libpriv/groupadd-wrapper.sh");

const RPMOSTREE_CORE_STAGED_RPMS_DIR: &str = "rpm-ostree/staged-rpms";

pub(crate) const OSTREE_BOOTED: &str = "/run/ostree-booted";
Expand Down Expand Up @@ -132,8 +136,10 @@ pub(crate) fn log_treefile(tf: &crate::treefile::Treefile) {
impl FilesystemScriptPrep {
/// Filesystem paths that we rename out of the way if present
const OPTIONAL_PATHS: &'static [&'static str] = &[SSS_CACHE_PATH];
const REPLACE_OPTIONAL_PATHS: &'static [(&'static str, &'static [u8])] =
&[(SYSTEMCTL_PATH, SYSTEMCTL_WRAPPER)];
const REPLACE_OPTIONAL_PATHS: &'static [(&'static str, &'static [u8])] = &[
(GROUPADD_PATH, GROUPADD_WRAPPER),
(SYSTEMCTL_PATH, SYSTEMCTL_WRAPPER),
];

fn saved_name(name: &str) -> String {
format!("{}.rpmostreesave", name)
Expand Down Expand Up @@ -360,19 +366,39 @@ mod test {
d.ensure_dir_with("usr/bin", &db)?;
d.ensure_dir_with("usr/sbin", &db)?;
d.atomic_write_with_perms(super::SSS_CACHE_PATH, "sss binary", mode.clone())?;
let original_systemctl = "original systemctl";
d.atomic_write_with_perms(super::SYSTEMCTL_PATH, original_systemctl, mode.clone())?;
// Replaced systemctl
// Neutered sss_cache.
{
assert!(d.try_exists(super::SSS_CACHE_PATH)?);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
assert!(!d.try_exists(super::SSS_CACHE_PATH)?);
g.undo()?;
assert!(d.try_exists(super::SSS_CACHE_PATH)?);
}
// Replaced systemctl.
{
let original_systemctl = "original systemctl";
d.atomic_write_with_perms(super::SYSTEMCTL_PATH, original_systemctl, mode.clone())?;
let contents = d.read_to_string(super::SYSTEMCTL_PATH)?;
assert_eq!(contents, original_systemctl);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let contents = d.read_to_string(super::SYSTEMCTL_PATH)?;
assert_eq!(contents.as_bytes(), super::SYSTEMCTL_WRAPPER);
g.undo()?;
let contents = d.read_to_string(super::SYSTEMCTL_PATH)?;
assert_eq!(contents, original_systemctl);
assert!(d.try_exists(super::SSS_CACHE_PATH)?);
}
// Replaced groupadd.
{
let original_groupadd = "original groupadd";
d.atomic_write_with_perms(super::GROUPADD_PATH, original_groupadd, mode.clone())?;
let contents = d.read_to_string(super::GROUPADD_PATH)?;
assert_eq!(contents, original_groupadd);
let g = super::prepare_filesystem_script_prep(d.as_raw_fd())?;
let contents = d.read_to_string(super::GROUPADD_PATH)?;
assert_eq!(contents.as_bytes(), super::GROUPADD_WRAPPER);
g.undo()?;
let contents = d.read_to_string(super::GROUPADD_PATH)?;
assert_eq!(contents, original_groupadd);
}
Ok(())
}
Expand Down
2 changes: 2 additions & 0 deletions rust/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ async fn inner_async_main(args: Vec<String>) -> Result<i32> {
"cliwrap" => rpmostree_rust::cliwrap::entrypoint(args).map(|_| 0),
// The `unlock` is a hidden alias for "ostree CLI compatibility"
"usroverlay" | "unlock" => builtins::usroverlay::entrypoint(args).map(|_| 0),
// A hidden wrapper to intercept some binaries in RPM scriptlets.
"scriptlet-intercept" => builtins::scriptlet_intercept::entrypoint(args).map(|_| 0),
// C++ main
_ => Ok(rpmostree_rust::ffi::rpmostree_main(args)?),
}
Expand Down
2 changes: 2 additions & 0 deletions src/app/libmain.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ static RpmOstreeCommand commands[] = {
/* Rust-implemented commands; they're here so that they show up in `rpm-ostree
* --help` alongside the other commands, but the command itself is fully
* handled Rust side. */
{ "scriptlet-intercept", static_cast<RpmOstreeBuiltinFlags> (RPM_OSTREE_BUILTIN_FLAG_HIDDEN),
Copy link
Member

Choose a reason for hiding this comment

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

Note if the command is hidden, there's no reason to have it here since it won't show up anyway in the --help output.

"Intercept some commands used by RPM scriptlets", NULL },
{ "usroverlay", static_cast<RpmOstreeBuiltinFlags> (RPM_OSTREE_BUILTIN_FLAG_REQUIRES_ROOT),
"Apply a transient overlayfs to /usr", NULL },
/* Legacy aliases */
Expand Down
13 changes: 13 additions & 0 deletions src/libpriv/groupadd-wrapper.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/usr/bin/bash
# Used by rpmostree-core.c to intercept `groupadd` calls.
# We want to learn about group creation and distinguish between
# static and dynamic GIDs, in order to auto-generate relevant
# `sysusers.d` fragments.
# See also https://github.com/coreos/rpm-ostree/issues/3762

if test -v RPMOSTREE_EXP_BRIDGE_SYSUSERS; then
rpm-ostree scriptlet-intercept groupadd -- "$0" "$@"
fi

# Forward to the real `groupadd` for group creation.
exec /usr/sbin/groupadd.rpmostreesave "$@"
4 changes: 4 additions & 0 deletions src/libpriv/rpmostree-scripts.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,10 @@ rpmostree_run_script_in_bwrap_container (int rootfs_fd, GLnxTmpDir *var_lib_rpm_

gboolean debugging_script = g_strcmp0 (g_getenv ("RPMOSTREE_SCRIPT_DEBUG"), pkg_script) == 0;

const char *bridge_sysusers = g_getenv ("RPMOSTREE_EXP_BRIDGE_SYSUSERS");
if (bridge_sysusers != NULL)
bwrap->setenv ("RPMOSTREE_EXP_BRIDGE_SYSUSERS", rust::String (bridge_sysusers));

/* https://github.com/systemd/systemd/pull/7631 AKA
* "systemctl,verbs: Introduce SYSTEMD_OFFLINE environment variable"
* https://github.com/systemd/systemd/commit/f38951a62837a00a0b1ff42d007e9396b347742d
Expand Down