HTTP AUTH password to REST server included in error

[ProactiveServices]

trafficstars

Output of restic version

restic 0.9.6 compiled with go1.13.4 on linux/amd64

How did you run restic exactly?

restic backup -r rest:https://user:[email protected]/ ~

Fatal: unable to open config file: client.Head: Head https://user:***@rest-server.invalid/config: dial tcp: lookup rest-server.invalid on 127.0.0.53:53: server misbehaving Is there a repository at the following location? rest:https://user:[email protected]/

Expected behavior

HTTP AUTH password squelched from error text.

Actual behavior

HTTP AUTH password is listed in error text.

Did restic help you today? Did it make you happy in any way?

restic helps me every day, which means to makes me happy every day :-)

[fd0]

Oh, interesting, that should not happen. Thanks a lot for letting us know!

I chuckled when reading the error message: server misbehaving 😆

[greatroar]

Having to pass passwords on the command line is a more serious issue since they can be seen by any user with shell access to the machine. Passing them through the environment is cleaner since only root can see them then.

[ProactiveServices]

Agreed, thanks for the tip. I came across the bug while working with a temporary test repo and since moved into envvars for production.

[JensErat]

This also happens in restic --help, where the repo URL (including the basic auth password) is listed as default for --repo.

[rawtaz]

@JensErat Please show the complete command and all of its output where this happens, with the latest version of restic (0.11.0).

[JensErat]

Sure! It happens when the help is printed, and lists the password in the defaults of the --repo flag. I purged some of the many help lines for brevity (scrolls out of the screen to the right in the formatted view, though):

$ RESTIC_REPOSITORY=rest:https://user:[email protected]/repo ./restic --help

restic is a backup program which allows saving multiple revisions of files and
directories in an encrypted repository stored on different backends.

[snip]

  -q, --quiet                      do not output comprehensive progress report
  -r, --repo repository            repository to backup to or restore from (default: $RESTIC_REPOSITORY) (default "rest:https://user:[email protected]/repo")
      --repository-file file       file to read the repository location from (default: $RESTIC_REPOSITORY_FILE)
      --tls-client-cert file       path to a file containing PEM encoded TLS client certificate and private key
  -v, --verbose n                  be verbose (specify multiple times or a level using --verbose=n, max level/times is 3)

Use "restic [command] --help" for more information about a command.
 
$ ./restic version                                                                                              
restic 0.11.0 compiled with go1.15.3 on linux/amd64

[rawtaz]

Ah, right. One has to set the environment variable for it. Thanks.

[JensErat]

Indeed, my original report missed that detail. ~~Checking further, this also happens for all kinds of (error) messages involving the repository.~~ (just realized I was indeed mixing up old and new restic versions, only observed this for the help text so far) I guess S3 storage will also be affected by this.

[rawtaz]

• That default value is only shown when there's a value set for the corresponding environment variable.
• We already mention in the description of the option what the default is calculated from (the environment variable).
• The way it looks when there's two parenthesis containing a default value description is rather ugly.
• I can't remember ever seeing a need for the printing of the actual default value as it is resolved, it's more interesting to know what it is resolved from.
• With all this in mind, I'd like to suggest that we start using the default value parameter in the pflag.StringVarP etc to define what the default value is resolved from rather than what it is.
• I suggest that we do this for all options throughout restic, not just this or other sensitive ones (where passwords may be shown).

E.g. we'd replace:

`f.StringVarP(&globalOptions.Repo, "repo", "r", os.Getenv("RESTIC_REPOSITORY"), "`repository` to backup to or restore from (default: $RESTIC_REPOSITORY)")`

with:

f.StringVarP(&globalOptions.Repo, "repo", "r", "$RESTIC_REPOSITORY", "`repository` to backup to or restore from")

which will produce the following output regardless of what the environment variable is set to, if anything:

-r, --repo repository            repository to backup to or restore from (default "$RESTIC_REPOSITORY")

This of course requires that we don't feel a need to print the actual resolved default values, which I don't think we need to do.

[JensErat]

@rawtaz any news on this? I'd be fine with providing a pull request, but it looks to me like this is not only a concept, but pretty much code already. Your approach seems very reasonable to me. Maybe one thing to consider though: what happens, if neither variable nor flag are set? Is this something we should explain to the user?

[MichaelEischer]

The full changes to only show the environment variable name are as follows. For example for RESTIC_KEY_HINT the code could look like this:

-       f.StringVarP(&globalOptions.KeyHint, "key-hint", "", os.Getenv("RESTIC_KEY_HINT"), "`key` ID of key to try decrypting first (default: $RESTIC_KEY_HINT)")
+       f.StringVarP(&globalOptions.KeyHint, "key-hint", "", "$RESTIC_KEY_HINT", "`key` ID of key to try decrypting first")
+       globalOptions.KeyHint = os.Getenv("RESTIC_KEY_HINT")

The default value shown on the command line is captured when defining the flag. But we can overwrite its value afterwards. The change should also be applied to flags using a similar pattern.