From cfa3073da76dd65304cb5e5717e6e6da8587ebc3 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:42:37 -0300 Subject: [PATCH] refactor(fips): generate the shell policy through a module sub xCAT-test/unit/fips_policy_sources.t ran build-utils/sync-fips-policy as a subprocess, so a file in the unit directory measured a program rather than a routine, and it asserted on the diagnostics the script printed instead of on an answer. buildrpms.pl and builddebs.pl started the same script with system() to check the generated libraries before packaging xCAT or xCAT-genesis-scripts. XCAT::BuildUtils gains fips_policy_artifacts, fips_policy_plan and sync_fips_policy. fips_policy_plan reads the canonical policy and both runtime libraries and reports what generation would write, without writing. sync_fips_policy applies that plan in write mode and returns the stale paths, the paths it rewrote and the diagnostics for its caller to print. The two builders now call sync_fips_policy in their own process and print the diagnostics before they stop, which keeps the output xCAT-test/native/ fips_build_guard.t asserts on. build-utils/sync-fips-policy keeps its --check and --write contract and its exit status, and is now a thin caller. The unit test calls the sub in-process and asserts on the returned hash. It also covers a library holding two policy blocks and a library replaced by a symbolic link, which the subprocess form did not reach. Ten mutants of the module, one per guard, each fail the assertion that names that guard. xCAT-test/bats/fips_policy_artifacts.bats copies XCAT::BuildUtils into its fixture, because the script it runs now loads the module. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- build-utils/lib/XCAT/BuildUtils.pm | 141 +++++++++++++++++- build-utils/sync-fips-policy | 42 +----- builddebs.pl | 7 +- buildrpms.pl | 7 +- xCAT-test/bats/fips_policy_artifacts.bats | 1 + xCAT-test/unit/fips_policy_sources.t | 173 +++++++++++++++------- 6 files changed, 272 insertions(+), 99 deletions(-) diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index e24d6a760..389d4220c 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -17,10 +17,10 @@ use Exporter 'import'; use Cwd (); use File::Copy qw(copy move); use File::Find qw(find); -use File::Basename qw(basename); +use File::Basename qw(basename dirname); use File::Path qw(make_path remove_tree); use File::Temp qw(tempdir tempfile); -use File::Slurper qw(read_text write_text); +use File::Slurper qw(read_binary read_text write_binary write_text); use POSIX qw(strftime); use Sys::Hostname (); use Pod::Usage qw(pod2usage); @@ -48,6 +48,7 @@ our @EXPORT_OK = qw( genesis_chroot_name genesis_target_arch genesis_build_plan genesis_log_errors genesis_log_deny_rules deb_belongs_to_dist genesis_dists genesis_dist_reason + fips_policy_artifacts fips_policy_plan sync_fips_policy ); # Both builders echo the commands they run under --verbose. Set once, after @@ -1275,3 +1276,139 @@ sub prepare_mock_resultdirs { } } + +#------------------------------------------------------------------------------- + +=head3 fips_policy_artifacts + + Descriptions: Report the runtime libraries that carry a generated copy of + the FIPS shell policy, relative to the repository root. + + Arguments: none. + + Returns: the list of relative paths, in generation order. + +=cut + +#------------------------------------------------------------------------------- +sub fips_policy_artifacts { + return ('xCAT-genesis-scripts/usr/lib/xcat/fips.sh', + 'xCAT/postscripts/xcatlib.sh'); +} + +#------------------------------------------------------------------------------- + +=head3 fips_policy_plan + + Descriptions: Read the canonical FIPS shell policy and each runtime + library, and report what generation would write. Reads + only; it writes nothing. + + Arguments: + root - the repository root + + Returns: a hash reference. + block - the delimited policy block generation inserts + artifacts - one hash reference per runtime library, in + generation order, with: relative, path, mode, + current, rendered and stale + + Dies when the policy is empty, does not end with a newline, or + when a library is not a regular file or does not hold exactly + one policy block. + +=cut + +#------------------------------------------------------------------------------- +sub fips_policy_plan { + my (%args) = @_; + my $root = $args{root} // die "fips_policy_plan needs a root\n"; + my $policy = read_binary("$root/build-utils/fips-policy.sh"); + die "FIPS policy is empty\n" unless length $policy; + die "FIPS policy must end with a newline\n" unless $policy =~ /\n\z/; + my $begin = "# BEGIN generated FIPS policy: build-utils/sync-fips-policy --write\n"; + my $end = "# END generated FIPS policy\n"; + my $block = $begin . $policy . $end; + + my @artifacts; + for my $relative (fips_policy_artifacts()) { + my $path = "$root/$relative"; + die "$relative must be a regular file\n" unless -f $path && !-l $path; + my $current = read_binary($path); + my $rendered = $current; + # A second block, or none, makes the insertion point ambiguous. Count + # before substituting: s///g on no match leaves the file unchanged and + # reads as current. + my $count = $rendered =~ s/\Q$begin\E.*?\Q$end\E/$block/sg; + die "$relative must contain one FIPS policy block\n" + unless $count == 1 + && (() = $current =~ /\Q$begin\E/g) == 1 + && (() = $current =~ /\Q$end\E/g) == 1; + push @artifacts, { + relative => $relative, + path => $path, + mode => (stat($path))[2] & oct('0777'), + current => $current, + rendered => $rendered, + stale => ($rendered eq $current ? 0 : 1), + }; + } + return {block => $block, artifacts => \@artifacts}; +} + +#------------------------------------------------------------------------------- + +=head3 sync_fips_policy + + Descriptions: Compare each runtime library with the canonical FIPS shell + policy, and in write mode replace a stale block. Both + builders call this before they package xCAT or + xCAT-genesis-scripts, so a stale library never reaches a + package. + + Arguments: + root - the repository root + mode - 'check' to report only, 'write' to regenerate + + Returns: a hash reference. + mode - the mode that ran + stale - the relative paths that did not match the policy + written - the relative paths this call rewrote; empty in check + mode + messages - one diagnostic per stale path in check mode; the + caller prints them + + Dies on every condition fips_policy_plan dies on. A stale path is + a result, not a failure, so the caller decides what it means. + +=cut + +#------------------------------------------------------------------------------- +sub sync_fips_policy { + my (%args) = @_; + my $mode = $args{mode} // 'check'; + die "FIPS policy mode must be check or write, not '$mode'\n" + unless $mode eq 'check' || $mode eq 'write'; + my $plan = fips_policy_plan(root => $args{root}); + my (@stale, @written, @messages); + for my $artifact (grep { $_->{stale} } @{$plan->{artifacts}}) { + push @stale, $artifact->{relative}; + if ($mode eq 'check') { + push @messages, "$artifact->{relative} is out of date;" + . ' run build-utils/sync-fips-policy --write'; + next; + } + # Rename over the library so a reader never sees a partial block, and + # carry the mode across: the Genesis copy is 0644 and xcatlib.sh 0755. + my ($fh, $temporary) = tempfile('.fips-policy-XXXXXX', + DIR => dirname($artifact->{path}), UNLINK => 1); + close $fh or die "close $temporary: $!\n"; + write_binary($temporary, $artifact->{rendered}); + chmod($artifact->{mode}, $temporary) or die "chmod $temporary: $!\n"; + rename($temporary, $artifact->{path}) + or die "replace $artifact->{path}: $!\n"; + push @written, $artifact->{relative}; + } + return {mode => $mode, stale => \@stale, written => \@written, + messages => \@messages}; +} diff --git a/build-utils/sync-fips-policy b/build-utils/sync-fips-policy index 9009ac090..48a8c5abb 100755 --- a/build-utils/sync-fips-policy +++ b/build-utils/sync-fips-policy @@ -3,44 +3,12 @@ use strict; use warnings; use FindBin; -use File::Basename qw(dirname); -use File::Slurper qw(read_binary write_binary); -use File::Temp qw(tempfile); +use lib "$FindBin::Bin/lib"; +use XCAT::BuildUtils qw(sync_fips_policy); my $mode = shift // ''; die "Usage: $0 --check|--write\n" unless !@ARGV && ($mode eq '--check' || $mode eq '--write'); -my $root = "$FindBin::Bin/.."; -my $policy = read_binary("$FindBin::Bin/fips-policy.sh"); -die "FIPS policy is empty\n" unless length $policy; -die "FIPS policy must end with a newline\n" unless $policy =~ /\n\z/; -my $begin = "# BEGIN generated FIPS policy: build-utils/sync-fips-policy --write\n"; -my $end = "# END generated FIPS policy\n"; -my $block = $begin . $policy . $end; -my $failed = 0; - -for my $relative ('xCAT-genesis-scripts/usr/lib/xcat/fips.sh', - 'xCAT/postscripts/xcatlib.sh') { - my $path = "$root/$relative"; - die "$relative must be a regular file\n" unless -f $path && !-l $path; - my $content = read_binary($path); - die "$relative must contain one FIPS policy block\n" - unless (() = $content =~ /\Q$begin\E/g) == 1 - && (() = $content =~ /\Q$end\E/g) == 1; - my $rendered = $content; - my $count = $rendered =~ s/\Q$begin\E.*?\Q$end\E/$block/sg; - die "$relative must contain one FIPS policy block\n" unless $count == 1; - next if $rendered eq $content; - if ($mode eq '--check') { - warn "$relative is out of date; run build-utils/sync-fips-policy --write\n"; - $failed = 1; - next; - } - my ($fh, $temporary) = tempfile('.fips-policy-XXXXXX', - DIR => dirname($path), UNLINK => 1); - close $fh or die "close $temporary: $!\n"; - write_binary($temporary, $rendered); - chmod((stat($path))[2] & oct('0777'), $temporary) or die "chmod $temporary: $!\n"; - rename($temporary, $path) or die "replace $path: $!\n"; -} -exit $failed; +my $result = sync_fips_policy(root => "$FindBin::Bin/..", mode => substr($mode, 2)); +warn "$_\n" for @{$result->{messages}}; +exit(@{$result->{stale}} && $result->{mode} eq 'check' ? 1 : 0); diff --git a/builddebs.pl b/builddebs.pl index 0a659d26a..d82697fc8 100755 --- a/builddebs.pl +++ b/builddebs.pl @@ -113,8 +113,11 @@ for my $pkg ($opts{packages}->@*) { } if (grep { $_ eq 'xCAT' || $_ eq 'xCAT-genesis-scripts' } $opts{packages}->@*) { - system($^X, "$FindBin::Bin/build-utils/sync-fips-policy", '--check') == 0 - or die "FATAL: generated FIPS policy is not current\n"; + my $fips = XCAT::BuildUtils::sync_fips_policy(root => $FindBin::Bin, mode => 'check'); + if (@{$fips->{stale}}) { + warn "$_\n" for @{$fips->{messages}}; + die "FATAL: generated FIPS policy is not current\n"; + } } my $ROOT = abs_path($FindBin::Bin); diff --git a/buildrpms.pl b/buildrpms.pl index 2b19585c4..3cdec4d62 100755 --- a/buildrpms.pl +++ b/buildrpms.pl @@ -990,8 +990,11 @@ sub main { if defined($native_subdir) && !$opts{source_only} && !$opts{gpg_sign}; if (grep { $_ eq 'xCAT' || $_ eq 'xCAT-genesis-scripts' } $opts{packages}->@*) { - system($^X, "$Bin/build-utils/sync-fips-policy", '--check') == 0 - or die "FATAL: generated FIPS policy is not current\n"; + my $fips = XCAT::BuildUtils::sync_fips_policy(root => $Bin, mode => 'check'); + if (@{$fips->{stale}}) { + warn "$_\n" for @{$fips->{messages}}; + die "FATAL: generated FIPS policy is not current\n"; + } } # ---- concurrency guard (mirrors cluster-test.pl's per-cluster lock) -------------------------- diff --git a/xCAT-test/bats/fips_policy_artifacts.bats b/xCAT-test/bats/fips_policy_artifacts.bats index dab4f3ba2..e9337846b 100644 --- a/xCAT-test/bats/fips_policy_artifacts.bats +++ b/xCAT-test/bats/fips_policy_artifacts.bats @@ -8,6 +8,7 @@ setup() ROOT="$BATS_TEST_TMPDIR/source" local relative for relative in build-utils/sync-fips-policy build-utils/fips-policy.sh \ + build-utils/lib/XCAT/BuildUtils.pm \ xCAT-genesis-scripts/usr/lib/xcat/fips.sh xCAT/postscripts/xcatlib.sh; do mkdir -p "$ROOT/$(dirname "$relative")" cp "$(repo_path "$relative")" "$ROOT/$relative" diff --git a/xCAT-test/unit/fips_policy_sources.t b/xCAT-test/unit/fips_policy_sources.t index 70fc141ed..5efa3c686 100644 --- a/xCAT-test/unit/fips_policy_sources.t +++ b/xCAT-test/unit/fips_policy_sources.t @@ -1,84 +1,145 @@ #!/usr/bin/env perl +# XCAT::BuildUtils FIPS policy generation. +# +# buildrpms.pl and builddebs.pl call sync_fips_policy before they package xCAT or +# xCAT-genesis-scripts, so a runtime library that no longer matches the canonical +# policy cannot reach a package. Every assertion below runs the sub and reads its +# answer; none of them runs the build-utils/sync-fips-policy script, which is a +# thin caller with nothing of its own to measure. use strict; use warnings; -use FindBin; -use lib "$FindBin::Bin/../lib"; -use File::Copy qw(copy); use File::Basename qw(dirname); +use File::Copy qw(copy); use File::Path qw(make_path); use File::Slurper qw(read_binary write_binary); use File::Temp qw(tempdir); +use FindBin; +use lib "$FindBin::Bin/../lib"; +use lib "$FindBin::Bin/../../build-utils/lib"; use Test::More; +use XCAT::BuildUtils qw(fips_policy_artifacts fips_policy_plan sync_fips_policy); use XCAT::Test::File qw(repo_path); -use XCAT::Test::Process qw(run_command); -my $sync = 'build-utils/sync-fips-policy'; my $source = 'build-utils/fips-policy.sh'; -my @libraries = ('xCAT-genesis-scripts/usr/lib/xcat/fips.sh', 'xCAT/postscripts/xcatlib.sh'); -my ($rc, $output) = run_command($^X, repo_path($sync), '--check'); -is($rc, 0, 'committed runtime artifacts match the canonical policy') or diag($output); -is($output, '', 'a current artifact check is quiet'); +my @libraries = fips_policy_artifacts(); +is_deeply(\@libraries, + ['xCAT-genesis-scripts/usr/lib/xcat/fips.sh', 'xCAT/postscripts/xcatlib.sh'], + 'both runtime libraries carry a generated policy block'); +# The committed tree. A stale block here would ship in a package. +my $committed = sync_fips_policy(root => repo_path('.'), mode => 'check'); +is_deeply($committed->{stale}, [], 'the committed libraries match the canonical policy'); +is_deeply($committed->{messages}, [], 'a current check reports no diagnostic'); +is_deeply($committed->{written}, [], 'a check writes nothing'); +is($committed->{mode}, 'check', 'the answer names the mode that ran'); + +# Everything below runs against a copy, so a failed assertion cannot leave the +# checkout's own libraries rewritten. my $root = tempdir(CLEANUP => 1); -for my $relative ($sync, $source, @libraries) { +for my $relative ($source, @libraries) { make_path(dirname("$root/$relative")); copy(repo_path($relative), "$root/$relative") or die "copy $relative: $!"; } -my %modes = ($libraries[0] => oct('0644'), $libraries[1] => oct('0755')); -for my $relative (@libraries) { - chmod($modes{$relative}, "$root/$relative") or die "chmod $relative: $!"; -} -my @original = map { scalar read_binary("$root/$_") } @libraries; +my %mode = ($libraries[0] => oct('0644'), $libraries[1] => oct('0755')); +chmod($mode{$_}, "$root/$_") or die "chmod $_: $!" for @libraries; +my @committed_text = map { scalar read_binary("$root/$_") } @libraries; + my $policy = read_binary("$root/$source"); write_binary("$root/$source", $policy . "\nxcat_policy_probe() { printf canonical; }\n"); -($rc, $output) = run_command($^X, "$root/$sync", '--check'); -is($rc, 1, 'a changed canonical policy invalidates both artifacts'); -for my $relative (@libraries) { - like($output, qr/\Q$relative\E is out of date/, 'check identifies the stale artifact'); + +# A plan reports what generation would write and writes nothing itself. +my $plan = fips_policy_plan(root => $root); +is_deeply([map { $_->{relative} } @{$plan->{artifacts}}], \@libraries, + 'the plan covers both libraries in generation order'); +is_deeply([map { $_->{stale} } @{$plan->{artifacts}}], [1, 1], + 'a changed policy makes both libraries stale'); +like($plan->{block}, qr/xcat_policy_probe/, 'the block carries the changed policy'); +for my $artifact (@{$plan->{artifacts}}) { + like($artifact->{rendered}, qr/xcat_policy_probe/, + "the plan renders the changed policy into $artifact->{relative}"); + unlike($artifact->{current}, qr/xcat_policy_probe/, + "the plan leaves $artifact->{relative} unchanged on disk"); } -is_deeply([map { scalar read_binary("$root/$_") } @libraries], \@original, - 'check does not modify stale artifacts'); -($rc, $output) = run_command($^X, "$root/$sync", '--write'); -is($rc, 0, 'write refreshes both artifacts') or diag($output); +is_deeply([map { scalar read_binary("$root/$_") } @libraries], \@committed_text, + 'a plan writes nothing'); + +my $check = sync_fips_policy(root => $root, mode => 'check'); +is_deeply($check->{stale}, \@libraries, 'a check names every stale library'); +is_deeply($check->{written}, [], 'a check writes nothing when libraries are stale'); +is(scalar @{$check->{messages}}, scalar @libraries, 'a check reports one diagnostic per stale library'); for my $relative (@libraries) { - is((stat("$root/$relative"))[2] & oct('0777'), $modes{$relative}, - "generation preserves the mode of $relative"); + like((grep { /\Q$relative\E/ } @{$check->{messages}})[0], + qr/is out of date; run build-utils\/sync-fips-policy --write/, + "the diagnostic for $relative says how to fix it"); } -my @rendered = map { scalar read_binary("$root/$_") } @libraries; -($rc, $output) = run_command($^X, "$root/$sync", '--write'); -is($rc, 0, 'repeated generation succeeds'); -is_deeply([map { scalar read_binary("$root/$_") } @libraries], \@rendered, +is_deeply([map { scalar read_binary("$root/$_") } @libraries], \@committed_text, + 'a check does not modify a stale library'); + +my $write = sync_fips_policy(root => $root, mode => 'write'); +is($write->{mode}, 'write', 'the answer names the write mode'); +is_deeply($write->{written}, \@libraries, 'a write names every library it rewrote'); +is_deeply($write->{stale}, \@libraries, 'a write reports which libraries were stale'); +is_deeply([map { scalar read_binary("$root/$_") } @libraries], + [map { $_->{rendered} } @{$plan->{artifacts}}], + 'a write stores exactly what the plan rendered'); +is_deeply([map { (stat("$root/$_"))[2] & oct('0777') } @libraries], + [map { $mode{$_} } @libraries], 'a write preserves the mode of each library'); + +my @generated = map { scalar read_binary("$root/$_") } @libraries; +my $again = sync_fips_policy(root => $root, mode => 'write'); +is_deeply($again->{stale}, [], 'a second write finds nothing stale'); +is_deeply($again->{written}, [], 'a second write rewrites nothing'); +is_deeply([map { scalar read_binary("$root/$_") } @libraries], \@generated, 'generation is idempotent'); -for my $relative (@libraries) { - is((stat("$root/$relative"))[2] & oct('0777'), $modes{$relative}, - "repeated generation preserves the mode of $relative"); -} -($rc, $output) = run_command($^X, "$root/$sync", '--check'); -is($rc, 0, 'refreshed artifacts pass the check'); +is_deeply([map { (stat("$root/$_"))[2] & oct('0777') } @libraries], + [map { $mode{$_} } @libraries], 'a second write preserves the mode of each library'); +is_deeply(sync_fips_policy(root => $root, mode => 'check')->{stale}, [], + 'a check passes on generated libraries'); -my $unterminated = $policy; -$unterminated =~ s/\n+\z//; -write_binary("$root/$source", $unterminated); -for my $mode (qw(--check --write)) { - ($rc, $output) = run_command($^X, "$root/$sync", $mode); - isnt($rc, 0, "$mode rejects a policy without a final newline"); - like($output, qr/FIPS policy must end with a newline/, - 'the error identifies the unterminated policy'); - is_deeply([map { scalar read_binary("$root/$_") } @libraries], \@rendered, - 'an unterminated policy leaves both artifacts untouched'); +# Every rejected input leaves both libraries alone, in both modes. A write that +# stopped half way would ship one library generated and one stale. +for my $case ( + ['a policy with no final newline', sub { my $text = $policy; $text =~ s/\n+\z//; + write_binary("$root/$source", $text) }, qr/FIPS policy must end with a newline/], + ['an empty policy', sub { write_binary("$root/$source", '') }, + qr/FIPS policy is empty/], + ['a missing policy', sub { unlink "$root/$source" or die "unlink: $!" }, + qr/\Qfips-policy.sh\E/], + ['a library with no policy block', sub { write_binary("$root/$source", $policy); + write_binary("$root/$libraries[0]", "#!/bin/sh\n") }, + qr/\Q$libraries[0]\E must contain one FIPS policy block/], + ['a library with two policy blocks', sub { + write_binary("$root/$libraries[0]", $generated[0] . $generated[0]) }, + qr/\Q$libraries[0]\E must contain one FIPS policy block/], + ['a library that is a symbolic link', sub { + write_binary("$root/$libraries[0]", $generated[0]); + rename("$root/$libraries[0]", "$root/$libraries[0]-real") or die "rename: $!"; + symlink("$root/$libraries[0]-real", "$root/$libraries[0]") or die "symlink: $!" }, + qr/\Q$libraries[0]\E must be a regular file/], +) { + my ($name, $break, $expected) = @{$case}; + $break->(); + my @before = map { -l "$root/$_" ? '' : scalar read_binary("$root/$_") } @libraries; + for my $mode (qw(check write)) { + my $answer = eval { sync_fips_policy(root => $root, mode => $mode) }; + is($answer, undef, "$mode rejects $name"); + like($@, $expected, "the $mode error identifies $name"); + } + is_deeply([map { -l "$root/$_" ? '' : scalar read_binary("$root/$_") } @libraries], + \@before, "$name leaves both libraries untouched"); } -unlink "$root/$source" or die "unlink policy: $!"; -($rc, $output) = run_command($^X, "$root/$sync", '--write'); -isnt($rc, 0, 'a missing source fails generation'); -is_deeply([map { scalar read_binary("$root/$_") } @libraries], \@rendered, - 'a missing source leaves both artifacts untouched'); -write_binary("$root/$source", $policy); -write_binary("$root/$libraries[0]", "#!/bin/sh\n"); -($rc, $output) = run_command($^X, "$root/$sync", '--write'); -isnt($rc, 0, 'a missing insertion block fails generation'); -is(read_binary("$root/$libraries[0]"), "#!/bin/sh\n", - 'generation does not replace a malformed artifact'); +unlink "$root/$libraries[0]" or die "unlink link: $!"; +rename("$root/$libraries[0]-real", "$root/$libraries[0]") or die "restore: $!"; +for my $mode ('', 'Write', 'check ', 'rewrite') { + my $answer = eval { sync_fips_policy(root => $root, mode => $mode) }; + is($answer, undef, "mode '$mode' is rejected"); + like($@, qr/FIPS policy mode must be check or write/, + "the error names the modes that exist, not '$mode'"); +} +my $answer = eval { fips_policy_plan() }; +is($answer, undef, 'a plan without a root is rejected'); +like($@, qr/fips_policy_plan needs a root/, 'the error asks for a root'); done_testing();