fix(xcatd): redact node passwords from the command logs - #7719
Open
viniciusferrao wants to merge 3 commits into
Open
fix(xcatd): redact node passwords from the command logs#7719viniciusferrao wants to merge 3 commits into
viniciusferrao wants to merge 3 commits into
Conversation
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.
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.
Run redact_password rather than inspecting its source, so that a secret which survives is a failure here rather than something the shape of the code hides. Each supported way of writing an assignment is exercised, including the table qualified form and spaces around the equals sign, together with the positional flag that the per-command table handles. The expected set is derived from Schema.pm rather than repeated, so an attribute added there with a secret column fails this test instead of quietly reaching the logs, and the test bails if that mapping cannot be parsed so it cannot pass vacuously. Detail that is not secret is asserted to survive, since redacting it would cost the log its usefulness without protecting anything.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Setting a password the ordinary way left it in the clear:
redact_passwordknew only bmcdiscover, mkhwconn and rspconfig, and none of those is how an administrator sets a password. Match by name instead: every attribute Schema.pm maps to a password, passwd, authkey or privkey column, plus the columns themselves, since a table qualified assignment likepasswd.password=also works.keynames a monitoring attribute andsshkeydiris a directory, so both stay readable.Only syslog got the redacted arguments. The auditlog table got the raw string, so a password stripped from syslog and commands.log still reached the database. Both now use the same text, which also picks up the
mkvm --passwordmasking the table never had.The per-command flag table stays, for a secret carried positionally like
bmcdiscover -p.The problem was noted in 42f6e9f on the unmerged lenovobuild branch, which removed every argument from commands.log. This keeps the arguments and redacts only the secrets.