Skip to content

Conversation

@morevnaproject
Copy link

Problem:
If BACKUPPC_WEB_USER environment variable is empty and we already have some configuration in /etc/backuppc/htpasswd, then initialization script adds "backuppc" user, which overrides previous admin user.

This PR fixes the problem and eliminates this security problem.

Copy link
Owner

@adferrand adferrand left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR! Indeed, this new behavior allows to construct as you wish the credentials to connect to backuppc.

Two improvements I think that would be relevant:

  • first, I think we do not need to rely on emptyness for both BACKUPPC_WEB_USER and BACKUPPC_WEB_PASSWD. I think emptyness on BACKUPPC_WEB_USER to trigger the logic. Then you do not need to specify anything for BACKUPPC_WEB_PASSWD, as it will ignored, and that makes a cleaner configuration,
  • second, you should add in the README a line about this, saying that you can deactivate the credentials provisioning by passing an empty string to BACKUPPC_WEB_USER, to let people know that is possible.

Thanks in advance!

@adferrand
Copy link
Owner

adferrand commented Jan 29, 2019

Also I did not catch that it was for the v3 branch. I will also integrate it on v4 once it is merged. Here I maintain my comments, as it will emerge eventually to the current branch.

@morevnaproject
Copy link
Author

Actually, I copied this approach from BackupPC v4 (master) branch -

# Configure WEB UI access
configure_admin=""
if [ ! -f /etc/backuppc/htpasswd ]; then
htpasswd -b -c /etc/backuppc/htpasswd "${BACKUPPC_WEB_USER:-backuppc}" "${BACKUPPC_WEB_PASSWD:-password}"
configure_admin="--config-override CgiAdminUsers='${BACKUPPC_WEB_USER:-backuppc}'"
elif [ -n "$BACKUPPC_WEB_USER" -a -n "$BACKUPPC_WEB_PASSWD" ]; then
touch /etc/backuppc/htpasswd
htpasswd -b /etc/backuppc/htpasswd "${BACKUPPC_WEB_USER}" "${BACKUPPC_WEB_PASSWD}"
configure_admin="--config-override CgiAdminUsers='$BACKUPPC_WEB_USER'"
fi

With v4 everything is fine, so when I discovered that problem for v3 I copied the solution.

@adferrand
Copy link
Owner

Tricked by an old PR ... For my defense, I lost PR history at some point. As it is on master, I take on my charge the actions for the comments I made if you want.

@morevnaproject
Copy link
Author

Yes, please. Sorry I do not have much time on handling this - just wanted to share the solution with users who possibly hit the same problems as me. ^__^"

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