-
Notifications
You must be signed in to change notification settings - Fork 198
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,4 +3,5 @@ | |
|
||
pub(crate) mod apply_live; | ||
pub(crate) mod compose; | ||
pub mod scriptlet_intercept; | ||
pub mod usroverlay; |
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> { | ||
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")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should either:
WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I think longer term we'd want to error out. |
||
.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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. Why would someone do that? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. I'm open to more brainstorming on this topic. /cc @jlebon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, looking at
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
(Though it's a bit fuzzy here because we're doing it for them, hehe, but it's in the same spirit I think). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
} | ||
} |
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(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
"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 */ | ||
|
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 "$@" |
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.
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.
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.
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.