#2985 PR `merged`: Do not leak the GALAXY11_PASSWORD value into the log file¶

Labels: fixed / solved / done, critical / security / legal

jsmeix opened issue at 2023-05-11 12:03:¶

Type: Bug Fix
Impact: Critical
Reference to related issue (URL):
https://github.com/rear/rear/pull/2156
How was this pull request tested?
I cannot test it because I do not have the needed backup software
Brief description of the changes in this pull request:

In
verify/GALAXY11/default/420_login_to_galaxy_and_setup_environment.sh
run commands that deal with GALAXY11_PASSWORD
in a confidential way via

{ confidential_command ; } 2>/dev/null

to not leak the GALAXY11_PASSWORD value into the log file

jsmeix commented at 2023-05-11 12:36:¶

verify/GALAXY11/default/420_login_to_galaxy_and_setup_environment.sh
is the only place where GALAXY11_PASSWORD is used according to

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

usr/share/rear/conf/default.conf:
{ GALAXY11_PASSWORD=${GALAXY11_PASSWORD:-} ; } 2>/dev/null

usr/share/rear/verify/GALAXY11/default/420_login_to_galaxy_and_setup_environment.sh:
    if [ -n "$GALAXY11_USER" ] && [ -n "$GALAXY11_PASSWORD" ]; then
            qlogin -u "${GALAXY11_USER}" -clp "${GALAXY11_PASSWORD}" || \
                    Error "Could not logon to Commvault CommServe with credentials from GALAXY11_USER ($GALAXY11_USER) and GALAXY11_PASSWORD. Check the log file."
            LogPrint "CommVault client logged in with credentials from GALAXY11_USER ($GALAXY11_USER) and GALAXY11_PASSWORD"

codefritzel commented at 2023-05-11 15:35:¶

@jsmeix
I tested your stand and it worked with Commvault (Galaxy11)

a comparison
e.g. GALAXY11_USER="my_commvault_user" and GALAXY11_PASSWORD="my_commvault_secret"

actual master version:

grep -n "my_commvault_secret" rear-myserver.log
2682:++ '[' -n 'my_commvault_secret' ']'
2683:++ qlogin -u 'my_commvault_user' -clp 'my_commvault_secret'

grep -n "qlogin" rear-myserver.log
2683:++ qlogin -u 'my_commvault_user' -clp 'my_commvault_secret'

your version:

grep -n "my_commvault_secret" rear-myserver-new.log
[empty]

grep -n "qlogin" rear-myserver-new.log
[empty]

In my opinion, it is very good to hide the credentials so that they are not uploaded by mistake.

Otherwise, in some places it may be useful to see them. But by default this should be disabled. This could be activated by an additional CMD line parameter (see 2967).

jsmeix commented at 2023-05-12 06:53:¶

@codefritzel
thank you so much for testing it and
for your comment what your preferred default behaviour is!

jsmeix commented at 2023-05-12 07:15:¶

@rear/contributors
I would like to merge it today afternoon
unless there are objections.

I know that with

{ qlogin -u "${GALAXY11_USER}" -clp "${GALAXY11_PASSWORD}" ; } 2>/dev/null

there are no longer any 'qlogin' stderr messages in the log
which could make it harder to find the root cause
when 'qlogin' fails - depending on what messages 'qlogin'
shows on stdout or stderr when it fails.

I think it is sufficient to see that 'qlogin' fails
(which is made clear here via the Error exit)
and then it is the user's task to debug on his own
why exactly 'qlogin' fails with his credentials
on his system in his environment.
At least I would never ever go so far to debug issues
where I would need to know secret user data
for further debugging.

In general user data is sacrosanct so
secret user data is topmost sacrosanct
which means some reasonable level of protection
from accidentally publishing secret user data
outweighs drawbacks in debugging specific cases.

schlomo commented at 2023-05-12 07:46:¶

You could add an error messages saying that qlogin failed and should be retried on the console to see the error

jsmeix commented at 2023-05-12 09:29:¶

A side note FYI:

My recent
https://github.com/rear/rear/pull/2985/commits/4a3865abd75484cb7c347c7af73f7925010722eb
shows an interesting bash coding subtleness:

# Using "if COMMAND ; then ... ; else echo COMMAND failed with $? ; fi" is mandatory
# because "if ! COMMAND ; then echo COMMAND failed with $? ..." shows wrong $? because '!' results $?=0

How bash behaves with $? versus $PIPESTATUS:

# true ; echo $?
0

# false ; echo $?
1

# ! true ; echo $?
1

# ! false ; echo $?
0

# true ; echo $PIPESTATUS
0

# false ; echo $PIPESTATUS
1

# ! true ; echo $PIPESTATUS
0

# ! false ; echo $PIPESTATUS
1

This is because '!' is a reserverd word in bash
so it behaves like a command i.e. it sets $?
so one has to avoid '!' like

# if ! cat qqqq ; then echo failed with $? ; else echo worked with $? ; fi
cat: qqqq: No such file or directory
failed with 0

# if cat qqqq ; then echo worked with $? ; else echo failed with $? ; fi
cat: qqqq: No such file or directory
failed with 1

Excerpts from "man bash" (GNU bash version 4.4.23)

A pipeline is a sequence of one or more commands ...

The return status of a pipeline is the exit status
of the last command, unless ...

If the reserved word ! precedes a pipeline,
the exit status of that pipeline is the logical negation
of the exit status as described above.

PIPESTATUS
An array variable ... containing a list of exit status values
from the processes in the most-recently-executed foreground
pipeline (which may contain only a single command).

jsmeix commented at 2023-05-12 13:40:¶

As an addedum to make the code easier to read
in particular to avoid needless nested braces { ... {...} ... {...} ... }
I removed superfluous braces {...} around variable names via
https://github.com/rear/rear/commit/07462c35c2c589343f8a57811e2e84167fabbb4f

[Export of Github issue for rear/rear.]

#2985 PR merged: Do not leak the GALAXY11_PASSWORD value into the log file¶