Update default.conf #2982

jsmeix · 2023-05-09T07:26:33Z

Type: Cleanup
Impact: Normal
Reference to related issue (URL):
Follow-up of
Increase USER_INPUT_INTERRUPT_TIMEOUT default from 10 to 30 seconds #2981
How was this pull request tested?
Not tested
Brief description of the changes in this pull request:

In default.conf cleaned up
all cases of config variables for secret values
i.e. have a generic explanation comment at the beginning
instead of several similar comments at each place

In default.conf cleaned up all cases of config variables for secret values i.e. have a generic explanation comment at the beginning instead of several similar comments at each place

When VAR is for a secret value we must also in case of VAR="${VAR:-default}" use { VAR="${VAR:-default}" ; } 2>/dev/null otherwise when VAR is already set as VAR=secret then with 'set -x' the plain VAR="${VAR:-default}" would reveal the secret in the log as 'VAR=secret'

jsmeix · 2023-05-09T09:08:38Z

In my recent
120de78
the comment is not fully correct because
it talks about VAR="${VAR:-default}"
(which I did not enter manually but via copy&paste - sigh!)
where { VAR="${VAR:-default}" ; } 2>/dev/null
must be used in any case to avoid 'VAR=default' in the log.

What is actually meant is the VAR="${VAR:-}" case
where the default value is empty
so 'VAR=' in the log does not reveal a secret.
But when VAR is already set as VAR=secret
then with 'set -x' the plain VAR="${VAR:-}"
would reveal the secret in the log as 'VAR=secret'.

usr/share/rear/conf/default.conf

Keep the information that default.conf is sourced by usr/sbin/rear directly (using 'source') while {site,local}.conf are sourced using Source() which can 'set -x'.

jsmeix · 2023-05-09T13:10:22Z

Regarding
#2982 (comment)

Wouldn't it be better though to avoid sourcing {site,local}.conf
with Source and thus avoid the possibility of '-x' being set
and exposing secret values entirely, without having to protect
every private variable (and thus having to remember to do it)?

Yes and no ;-)

Yes,
because using 'source' for config files makes things
more secure for the user who may forget to use the
rather special confidential way in his config files
to set secret config variable values.

No,
because using 'source' for config files makes it
harder for us to see in a debugscript log file
what (non-secret) config variable values the user
has actually set in all his config files.

And - as always - tertium datur:
Perhaps I can solve the whole thing reasonably well via
#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'.

pcahyna · 2023-05-09T14:26:53Z

Regarding #2982 (comment)
Wouldn't it be better though to avoid sourcing {site,local}.conf
with Source and thus avoid the possibility of '-x' being set
and exposing secret values entirely, without having to protect
every private variable (and thus having to remember to do it)?
Yes and no ;-)

Yes, because using 'source' for config files makes things more secure for the user who may forget to use the rather special confidential way in his config files to set secret config variable values.

Also, we should remember to declare every new secret variable in default.conf this way and if we do, it makes the file uglier.

No, because using 'source' for config files makes it harder for us to see in a debugscript log file what (non-secret) config variable values the user has actually set in all his config files.

Have you actually used a debugscript log file to find out what the user has set? Personally I always ask users to provide their {local,site}.conf files if I need to see configuration parameters for debugging.

And - as always - tertium datur: Perhaps I can solve the whole thing reasonably well via #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'.

You could use a heuristics : see the regex in #2967 (comment).
Also, if you need config variables in log file for debugging, and if using just source to source configuration files, then the DEBUG mode should output all config variable values at the beginning of the log (except the secret values, unless --expose-secrets is given). It could make it easier to investigate what parameters are set than by digging in the set -x debug output (OTOH you would see all the configuration values, evenb defaults, not just those set by the user).

Another idea: perhaps we could introduce yet another config file called private.conf (or similar) where we would instruct the user to put secret values? This way we could use plain source just for this file, and also it would be less likely to expose secret values in bug reports (we can say, provide us your local.conf and site.conf but not private.conf).

schlomo · 2023-05-10T13:10:07Z

usr/share/rear/conf/default.conf

@@ -2264,7 +2266,8 @@ GALAXY11_Q_ARGUMENTFILE=
 # CommVault login credentials for restore
 # Remember to adequately protect the rescue media if you include credentials in it
 GALAXY11_USER=${GALAXY11_USER:-}
-GALAXY11_PASSWORD=${GALAXY11_PASSWORD:-}
+{ GALAXY11_PASSWORD=${GALAXY11_PASSWORD:-} ; } 2>/dev/null
+# In local.conf set it confidentially via { GALAXY11_PASSWORD='secret_password' ; } 2>/dev/null


I'd prefer this type of explanation to appear only once at the top

I have this special additional explanation because

{ GALAXY11_PASSWORD=${GALAXY11_PASSWORD:-} ; } 2>/dev/null

is not a correct template how to set it in local.conf but

{ GALAXY11_PASSWORD='secret_password' ; } 2>/dev/null

is a template which can be used even for copy&paste.

I'd like to leave at least a little bit of own thinking to our users, so having a general explanation for how to set secret variables at the top of default.conf should really suffice. For my taste, we already have way too many long explanations in our default.conf that I need to scroll by while looking for the actual variables.

My experience over almost 25 years with various kind of users
tells that the majority of users who think about what they do
is not the problem - the problem are the few who do not think.

Nevertheless - hope dies last - so:
1d8d0e8

I understand that the long explanatory comments in default.conf
make it hard to get a concise overview of the config variables.

I think without any comments

# grep -v '^#' usr/share/rear/conf/default.conf

outputs too little information.

So I suggest to distinguish between essential comments
and additional explanatory comments by using
different comment marks, for example
'# ' for essential comments and
'## ' for additional explanatory comments
like:

#### # VAR # What to do. ## Via VAR you can specify what to do as an array of strings. ## The strings must be of the following form ... ## For example you could specify ... ## VAR=( ... ) ## Alternativery you may even use ## VAR=( ... ) ## for the special use case of ... # By default do this, that, and something else: VAR=( 'this' 'that' 'something else' ) ####

schlomo · 2023-05-10T13:13:36Z

Yes, we can add a new config (I'd call it secrets.conf) to be more explicit, but we won't be able to prevent users "doing it wrong". OTOH, I also find it totally acceptable to have users use the { VAR=value ; } 2>/dev/null syntax because that will force them to think about this secrets management issue. Maybe another 2 lines of explanation in local.conf and in the man page are enough for that?

In the sense of keeping things simple I'd prefer such a "low tech" approach, at least for now and as long as we use Bash to read configuration variables.

In etc/rear/local.conf added an explanation how to set a secret value

jsmeix · 2023-05-10T13:51:37Z

Via
211495b
I added an explanation how to set a secret value
in etc/rear/local.conf

Regarding the man page:
I would like to do that in a more general way via
#2967
because then I got a better overview how all that
is meant to work and to be used in ReaR.

jsmeix · 2023-05-10T14:11:22Z

@pcahyna regarding your
#2982 (comment)

of course it is our duty to implement special care
when dealing with secrets - usually with special code as in
#2156

In the past I have used the debugscript log file
a few times to find out what the user has set
in particular when the user did not yet provide
his config or did not provide his whole config
or provided some config manually with typos.
But I won't mind if config values are no longer
in the log file - it only makes it sometimes
harder for us to get the config values.

Show all values, also defaults, not just those set by the user,
and with --expose-secrets even secret values,
is what I like to implement via
#2967

jsmeix · 2023-05-10T14:27:53Z

A totally offhanded perhaps a bit crazy idea:

When all possibly secret variables are set in default.conf
in one of those ways

{ VAR='' ; } 2>/dev/null
{ VAR='default' ; } 2>/dev/null
{ VAR="${VAR:-}" ; } 2>/dev/null
{ VAR="${VAR:-default}" ; } 2>/dev/null

we could extract those variable names via some regexp
and check the user config files whether or not
those variable names appear in user config files without

{ VAR=... ; } 2>/dev/null

I.e. do something similar as what we currently do via
init/default/001_verify_config_arrays.sh

schlomo

One more extra explanation.

I was thinking that of course default values can never be secret, so that strictly speaking it is not required to do for example { YUM_ROOT_PASSWORD='root' ; } 2>/dev/null.

I do see value in using this way of specifying the variables as a way to mark for us and our users that we mean this to be a secret variable.

Thanks a lot for cleaning up the default conf and feel free to merge when you are ready.

usr/share/rear/conf/default.conf

Less duplicate expanation in local.conf - instead a generic reference to default.conf

In doc/user-guide/04-scenarios.adoc explain that when a command like export VAR='secret_value' is run on the original system, then one must ensure to not keep that command in a shell history file. This is an addedum to #2156 which was triggered by what was done in #2982

jsmeix · 2023-05-11T09:34:30Z

In the new explanatory comment in default.conf the part

Do not use something like
  echo 'my_secret_password' | openssl passwd -6 -stdin
because that stores the whole command in a history file (e.g. ~/.bash_history)
(unless you know how to run commands without keeping the history).

triggered
edd7097

jsmeix · 2023-05-11T13:10:14Z

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.

According to the output of

# for v in OPAL_PBA_DEBUG_PASSWORD OPAL_PBA_TKNKEY BACKUP_PROG_CRYPT_KEY TTY_ROOT_PASSWORD SSH_ROOT_PASSWORD ZYPPER_ROOT_PASSWORD YUM_ROOT_PASSWORD ; \
 do echo $v ; \
  find usr/sbin/rear usr/share/rear -type f \
   | xargs grep "\$$v" \
   | grep -v ': *#' ; \
  echo =================== ; done

all except BACKUP_PROG_CRYPT_KEY
leak their value into the log file.

For GALAXY11_PASSWORD I did already
#2985

Update default.conf

da0cd3b

In default.conf cleaned up all cases of config variables for secret values i.e. have a generic explanation comment at the beginning instead of several similar comments at each place

jsmeix added the cleanup label May 9, 2023

jsmeix added this to the ReaR v2.8 milestone May 9, 2023

jsmeix requested a review from a team May 9, 2023 07:26

jsmeix self-assigned this May 9, 2023

jsmeix mentioned this pull request May 9, 2023

Increase USER_INPUT_INTERRUPT_TIMEOUT default from 10 to 30 seconds #2981

Merged

pcahyna reviewed May 9, 2023

View reviewed changes

usr/share/rear/conf/default.conf Show resolved Hide resolved

Update default.conf

3c580c5

Keep the information that default.conf is sourced by usr/sbin/rear directly (using 'source') while {site,local}.conf are sourced using Source() which can 'set -x'.

jsmeix mentioned this pull request May 9, 2023

Confidential values leaked into log file in debug mode #2967

Closed

schlomo reviewed May 10, 2023

View reviewed changes

Update local.conf

211495b

In etc/rear/local.conf added an explanation how to set a secret value

Update default.conf

1d8d0e8

schlomo approved these changes May 10, 2023

View reviewed changes

usr/share/rear/conf/default.conf Show resolved Hide resolved

Update local.conf

d404887

Less duplicate expanation in local.conf - instead a generic reference to default.conf

jsmeix merged commit 4b5dc28 into master May 11, 2023
16 checks passed

jsmeix added the fixed / solved / done label May 11, 2023

jsmeix deleted the jsmeix-default-conf-secrets branch May 11, 2023 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update default.conf #2982

Update default.conf #2982

jsmeix commented May 9, 2023 •

edited

jsmeix commented May 9, 2023

jsmeix commented May 9, 2023

pcahyna commented May 9, 2023 •

edited

schlomo May 10, 2023

jsmeix May 10, 2023

schlomo May 10, 2023

jsmeix May 10, 2023

jsmeix May 11, 2023

schlomo commented May 10, 2023

jsmeix commented May 10, 2023 •

edited

jsmeix commented May 10, 2023

jsmeix commented May 10, 2023

schlomo left a comment

jsmeix commented May 11, 2023

jsmeix commented May 11, 2023 •

edited

Update default.conf #2982

Update default.conf #2982

Conversation

jsmeix commented May 9, 2023 • edited

jsmeix commented May 9, 2023

jsmeix commented May 9, 2023

pcahyna commented May 9, 2023 • edited

schlomo May 10, 2023

Choose a reason for hiding this comment

jsmeix May 10, 2023

Choose a reason for hiding this comment

schlomo May 10, 2023

Choose a reason for hiding this comment

jsmeix May 10, 2023

Choose a reason for hiding this comment

jsmeix May 11, 2023

Choose a reason for hiding this comment

schlomo commented May 10, 2023

jsmeix commented May 10, 2023 • edited

jsmeix commented May 10, 2023

jsmeix commented May 10, 2023

schlomo left a comment

Choose a reason for hiding this comment

jsmeix commented May 11, 2023

jsmeix commented May 11, 2023 • edited

jsmeix commented May 9, 2023 •

edited

pcahyna commented May 9, 2023 •

edited

jsmeix commented May 10, 2023 •

edited

jsmeix commented May 11, 2023 •

edited