-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
now with just 2 files and no old stuff from month ago #1314
Conversation
got rid of it
Breaking the visual flow of the script, the whiptail used in the installation process should be avoided. My suggestion is to refrain from setting an admin token during installation and instead utilize the update menu to set or reset an admin token. |
also fixed the missing "," after the ADMINTOKEN line added a check if argon2 is installed or not during update
should I add a hint here for the user that let him know how to setup an admin_token if he wants to? echo -e "${APP} should be reachable by going to the following URL.
${BL}http://${IP}:8000${CL} \n" something like this echo -e "ADMIN_TOKEN can be setup by running the Update-Script inside the LXC.
${APP} should be reachable by going to the following URL.
${BL}http://${IP}:8000${CL} \n" |
I'd just leave that to the instructions |
we don't need to overwrite the other config variables inside the vaultwarden file
now it is more consistent to the other sed command for the other file :) |
Is this any better?
|
No it is not, if you check if the file doesn't exists you don't have to echo anything in it. But if the file exists you should keep all the other 20-30 lines in it and don't overwrite them, that's why I used the sed command. And vise versa we don't need to change anything in the conf.d file if the user already made changes in the webui because then the admin_token will be used from the config.json and the remaining 4 settings from the conf.d file are still needed in that case |
By reading the code, you will notice that it creates the "/var/lib/vaultwarden/config.json" file if it's not already there. This ensures that all installations, whether old or new, are identical. The two sed commands achieve the same result as your four commands but in a more succinct manner. |
This would be my suggestion.
|
before I go in detail about your suggestion, I have a question for you that I want to understand :) if ! command -v argon2 >/dev/null 2>&1; then apk add argon2 &>/dev/null; fi here for comparison: if ! apk -e info newt >/dev/null 2>&1; then apk add -q newt; fi wouldn't it be easier to read and be more consistent with this? if ! apk -e info argon2 >/dev/null 2>&1; then apk add -q argon2; fi my 2 SED commands for one file was just because I didn't find the right syntax to get all done in a oneiner, but you got that point. I checked the right permissions for the file and ownership to make it feel vaultwarden it got the file created by itself. Your syntax for the file isn't correct and will lead to a not working vaultwarden. The If we follow your suggestion I would recommend this code here but then we have a small coding issue. If there is no config.json file, then we create a new JSON object that is filled with the value for the token. In that case we don't need the So here my suggestion with your code and right syntax for the file and ownership: if NEWTOKEN=$(whiptail --passwordbox "Set the ADMIN_TOKEN" 10 58 3>&1 1>&2 2>&3); then
if [[ -z "$NEWTOKEN" ]]; then exit; fi
if ! command -v argon2 >/dev/null 2>&1; then apk add argon2 &>/dev/null; fi
TOKEN=$(echo -n ${NEWTOKEN} | argon2 "$(openssl rand -base64 32)" -t 2 -m 16 -p 4 -l 64 -e)
if [[ ! -f /var/lib/vaultwarden/config.json ]]; then
echo '{"admin_token": ""}' >/var/lib/vaultwarden/config.json
chown 100:101 /var/lib/vaultwarden/config.json
chmod 644 /var/lib/vaultwarden/config.json
fi
sed -i "s|\"admin_token\": .*|\"admin_token\": \"${TOKEN}\",|" /var/lib/vaultwarden/config.json
sed -i "s|export ADMIN_TOKEN=.*|export ADMIN_TOKEN='${TOKEN}'|" /etc/conf.d/vaultwarden
rc-service vaultwarden restart -q
fi And here me suggestion with the sed command I used before without the issue from above, much smaller now with your syntax for sed and we don't have to set permissions on the file if NEWTOKEN=$(whiptail --passwordbox "Setup your ADMIN_TOKEN (make it strong)" 10 58 3>&1 1>&2 2>&3); then
if [[ -z "$NEWTOKEN" ]]; then exit-script; fi
if ! command -v argon2 >/dev/null 2>&1; then apk add argon2 &>/dev/null; fi
TOKEN=$(echo -n ${NEWTOKEN} | argon2 "$(openssl rand -base64 32)" -e -id -k 19456 -t 2 -p 1)
if [[ ! -f /var/lib/vaultwarden/config.json ]]; then
sed -i "s|export ADMIN_TOKEN=.*|export ADMIN_TOKEN='${TOKEN}'|" /etc/conf.d/vaultwarden
else
sed -i "s|\"admin_token\": .*|\"admin_token\": \"${TOKEN}\",|" /var/lib/vaultwarden/config.json
fi
rc-service vaultwarden restart -q
fi In my opinion we shouldn't care about the 2 files with 2 times the admin_token in it. Because that is the normal behaviour of vaultwarden. If a user changes something (like SMTP Server f.e. or the admin_token itself through the webui) the old conf.d file will be left behind (except those 4 needed lines) and everything else will be taken from the config.json. |
Consistency is important in programming, and both code snippets achieve the same goal functionally. However, the second code snippet is a more versatile solution since it verifies the availability of the argon2 command in the shell's search path, making it useful in situations where the package manager is different or unavailable. You can choose the method that suits you best. Once you make the necessary changes, I can merge the code. Personally, I find coding for Alpine Linux unpleasant due to its minimalist approach. |
I remember now, |
yeah then we had to check whiptail as the command... I guess it is a good idea to stay at your recommendation because of the availability of the package manager... so my last commit already included everything, you can review it :) |
Description
The ADMIN_TOKEN should made hashed instead of plain-text. Further information can be found here: https://github.com/dani-garcia/vaultwarden/wiki/Enabling-admin-page#secure-the-admin_token
We get this on the
/admin
page as a warning on login.If we want to fix it with a already running installation, we just have to run the update script to setup a new or the old admin token. Then this will be hashed with the
owasp
preset and inserted on the right places of the config files.Here a few screenshots during the setup:
This will be shown if a user doesn't setup an ADMIN_TOKEN during the installation :)
Type of change