#2011 PR `merged`: Allow non-interactive rsync authentication¶

Labels: enhancement, cleanup, fixed / solved / done

ivarmu opened issue at 2018-12-26 17:04:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Please fill in the following items before submitting a new pull request:

Pull Request Details:¶

Type: Enhancement
Impact: Low
Reference to related issue (URL): No issue existing
How was this pull request tested?: at my installation:

`
Source system: Fedora release 28
ReaR Version: rear-2.4-1.fc28.x86_64

Remote system: QNAP TS-253A (Version QTS 4.3.4 (20180830))
`

Brief description of the changes in this pull request:

I've added the correct variables to allow non-interactive authentication using rsync protocol. Now, one can use RSYNC_OPTIONS to authenticate using the "--password-file=/full/path/to/file" rsync's option.

I already fixed the missing username when rsync protocol at checking stage.

gdha commented at 2018-12-28 11:48:¶

@ivarmu Why did you not use the BACKUP_RSYNC_OPTIONS variable to add your additional option --password-file=/full/path/to/file? You could accomplish this in the local.conf file:
For example:

BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" --password-file=/full/path/to/file )

If the above works then this PR is not required anymore.

ivarmu commented at 2018-12-28 13:33:¶

If I remember correctly, the first steps involve a simple ssh/rsync
conection to check connectivity, and as per this step, I think
BACKUP_RSYNC_OPTIONS is not the most convenient variable to be used.

What do you think?

Thanks!
Ivan

El vie., 28 dic. 2018 12:48, gdha notifications@github.com escribió:

@ivarmu https://github.com/ivarmu Another remark - why did you not use
the BACKUP_RSYNC_OPTIONS variable to add your additional option
--password-file=/full/path/to/file? You could accomplish this in the
local.conf file:
For example:

BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" --password-file=/full/path/to/file )

If the above works then this PR is not required anymore.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rear/rear/pull/2011#issuecomment-450347310, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AZlFm4ENOKSKiOFMYfryX8Rhjzb7z843ks5u9gUKgaJpZM4Zh6-X
.

gdha commented at 2019-01-09 07:32:¶

@ivarmu I believe if we can enhance script usr/share/rear/prep/RSYNC/default/100_check_rsync.sh a bit so it recognizes (and extract what it needs, like the --password-file option) the variable BACKUP_RSYNC_OPTIONS we can reduces this PR to the bare minimum. Please give it a try.
Thanks.

jsmeix commented at 2019-01-09 09:50:¶

I am not a rsync user ( I use only tar)
so that I cannot review the details here.

In general:

When we already have a config variable for a particular
kind of functionality (BACKUP_RSYNC_OPTIONS in this case)
we should try to avoid adding more config variables for the same
functionality (i.e. adding RSYNC_OPTIONS in this case)
unless several separated config variables for the same functionality
are needed to configure several separated parts of that functionality.

But then the config variables must have meaningful separated names
to make it clear to the user how the usage of the config variables differs
(e.g. see the USER_INPUT_* config variable names).

In this case it means if two config variables are needed here
their current names BACKUP_RSYNC_OPTIONS and RSYNC_OPTIONS
look confusing to me because I cannot see how their usage differs, cf.
Variables and functions must have names that explain what they do, even if it makes them longer in
https://github.com/rear/rear/wiki/Coding-Style

To me it looks as if RSYNC_OPTIONS specifies generic options
that are used for all rsync calls in ReaR while
BACKUP_RSYNC_OPTIONS specifies additional options that are
only used for making the backup with rsync and for restoring it.

But the current changes here do not apply RSYNC_OPTIONS
to all rsync calls in ReaR so that this config variable must be
named more specifically according to what its actual usage is,
e.g. RSYNC_CONNECTION_CHECK_OPTIONS according to
https://github.com/rear/rear/pull/2011#issuecomment-450359947
if that is really the actual usage of that config variable.

By the way:

I wonder how making the backup with rsync and restoring it
works without the --password-file=/full/path/to/file option
when that option is needed for the rsync connection check
(but I am no rsync user).

A side note FWIW:

What even more confuses me is that we have in default.conf (excerpts):

BACKUP_OPTIONS=
...
# NOTE: The BACKUP_* variables relate to ALL builtin backup methods !
# (NETFS, ISO, TAPE ...)
BACKUP_PROG=tar
...
BACKUP_PROG_OPTIONS=( "--anchored" )
...
BACKUP_PROG_COMPRESS_OPTIONS=( --gzip )
...
BACKUP_RSYNC_OPTIONS=(--sparse --archive --hard-links --numeric-ids --stats)

It seems we got from the past some mess with our BACKUP_*
config variable names that should be cleaned up first of all.
But we cannot change config variable names because
that would cause regressions for our users.

ivarmu commented at 2019-01-09 11:25:¶

Hi all,

as per I can see, RSYNC_OPTIONS is already defined somewhere at rear code and translated to BACKUP_RSYNC_OPTIONS at /usr/share/rear/prep/default/020_translate_url.sh

grep -n RSYNC_OPTIONS /usr/share/rear/prep/default/020_translate_url.sh
25:if [[ "$RSYNC_OPTIONS" ]] ; then
26:    BACKUP_RSYNC_OPTIONS=$RSYNC_OPTIONS

Anyway, we are only talking about a variable name and it's related documentation, so I'm changing the PR to use BACKUP_RSYNC_OPTIONS instead RSYNC_OPTIONS, although I think we should make that two concepts differentiated, as commented by @jsmeix .

I'm introducing another change to the rsync checkings, concretely I've changed the behaviour of --fake-root on rsync protocol versions < 29, as it may not return an error it the option is not used at all.

jsmeix commented at 2019-01-09 13:26:¶

@ivarmu
a side note:
In ReaR we have the IsInArray ( needle hayblade1 hayblade2 ... ) function
(its name is misleading because it does not expand an array variable,
see the implementation in usr/share/rear/lib/array-functions.sh)
so that you could instead of

if [ ${BACKUP_RSYNC_OPTIONS[@]/--fake-super/} != ${BACKUP_RSUNC_OPTIONS[@]} ]; then

use

if IsInArray "--fake-super" "${BACKUP_RSYNC_OPTIONS[@]}" ; then

jsmeix commented at 2019-01-09 13:41:¶

@ivarmu
FYI regarding RSYNC_OPTIONS => BACKUP_RSYNC_OPTIONS see
https://github.com/rear/rear/commit/20b214db4d8142814ec9c38536f93d40ef46645d
i.e. RSYNC_OPTIONS is outdated and should no longer be used
because nowadays it is BACKUP_RSYNC_OPTIONS and
the related code in usr/share/rear/prep/default/020_translate_url.sh
is only there to provide some backward compatibility.

ivarmu commented at 2019-01-09 13:45:¶

Nice @jsmeix. I've changed all the needed files (I'm pushing a new commit) and use only the BACKUP_RSYNC_OPTIONS. I'm already loging the obsolescence of RSYNC_OPTIONS at 020_translate_url.sh file.

jsmeix commented at 2019-01-09 13:54:¶

@ivarmu
instead of only a log message

Log "Using RSYNC_OPTIONS is deprecated ...

I would also tell the user on his terminal about it via

LogUserOutput "Using RSYNC_OPTIONS is deprecated ...

or as you like via LogPrintError (that won't exit as the Error function does)

LogPrintError "Using RSYNC_OPTIONS is deprecated ...

see the LogUserOutput and LogPrintError and their
base functions like UserOutput and PrintError in
usr/share/rear/lib/_input-output-functions.sh

jsmeix commented at 2019-01-09 14:03:¶

@gdha
perhaps nitpicking in practice but at least out of curiosity:
In usr/share/rear/prep/default/020_translate_url.sh I wonder about

BACKUP_RSYNC_OPTIONS=$RSYNC_OPTIONS

shouldn't that better be something like

BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" $RSYNC_OPTIONS )

so that the BACKUP_RSYNC_OPTIONS from default.conf are kept?
I don't know what the meaning/usage of RSYNC_OPTIONS was.
Could it have been also an array?

jsmeix commented at 2019-01-09 14:08:¶

In
https://github.com/rear/rear/commit/20b214db4d8142814ec9c38536f93d40ef46645d
I found in the changes for usr/share/rear/conf/default.conf

RSYNC_OPTIONS=(--sparse --archive --hard-links --verbose --numeric-ids --stats)

so RSYNC_OPTIONS was an array so that in
usr/share/rear/prep/default/020_translate_url.sh
it must be either

BACKUP_RSYNC_OPTIONS=( "${RSYNC_OPTIONS[@]}" )

or

BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" "${RSYNC_OPTIONS[@]}" )

ivarmu commented at 2019-01-09 14:18:¶

@ivarmu
instead of only a log message
Log "Using RSYNC_OPTIONS is deprecated ...
I would also tell the user on his terminal about it via
LogUserOutput "Using RSYNC_OPTIONS is deprecated ...
or as you like via LogPrintError (that won't exit as the Error function does)
LogPrintError "Using RSYNC_OPTIONS is deprecated ...
see the LogUserOutput and similar functions like LogPrintError in
usr/share/rear/lib/_input-output-functions.sh

I preffer LogUserOutput option. I'm updating code.

ivarmu commented at 2019-01-09 14:22:¶

@gdha
perhaps nitpicking in practice but at least out of curiosity:
In usr/share/rear/prep/default/020_translate_url.sh I wonder about
BACKUP_RSYNC_OPTIONS=$RSYNC_OPTIONS
shouldn't that better be something like
BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" $RSYNC_OPTIONS )
so that the BACKUP_RSYNC_OPTIONS from default.conf are kept?
I don't know what the meaning/usage of RSYNC_OPTIONS was.
Could it have been also an array?

I don't know what the meaning/usage of RSYNC_OPTIONS was, too, as it existed previously and that is my first PR on ReaR code. I agree with you, but think maybe there's a reason to stay like that... isn't that supposed to let "site.conf" (for example) to override the defaults? That could be an explanation. Doesn't matter if it could be an array or not, as bash interpretes both as well.

ivarmu commented at 2019-01-09 14:27:¶

In
20b214d
I found in the changes for usr/share/rear/conf/default.conf
RSYNC_OPTIONS=(--sparse --archive --hard-links --verbose --numeric-ids --stats)
so RSYNC_OPTIONS was an array so that in
usr/share/rear/prep/default/020_translate_url.sh
it must be either
BACKUP_RSYNC_OPTIONS=( "${RSYNC_OPTIONS[@]}" )
or
BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" "${RSYNC_OPTIONS[@]}" )

Regarding my last comment, a correct option would be:

BACKUP_RSYNC_OPTIONS=( ${RSYNC_OPTIONS[@]} )

but it does'nt matter if one do:

RSYNC_OPTIONS="--password_file=/tmp/file"
BACKUP_RSYNC_OPTIONS=${RSYNC_OPTIONS}
rsync ${BACKUP_RSYNC_OPTIONS[@]} bla bla bla

That would work as well.

jsmeix commented at 2019-01-09 14:34:¶

@ivarmu
it does matter if a variable is an array or not, e.g. see:

# BACKUP_RSYNC_OPTIONS=( this that 'something else' )

# RSYNC_OPTIONS=( 'foo bar' baz )

# for e in "${BACKUP_RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'this'
'that'
'something else'

# for e in "${RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'foo bar'
'baz'

# BACKUP_RSYNC_OPTIONS=$RSYNC_OPTIONS

# for e in "${BACKUP_RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'foo bar'
'that'
'something else'

# BACKUP_RSYNC_OPTIONS=( this that 'something else' )

# BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" "${RSYNC_OPTIONS[@]}" )

# for e in "${BACKUP_RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'this'
'that'
'something else'
'foo bar'
'baz'

In general using ${VAR[*]} is problematic and using ${VAR[@]} without
double-quotes is also problematic, see 'Arrays' in "man bash" and
see the initial comment in usr/share/rear/conf/default.conf and
see https://github.com/rear/rear/issues/1068 for some examples.

gdha commented at 2019-01-09 14:44:¶

@jsmeix https://github.com/rear/rear/pull/2011#issuecomment-452706857 I think RSYNC_OPTIONS variable was probably not an array in the beginning (that is my guess). And, to be honest I did not know it was still there (now I understand why @ivarmu used that variable in the first place).
I would propose to remove the variable completely from ReaR itself as it is OLD and obsolete.

ivarmu commented at 2019-01-09 14:49:¶

@ivarmu
it does matter if a variable is an array or not, e.g. see:

# BACKUP_RSYNC_OPTIONS=( this that 'something else' )

# RSYNC_OPTIONS=( 'foo bar' baz )

# for e in "${BACKUP_RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'this'
'that'
'something else'

# for e in "${RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'foo bar'
'baz'

# BACKUP_RSYNC_OPTIONS=$RSYNC_OPTIONS

# for e in "${BACKUP_RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'foo bar'
'that'
'something else'

# BACKUP_RSYNC_OPTIONS=( this that 'something else' )

# BACKUP_RSYNC_OPTIONS=( "${BACKUP_RSYNC_OPTIONS[@]}" "${RSYNC_OPTIONS[@]}" )

# for e in "${BACKUP_RSYNC_OPTIONS[@]}" ; do echo "'$e'" ; done
'this'
'that'
'something else'
'foo bar'
'baz'

Yes, I Know that, but I can't find any code at ReaR looping through the BACKUP_RSYNC_OPTIONS as would be expected for an array to be managed (you can "grep -R BACKUP_RSYNC_OPTIONS" and see no for/while/do is involved).

Anyway... RSYNC_OPTIONS is obsoleted... so I'm happy with the current assignation, as had been working since today... I can change it to the following if you prefer...

BACKUP_RSYNC_OPTIONS=( ${RSYNC_OPTIONS[@]} )

jsmeix commented at 2019-01-09 14:51:¶

Perfectly fine with me to remove RSYNC_OPTIONS support.

The only place where it appears is in prep/default/020_translate_url.sh

But I would not like to let users who may still use RSYNC_OPTIONS
from ancient times in their local.conf learn the hard way by obscure failures
that RSYNC_OPTIONS is no longer supported.

Therefore I suggest to replace in prep/default/020_translate_url.sh

if [[ "$RSYNC_OPTIONS" ]] ; then
    BACKUP_RSYNC_OPTIONS=$RSYNC_OPTIONS
fi

with

test "$RSYNC_OPTIONS" && Error "RSYNC_OPTIONS is no longer supported. Use BACKUP_RSYNC_OPTIONS instead."

ivarmu commented at 2019-01-09 14:55:¶

Perfect for me too, pushing another commit 👍

jsmeix commented at 2019-01-10 10:32:¶

@ivarmu
is it o.k. for you when we merge it
or do you need more time to test it?

@gdha
would you merge it (because it is assigned to you)
or should I merge it?

schlomo commented at 2019-01-10 12:52:¶

FWIW, I find prefixing the variable with BACKUP_ to be important, so that rear dump will show it.

I find it important that rear dump provides all relevant information.

jsmeix commented at 2019-01-11 12:03:¶

@ivarmu
thank you for your contribution to ReaR
and for your patience with us!

Apparently you touched older code so that
things became more complicated than expected
but it was also a starting point for further improvements
like https://github.com/rear/rear/pull/2014

ivarmu commented at 2019-01-11 16:09:¶

I've given you more diversion! 😊

pcahyna commented at 2021-06-16 10:10:¶

A a result of this PR there are now some places where ${BACKUP_RSYNC_OPTIONS[@]} is in the rsync command line in case of RSYNC_PROTO=rsync but not for RSYNC_PROTO=ssh. Is that intentional?

Also, I am surprised that ${BACKUP_RSYNC_OPTIONS[@]} was not needed here:
https://github.com/rear/rear/blob/4768fe217c440ed74c1fc8f69109d53cb522bfc3/usr/share/rear/verify/RSYNC/default/550_check_remote_backup_archive.sh#L11
have you tested restoring with rsync and non-interactive authentication?

[Export of Github issue for rear/rear.]

#2011 PR merged: Allow non-interactive rsync authentication¶