Cron friendly

[prontog]

Hello and thanks for this very useful tool!

Regarding #181, I made a couple changes that under circumstances always wrote to STDERR. This is a problem with cron where you usually redirect STDOUT to /dev/null but want to keep STDERR as is so that you get an email notification when a warning or error occurs.

The first change was with the df check on SRC_FOLDER right after the trailling / is stripped. In the case of SRC_FOLDER == / , the / was stripped and df wrote to STDERR because FILE was the empty string.

The second change was the -o UserKnownHostsFile=/dev/null fixed ssh argument. This lead to ssh always writing to STDERR. This is part of #128. I would expect people to first setup the ssh access and then use rsync_tmbackup.sh. I cant' see a reason in bypassing the local configuration by default. There is always the -e option of rsync which can be used to set ssh options.

Best regards,

Panos

[kapitainsky]

Regarding #181 - I have just tested and it fails. But reason is not your fix but original #170 - df -T works different on macOS. To keep this script cross platform it should be reworked.

as per man df:

 -T      Only print out statistics for filesystems of the specified types.  More than one type may be specified in a comma separated list.  The list of
         filesystem types can be prefixed with ``no'' to specify the filesystem types for which action should not be taken.  For example, the df command:

               df -T nonfs,mfs

         lists all filesystems except those of type NFS and MFS.  The lsvfs(1) command can be used to find out the types of filesystems that are available on
         the system.

It means that as it is now does not work on macOS.

[prontog]

Is it necessary to add this fix in this PR (I have no access to a Mac)?

As you say it has to do with #170. GNU df prints the fs type with -T.

[kapitainsky]

I think course of action is up to @laurent22 and @omer-musa-battal who created PR for #170

[reactive-firewall]

This looks like the wrong place to fix this ... would fn_df_t_src not be a better place to check arguments what happens if SRC_FOLDER="/"
this makes it "//" - which seems very unessasarry
atleast ${SRC_FOLDER%/}/ would be nessassary here

[reactive-firewall]

as with l SRC_FOLDER above
atleast ${DEST_FOLDER%/}/ would be nessassary here

[prontog]

Why is this necessary? Is there anybody crazy enough to dump all these backup folders etc. on the root of the dest host...?

[prontog]

You are right but I wanted to make the smallest possible change that works and not change a function that might be used somewhere else. Anyway I thought that the maintainer of this repo would know what to do best.

[reactive-firewall]

Why is this necessary? Is there anybody crazy enough to dump all these backup folders etc. on the root of the dest host...?

🤔 ... I can't speak to crazy ... however given #181 ... this was more a 'why make the dest host an exception in how paths are handled?' ... further I was thinking more of backing up to a restricted chroot container on a backup host where / is the lowest level of the chroot not the actual real root /. (this will be one of my use-cases)
(further if you think it's crazy in my example, checkout https://yakking.branchable.com/posts/falsehoods-programmers-believe-about-file-paths/ note the falsehood about "Absolute paths start with a /" for another error-case)

---

You are right but I wanted to make the smallest possible change that works and not change a function that might be used somewhere else. Anyway I thought that the maintainer of this repo would know what to do best.

I'm not so convinced; the more I read this project's code the less convinced I am this is the right place for this change ... perhaps (given you wish the smallest possible change) this path issue should be addressed in https://github.com/laurent22/rsync-time-backup/pull/170/files#diff-703e143870d3454e964014cb5854a43eR253-R259 where it was introduced by #170 ... otherwise as we discussed in #181 I think a slightly bigger change to the argument handling rather than an exception would be easer to maintain and more readable down here in the thick of the code.

Thoughts? (both 'fix now for cron here then fix file paths in general later separately' AND 'take on the argument sanitation of file paths for the sake of cron and glory' seem to have merits, 🤷‍♂ I'm fine with either, and honestly more focused on adding some tests for data-driven decisions going forward )

[prontog]

I apologize for describing this as crazy...apparently I couldn't imagine the use case you describe.

As far as the best approach here, it might be the handling it during argument validation/normalization but I don't think I can handle that at the moment. To be honest in my TODO list is a replace rsync-time-backup. There are some candidates but I still I haven't tried them out.

I agree with you that adding tests to this project will be a major step forward. How else will anyone be able to work on strategy related issues. (by the way I stumbled across prunef the other day).

[reactive-firewall]

This is line is already multiple security flaws in one, namely: CWE-547, CWE-347 and CWE-1188.
if the environment is unable to use ssh key checking properly, WHY are you even using ssh?
TL;DR; all you gain is a slower backup that is also weakening your encryption (see line 547 ... encryption is the opposite of compression ... when security matters avoid using both at the same time, it's speed vs security, not both)
at the very least these exceptions belong in the /etc/ssh/ssh_config file when appropriate. Better to add these to a variable for the time being:

# insert at 548
# TODO: add a --no-key-checks (or --cron) option to this tool
SSH_ARGS="-o StrictHostKeyChecking=no"
if [ -n "$ID_RSA" ] ; then
SSH_ARGS="-i $ID_RSA $SSH_ARGS"
fi
CMD="$CMD  -e 'ssh -p $SSH_PORT $SSH_ARGS"
# continue at line 552

this allows for the current code's function and allows other code to cope with security, excusing this code from the issue without taking it on all at once.

[prontog]

I agree with you but this PR is just regarding cron friendliness and not about security. Another PR would be more appropriate for such a change.

[reactive-firewall]

I agree with you but this PR is just regarding cron friendliness and not about security. Another PR would be more appropriate for such a change.

😕 opps! sorry ✔️ I agree with your second point the ...

This is line is already multiple security flaws in one, namely: CWE-547, CWE-347 and CWE-1188.
... should be a separate issue or PR ...

✔️ I get this was about #128 (and thus also #104) only as they relate to cron usage ... correct me if I'm wrong.

I agree with

this PR is just regarding cron friendliness and not about security
but I still hold what I said
Better to add these to a variable for the time being:

# insert at 548
# TODO: add a --no-key-checks (or --cron) option to this tool
SSH_ARGS="-o StrictHostKeyChecking=no"
if [ -n "$ID_RSA" ] ; then
SSH_ARGS="-i $ID_RSA $SSH_ARGS"
fi
CMD="$CMD  -e 'ssh -p $SSH_PORT $SSH_ARGS"
# continue at line 552

this allows for the current code's function and allows other code to cope with security, excusing this code from the issue without taking it on all at once.

And your initial comment:

The second change was the -o UserKnownHostsFile=/dev/null fixed ssh argument. This lead to ssh always writing to STDERR. This is part of #128. I would expect people to first setup the ssh access and then use rsync_tmbackup.sh. I cant' see a reason in bypassing the local configuration by default. There is always the -e option of rsync which can be used to set ssh options.

so...

I guess the --cron option would be the easiest to maintain, and keeps this clearly out of the security holy war. 🤔 ...Thoughts?

[prontog]

I'm not sure I get this. Why add a --cron flag? Why should normal use write to STDERR everytime? By redirecting STDOUT to /dev/null you should be able to expect emails (from cron) only when something unexpected happens.