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.

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

The backup-with-borg script shouldnt be called with sudo ... #134

Open
wants to merge 1 commit into
base: testing
Choose a base branch
from

Conversation

alexAubin
Copy link
Member

Problem

Borg is expected to add a sudoer file allowing borg to run a few specific command as root ...

But instead the entire script is apparently run as root so there's a false sense of "security" ...

Solution

Don't run the script as root ... It's supposed to work without running it as root because it calls sudo on commands allowed in the sudoer file ...

PR Status

  • Code finished and ready to be reviewed/tested
  • The fix/enhancement were manually tested (if applicable)

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

@zamentur
Copy link
Contributor

Need to be tested on the battle field to be sure that nothing fail...

@zamentur
Copy link
Contributor

What's the worse ?

No backup because the damned script use a new forbidden command (missing in sudoer)
OR to give sudo permission on all the script ...

On my side, i consider we should prefer to be sure to have backup (at least until we have a way to do CI test on specific action like trigger a backup).

@alexAubin
Copy link
Member Author

Wokay but that's just a false dilemna ... like "Do you prefer something that works and is insecure, or that is secure but doesnt work" ... Like, yeah, actually we can have both, this just need to be carefully tested, and the cron checks are supposed to notify when backups are not properly created x_x ...

@zamentur
Copy link
Contributor

and the cron checks are supposed to notify when backups are not properly created x_x ...

borgserver cron only check that the repo has changed, but some archive could be incomplete or missing.

The cron task on borg_ynh send a report of what succeed and what failed (and diagnosis too cause borg.service is marked as failed), but it's still easy to have temporary issues triggering alert that mask a big problem.

You also know the issue of people that never consult their self-hosted admin email...

As florent said we miss something to check backup (in an ideal world that archives contains sql dumps with a lot of data).

something that works and is insecure
I don't think putting sudo on this script enlarge so much the attack area...

Remember that i added the borg user only cause linter was crying, but the truth is this app need to access on all sensible data to backup it. Keep in mind that the yunohost backup borg implementation run as root too.

Base automatically changed from testing to master August 5, 2024 16:37
@kay0u kay0u changed the base branch from master to testing August 14, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants