Glob values in COPY_AS_IS shouldn't be quoted. #2405

flyinggreenfrog · 2020-05-24T16:29:43Z

Otherwise they are not included.

Pull Request Details:

Type: Bug Fix
Impact: Normal (for me it's High otherwise I can't restore to LUKS encrypted device)
Reference to related issue (URL): No new explicit issue, but reoccurrence of cryptsetup pw dict files missing #679 (or cryptsetup fails on restore due to missing cracklib dictionaries #972)
How was this pull request tested? openSUSE Leap 15.1. Baremetal via USB rescue medium
Brief description of the changes in this pull request:

With a3ce9fe needed cracklib files were added. In #679 (comment) it was stated that this commit was fixing that issue.

However, if I test from current master ad0f263 I ran into the same problems as in #679 (comment).

If I change /usr/share/cracklib/\* to /usr/share/cracklib/* (this PR) needed cracklib files are present in rescue/restore system and restore works like expected.

Besides that PR, I'm asking myself:

What is the correct way to specify things to COPY_AS_IS with glob patterns?
According to conf/default.conf (line 1358 and following):
```
# Usually globbing patterns in COPY_AS_IS are specified without quoting
# like COPY_AS_IS+=( /my/directory/* /path/to/my/files* )
# so that the bash pathname expansion works as usually intended
```
Why was in a3ce9fe /usr/share/cracklib/\* added instead of /usr/share/cracklib/* and it was working, and now in current master it doesn't work again?
I don't know, if Filter out duplicates in COPY_AS_IS and copy_as_is_filelist_file #2378 changed some behavior unintentionally?

Otherwise they are not included.

flyinggreenfrog · 2020-05-24T17:05:38Z

Just tested with commit aa82834 (Tag 2.5): there the cracklib files were added to the rescue system.

jsmeix · 2020-05-25T09:40:21Z

@flyinggreenfrog
thank you for your issue report.

Something severe has changed since the ReaR 2.5
in general what build/GNU/Linux/100_copy_as_is.sh
copies.

A lot of other stuff is no longer copied into the recovery system
because of lots of tar arguments that are now quoted.

This affects all places where tar arguments are set as

COPY_AS_IS+=( '/path/to/directory/*' )

but also in the older form

COPY_AS_IS=( "${COPY_AS_IS[@]}" '/path/to/directory/*' )

It seems the root cause happens before the script
usr/share/rear/build/GNU/Linux/100_copy_as_is.sh
is run because it seems when it is run those tar arguments
appear already quoted so that then tar is run as

tar ... '/path/to/directory/*'

which lets tar report

tar: /path/to/directory/*: Cannot stat: No such file or directory

for example:

# tar -cvf /tmp/testy.tar /usr/share/cracklib/*
tar: Removing leading `/' from member names
/usr/share/cracklib/cracklib.magic
tar: Removing leading `/' from hard link targets
/usr/share/cracklib/pw_dict.hwm
/usr/share/cracklib/pw_dict.pwd
/usr/share/cracklib/pw_dict.pwi

# tar -cvf /tmp/testy.tar '/usr/share/cracklib/*'
tar: Removing leading `/' from member names
tar: /usr/share/cracklib/*: Cannot stat: No such file or directory
tar: Exiting with failure status due to previous errors

jsmeix

The change makes this particular item work
but it seems we have a global problem, cf.
#2405 (comment)

So I do not approve this particular change
(regardless that it fixed this particular item)
because I am analyzing the root cause of our global problem...

jsmeix · 2020-05-25T10:22:34Z

Got it!

I can make things work again when I add

COPY_AS_IS=( ${COPY_AS_IS[@]} )

at the beginning of usr/share/rear/build/GNU/Linux/100_copy_as_is.sh

Ugh!
What a mess we have in our code!

In ReaR 2.5 we had for example in
rescue/GNU/Linux/550_copy_ldconfig.sh

COPY_AS_IS=( ${COPY_AS_IS[@]} /etc/ld.so.conf /etc/ld.so.conf.d/* )

which is the last script that had always re-written COPY_AS_IS
before build/GNU/Linux/100_copy_as_is.sh is run
but now that was changed to

COPY_AS_IS+=( /etc/ld.so.conf /etc/ld.so.conf.d/* )

which does no longer re-write COPY_AS_IS
but keep its already existing entries as is.

The falsely used unquoted ${COPY_AS_IS[@]}
had resulted that the COPY_AS_IS array members
got rewritten in a duplicated falsely false way
so that things had worked by accident for tar.

For example

# tarfiles=( '/usr/share/cracklib/*' )

# tarfiles=( "${tarfiles[@]}" '/etc/issue' )

# ( set -x ; echo "${tarfiles[@]}" )
+ echo '/usr/share/cracklib/*' /etc/issue
/usr/share/cracklib/* /etc/issue

# ( set -x ; tar -cvf /tmp/testy.tar "${tarfiles[@]}" )
+ tar -cvf /tmp/testy.tar '/usr/share/cracklib/*' /etc/issue
tar: Removing leading `/' from member names
tar: /usr/share/cracklib/*: Cannot stat: No such file or directory
/etc/issue
tar: Removing leading `/' from hard link targets
tar: Exiting with failure status due to previous errors

or the same behaviour with array+=( additional elements )

# tarfiles=( '/usr/share/cracklib/*' )

# tarfiles+=( '/etc/issue' )

# ( set -x ; echo "${tarfiles[@]}" )
+ echo '/usr/share/cracklib/*' /etc/issue
/usr/share/cracklib/* /etc/issue

# ( set -x ; tar -cvf /tmp/testy.tar "${tarfiles[@]}" )
+ tar -cvf /tmp/testy.tar '/usr/share/cracklib/*' /etc/issue
tar: Removing leading `/' from member names
tar: /usr/share/cracklib/*: Cannot stat: No such file or directory
/etc/issue
tar: Removing leading `/' from hard link targets
tar: Exiting with failure status due to previous errors

versus duplicated falsely false things that work by accident:

# tarfiles=( '/usr/share/cracklib/*' )

# tarfiles=( ${tarfiles[@]} '/etc/issue' )

# ( set -x ; echo "${tarfiles[@]}" )
+ echo /usr/share/cracklib/cracklib.magic /usr/share/cracklib/pw_dict.hwm /usr/share/cracklib/pw_dict.pwd /usr/share/cracklib/pw_dict.pwi /etc/issue
/usr/share/cracklib/cracklib.magic /usr/share/cracklib/pw_dict.hwm /usr/share/cracklib/pw_dict.pwd /usr/share/cracklib/pw_dict.pwi /etc/issue

# ( set -x ; tar -cvf /tmp/testy.tar "${tarfiles[@]}" )
+ tar -cvf /tmp/testy.tar /usr/share/cracklib/cracklib.magic /usr/share/cracklib/pw_dict.hwm /usr/share/cracklib/pw_dict.pwd /usr/share/cracklib/pw_dict.pwi /etc/issue
tar: Removing leading `/' from member names
/usr/share/cracklib/cracklib.magic
tar: Removing leading `/' from hard link targets
/usr/share/cracklib/pw_dict.hwm
/usr/share/cracklib/pw_dict.pwd
/usr/share/cracklib/pw_dict.pwi
/etc/issue

I will clean that up...

jsmeix · 2020-05-25T10:53:27Z

Currently I think a minimal change to make things work again
and to not cause unexpected regressions for users that use
in their etc/rear/local.conf any kind of the methods

COPY_AS_IS=( "${COPY_AS_IS[@]}" '/path/to/directory/*' )
COPY_AS_IS=( ${COPY_AS_IS[@]} /path/to/directory/* )
COPY_AS_IS+=( '/path/to/directory/*' )
COPY_AS_IS+=( /path/to/directory/* )

is the following diff in build/GNU/Linux/100_copy_as_is.sh

--- a/usr/share/rear/build/GNU/Linux/100_copy_as_is.sh
+++ b/usr/share/rear/build/GNU/Linux/100_copy_as_is.sh
@@ -84,7 +84,7 @@ done >$copy_as_is_exclude_file
 #  -rw-r--r-- root/root         4 2017-10-12 11:31 foo
 #  -rw-r--r-- root/root         4 2017-10-12 11:31 baz
 # Because pipefail is not set it is the second 'tar' in the pipe that determines whether or not the whole operation was successful:
-if ! tar -v -X $copy_as_is_exclude_file -P -C / -c "${COPY_AS_IS[@]}" 2>$copy_as_is_filelist_file | tar $v -C $ROOTFS_DIR/ -x 1>/dev/null ; then
+if ! tar -v -X $copy_as_is_exclude_file -P -C / -c ${COPY_AS_IS[@]} 2>$copy_as_is_filelist_file | tar $v -C $ROOTFS_DIR/ -x 1>/dev/null ; then
     Error "Failed to copy files and directories in COPY_AS_IS minus COPY_AS_IS_EXCLUDE"
 fi
 Log "Finished copying files and directories in COPY_AS_IS minus COPY_AS_IS_EXCLUDE"

i.e. use unquoted ${COPY_AS_IS[@]} in the tar command call
or use ${COPY_AS_IS[*]} to make it more obvious what is meant.

This reverts commit 64281d2.

…ystem Use plain ${COPY_AS_IS[*]} instead of quoted "${COPY_AS_IS[@]}" in the tar command call that copies things into the recovery system to ensure "things work as usually expected" for any methods that are used to add elements to the COPY_AS_IS array, see #2405 (comment)

jsmeix · 2020-05-25T11:30:57Z

With
014867e
I use now plain ${COPY_AS_IS[*]} for tar
to copy things into the recovery system, cf.
#2405 (comment)

jsmeix · 2020-05-25T11:39:00Z

@flyinggreenfrog
could you verify if things work again for you
with current GitHub master code i.e. with an unchanged
layout/save/GNU/Linux/260_crypt_layout.sh but with
014867e

flyinggreenfrog · 2020-05-25T11:44:30Z

Just for the sake of completeness, I added 6c52b44 to revert my previous commit for fixing this particular item, to prefer the global solution 014867e from @jsmeix.

Will test current master with 014867e.

flyinggreenfrog · 2020-05-25T12:30:42Z

@flyinggreenfrog
could you verify if things work again for you
with current GitHub master code i.e. with an unchanged
layout/save/GNU/Linux/260_crypt_layout.sh but with
014867e

At current master with your commit it's working for me again.

Just tested backup/restore for me.

@jsmeix Thanks for your fast solution of this issue.

I assume I just delete this branch, since finally no new commit was added? Or will it still be merged with commit and reverting commit to have this issue shown in the history?

jsmeix

No changes so I can trivially approve the empty set ;-)

jsmeix · 2020-05-25T12:50:15Z

The underlying issue is fixed via
014867e
so I think I can close this pull request.

jsmeix · 2020-05-25T13:28:48Z

A side note:

Currently we have several places where we specify

COPY_AS_IS+=( /path/to/directory/* )

instead of just the wanted directory

COPY_AS_IS+=( /path/to/directory )

I found out that the /path/to/directory/* form is another fragile way
how ReaR code works in certain cases because of sublte side-effects.

I tried if in usr/share/rear/conf/GNU/Linux.conf
the following diff also works

-COPY_AS_IS+=( '/etc/ssl/certs/*' '/etc/pki/*' '/usr/lib/ssl/*' '/usr/share/ca-certificates/*' '/etc/ca-certificates/*' )
+COPY_AS_IS+=( /etc/ssl/certs/ /etc/pki/ /usr/lib/ssl/ /usr/share/ca-certificates/ /etc/ca-certificates/ )

That failed because I got only the plain /etc/ssl/certs/ directory
without any files therein copied into the recovery system.
But the other directories were copied with all their contents.

The reason is that (at least on my openSUSE leap 15.1 system)
/etc/ssl/certs/ is a symlink with target /var/lib/ca-certificates/pem
and the tar in build/GNU/Linux/100_copy_as_is.sh must intentionally
not follow symlinks (see the comment in 100_copy_as_is.sh).

So when /path/to/directory is a symlink it helps to specify
COPY_AS_IS+=( /path/to/directory/* ) to get all directory contents
copied into the recovery system (of course except files that start with .)

In particular regarding the /etc/ssl/certs/* usage see
#1971

pcahyna · 2020-05-25T17:40:53Z

@jsmeix in order to perform filename expansion (glob expansion) but not word splitting (to support filenames with spaces properly), one can apparently set IFS to an empty string.

pcahyna · 2020-05-25T17:50:50Z

... or maybe not, not sure myself whether it behaves properly.

jsmeix · 2020-05-26T06:25:03Z

@pcahyna
I would very much appreciate it if you could help to find a good solution
for our current fragile way how things cet copied into the recovery system.

For now - i.e. for the ReaR 2.6 release - I will better explain in default.conf
how COPY_AS_IS must be used, in particular that symlinks cannot be followed.

After the ReaR 2.6 release we can think about how to improve things
in a more relaxed way.

The current tar ... ${COPY_AS_IS[*]} works same as things had worked
all the time before via accidental COPY_AS_IS=( ${COPY_AS_IS[@]} ... )
so there are no regressions but now it is described at the right place
(i.e. in build/GNU/Linux/100_copy_as_is.sh) how things work.

pcahyna · 2020-05-26T09:19:53Z

@jsmeix I understand and very much appreciate the point about no regressions. I would like to improve this area, but unfortunately the amount of time I can spend on ReaR nowadays is limited, so I can't promise anything.

Better explain in default.conf how COPY_AS_IS works, in particular that symlinks cannot be followed and that files or directories that contain blanks or other $IFS characters cannot be specified, cf. #2405 (comment)

jsmeix · 2020-05-26T09:45:23Z

@pcahyna
no worries - take your time - cf. my
#799 (comment)

jsmeix · 2020-05-26T09:46:37Z

Via
4cbd424
it is now better explained in default.conf how COPY_AS_IS works,
in particular that symlinks cannot be followed and
that files or directories that contain blanks or
other $IFS characters cannot be specified.

Glob values in COPY_AS_IS shouldn't be quoted.

64281d2

Otherwise they are not included.

jsmeix self-assigned this May 25, 2020

jsmeix added blocker The next ReaR version is not released unless that issue is solved. bug The code does not do what it is meant to do labels May 25, 2020

jsmeix added this to the ReaR v2.6 milestone May 25, 2020

jsmeix requested changes May 25, 2020

View reviewed changes

jsmeix mentioned this pull request May 25, 2020

ReaR v2.6 Release Preps #2368

Closed

4 tasks

Revert "Glob values in COPY_AS_IS shouldn't be quoted."

6c52b44

This reverts commit 64281d2.

flyinggreenfrog requested a review from jsmeix May 25, 2020 12:30

jsmeix approved these changes May 25, 2020

View reviewed changes

jsmeix added the fixed / solved / done label May 25, 2020

jsmeix closed this May 25, 2020

jsmeix added critical / security / legal and removed blocker The next ReaR version is not released unless that issue is solved. bug The code does not do what it is meant to do labels May 25, 2020

flyinggreenfrog deleted the fix-copy-as-is-cryptsetup-cracklib branch May 25, 2020 12:54

jsmeix mentioned this pull request May 25, 2020

Add /etc/ca-certificates directory to ReaR recovery system for Arch Linux #1971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Glob values in COPY_AS_IS shouldn't be quoted. #2405

Glob values in COPY_AS_IS shouldn't be quoted. #2405

flyinggreenfrog commented May 24, 2020

flyinggreenfrog commented May 24, 2020

jsmeix commented May 25, 2020

jsmeix left a comment

jsmeix commented May 25, 2020 •

edited

jsmeix commented May 25, 2020 •

edited

jsmeix commented May 25, 2020

jsmeix commented May 25, 2020

flyinggreenfrog commented May 25, 2020

flyinggreenfrog commented May 25, 2020

jsmeix left a comment

jsmeix commented May 25, 2020

jsmeix commented May 25, 2020 •

edited

pcahyna commented May 25, 2020

pcahyna commented May 25, 2020

jsmeix commented May 26, 2020

pcahyna commented May 26, 2020

jsmeix commented May 26, 2020

jsmeix commented May 26, 2020

Glob values in COPY_AS_IS shouldn't be quoted. #2405

Glob values in COPY_AS_IS shouldn't be quoted. #2405

Conversation

flyinggreenfrog commented May 24, 2020

Pull Request Details:

flyinggreenfrog commented May 24, 2020

jsmeix commented May 25, 2020

jsmeix left a comment

Choose a reason for hiding this comment

jsmeix commented May 25, 2020 • edited

jsmeix commented May 25, 2020 • edited

jsmeix commented May 25, 2020

jsmeix commented May 25, 2020

flyinggreenfrog commented May 25, 2020

flyinggreenfrog commented May 25, 2020

jsmeix left a comment

Choose a reason for hiding this comment

jsmeix commented May 25, 2020

jsmeix commented May 25, 2020 • edited

pcahyna commented May 25, 2020

pcahyna commented May 25, 2020

jsmeix commented May 26, 2020

pcahyna commented May 26, 2020

jsmeix commented May 26, 2020

jsmeix commented May 26, 2020

jsmeix commented May 25, 2020 •

edited

jsmeix commented May 25, 2020 •

edited

jsmeix commented May 25, 2020 •

edited