#2982 PR merged: Update default.conf¶
Labels: cleanup, fixed / solved / done
jsmeix opened issue at 2023-05-09 07:26:¶
-
Type: Cleanup
-
Impact: Normal
-
Reference to related issue (URL):
Follow-up of
https://github.com/rear/rear/pull/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
jsmeix commented at 2023-05-09 09:08:¶
In my recent
https://github.com/rear/rear/pull/2982/commits/120de787904e4b94eb07e2a3a0ccc24f9204cdf6
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'.
jsmeix commented at 2023-05-09 13:10:¶
Regarding
https://github.com/rear/rear/pull/2982#discussion_r1188485703
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
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'.
pcahyna commented at 2023-05-09 14:26:¶
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
https://github.com/rear/rear/issues/2967#issuecomment-1507557009.
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 commented at 2023-05-10 13:13:¶
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.
jsmeix commented at 2023-05-10 13:51:¶
Via
https://github.com/rear/rear/pull/2982/commits/211495bf02695b88157f62c8cd4b291290436fc6
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
https://github.com/rear/rear/issues/2967
because then I got a better overview how all that
is meant to work and to be used in ReaR.
jsmeix commented at 2023-05-10 14:11:¶
@pcahyna regarding your
https://github.com/rear/rear/pull/2982#issuecomment-1540235162
of course it is our duty to implement special care
when dealing with secrets - usually with special code as in
https://github.com/rear/rear/pull/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
https://github.com/rear/rear/issues/2967
jsmeix commented at 2023-05-10 14:27:¶
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
jsmeix commented at 2023-05-11 09:34:¶
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
https://github.com/rear/rear/commit/edd70979067c2da531df88637a1280da482ca413
jsmeix commented at 2023-05-11 13:10:¶
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
https://github.com/rear/rear/pull/2985
[Export of Github issue for rear/rear.]