#2967 Issue open: Confidential values leaked into log file in debug mode

Labels: enhancement, documentation, critical / security / legal

jsmeix opened issue at 2023-04-05 09:43:

  • ReaR version ("/usr/sbin/rear -V"):

current GitHub master code

  • ReaR configuration files ("cat /etc/rear/site.conf" and/or "cat /etc/rear/local.conf"):

excerpt:

SSH_ROOT_PASSWORD="rear"

BACKUP_PROG_CRYPT_ENABLED="yes"
{ BACKUP_PROG_CRYPT_KEY='my_secret_passphrase' ; } 2>/dev/null
  • Description of the issue (ideally so that others can reproduce it):

Since
https://github.com/rear/rear/commit/b83059a72d2ea8735719cf415c6cafd5f43312f0
there is - by the way - a new
init/default/998_dump_variables.sh
which is unrelated to what the commit message describes.

The new script does currently only

Debug "Runtime Configuration:$LF$(declare -p)"

which outputs in debug modes also confidential
variable values into the ReaR log file like

# usr/sbin/rear -d help
Running 'init' stage ======================
Running workflow help on the normal/original system
...

# egrep -i 'password|key' var/log/rear/rear-linux-h9wr.log.lockless
...
declare -- BACKUP_PROG_CRYPT_KEY="my_secret_passphrase"
declare -- GALAXY11_PASSWORD=""
declare -- OPAL_PBA_DEBUG_PASSWORD=""
declare -- SSH_ROOT_PASSWORD="rear"
declare -- TTY_ROOT_PASSWORD=""
declare -- YUM_ROOT_PASSWORD="root"
declare -- ZYPPER_ROOT_PASSWORD="root"

In particular BACKUP_PROG_CRYPT_KEY is critical
because that new script foils my security/privacy
code enhancements that I did for BACKUP_PROG_CRYPT_KEY
see https://github.com/rear/rear/issues/2155
and https://github.com/rear/rear/pull/2156

jsmeix commented at 2023-04-05 10:18:

https://github.com/rear/rear/commit/95f30827c3b51992c9b41aef9b3cf718c82eb58c
is a quick initial attempt to mitigate this issue.

schlomo commented at 2023-04-05 12:01:

I have two conflicting thoughts about this:

  1. in debug mode we should dump all variables and also give a hint that the dump can contain secrets - this helps with debugging
  2. in debug mode we should mask/hide secrets (as your suggestion does) and give a hint that secrets are skipped - and debug mode won't help with secrets related problems

Personally, I'd prefer option 1) (full dump) and expect users to check the output they send us. We could also add another warning to our GitHub issue template to remind users to scrape secrets from the debug log, if they set any.

For reference, other tools like for example the aws CLI also dump all secrets in debug mode and expect the user to be circumspect.

@rear/contributors other opinions?

jsmeix commented at 2023-04-05 12:09:

maintain a list of variables that contain confidential values
(e.g. a new array in default.conf that contains such variable names)
and hide/skip/suppress the values of those variables in the output
(we at ReaR usptream should never need secret values from a user
to debug an issue).

jsmeix commented at 2023-04-06 07:42:

Sleeping on it made it even more critical at least for me:

Now I have legal liability worries about it, at least
as far as I understand general principles of German law:

I think in general when someone does something
one has generally some reasonable amount of
legal liability for what one does.

I think in particular when making software this means
that one's program must not knowingly do something bad
except the user had explicity requested to do it.

In this case init/default/998_dump_variables.sh
leaks user's confidential values into the log file
without the user's explicit request to do exactly this.
Running a program in debug mode is no explicit request
to get (arbitrary) confidential values shown.

Furthermore - and this is the crucial point for me
why I have now legal liability worries:
It is now know to all of us ReaR upstream developers
that init/default/998_dump_variables.sh
does bad things for the user without explicit request.

I think - at least as far as I understand general principles
of German law - when one knows about something bad,
then one becomes to some reasonable extent responsible
to (try to) do something against that bad
as far as possible within reason.

jsmeix commented at 2023-04-06 07:54:

With
https://github.com/rear/rear/commit/65f0ad5729ce32eff0d07f4da44e93504a46d277
I feel better now (so I can have a relaxed Easter public holiday)
because now I did something against that (possibly) bad
as far as possible for me right now within reason.

jsmeix commented at 2023-04-06 08:00:

@rear/contributors
I think there is no longer urgency / time pressure here
so we can take our time to develop step by step
how to solve the generic problem properly.

The generic problem is that debug information may in general
reveal data that is confidential for the user, not only
obviously confidential data like secrets (e.g. passwords or keys)
but also non-obvious confidential data that could be misused
by others e.g. for spear phishing attacks or things like that.

schlomo commented at 2023-04-09 06:50:

@jsmeix I strongly disagree with this approach.

At some level ReaR debugging must be "complete", that is normal for every software. DEBUGSCRIPTS is actually our "strongest" debug mode so for me this is where full variable disclosure belongs. I'm happy to add extra warnings to it and also to the help message or manpage, but in the end full debug-level variable disclosure must be possible.

In any case, we won't be able to find a 100% "safe" solution here, as users can add additional variables with potential secrets.

About your worries under German law: That is why ReaR is shipped under the GPL where we explicitly shed responsiilities for what ReaR does after a user installs and uses it, like any other Open Source software. As long as there is no obvious malicious intent we are safe. Malicious intent could be a piece of code that uploads all users' secrets to a website while obfuscating what it does.

If it helps I'm happy to expand the variable dump feature to mask out all KEY|SECRET|PASSWORD type variables unless the user explicitly disables this behaviour.

BTW, your check for variable naming should probably be expanded to only match on the variable names and not content.

pcahyna commented at 2023-04-13 20:13:

For inspiration, I checked what Ansible does - there are similar concerns, because it is easy to show all Ansible module call parameters (including values) during playbook execution (merely by requesting verbose output), and secrets are passed to modules using parameters, so they would leak quite often. Ansible allows marking some module parameters as no_log and they are then omitted from the log output. If a parameter name seems to indicate that it is a secret and is not marked as no_log, Ansible shows the value, but emits a warning. This allows the module developer to catch the problem before the module gets to production.
I think we could reuse the heuristic that determines whether the parameter is a secret (apply it to shell variables). The regular expression used in Ansible is

^(.+[-_\s])?pass([-_\s]?(word|phrase|wrd|wd)?)([-_\s].+)?

We would use upper case of course, and we should add KEY to the possibilities (not sure about SECRET). I already see one false positive : BACKUP_DUPLICITY_ASK_PASSPHRASE.
The regex would be used to mask variables in variable dump (not just for warning, unlike in Ansible).

The question about debugscript/-D/set -x is a more difficult one: I see @jsmeix changed code to protect against that in #2156, but doing it for all such variables will be difficult.
By the way, is assigning such variables in local.conf dangerous ? I see the comment in default.conf:

# ... insert it between the single quotes of the following line:
#        { TTY_ROOT_PASSWORD=''; } 2>/dev/null
#    NOTE: stderr is redirected in the above line to avoid exposing the password hash
#          in the log file when ReaR runs in debugscript mode.

but there is also this comment

# In debugscript mode only scripts sourced by the Source function in lib/framework-functions.sh
# are run with 'set -x' but default.conf is sourced by usr/sbin/rear directly.
# See the comment of the UserInput function in lib/_input-output-functions.sh
# how to keep things confidential when usr/sbin/rear is run in debugscript mode

which seem contradictory.

Regarding

At some level ReaR debugging must be "complete", that is normal for every software. DEBUGSCRIPTS is actually our "strongest" debug mode so for me this is where full variable disclosure belongs.

this makes some sense, as an example Ansible also shows all values when you debug it itself by setting ANSIBLE_DEBUG=1 (even the no_log variables). But note that our issue template asks the user to submit logs generated using -D. So it would actually encourage users who report issues to leak confidential data, if this logging is "complete" in this sense, and I see why this is not desirable. Not sure what to do here - perhaps another debug level that would show really everything, but we would normally not request its output?

schlomo commented at 2023-04-14 12:19:

OK, how about we add a new option --debug-complete or --expose-secrets that enables the full variable dump?

jsmeix commented at 2023-04-17 07:40:

In general individual agreements (like GPL)
do not take precedence over general law.

In a specific case one needs to get a legal judgement
to find out how general law and individual agreements
apply (i.e. one has to actually take a court action).

Without a court decision about this particular case
there is the risk of legal issues.

jsmeix commented at 2023-04-17 07:50:

@schlomo
I strongly disagree with your approach.

In the past I never needed all variables values
to find the root cause of an issue in ReaR.

In the past the 'set -x' debugscript mode output
was sufficient for me to find the root cause.

If I needed even more details I could easily add
a script like yours or add commands for debugging
to existing scripts as needed in the specific case.
I vaguely remember there was one single case where
the latter was needed (i.e. I told the user to add some
specific commands for debugging into an existing script).

schlomo commented at 2023-04-17 14:53:

If I'm the only one who would like to see all variables in debug logs then I'm fine with removing that script altogether and relying on on-demand hacks if needed.

pcahyna commented at 2023-04-17 16:01:

I have felt a need for such a script myself some time ago (and not for debugging, so I would even introduce a separate workflow), so I would keep it, if the secrets issue can be addressed sufficiently.

schlomo commented at 2023-04-17 20:40:

@jsmeix @pcahyna how about we extract the script from the current place into a dedicated show-all-variables workflow that will then, as advertised, dump all variables, including secrets - of course with a suitable warning?

jsmeix commented at 2023-04-18 08:10:

I asked colleagues about their general thoughts regarding
"show also secrets in debug mode":

A colleague told me about this

# wpa_supplicant -h
...
 -K = include keys (passwords, etc.) in debug output

which matches what @schlomo suggests in
https://github.com/rear/rear/issues/2967#issuecomment-1508418115

Currently I think a command line option is the best way
to solve the conflict that @schlomo described in
https://github.com/rear/rear/issues/2967#issuecomment-1497372237

My reasoning:

My reasoning is primarily about legal things and usability:

A command line option must be explicitly typed in by the user.
When the command line option name tells what it means for the user
typing in that command line option name is an explicit request
by the user to let ReaR do exactly this.
Because running a program in whatever debug mode
is no explicit request to (also) get secret values shown
(if at all debug mode might be an implicit request),
the command line option name '--debug-complete'
is no explicit request to get secret values shown.
But the command line option name '--expose-secrets'
is an explicit request to get secret values shown.

A command line option provides better usability
(compared to a separated new workflow)
because it works for any workflow so that the user
can easily increase debug info step by step
for the workflow that fails for him like

# rear WORKFLOW
[fails]

# rear -d WORKFLOW
[insufficient info why it fails]

# rear -d --expose-secrets WORKFLOW
[indicates a possible reason of the failure]

# rear -D WORKFLOW
[not clear how exactly it fails]

# rear -D --expose-secrets WORKFLOW
[now it is clear how exactly it fails]

Finally with a generic command line option
we can implement what is right in each specific code place.
For example there might be very specific use cases where

# rear --expose-secrets WORKFLOW

could make sense even in normal (non-debug) node
e.g. to show some specific secrets on the user's terminal.

jsmeix commented at 2023-04-20 13:17:

@schlomo @pcahyna @gdha
if you agree with my above reasoning in my
https://github.com/rear/rear/issues/2967#issuecomment-1512648910
I would try to implement what @schlomo suggests in his
https://github.com/rear/rear/issues/2967#issuecomment-1508418115
i.e. implement a new command line option '--expose-secrets'.

jsmeix commented at 2023-04-27 10:52:

Regarding documentation:

By chance I noticed what I wrote in
https://github.com/rear/rear/issues/2917#issuecomment-1423967071
at "Caution with possible secrets in a full debug log file: ..."

jsmeix commented at 2023-04-27 11:25:

A side note FYI:

As shown by @pcahyna in his above
https://github.com/rear/rear/issues/2967#issuecomment-1507557009
leaking confidential values can also happen
in verbose mode of certain commands.

But - as far as I see - since
https://github.com/rear/rear/issues/2416
and
https://github.com/rear/rear/pull/2633
ReaR is no longer affected by this when ReaR is run in verbose mode
because we call commands in verbose mode via '-v' or '--verbose'
only when ReaR is run in debug mode, see usr/sbin/rear at
https://github.com/rear/rear/blob/rear-2.7/usr/sbin/rear#L272
and see also
https://github.com/rear/rear/issues/2416#issuecomment-855960979

jsmeix commented at 2023-05-09 14:05:

Mainly for my own information:

In
https://github.com/rear/rear/pull/2982#issuecomment-1540097471
I wrote (excerpt):

Perhaps I can solve the whole thing reasonably well via
https://github.com/rear/rear/issues/2967
by using 'source' for config files
to get things more secure for the user
plus
a sufficiently proper implementation that shows
all (non-secret) config variable values
in debug or debugscript mode.
My current offhanded idea is to maintain a list of
config variable names that can contain a secret value
(e.g. an array of variable names in default.conf
so the user can specify confidential config variable names
as needed to provide "final power to the user")
where it is only reported whether or not such variables have
a non-empty value but the actual secret value is not shown
unless usr/sbin/rear is run with the new command line option
'--expose-secrets'.

jsmeix commented at 2023-05-12 11:23:

Since https://github.com/rear/rear/pull/2982
those are the config variables that could have secret values:

# grep '{ .* ; } 2>/dev/null' usr/share/rear/conf/default.conf  | grep -v '^#'

{ OPAL_PBA_DEBUG_PASSWORD='' ; } 2>/dev/null
{ OPAL_PBA_TKNKEY='tpm:opalauthtoken:7' ; } 2>/dev/null
{ BACKUP_PROG_CRYPT_KEY="${BACKUP_PROG_CRYPT_KEY:-}" ; } 2>/dev/null
{ TTY_ROOT_PASSWORD='' ; } 2>/dev/null
{ SSH_ROOT_PASSWORD='' ; } 2>/dev/null
{ GALAXY11_PASSWORD=${GALAXY11_PASSWORD:-} ; } 2>/dev/null
{ ZYPPER_ROOT_PASSWORD='root' ; } 2>/dev/null
{ YUM_ROOT_PASSWORD='root' ; } 2>/dev/null

For each of them I will check our code that
we do not leak their values into the log file.

For BACKUP_PROG_CRYPT_KEY this was already done via
https://github.com/rear/rear/pull/2156

For GALAXY11_PASSWORD this is done right now via
https://github.com/rear/rear/pull/2985

jsmeix commented at 2023-05-12 11:25:

SSH_ROOT_PASSWORD:

# find usr/sbin/rear usr/share/rear -type f | xargs grep '$SSH_ROOT_PASSWORD' | grep -v ': *#'

usr/share/rear/build/default/500_ssh_setup.sh:    [[ -n "$SSH_ROOT_PASSWORD" ]] && password_authentication_value=yes
usr/share/rear/rescue/default/500_ssh.sh:    test "$SSH_ROOT_PASSWORD" && LogPrintError "SSH_ROOT_PASSWORD cannot work when SSH_FILES is set to a 'false' value"
usr/share/rear/rescue/default/500_ssh.sh:if ! test -f "$ROOT_HOME_DIR/.ssh/authorized_keys" -o "$SSH_ROOT_PASSWORD" ; then
usr/share/rear/rescue/default/500_ssh.sh:if test "$SSH_ROOT_PASSWORD" ; then
usr/share/rear/rescue/default/500_ssh.sh:    case "$SSH_ROOT_PASSWORD" in
usr/share/rear/rescue/default/500_ssh.sh:            echo "root:$SSH_ROOT_PASSWORD:::::::" > $ROOTFS_DIR/etc/shadow
usr/share/rear/rescue/default/500_ssh.sh:            echo "root:$( echo $SSH_ROOT_PASSWORD | openssl passwd -1 -stdin ):::::::" > $ROOTFS_DIR/etc/shadow
usr/share/rear/restore/YUM/default/970_set_root_password.sh:test "$SSH_ROOT_PASSWORD" && root_password="$SSH_ROOT_PASSWORD"
usr/share/rear/restore/ZYPPER/default/970_set_root_password.sh:test "$SSH_ROOT_PASSWORD" && root_password="$SSH_ROOT_PASSWORD"

SSH_ROOT_PASSWORD should be fixed via
https://github.com/rear/rear/pull/2986
which by the way also fixes
ZYPPER_ROOT_PASSWORD in current master code

# find usr/sbin/rear usr/share/rear -type f | xargs grep '$ZYPPER_ROOT_PASSWORD' | grep -v ': *#'

usr/share/rear/restore/ZYPPER/default/970_set_root_password.sh:test "$ZYPPER_ROOT_PASSWORD" && root_password="$ZYPPER_ROOT_PASSWORD"
`

and YUM_ROOT_PASSWORD in current master code

# find usr/sbin/rear usr/share/rear -type f | xargs grep '$YUM_ROOT_PASSWORD' | grep -v ': *#'

usr/share/rear/restore/YUM/default/970_set_root_password.sh:test "$YUM_ROOT_PASSWORD" && root_password="$YUM_ROOT_PASSWORD"

jsmeix commented at 2023-05-12 12:05:

TTY_ROOT_PASSWORD:

# find usr/sbin/rear usr/share/rear -type f | xargs grep '$TTY_ROOT_PASSWORD' | grep -v ': *#'

usr/share/rear/build/default/503_store_tty_root_password.sh:{ [[ -n "$TTY_ROOT_PASSWORD" ]] && echo "$TTY_ROOT_PASSWORD" > "$ROOTFS_DIR/.TTY_ROOT_PASSWORD"; } 2>/dev/null
usr/share/rear/skel/default/bin/login:            if [[ "$password_signature" == "$TTY_ROOT_PASSWORD" ]]; then  # Password matches: Success!
usr/share/rear/prep/default/340_include_password_tools.sh:{ [[ -n "$TTY_ROOT_PASSWORD" ]] && REQUIRED_PROGS+=( openssl ); } 2>/dev/null

Things were already done for
usr/share/rear/build/default/503_store_tty_root_password.sh
and
usr/share/rear/prep/default/340_include_password_tools.sh
via
https://github.com/rear/rear/pull/2539/commits/8bc2b1cccccbd4806feb0bfa5fdcc41d0ca47942
and the subsequent merge commit
https://github.com/rear/rear/commit/54b81fffa0ffec75e43daddefb7b1a1a58100ff9

In
https://github.com/rear/rear/pull/2539
I found
https://github.com/rear/rear/pull/2539#discussion_r540783195
and
https://github.com/rear/rear/issues/2540
which got lost at that time (between Dec. 2020 and Feb. 2021)

Regarding
usr/share/rear/skel/default/bin/login
I noticed no comment that tells that also all is OK in this case.
I guess all is OK in this case because I assume this script
is never run with 'set -x' (in the ReaR recovery system).

jsmeix commented at 2023-05-12 12:33:

OPAL_PBA_DEBUG_PASSWORD:

# find usr/sbin/rear usr/share/rear -type f | xargs grep '$OPAL_PBA_DEBUG_PASSWORD' | grep -v ': *#'

usr/share/rear/build/OPALPBA/Linux-i386/820_store_settings.sh:OPAL_PBA_DEBUG_PASSWORD='$OPAL_PBA_DEBUG_PASSWORD'
usr/share/rear/skel/default/etc/scripts/unlock-opal-disks:    [[ -z "$OPAL_PBA_DEBUG_PASSWORD" ]] && return 1  # debug password not configured
usr/share/rear/skel/default/etc/scripts/unlock-opal-disks:    IFS='\$' read -r _ method salt _ <<<"$OPAL_PBA_DEBUG_PASSWORD"
usr/share/rear/skel/default/etc/scripts/unlock-opal-disks:    [[ "$password_signature" == "$OPAL_PBA_DEBUG_PASSWORD" ]]
usr/share/rear/prep/OPALPBA/Linux-i386/001_configure_workflow.sh:[[ -n "$OPAL_PBA_DEBUG_PASSWORD" ]] && REQUIRED_PROGS+=( openssl )

As far as I see it seems
usr/share/rear/build/OPALPBA/Linux-i386/820_store_settings.sh
is OK because

cat > FILE << EOF
secret value
EOF

does not show 'secret value' when that is run with 'set -x'
because in this case 'set -x' only shows '+ cat'
at least for me with GNU bash version 4.4.23

# OPAL_PBA_DEBUG_PASSWORD=mypassword

# set -x

# cat > OPAL_PBA_SETTINGS.sh << -EOF-
> OPAL_PBA_DEBUG_PASSWORD='$OPAL_PBA_DEBUG_PASSWORD'
> -EOF-
+ cat

# set +x

# cat OPAL_PBA_SETTINGS.sh
OPAL_PBA_DEBUG_PASSWORD='mypassword'

As far as I see it seems also
usr/share/rear/skel/default/etc/scripts/unlock-opal-disks
is OK because it seems this is run via

# find usr/sbin/rear usr/share/rear -type f | xargs grep 'unlock-opal-disks' | grep -v ': *#'

usr/share/rear/skel/default/usr/lib/systemd/system/sysinit-opalpba.service:ExecStart=/etc/scripts/unlock-opal-disks

usr/share/rear/skel/OPALPBA/etc/inittab:ss::bootwait:/etc/scripts/unlock-opal-disks

and I assume what is run this way
is never run with 'set -x' (in the ReaR recovery system).

So the only remaining part is
usr/share/rear/prep/OPALPBA/Linux-i386/001_configure_workflow.sh
which should be fixed via
https://github.com/rear/rear/commit/64dd3ba9bd3ca7b6bf335a534bb5af5a71532d8e

jsmeix commented at 2023-05-12 13:04:

OPAL_PBA_TKNKEY:

# find usr/sbin/rear usr/share/rear -type f | xargs grep '$OPAL_PBA_TKNKEY' | grep -v ': *#'

usr/share/rear/build/OPALPBA/Linux-i386/820_store_settings.sh:OPAL_PBA_TKNKEY='$OPAL_PBA_TKNKEY'
usr/share/rear/skel/default/etc/scripts/unlock-opal-disks:            at_password=$(authtkn_load "$authtkn_path" "$OPAL_PBA_TKNOFFSET" "$OPAL_PBA_TKNKEY" "$authtkn_bind2dev" 2>>$authtkn_stderr)
usr/share/rear/skel/default/etc/scripts/unlock-opal-disks:                    if authtkn_store "$authtkn_path" "$OPAL_PBA_TKNOFFSET" "$at_password" "$OPAL_PBA_TKNKEY" "$authtkn_bind2dev" >>$authtkn_stderr 2>&1; then

usr/share/rear/build/OPALPBA/Linux-i386/820_store_settings.sh
and
usr/share/rear/skel/default/etc/scripts/unlock-opal-disks
were already considered to look OK right before in
https://github.com/rear/rear/issues/2967#issuecomment-1545674020

codefritzel commented at 2023-05-24 20:05:

@jsmeix
How about moving something like that into functions like LogSecret "$secret_string" or SecretCommand "$cmd" which is only displayed when the --expose-secrets is activated?
Especially in development and debugging can this be very useful.
To test #2991, I had to remove 2>/dev/null to see if the right parameters were passed to lftp.

jsmeix commented at 2023-05-25 11:09:

@codefritzel
I assume you mean something like

if { COMMAND $SECRET_ARGUMENT ; } 2>/dev/null ; then
    Log "COMMAND succeeded"
else
    { LogSecret "'COMMAND $SECRET_ARGUMENT' failed with exit code $?" ; } 2>/dev/null
    Error "COMMAND failed"
fi

I like the idea and as far as I see I think it can be implemented.
The LogSecret command itself must be within '{ ... ; } 2>/dev/null'
because otherwise $SECRET_ARGUMENT in the LogSecret command
would leak into the log file in debuscript mode via 'set -x'.

jsmeix commented at 2023-05-25 11:22:

Only an offhanded idea:

Alternatively we may introduce in usr/sbin/rear by default

SECRET_OUTPUT_DEV="null"

and set it to

SECRET_OUTPUT_DEV="stderr"

only if '--expose-secrets' is set
in a similar way as we currently use
DEBUG_OUTPUT_DEV and DISPENSABLE_OUTPUT_DEV

For example like

if { COMMAND $SECRET_ARGUMENT ; } 2>>/dev/$SECRET_OUTPUT_DEV ; then
    Log "COMMAND succeeded"
else
    { Log "'COMMAND $SECRET_ARGUMENT' failed with exit code $?" ; } 2>>/dev/$SECRET_OUTPUT_DEV
    Error "COMMAND failed"
fi

No special LogSecret function is needed and now
also the command call itself can appear in the
log file in debuscript mode via 'set -x'
when additionally --expose-secrets is set.

Additionally the

{ ... ; } 2>>/dev/$SECRET_OUTPUT_DEV

syntax makes it clear which redirections are
explicitly meant to hide secrets to distinguish them
from usual unwanted output discard via '2>/dev/null'

By the way:
The

Log "COMMAND succeeded"

is needed to have something in the log because
the command call itself is normally invisible.

codefritzel commented at 2023-05-25 20:46:

Only an offhanded idea:

Alternatively we may introduce in usr/sbin/rear by default

SECRET_OUTPUT_DEV="null"

and set it to

SECRET_OUTPUT_DEV="stderr"

only if '--expose-secrets' is set in a similar way as we currently use DEBUG_OUTPUT_DEV and DISPENSABLE_OUTPUT_DEV

For example like

if { COMMAND $SECRET_ARGUMENT ; } 2>>/dev/$SECRET_OUTPUT_DEV ; then
    Log "COMMAND succeeded"
else
    { Log "'COMMAND $SECRET_ARGUMENT' failed with exit code $?" ; } 2>>/dev/$SECRET_OUTPUT_DEV
    Error "COMMAND failed"
fi

No special LogSecret function is needed and now also the command call itself can appear in the log file in debuscript mode via 'set -x' when additionally --expose-secrets is set.

Additionally the

{ ... ; } 2>>/dev/$SECRET_OUTPUT_DEV

syntax makes it clear which redirections are explicitly meant to hide secrets to distinguish them from usual unwanted output discard via '2>/dev/null'

By the way: The

Log "COMMAND succeeded"

is needed to have something in the log because the command call itself is normally invisible.

I think the idea is very good. For debugging purposes, the user or developer can see the executed command, if activated.

@jsmeix
Do you want to implement this and create a WIP RP?

jsmeix commented at 2023-05-26 06:34:

@codefritzel
I will try it out - likely next week (as time permits)
and if it works OK for me I will make a pull request.

jsmeix commented at 2023-06-05 12:43:

Ouch!
One must be super careful to not leak secrets by accident:

if { COMMAND $SECRET_ARGUMENT ; } 2>>/dev/$SECRET_OUTPUT_DEV ; then
    Log "COMMAND succeeded"
else
    { Log "'COMMAND $SECRET_ARGUMENT' failed with exit code $?" ; } 2>>/dev/$SECRET_OUTPUT_DEV
    Error "COMMAND failed"
fi

leaks $SCRET_ARGUMENT when COMMAND failed because

{ Log "'COMMAND $SECRET_ARGUMENT' ... " ; } 2>>/dev/null

outputs into the log file in any case because
in lib/_input-output-functions.sh there is

function Log () {
    ...
    echo "$log_message" >>"$RUNTIME_LOGFILE" || true
}

So we also need the 'LogSecret' function.

jsmeix commented at 2023-06-06 12:20:

In https://github.com/rear/rear/pull/3006
I implemented the basics of the new

  • -e / --expose-secrets option for sbin/rear
  • SECRET_OUTPUT_DEV
  • LogSecret function

Nothing is documented yet in "rear help" or "man rear"
until I did some more tests and until I got some feedback
what others think about it and how it behaves for them.

[Export of Github issue for rear/rear.]