Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cron friendly #184

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cron friendly #184

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30201a8
add trailing / on fn_df_t_src call (these use df to validate the folder)
prontog Nov 12, 2019
1c96775
remove setting ssh -o UserKnownHostsFile to /dev/null. This always wr…
prontog Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions rsync_tmbackup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,11 +408,11 @@ fi
#
# The check is performed by taking the second row
# of the output of the first command.
if [[ "$(fn_df_t_src "${SRC_FOLDER}" | awk '{print $2}' | grep -c -i -e "fat")" -gt 0 ]]; then
if [[ "$(fn_df_t_src "${SRC_FOLDER}/" | awk '{print $2}' | grep -c -i -e "fat")" -gt 0 ]]; then
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fn_log_info "Source file-system is a version of FAT."
fn_log_info "Using the --modify-window rsync parameter with value 2."
RSYNC_FLAGS="${RSYNC_FLAGS} --modify-window=2"
elif [[ "$(fn_df_t "${DEST_FOLDER}" | awk '{print $2}' | grep -c -i -e "fat")" -gt 0 ]]; then
elif [[ "$(fn_df_t "${DEST_FOLDER}/" | awk '{print $2}' | grep -c -i -e "fat")" -gt 0 ]]; then
Copy link

@reactive-firewall reactive-firewall Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

fn_log_info "Destination file-system is a version of FAT."
fn_log_info "Using the --modify-window rsync parameter with value 2."
RSYNC_FLAGS="${RSYNC_FLAGS} --modify-window=2"
Expand Down Expand Up @@ -546,9 +546,9 @@ while : ; do
if [ -n "$SSH_CMD" ]; then
RSYNC_FLAGS="$RSYNC_FLAGS --compress"
if [ -n "$ID_RSA" ] ; then
CMD="$CMD -e 'ssh -p $SSH_PORT -i $ID_RSA -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null'"
CMD="$CMD -e 'ssh -p $SSH_PORT -i $ID_RSA -o StrictHostKeyChecking=no'"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else
CMD="$CMD -e 'ssh -p $SSH_PORT -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null'"
CMD="$CMD -e 'ssh -p $SSH_PORT -o StrictHostKeyChecking=no'"
fi
fi
CMD="$CMD $RSYNC_FLAGS"
Expand Down