The module overrides two methods of XML::Simple, one for a recent version and
one for an older version, and each built its own parser and set its own
handlers. The two bodies were the same apart from spacing, so a change to one
refusal had to be repeated in the other, and a reader had to compare them to
see that they agreed.
Build the parser in one routine that both call. Behaviour does not change.
The previous commit deleted the $effective_provmethod override on the grounds
that %image_hash never carries a provmethod. That was wrong, and the review
caught it: makescript builds %image_hash, calls getImage() on it, and then
hands the SAME hashref to getScripts(), which fills provmethod for every
osimage from the osimage table. getDisklessNet() already reads that key the
same way. The override was live, not dead.
Restore it and say what is actually true in the comment. nodetype.provmethod is
frequently an osimage name rather than 'install', and resolving it is the point
of the lookup.
Also close the gap that made the wrong deletion so easy to ship: reverting
mkinstall's subiquity branch to its pre-fix body left the whole unit suite
green. debian_mkinstall_subiquity_branch.t lifts that branch out and drives it
inside a real loop, so the `next` it performs is the one under test, with
report_node_error and the getipaddr seam stood in for. It calls
subiquity_boot_params with no injected resolver, exactly as production does.
The branch is selected out of debian.pm by what it contains rather than by
where it sits -- there are four `if (using_subiquity(...))` in that file, and an
earlier draft of this test silently matched the wrong one and ran past its
block.
Now observable, each verified by mutation: swapping $pkgdir and $httpport at
the call site reddens the nfsroot assertion; reverting the branch wholesale
fails the extraction guard rather than passing.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The daemon reads the XML of every request through this parser. A request could
declare an entity in its own document type declaration, and the parser expanded
it. An entity that refers to other entities grows on each level, so a short
request expands into a large document and consumes the memory and the time of
the daemon. A client holds a certificate before it can send a request, so this
needs an account, but the daemon should not accept the work.
Refuse the declaration itself. The option that stops the parser from expanding
an entity does not cover an entity that a request names inside an attribute, so
it leaves the same growth available through a different part of the document.
Measured on XML::Parser 2.46, a request of 204 bytes that names its entity in
an attribute still grew to 1014 bytes with that option set, which is what the
parser does without it.
No request that xCAT sends carries a document type declaration. The client
builds every request with XML::Simple, which does not write one.
The handler that refuses an external entity stays, so a parser that reaches it
by another route still refuses to read the named file.
Both parser constructors passed a list of options to XML::Parser as an array
reference:
XML::Parser->new(Style => 'Tree', [ load_ext_dtd => 0, ... ]);
XML::Parser->new takes a flat list of pairs. The reference is one value in that
list, so the constructor reads the pairs as Style => 'Tree' and then the
reference as the name of an option with no value. Every option inside the
reference is dropped. The names are also the names that XML::LibXML uses, not
the names that XML::Parser uses, so the parser would ignore them even if it
received them.
The options therefore never did anything, and they give the reader the
impression that the parser refuses an external entity because of them. The
handler on the next line is what refuses an external entity.
Remove them. Behaviour does not change.
The syncfiles deferral resolved the node's provmethod through
$image_hash{$osimgname}{provmethod} when the node names an osimage. makescript
fills %image_hash from getImage(), which stores pkglist, pkgdir, otherpkglist,
otherpkgdir and environvar -- and no provmethod. getScripts() has a separate
hash that does store one, which is where the pattern was copied from. So the
lookup was always undef, the override never fired, and the code claimed a
behaviour it did not have.
Pass $provmethod directly and say in the comment why there is nothing to
resolve it with. No behaviour changes -- the branch was inert -- so there is no
red to show first; what the deletion needs is coverage that the path it was
supposed to serve still works.
That is what the two new assertions do: an osimage-named provmethod with
nodesetstate 'install' still defers, and the same name with no nodesetstate is
not mistaken for a diskful install. nodesetstate is what carries the install
signal here, which is why the override was never load-bearing. Making the
deferral ignore nodesetstate and require provmethod eq 'install' reddens both.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
noderes.installnic names the adapter that deploys the operating system. The
kickstart network line ignored it and named the adapter from mac.mac alone, so
a node that sets installnic got a kernel command line that obeys installnic and
a kickstart that configures a different adapter. On a node with more than one
adapter the installer then brings up the wrong one and cannot reach the
repository.
Name the device from the shared resolution, which gives the interface name when
installnic or primarynic names one, and the address otherwise. A node that sets
neither attribute keeps the address it has today.
The unique local address still comes from the hardware address, because
autoulaaddress builds the address from it.
The order that names the install device is noderes.installnic, then
noderes.primarynic, then mac.mac. gen_net_boot_params owns that order for the
netboot kernel parameters. Only the Ubuntu template reused it. Every other
install template reads mac.mac on its own.
Move the reuse into install_device_params so that any install template can
share it. subiquity_install_netcfg keeps its own name and its own return
value, because netplan needs the pair of a name and an address. Behaviour
does not change.
Own collection, sensitivity, finalization, and reset as one request-scoped state object so xcatd only forwards callbacks and appends the finalized text.
A node may list syncfiles in both postscripts and postbootscripts -- once for the install and
once for the booted node. The deferral prepended its own copy regardless, so the booted node ran
syncfiles twice in a row inside a marker block that made it look like xCAT had generated the
duplicate.
Keep the entry the node already has and still remove the install-time one, which is the copy
that cannot work in the in-target chroot.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
Three passes of the same reasoning had accumulated: in the code, in the POD of the routines the
previous commit extracted, and again in the test headers. Say each once, where the reader needs it.
The POD blocks were the worst of it -- extracting subiquity_kcmdline() and
defer_syncfiles_to_postboot() moved the essays out of the routines but did not shorten them. The
toram paragraph ran eleven lines for one kernel argument; the exact systemd-shutdown message and
the size of the 24.04 layers are colour, not reasoning.
The template's two comments and the apt-sources pair are trimmed the same way: the sources.list
explanation lives in Template.pm, and the test says which case it is checking and points there.
That the tests still pass after rewriting the template is the point of the previous commit -- a
test matching its text would have broken here.
47 comment and POD lines removed, no behaviour change.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The tests added with this fix matched regexes against the text of debian.pm, Postage.pm and
compute.subiquity.tmpl. A source match cannot tell whether the code it found ever runs: moving
the Subiquity command line into its own routine leaves every one of those assertions passing,
and reformatting a line fails them while the behaviour is untouched. One of them pinned Perl
syntax outright, qr/\$kcmdline\s*\.=\s*" ---";/, and another matched the text of a substitution.
Run the code instead.
Two decisions were lifted out of the routines that had grown around them, so a test can call
them: subiquity_kcmdline() in debian.pm builds the installer command line from its inputs, and
defer_syncfiles_to_postboot() in Postage.pm returns the adjusted postscript lists. Both are pure
and carry the reasoning that used to sit inline. The callers keep their behaviour exactly.
The template's two shell fragments are extracted and executed: the boot flip runs against a
stand-in for xcatd on the install-monitor port, and the resolv.conf step runs with a getent that
answers as the case requires.
Every assertion now fails when the behaviour it describes is removed, which is what the source
matches only appeared to do:
boot=casper dropped from the command line 1 assertion fails
toram dropped 1 assertion fails
nfsroot given the host name instead of the IP 2 assertions fail
the deferral made a no-op 7 assertions fail
the wrong token sent to xcatd 1 assertion fails
the failed-flip log line removed 3 assertions fail
the retry loop reduced to one attempt 2 assertions fail
resolv.conf given the host name 3 assertions fail
the resolution fallback removed 2 assertions fail
ubuntu_subiquity_bootparams.t is removed. Its three matches are covered by execution in
debian_subiquity_netboot.t, and it demonstrated the problem: it still passed after the command
line moved into another routine.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The template renderer writes the HTTP port into the URLs of a kickstart
file, an autoyast profile and a preseed file. It writes the port always, so
a URL gets the text ":80" when the site keeps the default port.
A URL that gives no port already goes to port 80. Write the port only when
the site sets a port that is not 80. The netboot plugins xnba, dhcp and mknb
already do this.
The module gave the port in three different ways. One place wrote the port
always. One place wrote the port only when the port was not 80. One place
wrote the port always from the environment. Put the rule in one routine and
let the four places use that routine.
The routine also accepts a port that is set to nothing. Before, an empty
value made a URL that ends with a colon.
The commands.log response classifier used a "passw" text match on the
request arguments. A secret whose name has no such text passed the
check, so a read of an authentication key, a privacy key or the snmpc
site value logged its bare value in the response. A command that
expands an argument also passed the check: nodels with a table name
returns every column of the table, and lsdef returns attributes that
the request never names. The daemon also ran redact_password over the
whole connection log on each request, so the redactor split at the
first request of the connection and the change signal swept the text of
earlier requests and responses.
Add secret_in_request. The routine reports a request that names a
secret attribute, selects a secret site key, or dumps a table that owns
a secret column through tabdump or nodels, from the same secret set
that the argument redaction uses. The response classifier calls it, so
the response of such a request logs as redacted.
Add secret_in_response. The routine reports response text that holds
"passw" or a secret attribute name in assignment or column form. The
response finalizer calls it in place of the bare text match, so an
expanded listing that carries an authentication key or a product key
logs as redacted even when the request never names it. The lsvm
response is the directory entry, whose passwords are positional, so
the classifier marks the command itself.
Build each request segment alone, redact the segment, and then append
it to the connection log. The redactor now always sees the current
command, and the change signal covers only the current request.
Treating an empty noderes.installnic as "mac" skipped noderes.primarynic: a node
that configures only primarynic was installed with a netplan matching on mac.mac
that never renamed the interface, and a primarynic holding a MAC address was
ignored. The order xCAT uses everywhere else is installnic -> primarynic ->
mac.mac, with either attribute allowed to hold an interface name or a MAC
address, and xCAT::NetworkUtils::gen_net_boot_params already implements it for
the netboot kernel parameters.
Resolve it in Perl and hand the template the answer:
- xCAT::Template::subiquity_install_netcfg() delegates the order to
gen_net_boot_params and returns the netplan set-name (empty when the device is
matched by MAC alone) and the address to match on, with mac.mac parsed by
xCAT::Utils::parseMacTabEntry so |-separated, !hostname-suffixed entries
resolve to THIS node rather than to whichever entry came first.
- #SUBIQUITYINSTALLNIC# / #SUBIQUITYINSTALLMAC# render those two values.
installnic and primarynic are read blank-okay -- a node that sets neither is
normal, and is exactly what "match on mac.mac" means -- while mac.mac stays a
required lookup, as it was when the template read it directly.
- compute.subiquity.tmpl consumes the resolved values. No part of the fallback,
and no mac.mac parsing, is left in shell: it only chooses whether to emit
set-name, based on whether a name was resolved.
ubuntu_subiquity_installnic.t now passes: the six resolution cases, multi-entry
mac.mac, and the netplan the template's own late-command writes for a resolved
name and for none. ubuntu_subiquity_template.t asserts the template no longer
reads installnic/primarynic or parses MACs itself.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
Boot the live installer correctly. Add boot=casper so casper actually processes
netboot=nfs instead of scanning local disks and panicking, resolve the install server
to a literal IP because casper mounts the live filesystem with klibc's nfsmount which
has no resolver, and add toram so casper copies the squashfs into RAM and unmounts the
NFS source. That last one is what lets the node reboot at all: with the NFS root still
mounted, systemd-shutdown waits forever on an lvm/pvscan wedged in uninterruptible I/O
on it and the node never power-cycles into the disk it just installed. casper has no
cmdline knob for NFS mount options -- it parses only nfsroot= and takes the whole value
as the path -- so toram is its supported way to avoid a network root.
Write the installer's resolv.conf nameserver as an IP. glibc's resolver discards a
hostname given on a nameserver line, so the xcatmaster name left the installer, and
the in-target apt that inherits the file, with no DNS at all.
Add the online archive through apt `sources:` on classic-sources releases, where
Subiquity renders the target sources.list from the install media alone and in-target
apt cannot find packages the ISO does not carry. Deb822 releases are excluded: their
primary mirror already lands in ubuntu.sources, so legacy .list files would configure
the same suites twice.
Flip the node to local-disk boot from the live installer over bash's /dev/tcp instead
of relying on updateflag.awk, which needs gawk's |& coprocess while Ubuntu's
/usr/bin/awk is mawk. The exchange is checked, and a failure is recorded in the
install log rather than silently PXE-looping into another install.
Defer syncfiles to the postbootscripts on the diskful install path, so it runs on the
booted node with sshd up rather than inside the in-target chroot where the MN cannot
reach it.
Make the test harness's provision wait overridable through WAIT_FOR_PROVISION; the
default stays short so a boot loop still fails fast.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
The daemon redacted secret attributes on the joined command string. The
match failed when a value held a space. The match also failed for a "+="
splice assignment. The validate() path did not quote the arguments, so a
multi-word secret value kept its later words in syslog and in the
auditlog table. A password that a command receives through an option or
a positional operand was not redacted at all. The debug dispatch trace
wrote the raw arguments to syslog when site.xcatdebugmode was set.
Redact the argument vector before the daemon joins it. Add
redact_password_args for this task. The routine masks the value of a
secret attribute in any argument, at the start or embedded after another
token. An embedded secret assignment masks to the end of the argument,
because a shell value may hold quotes and spaces. The routine allows
spaces around the operator. It accepts the "=", "+=", ",=", "^=", "!=",
"=~" and "!~" operators that chdef, nodech and node selection use. It
masks a password option value in each form that Getopt::Long accepts: a
separate argument, a compact short option, a bundle of short options
with the "?" help letter, a "+" option prefix, a single-letter option
with two dashes, a long option, a long option with an equals sign, and
an abbreviated long option. The long-name match runs first, so a long
option keeps its name and masks its value. A walk over each bundle then
finds the first secret letter, so the mask always starts at the option
and the result does not depend on hash order. The walk knows which
other letters of a command take a value, so a secret letter inside such
a value does not redact and the audit text stays correct. The walk also
knows which letters take an integer, because the z/VM cpu option
consumes only its signed digits and the parser then continues the
bundle into the password option. The value stops match letter case,
because a bundle keeps short options case sensitive and an unknown
capital letter does not absorb the rest. The mkvm secret match ignores
letter case, because the z/VM parser keeps the Getopt::Long default for
long names. The mkhwconn match keeps letter case, because -p
is the hardware control point and -P is the password. The routine knows
the password options of bmcdiscover, switchdiscover, mkhwconn, mkvm,
createvcluster, lsvcluster and rmvcluster, the rspconfig password
assignments, the mkvm clone pw= operand, and the positional password
operands of chvm. It masks the site.value argument of tabch and chtab
when a selector or a site.key assignment names snmpc. An exact short
option that takes a non-secret value stays visible, so the PPC mkvm -p
profile is not an abbreviation of --password. The dispatch trace builds
its text from the redacted vector.
Add snmpc, productkey, prodkey.key, tokenid and token.tokenid to the
secret list, with community and pdu.community. The secret list holds
only attributes that map to a secret column, so key and sshkeydir stay
visible.
redact_password keeps a second pass over the joined string. This pass
masks an embedded secret assignment to the end of the line, because the
argument boundaries are gone after the join.
The commands.log response classifier marks a response sensitive when the
request was redacted. The argument vector pass sets that signal, so a
secret whose name has no "passw" text still marks its response.
xcatd expands the request noderange before it authorizes the caller: once to
count the nodes, and once in validate() to match the policy rules. The ^
operator makes xcatd open a caller-named file at that point. A client can
connect without a certificate, because the listener does not require one, and
such a client has no peername.
Expand these two pre-authorization noderanges with nofile when the caller has
no peername (checked with defined, so the identity "0" still counts as
authenticated). If validate() finds a rejected ^file atom on such a request,
deny it. An authenticated caller expands ^file as before.
xCAT::XML wraps XML::Simple. It blocks XML external entities. An external
entity can read a local file or a network resource.
XML::Simple with new_xml_parser() installs an ExternEnt handler. The
handler returns the system id, not the content.
XML::Simple 2.20 to 2.24 has no new_xml_parser(). On these versions xCAT
uses build_tree_xml_parser() instead. This function did not install the
handler. A SYSTEM entity then expanded. It leaked a local file into the
parsed data.
This change adds the same handler to build_tree_xml_parser(). Both paths
pass parser options in an array reference. XML::Parser->new ignores that
reference. The handler is therefore the real defense. This fix keeps
#6917, which retains the compatibility path for those releases.
This fix was recovered from the lenovobuild branch. See PR #6505.
Only syslog received the redacted arguments. The auditlog table was given the
raw string, so a password removed from syslog and from commands.log was still
written to the database, which persists and is readable by anything with access
to it.
Use the redacted text for both. This also covers the mkvm --password and -w
masking a few lines above, which the table did not have either.
redact_password only knew about bmcdiscover, mkhwconn and rspconfig, so setting
a password the ordinary way left it in the clear:
[Request] chdef node01 'bmcpassword=SEKRET'
Those secrets are carried as an assignment on whichever command happens to set
them, so match them by name rather than by command. The names are every
attribute Schema.pm maps to a password, passwd, authkey or privkey column, and
the columns themselves, since a table qualified assignment such as
passwd.password= is accepted too. An assignment may be written with spaces
around the equals sign and the value may contain spaces, so a quoted argument is
redacted to its closing quote.
Attributes such as key, which names a monitoring attribute, and sshkeydir, which
is a directory, are not secrets and are left readable. The existing per-command
table is kept for a flag like bmcdiscover -p, which carries its secret in the
following argument where there is no name to match.
Goconserver.pm shells out to `service goconserver start|stop|restart` (and
`service conserver stop`). That command is provided by the initscripts package,
which is NOT installed on a minimal EL9/EL10 management node -- there, every one
of these calls fails with rc!=0.
The damaging case is restart_service(). makegocons calls build_conf(), which
rewrites /etc/goconserver/server.conf with the cert-enabled configuration
(global.ssl_key_file/ssl_cert_file/ssl_ca_cert_file), and then calls
restart_service() so the daemon picks it up. When the restart fails, xCAT only
logs "Could not restart goconserver service." and the daemon keeps running with
the configuration it read at boot -- the packaged default, which has no ssl_*
fields. goconserver's TLS is gated on those fields being set (sslEnable stays
false), so it silently serves plain HTTP on the api port while Goconserver.pm
always talks to it over https. Every subsequent request then dies with
SSL connect attempt failed error:0A0000C6:SSL routines::packet length too long
so makegocons/makegocons -d report "Failed to send delete request." and the
console can never be registered. On EL8 the bug is invisible because initscripts
happens to be installed there.
Use the xCAT::Utils service helpers (startservice/stopservice/restartservice),
which resolve the unit through servicemap() and issue `systemctl <action> <unit>`
on systemd hosts, falling back to `service`/`initctl` only where appropriate.
This is the same mechanism the rest of the tree (AAsn.pm and friends) already
uses for named, dhcpd, nfs and others.
Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
reseat_node opens an IPMI session to the FPC/CMM to reseat a node. The FPC
needs packets sent without maxpending flow-control, but that state is only held
transiently. Add a persistent per-session 'neverwait' flag, set it on the FPC
reseat session, and honor it in sendpayload.
The flag defaults off, so it is a no-op for every other IPMI session; only the
FPC reseat path opts in. (The lenovobuild original also reworked the shared
login/admin_level_set nowait handling; that part is left out to avoid changing
flow-control for all BMCs.)
Not lab-validated (no NeXtScale FPC/CMM available).
Recovered from the unmerged lenovobuild branch (eda85df3, neverwait hunks only).
envvar() resolves a template variable purely from %ENV, so a template that
references $XCATROOT expands to an empty string whenever XCATROOT is not
present in the environment of the process doing the substitution. $::XCATROOT
is set as a package global at module load and is reliably available, so use it
for that one variable before falling back to %ENV.
Recovered from the unmerged lenovobuild branch (original da72c6b8).
Co-authored-by: Jarrod Johnson <10814490+jjohnson42@users.noreply.github.com>
Daniel Hilst
Vinícius Ferrão
Vinícius Ferrão
Vinícius Ferrão