Fix restores from backup logs

[iliaross]

Hey Jamie,

As discussed in the PM.

[jcameron]

It looks like you are inverting the meaning of the $safe_backup variable. What does that fix?

[iliaross]

It looks like you are inverting the meaning of the $safe_backup variable. What does that fix?

It does, because currently any newly created scheduled backup cannot later be restored using backup logs page even being root user.

And the function that checks on features expects either safe or not-safe user flag. So, if user is safe we don’t allow all those other features other than safe. Currently it’s the opposite.

You can spin up a new instance, and give it a shot yourself. Please try.

[jcameron]

Hang on, I just created a new schedule backup in Virtualmin, let it run, and then was able to restored from the Backup Logs page just fine.

[jcameron]

On the backup log page for a problem backup, what does the "Run by web user" field?

[iliaross]

On the backup log page for a problem backup, what does the "Run by web user" field?

It clearly says it's run by root because the backup was triggered by root. It's also in the backup log.

There is clearly a bug. This PR fixes it so things work. Though, we'll need to discuss more in the chat about what we expect the domain owner to do in terms of features and if they should ever be allowed to make restores with all features enabled.

I sent you the link via email to one of our servers running Virtualmin with a backup made by root just now. It behaves exactly as I described earlier. I'm surprised you couldn't reproduce it.

[jcameron]

Actually I think the bug is here : https://github.com/virtualmin/virtualmin-gpl/blob/master/restore_form.cgi#L35

I'll check it out later today ...

[jcameron]

Ok, check out this fix : df5d130

There was indeed a bug!

[iliaross]

Alright, thanks! Yet, it doesn't address other parts of the code that I addressed in this PR. In particular calls to get_available_backup_features.

[iliaross]

You can apply your patch to the Virtualmin server I mentioned earlier in the email and see if the issue still persists.

[jcameron]

I applied the patch - please try it out

[iliaross]

Alright, I have applied the patch to my development system and tested things out—it does seem to work for root user now.

A few questions though:

1. Will it work correctly for domain owner?
2. Do we expect a domain owner ever restore full backups like Apache settings or they are always locked to restore save features only? For example if a backup and restore are allowed for domain owner—do we expect backup and restore work with all features or only safe? And, the same question when master administrator allows owner restore—do we expect backup and restore work with all features or only safe?

[jcameron]

1. Yes it worked on my system.
2. Yes, a domain owner can safely restore a backup created by root, with all features

[iliaross]

2. Yes, a domain owner can safely restore a backup created by root, with all features

Can a domain owner safely restore backups with all features ever? Is so, when?

[jcameron]

Can a domain owner safely restore backups with all features ever? Is so, when?

Yes, if the backup was created by root.

[iliaross]

Yes, if the backup was created by root.

What if the backup was created by a domain owner?

[jcameron]

I think yes, as long as it's the same owner, and if it's signed.

[iliaross]

I think yes, as long as it's the same owner, and if it's signed.

1. What difference does it make if it's signed or not?
2. The domain owner can set up a "backup encryption key" too, right?

[jcameron]

The signing is a guarantee that a backup made by Virtualmin wasn't modified by someone else.

[iliaross]

If a user has an access to a private key they can modify a backup still, and potentially corrupt one of the global features?

[jcameron]

Yes, correct, which is why the Virtualmin private keys need to be kept secure.