Allow non-interactive rsync authentication #2011

ivarmu · 2018-12-26T17:04:10Z

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.

…ion 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 at checking stage. On branch feature/allow_rsync_user_and_options Changes to be committed: modified: usr/share/rear/output/RSYNC/default/200_make_prefix_dir.sh modified: usr/share/rear/output/RSYNC/default/900_copy_result_files.sh modified: usr/share/rear/prep/RSYNC/default/100_check_rsync.sh

gdha · 2018-12-28T11:48:25Z

@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 · 2018-12-28T13:33:35Z

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 <#2011 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AZlFm4ENOKSKiOFMYfryX8Rhjzb7z843ks5u9gUKgaJpZM4Zh6-X> .

gdha · 2019-01-09T07:32:22Z

@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 · 2019-01-09T09:50:01Z

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
#2011 (comment)
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.

jsmeix

@ivarmu
each config variable that is meant to be specified by the user (if needed)
must be at least described in usr/share/rear/conf/default.conf
and set to a reasonable default value if possible
(i.e. except exceptions like TMPDIR).

…ion using rsync protocol. Now, one can use BACKUP_RSYNC_OPTIONS to authenticate using the "--password-file=/full/path/to/file" rsync's option. I already fixed the missing username at checking stage. I already fixed uefi-funcions.sh (I'm on Fedora29) as described at #1996 I already 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. Changes to be committed: modified: usr/share/rear/lib/uefi-functions.sh modified: usr/share/rear/output/RSYNC/default/200_make_prefix_dir.sh modified: usr/share/rear/output/RSYNC/default/900_copy_result_files.sh modified: usr/share/rear/prep/RSYNC/default/100_check_rsync.sh modified: usr/share/rear/prep/RSYNC/default/150_check_rsync_protocol_version.sh

ivarmu · 2019-01-09T11:25:55Z

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

I am not a rsync user so that I cannot review the details here
but from plain looking at the code it looks o.k. to me.

jsmeix · 2019-01-09T13:26:33Z

@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 · 2019-01-09T13:41:15Z

@ivarmu
FYI regarding RSYNC_OPTIONS => BACKUP_RSYNC_OPTIONS see
20b214d
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 · 2019-01-09T13:45:57Z

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.

added comment about RSYNC_OPTIONS obsolescence Changes to be committed: modified: usr/share/rear/backup/RSYNC/GNU/Linux/610_start_selinux.sh modified: usr/share/rear/backup/RSYNC/GNU/Linux/620_force_autorelabel.sh modified: usr/share/rear/backup/RSYNC/default/700_copy_backup_log.sh modified: usr/share/rear/prep/default/020_translate_url.sh

jsmeix · 2019-01-09T13:54:43Z

@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 · 2019-01-09T14:03:24Z

@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 · 2019-01-09T14:08:16Z

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[@]}" )

ivarmu · 2019-01-09T14:18:57Z

@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 · 2019-01-09T14:22:45Z

@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 · 2019-01-09T14:27:15Z

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 · 2019-01-09T14:34:00Z

@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 · 2019-01-09T14:44:27Z

@jsmeix #2011 (comment) 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 · 2019-01-09T14:49:09Z

@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 · 2019-01-09T14:51:53Z

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 · 2019-01-09T14:55:40Z

Perfect for me too, pushing another commit 👍

Changes to be committed: modified: usr/share/rear/prep/default/020_translate_url.sh

jsmeix · 2019-01-10T10:32:34Z

@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 · 2019-01-10T12:52:34Z

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 · 2019-01-11T12:03:05Z

@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 #2014

ivarmu · 2019-01-11T16:09:40Z

I've given you more diversion! 😊

pcahyna · 2021-06-16T10:10:14Z

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:

rear/usr/share/rear/verify/RSYNC/default/550_check_remote_backup_archive.sh

Line 11 in 4768fe2

    
           $BACKUP_PROG "${RSYNC_PROTO}://${RSYNC_USER}@${RSYNC_HOST}:${RSYNC_PORT}/${RSYNC_PATH}/${RSYNC_PREFIX}/backup" >/dev/null 2>&1

have you tested restoring with rsync and non-interactive authentication?

pcahyna · 2021-06-18T09:52:31Z

usr/share/rear/prep/RSYNC/default/150_check_rsync_protocol_version.sh

+            Error "rsync --fake-super not possible on system ($RSYNC_HOST) (please upgrade rsync to 3.x)"
+        else
+            Log "Warning: rsync --fake-super not possible on system ($RSYNC_HOST) (please upgrade rsync to 3.x)"
+        fi


I suppose the intent was to not abort in the case when the rsync daemon is used, where we assume that version is 29. This whole part could never work properly though. There is a typo, the use of [@] is wrong (you need [*] with quotes to prevent word splitting) and the intent is wrong: if the protocol is 29, we should abort unconditionally, because we need fake super, even if not specified in BACKUP_RSYNC_OPTIONS. --rsync-path="rsync --fake-super" will not work for tthe rsync daemon connection anyway.

gdha added the enhancement Adaptions and new features label Dec 28, 2018

gdha added this to the ReaR v2.5 milestone Dec 28, 2018

gdha self-assigned this Dec 28, 2018

gdha requested a review from jsmeix December 28, 2018 11:36

gdha added the waiting for info label Jan 9, 2019

jsmeix requested changes Jan 9, 2019

View reviewed changes

Merge branch 'master' into feature/allow_rsync_user_and_options

5739110

jsmeix approved these changes Jan 9, 2019

View reviewed changes

jsmeix removed the waiting for info label Jan 9, 2019

Obsoleting RSYNC_OPTIONS in favour of BACKUP_RSYNC_OPTIONS

d2cd92f

Changes to be committed: modified: usr/share/rear/prep/default/020_translate_url.sh

gdha approved these changes Jan 9, 2019

View reviewed changes

jsmeix added the cleanup label Jan 9, 2019

gdha merged commit 4768fe2 into rear:master Jan 10, 2019

ivarmu deleted the feature/allow_rsync_user_and_options branch January 10, 2019 14:10

jsmeix added the fixed / solved / done label Jan 10, 2019

jsmeix mentioned this pull request Jan 10, 2019

Improve rear dump output to clearly distinguish array elements #2014

Merged

pcahyna reviewed Jun 18, 2021

View reviewed changes

pcahyna mentioned this pull request Jun 21, 2021

Cleanup rsync and fix error reporting #2632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow non-interactive rsync authentication #2011

Allow non-interactive rsync authentication #2011

ivarmu commented Dec 26, 2018

gdha commented Dec 28, 2018 •

edited

ivarmu commented Dec 28, 2018 via email

gdha commented Jan 9, 2019

jsmeix commented Jan 9, 2019

jsmeix left a comment

ivarmu commented Jan 9, 2019

jsmeix left a comment

jsmeix commented Jan 9, 2019 •

edited

jsmeix commented Jan 9, 2019

ivarmu commented Jan 9, 2019

jsmeix commented Jan 9, 2019 •

edited

jsmeix commented Jan 9, 2019

jsmeix commented Jan 9, 2019

ivarmu commented Jan 9, 2019

ivarmu commented Jan 9, 2019

ivarmu commented Jan 9, 2019

jsmeix commented Jan 9, 2019 •

edited

gdha commented Jan 9, 2019

ivarmu commented Jan 9, 2019

jsmeix commented Jan 9, 2019 •

edited

ivarmu commented Jan 9, 2019

jsmeix commented Jan 10, 2019

schlomo commented Jan 10, 2019

jsmeix commented Jan 11, 2019

ivarmu commented Jan 11, 2019

pcahyna commented Jun 16, 2021

pcahyna Jun 18, 2021

Allow non-interactive rsync authentication #2011

Allow non-interactive rsync authentication #2011

Conversation

ivarmu commented Dec 26, 2018

Relax-and-Recover (ReaR) Pull Request Template

Pull Request Details:

gdha commented Dec 28, 2018 • edited

ivarmu commented Dec 28, 2018 via email

gdha commented Jan 9, 2019

jsmeix commented Jan 9, 2019

jsmeix left a comment

Choose a reason for hiding this comment

ivarmu commented Jan 9, 2019

jsmeix left a comment

Choose a reason for hiding this comment

jsmeix commented Jan 9, 2019 • edited

jsmeix commented Jan 9, 2019

ivarmu commented Jan 9, 2019

jsmeix commented Jan 9, 2019 • edited

jsmeix commented Jan 9, 2019

jsmeix commented Jan 9, 2019

ivarmu commented Jan 9, 2019

ivarmu commented Jan 9, 2019

ivarmu commented Jan 9, 2019

jsmeix commented Jan 9, 2019 • edited

gdha commented Jan 9, 2019

ivarmu commented Jan 9, 2019

jsmeix commented Jan 9, 2019 • edited

ivarmu commented Jan 9, 2019

jsmeix commented Jan 10, 2019

schlomo commented Jan 10, 2019

jsmeix commented Jan 11, 2019

ivarmu commented Jan 11, 2019

pcahyna commented Jun 16, 2021

pcahyna Jun 18, 2021

Choose a reason for hiding this comment

gdha commented Dec 28, 2018 •

edited

jsmeix commented Jan 9, 2019 •

edited

jsmeix commented Jan 9, 2019 •

edited

jsmeix commented Jan 9, 2019 •

edited

jsmeix commented Jan 9, 2019 •

edited