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

Store sensitive values in OS keyring #548

Open
homandr opened this issue Nov 11, 2024 · 4 comments
Open

Store sensitive values in OS keyring #548

homandr opened this issue Nov 11, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@homandr
Copy link

homandr commented Nov 11, 2024

First of all, thank you for this awesome piece of software. Backrest is lightweight, has clear and intuitive interface, and has pretty much all the features I ever needed in a backup tool. Restic is awesome on its own, but an average Windows home user really needs a GUI. I can now recommend Backrest+restic to all my non-technical friends.

Currently Backrest stores repository password and extra environment variables (often used to store credentials for cloud storage) in plain text in config.json. This is less than ideal. The standard place for sensitive information is OS keyring. It appears there are several Go-based libraries that can help with that:
https://github.com/99designs/keyring
https://github.com/zalando/go-keyring
https://github.com/tmc/keyring

Admittedly, I don't know how much true security this would add since presumably if the host is compromised under user's account, that account's keyring is also compromised. However, I think it's still much better than just keeping credentials in plain text, which doesn't require any special skills to read. I'm not a security expert, although I'm sure OS keyring has its own weaknesses. Nevertheless, it's a standard place for these things and would move this responsibility from Backrest to OS implementation.

There are probably other ways to store passwords securely even outside of keyring. For example, on Windows in PowerShell I can use ConvertTo-SecureString and ConvertFrom-SecureString to produce an encrypted string that can be decrypted only under the same user account. This string could be stored in a plain text file.

For the time being at the very least I would recommend Windows users to encrypt their %appdata%\backrest folder (not available in Home Edition):
https://support.microsoft.com/en-us/windows/how-to-encrypt-a-file-1131805c-47b8-2e3e-a705-807e13c10da7
This way, at least if user's laptop is lost or stolen, their backup should be reasonably safe.

@homandr homandr added the enhancement New feature or request label Nov 11, 2024
@garethgeorge
Copy link
Owner

garethgeorge commented Nov 11, 2024

Agree with all of your recommendations on the disk encryption front, this is a good best practice. I'd also like to aim to get a security article written for backrest's docs.

I'd add to this with the recommendation that users enable a 30 day object retention on their backup server (e.g. s3, b2) to protect against a compromised key being used to delete backups (i.e. ransomware attacks).

On the keyring front I think a decent implementation direction would be an implementation of ConfigStore interface https://github.com/garethgeorge/backrest/blob/main/internal/config/config.go#L15-L18 to store the whole config in the keyring -- looks like most keyrings support large entry sizes, large enough for the whole thing as a JSON string.

This would probably make most sense as an opt in feature controlled by either an env var or a cli flag (e.g. it wouldn't make sense as a default for docker).

@homandr
Copy link
Author

homandr commented Nov 12, 2024

Thank you for looking into this! I'm not sure it would be a good idea to store the whole config there. A few concerns come to mind:

  1. I did a bit of searching, and it appears that Windows credentials store is quite limited. See Windows credential helper doesn't support passwords greater than 2,500 characters in length docker/docker-credential-helpers#190 and Lack of support for tokens over 2500 characters danieljoos/wincred#18 (comment). I quickly tested from GUI side by trying to add a very long password to a new Windows credential, it didn't take it.
  2. I see you have a neat feature of backing up config.json. This functionality would be broken.
  3. Sometimes it's nice to see the actual configuration JSON, say, when troubleshooting.

I discovered restic already has --password-command option that could be used to obtain the repo password securely. I'm going to look deeper into that, not sure if it would take precedence over RESTIC_PASSWORD that Backrest sets up. However, the issue of storage backend credentials remains. Arguably, this is even more important than the repo password.

I'm thinking now that maybe an easier interim solution to implement would be to have Backrest run a pre-restic command hook for setting up the variables. And then leave it up to the user what script and tool they want to use. Some might not want to use OS keyring but something like gpg or a password manager. Technically, it's doable already if launching Backrest from a script, but now we are talking about setting up shortcuts, a way to hide the shell window (on Windows) and so on. Not something an average desktop user knows how to deal with.

Sure, having native keyring support would be a nice thing out of the box, I don't know what would be easier to implement. Ideally, both! But we can't have everything 😄 , Backrest already does a lot to make users' life easier.

@garethgeorge
Copy link
Owner

Thanks for calling out that windows limit -- thinking a bit more about this over the last few days I think it would also be fine for backrest to simply store an encryption key in the secret store and to store an encrypted config on disk. E.g. ~/.local/share/backrest/config.json.aes-enc or such. This approach would also be compatible with the config backups as you point out -- but would be more painful re: an opaque encrypted file is essentially impossible to edit outside of backrest's UI.

I think that could be mitigated by adding a JSON preview of the config or such in Backrest's UI possibly.

On the password command side of things, if you provide RESTIC_PASSWORD_COMMAND as an env var you are able to leave the password field of the repo empty.

@homandr
Copy link
Author

homandr commented Nov 16, 2024

I agree it's the best approach. I was just going to suggest the same idea after pondering over it. It could work like so:

  1. Upon starting Backrest checks OS keyring for a predefined credential name.
  2. If it doesn't exist, proceed as is today.
  3. If it exists, obtain the password/key.
  4. Use this password/key for encrypting/decrypting config.json for any write/read disk operations with it.

This encryption password should be an optional feature, maybe even autogenerated by WebUI when enabled, with the ability for user to override to a custom password value. Sorry I can't provide code contributions, I know zero about Go.

but would be more painful re: an opaque encrypted file is essentially impossible to edit outside of backrest's UI.
I think that could be mitigated by adding a JSON preview of the config or such in Backrest's UI possibly.

That's a good point, it's inevitable with full encryption approach. Maybe even allow edits in JSON preview in the UI? Or allow to export/import the config.

For now I implemented a workaround for Windows. Sharing it here in case anyone might find it useful. This approach uses Windows Data Protection API to stored encrypted values on disk (see https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.utility/export-clixml?view=powershell-5.1). The sensitive variables are then decrypted and available as plain text in the session environment for Backrest process. It's not the highest security but good enough to prevent plain text on disk.

Open PowerShell and run this snippet to save encrypted password to a file.
You may use anything for username, it won't be used.

Get-Credential | Export-Clixml $env:APPDATA\backrest\repo_cred.xml

In Backrest UI configuration add this backup flag to pass to restic:

--password-command "powershell (Import-Clixml $env:APPDATA/backrest/repo_cred.xml).GetNetworkCredential().Password"

Add this flag to both repo configuration and the backup plan. Note the use of forward slashes; this is intentional. Restic doesn't like backslashes with this flag, and PowerShell is smart enough to understand either one. Restic itself can take backslashes with proper quoting, but adding all the extra quotes breaks Backrest. Just use the approach above, it's easier anyway.

That's it for the repo password. Encrypting variables for cloud storage provider is trickier because right now the only way to supply environment variables to Backrest is with a wrapper script. This script will decrypt the values and create the variables, then launch Backrest.

Open PowerShell and run this snippet to save encrypted S3 access credentials.

$vars = @(  
  "AWS_ACCESS_KEY_ID"
  "AWS_SECRET_ACCESS_KEY"
)
$credentials = foreach ($var in $vars) {
  Get-Credential -UserName "$var" -Message "Enter your credentials"
}
$credentials | Export-Clixml $env:APPDATA\backrest\s3_cred.xml

Next, you need to create a startup script. Create start-backrest.ps1 file in %APPDATA%\backrest with this contents:

$appdir = "$env:APPDATA\backrest"
$creds = Import-CliXml "$appdir\s3_cred.xml"
foreach ($c in $creds) {
  [Environment]::SetEnvironmentVariable($c.UserName, $c.GetNetworkCredential().Password)
}
Start-Process "C:\Program Files\Backrest\backrest-windows-tray.exe" -WorkingDirectory "$appdir"

Next, you need to remove the original startup Backrest shortcut. Go to Start - Run, type shell:startup. Remove the shortcut.

Next, there are two options to start this script upon logon.
Option 1 - startup folder
In the same shell:startup folder right-click - New - Shortcut.
Copy and paste this:

powershell -ExecutionPolicy Bypass -File %APPDATA%\backrest\start-backrest.ps1

Go to properties and change Run to "minimized".

Option 2 - Task Scheduler
In PowerShell window run this snippet to create a scheduled task (you could also do it manually in GUI):

$trigger = New-ScheduledTaskTrigger -AtLogOn -User $env:USERNAME
$action = New-ScheduledTaskAction -Execute "powershell.exe" -Argument "-ExecutionPolicy Bypass -File %APPDATA%\backrest\start-backrest.ps1" -WorkingDirectory "%APPDATA%\backrest"
$settings = New-ScheduledTaskSettingsSet -AllowStartIfOnBatteries -ExecutionTimeLimit 0 -DontStopIfGoingOnBatteries -Hidden
Register-ScheduledTask -TaskName "Backrest" -Trigger $trigger -Action $action -Settings $settings
Start-ScheduledTask "Backrest"

If you use more than one bucket with the same provider, change the vars array above with some unique variable names like AWS_ACCESS_KEY_ID_2. You will then need to reference it in Backrest UI like so: AWS_ACCESS_KEY_ID=${AWS_ACCESS_KEY_ID_2} in order for this repo to use each own variables. That's because the variables above are defined within Backrest process environment and will be inherited by all restic instances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants