Skip to content

fix(xcattest): a [Table_*] value stops at its first colon or slash - #7996

Open
dhilst wants to merge 21 commits into
xcat2:masterfrom
VersatusHPC:fix/xcattest-table-value-class
Open

fix(xcattest): a [Table_*] value stops at its first colon or slash#7996
dhilst wants to merge 21 commits into
xcat2:masterfrom
VersatusHPC:fix/xcattest-table-value-class

Conversation

@dhilst

@dhilst dhilst commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

A [Table_*] value in an xcattest conf is truncated at its first character outside the word class. A path, an IPv6 address, a URL or a comma-separated list reaches chtab cut short, and no warning is printed, so the run continues with a wrong attribute value.

xCAT::Test::Conf matched a table value with ([\w\.\-]+), which stops at : and /. [Object_*] values were already matched more permissively.

The table value now matches to the end of the value, with an inline comment still removed.

xCAT-test/unit/xcattest_conf_table_value_class.t parses conf fixtures and asserts the value each attribute receives. Without the fix 7 of its 22 assertions fail and 15 pass: the IPv6, path, list and inline-comment cases go red, and the plain word-character cases stay green.

Depends on #7973 and includes its commits.

dhilst added 16 commits October 21, 2026 19:36
@dhilst
test(kvm): a node cannot ask for UEFI that verifies what it boots
e3956e7 
vm.othersettings firmware:efi asks libvirt for UEFI and states nothing
about Secure Boot. libvirt then takes the firmware descriptor of highest
priority that fits the guest, and that order belongs to the hypervisor.
The same node boots a firmware that verifies its kernel on one host and
one that does not on another, and the domain reports neither.

kvm_secureboot_firmware.t reads firmware:efi-secureboot through
requested_firmware, guest_arch_profile and guest_os. Six of its twelve
checks fail on this commit: the value is refused as unknown, so the node
keeps SeaBIOS and no <loader secure='yes'/> reaches the domain.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
feat(kvm): ask libvirt for UEFI that verifies what it boots
772b2c6 
vm.othersettings now takes firmware:efi-secureboot. guest_os puts
<loader secure='yes'/> on the domain beside firmware="efi". libvirt
reads that element while it selects the firmware and keeps only the
descriptors that carry secure-boot, so a hypervisor with no such
firmware refuses the domain instead of starting the node on a firmware
that verifies nothing.

firmware:efi keeps its meaning and states no requirement.

Measured with libvirt 11.10.0 on an x86_64 host: the domain resolves to
OVMF_CODE.secboot.fd with <smm state='on'/>, and the same domain with
secure-boot asked off resolves to OVMF_CODE.fd.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
test(xcattest): the harness tests stage one module by name
687d2c1 
xcattest_report_every_check.t and autotest_check_lines_are_understood.t
copy xcattest into a scratch tree and run it. Each copied
xCAT/Test/OS.pm by name, so a second module under xCAT-test/lib/xCAT
stops the copied xcattest before its first case, and the test reports
that as a harness error.

Both now copy every .pm under that directory.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
test(xcattest): a conf cannot set a vm.othersettings with two settings
b518157 
An [Object_*] line of an xcattest conf declares an xCAT node attribute.
vm.othersettings holds several settings separated by a semicolon, which
is the syntax the kvm plugin reads, and such a line reaches the node cut
at its first semicolon with no message. The reader drops the rest, and
the chdef that applies it runs through /bin/bash -c unquoted, so the
shell would read the semicolon as a command separator.

load_config_file read each line with a regular expression written inline
and built the chdef by concatenation, so neither could be measured
without a database. xCAT::Test::Conf now holds the three readers and the
command builder, and xcattest calls them. The routines keep the behaviour
they replace, and both packages install the module.

xcattest_conf_object_attr.t fails five of its nine checks on this commit:
the semicolon value, the quoting and the order of the assignments.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(xcattest): an [Object_*] value stops at its first semicolon
6e0b113 
A conf that declares vmothersetting=cpumode:host-passthrough;firmware:efi
reaches the node as vmothersetting=cpumode:host-passthrough. The value
class of object_attr held no semicolon, so the reader kept the characters
up to it and dropped the rest without a message. vm.othersettings
separates its settings with a semicolon, so no conf could set two of them.

object_attr now keeps the semicolon, and chdef_command quotes every word
it takes from the conf. The quoting is what makes the semicolon safe:
xcattest runs each command through /bin/bash -c, where an unquoted
semicolon ends the command. chdef_command also sorts the assignments, so
one conf builds one command.

xcattest_conf_object_attr.t covers both halves.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(copycds): the web server cannot read the EFI binaries of the media
86f437d 
A node with netboot=ipxe and Secure Boot on fetches the shim of its
install source over HTTP, and the server answers 403. EL media ships the
EFI binaries of its boot loader mode 0700, copycds copies the media with
the modes the media carries, and xCAT serves the install directory over
HTTP. Measured on an AlmaLinux 10 tree: EFI/BOOT/grub.cfg is 0444 and
answers 200, and BOOTX64.EFI, grubx64.efi and mmx64.efi are 0700 and
answer 403. The node reports "Verification failed: Security Policy
Violation" for its kernel and then "Operation not permitted" for the
shim, and boots nothing.

copycd gave the top directory of the copy mode 0755 and left every path
under it as the media had it. Both copycd plugins now call
xCAT::MediaPerms::make_readable on the copied tree, which adds read
permission for the group and for others, and search permission on a
directory. It adds no write permission, it adds no execute permission to
a file, and it leaves a path that is already group and other readable
alone, so a second call changes nothing.

On the same AlmaLinux 10 tree it walks 5850 paths, 5835 files and 15
directories, and changes 3 of them: the three files above, each from 0700
to 0744. An install tree that is already copied keeps its modes until
copycds runs again for that osimage, so an operator with an existing tree
re-runs copycds or gives those files read permission.

media_perms_readable.t covers the modes and the idempotence. Three
mutations of the module each turn it red.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(kvm): a machine type of vm.othersettings takes a UEFI node to i440fx
242cff9 
guest_arch_profile moves an x86_64 node that asks for efi to q35, because
every x86_64 OVMF descriptor targets pc-q35-* only. guest_os then writes
a machine: setting of vm.othersettings over that machine type, so a node
with firmware:efi and machine:pc reaches libvirt asking for a UEFI
firmware on i440fx. libvirt answers "Unable to find 'efi' firmware that
is compatible with the current configuration" and names neither the node
nor either setting, and rpower boot fails with no way to see why.

firmware_machine_error refuses the pair, and build_xmldesc returns it
before the domain is built. The message names the node, both settings and
the machine type to use. A node that names no machine type, a q35 of any
version, and the machine types of another architecture are all unchanged.

kvm_secureboot_firmware.t covers the refusal and the five values that are
not refused.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(xcattest): the diskless case takes a UEFI node to i440fx
1ab2f68 
reg_linux_diskless_installation_flat asks vm_machine_type.sh for a valid
machine type, and the helper answers machine:pc for every x86_64 node. A
node with firmware:efi then cannot boot: libvirt refuses a UEFI firmware
on i440fx, rpower boot fails and the case reports a provision failure
that has nothing to do with provisioning.

vm_machine_type_for_arch now reads the vmothersetting it saved and
answers machine:q35 for an x86_64 node that asks for a UEFI firmware. An
x86_64 node without one still gets machine:pc.

vm_machine_type.bats covers both, and a mutation that answers machine:pc
for a UEFI node turns the two new tests red.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(kvm): the firmware guard refuses a machine type xCAT cannot judge
1edcdda 
firmware_machine_error refused every machine type that did not contain
q35, so a node with firmware:efi and a machine type xCAT does not know
failed at mkvm. xCAT does not know which machine types a hypervisor
offers, and reg_linux_diskless_installation_flat relies on mkvm accepting
machine:invalid so that it can measure the provisioning failure that
follows. That case failed at the mkvm before it reached the measurement.

The guard now names the i440fx family alone: pc, pc-i440fx-* and
i440fx-*, whose descriptors carry no UEFI firmware. Any other value goes
to libvirt, which reports a machine type it cannot use.

kvm_secureboot_firmware.t covers both halves, and a mutation that
restores the q35 test turns the invalid-machine check red.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
docs(ipxe): say which kernels the ipxe path boots with Secure Boot on
789913b 
The Secure Boot chapter described the shim that each kind of node uses
and did not say which of them boots. A reader took the MOK paragraph for
a supported procedure, and an operator configuring discovery found
nothing about Secure Boot at all.

The chapter now states what the path supports: a node with netboot=ipxe
boots the kernel of a distribution with Secure Boot on, for a disk
installation and for a netboot image. Genesis requires Secure Boot off on
the node, because xCAT-genesis-base ships its kernel unsigned and a shim
verifies a kernel only against the signature database of the firmware or
a key in the MOK list. That covers discovery, the Genesis shell and the
standby and runcmd destinies. The section on clients without a node
definition names the same requirement and links to it.

A site may still sign the Genesis kernel with its own key and enroll it
with mokutil. xCAT ships no key and no signature.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(kvm): a Secure Boot domain can start on a firmware with no enroll…
94d2ce2 
…ed keys

guest_os stated <loader secure='yes'/> alone for a node with
vm.othersettings firmware:efi-secureboot. libvirt reads that element as
the secure-boot feature, and a firmware with secure-boot and an empty key
store boots an unsigned binary. The two x86_64 descriptors of an EL 10
host name the same OVMF_CODE.secboot.fd and differ in the nvram template,
OVMF_VARS.secboot.fd against OVMF_VARS.fd, so only descriptor priority
picked the one that holds keys. Commit 772b2c6 of this branch says that
element selects the descriptor with enrolled keys. It does not.

firmware_features names secure-boot and enrolled-keys for a Secure Boot
guest, and add_firmware_features writes them into the <os> of the domain.
guest_os cannot carry the element: the firmware attribute of <os> and the
<firmware> child of <os> are one key of one hash for XMLout, and the
attribute is the one that starts the selection. firmware:efi asks for no
feature and is unchanged.

Measured with libvirt 11.10.0 on an x86_64 host, with the enrolled
descriptor masked for one connection: the old XML starts on OVMF_VARS.fd
with enrolled-keys resolved to no, and the new XML is refused with
"Unable to find 'efi' firmware that is compatible with the current
configuration". virt-xml-validate accepts the new XML.

kvm_secureboot_firmware.t covers the features and the element. Two
mutations turn it red: dropping enrolled-keys from firmware_features, and
making add_firmware_features return its argument.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(test): the count of changed media modes depends on the umask of t…
832ca0b 
…he caller

media_perms_readable.t asked make_path for two directories and then
counted the paths make_readable changed. make_path takes the mode from
the umask of the caller, so under umask 027 or 077 those two directories
need a mode change as well and the count is 6, not 4. The test failed
with "got '6', expected '4'" for anyone whose umask is not 022.

The fixture now sets the mode of both directories, so every mode the
count depends on is stated in the test.

Measured on the unfixed file: red under umask 027 and under umask 077.
The fixed file passes under 022, 027 and 077.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(test): five checks of the Secure Boot branch pass without the cod…
294f7c8 
…e they name

Each of these five checks held with the behaviour it describes removed.

kvm_secureboot_firmware.t asked firmware_machine_error for machine virt
on riscv64 to measure the x86_64 guard. virt matches neither pc nor the
i440fx pattern, so the answer was undef with the guard removed as well.
The file now asks for pc and for a versioned i440fx on another
architecture, and for pc with nodetype.arch unset.

media_perms_readable.t asked make_readable for a directory that does not
exist to measure the -d test. File::Find returns without changing a path
there in either case, so only the warning it prints differs. The check
now also reads the warnings, which is what copycds shows the admin.
MediaPerms.pm skipped a symbolic link and nothing measured it, so the
fixture now holds one that names a file outside the tree.

vm_machine_type.bats kept two cases that a machine:pc answer for every
x86_64 node left green. "x86_64 without a firmware" repeats the second
line of test 2 and is deleted. assert_round_trip never read the machine
type apply wrote, so it now takes the expected machine type and the
Secure Boot round trip states q35.

Four mutations turn the three files red: the x86_64 guard, the -d test,
the symbolic-link test, and machine:pc for every x86_64 node.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
docs(kvm): the Secure Boot documentation omits the keys and the rpowe…
2cd5c70 
…r path

Two statements about this branch are narrower than the code.

Commit 1edcdda says a node with a machine type xCAT cannot judge
"failed at mkvm". firmware_machine_error is returned by build_xmldesc,
and makedom calls build_xmldesc for a node whose domain is not yet built,
so the refusal also reaches the rpower boot of that node. The POD of
firmware_machine_error now names both callers.

The Secure Boot section of the ipxe guide did not name
firmware:efi-secureboot at all. It now says what mkvm asks libvirt for,
why both the secure-boot feature and enrolled-keys are asked for, and
what a hypervisor without such a firmware answers.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
docs(ipxe): the Secure Boot chapter does not say which distributions …
86b8bb9 
…are supported

The chapter rules out ppc64le, aarch64 and riscv64, and says nothing about the
distribution. The shim chain is UEFI and distribution independent, so a reader
concludes that any distribution xCAT installs boots with Secure Boot on, and nothing
in the chapter contradicts it.

Operator decision, 2026-10-22: Secure Boot is Enterprise Linux and x86_64 for 2.20.
State it, and say what is measured rather than what is possible -- the mechanism does
work elsewhere, and that is exactly why the support claim has to be explicit.

The CI side carries the same scope: the Secure Boot case is named by the EL x86_64
leaves only, in xcat-core-ci-cd cells.tsv.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(xcattest): a [Table_*] value stops at its first colon or slash
839117f 
xcattest applies a conf's [Table_*] blocks itself, once, before the first case
runs, so whatever table_attr returns is what reaches chtab. Its value class was
([\w\.\-]+), which stops at a colon or a slash and writes the truncated value
with no warning: on a management node provisioning over IPv6,
`net=2001:db8:176::/64` reached chtab as `net=2001`, and xcattest re-applied that
junk row on every run. [Table_site] value= has the same hole, so site.master
cannot be an IPv6 address either, and any table value holding a path or a
comma-separated list loses everything after the first slash or colon.

object_attr, beside it in the same module, already accepts ":,;/" -- which is
why an [Object_node] ip= carries an IPv6 address and a [Table_networks] net=
silently could not. table_attr now uses the same class. Both are faithful to
what they replaced: upstream xcattest still matches tables with ([\w\.\-]+) at
line 817 and objects with ([\w\.\-\+\ \/:]+) at line 854, and moving the pair
into xCAT::Test::Conf is what made the asymmetry visible and testable.

The class holds no whitespace and no '#', so a trailing inline comment is still
excluded -- asserted, because that is the behaviour a wider class could have
broken.

xcattest_conf_table_value_class.t covers one line per kind of value a table
holds. Seven of its 22 assertions fail without the fix, including the three IPv6
forms and the absolute path; xcattest_conf_object_attr.t stays green, which is
the control that colons were never meant to be refused.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
dhilst marked this pull request as ready for review October 26, 2026 18:02
@dhilst dhilst self-assigned this Oct 26, 2026
@dhilst dhilst added this to the 2.20 milestone Oct 26, 2026
@viniciusferrao
viniciusferrao self-requested a review October 27, 2026 01:07
viniciusferrao
viniciusferrao requested changes Oct 27, 2026
View reviewed changes

@viniciusferrao viniciusferrao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @dhilst, table_attr() now accepts semicolons, but the [[chtab](https://github.com/xcat2/xcat-core/blob/839117f9eff2712ccbcb5b15675790bd11c2d878/xCAT-test/xcattest#L1596-L1605) command](

xcat-core/xCAT-test/xcattest

Lines 1596 to 1605 in 839117f

$cmd = "chtab $$config_ref{table}{$type}{$name}{__KEY__}=$name";
foreach my $attr (keys %{ $$config_ref{table}{$type}{$name} }) {
if ($attr ne '__KEY__') {
$cmd = $cmd . " $type.$attr=$$config_ref{table}{$type}{$name}{$attr}";
}
}
log_this($running_log_fd, "$cmd");
&runcmd($cmd);
if ($::RUNCMD_RC != 0) {
$$error_ref = "Failed to run $cmd";
) still uses unquoted values.

With [Table_vm] and othersettings=cpumode:host-passthrough;firmware:efi-secureboot, chtab receives only the first setting. Bash then tries to run firmware:efi-secureboot as a command, and environment setup fails.

Quote all chtab arguments, including the row selector, as you already do for chdef. Add a test that checks the arguments received by chtab.

Thanks.

dhilst added 5 commits October 29, 2026 14:45
@dhilst
Merge branch 'master' into fix/xcattest-table-value-class
9f4a296 
xCAT-server/lib/xcat/plugins/anaconda.pm conflicted in the module
imports: this branch adds "use xCAT::MediaPerms" and master adds
"use xCAT::SELinux" on the same line. Both imports are kept. The two
modules are used in different subs, and no other hunk conflicted.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
Merge branch 'master' into fix/xcattest-table-value-class
8a490a6 
xCAT-test/autotest/testcase/commoncmd/vm_machine_type.sh conflicted:
this branch picks q35 for an x86_64 node with firmware:efi, and master
asks the vmhost libvirt for the machine type. The vmhost probe from
master is kept. vm_machine_type_alias now takes the vmothersetting and
returns q35 for firmware:efi, and q35 does not fall back to the libvirt
default, because the x86_64 default is i440fx.

xCAT-test/bats/vm_machine_type.bats conflicted where both sides added
tests before assert_round_trip. Both sets are kept, with the
assert_round_trip of this branch, plus one test for the q35 rule.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
Merge branch 'master' into fix/xcattest-table-value-class
1d148d1 
No conflicts. master gained xcat2#7990, xcat2#7979 and xcat2#7977 after the previous merge.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(xcat-core): no test shows a [Table_*] semicolon breaks the chtab …
e813f1e 
…command

xcattest builds the chtab command for a [Table_*] row with unquoted words and
runs it through /bin/bash -c. A [Table_vm] othersettings value such as
cpumode:host-passthrough;firmware:efi-secureboot ends the command at the
semicolon: chtab receives only the first setting, and bash runs the second as
a command, so the environment setup fails.

Move the command builder from setup_env_by_configure_file into
xCAT::Test::Conf::chtab_command, with the same output, so a test can read the
words chtab receives. xcattest_conf_chtab_command.t splits the command the way
the shell does. Three of its four checks fail on this commit.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
fix(xcat-core): chtab receives only the first setting of a [Table_*] …
2107bd6 
…value

With [Table_vm] othersettings=cpumode:host-passthrough;firmware:efi-secureboot,
xcattest gives chtab only cpumode:host-passthrough, and bash runs
firmware:efi-secureboot as a command. The environment setup then fails.

chtab_command joined its words unquoted, and xcattest runs the command
through /bin/bash -c. chtab_command now quotes every word, the row selector
included, with the same _quote that chdef_command uses.

xcattest_conf_chtab_command.t passes with this change and fails 3 of 4
checks on the previous commit.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst
dhilst enabled auto-merge October 30, 2026 13:05
@dhilst
dhilst requested a review from viniciusferrao October 30, 2026 13:05

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viniciusferrao viniciusferrao Awaiting requested review from viniciusferrao

Requested changes must be addressed to merge this pull request.

Assignees

@dhilst dhilst

Labels

None yet

Projects

None yet

Milestone

2.20

Development

Successfully merging this pull request may close these issues.

2 participants

@dhilst @viniciusferrao