vmbackup:Facilitate Flag-Based Provisioning for Sensitive Data in snapshot.createURL

[wasim-nihal]

Is your feature request related to a problem? Please describe

Presently, when utilizing snapshot.createURL with basic authentication or a snapshotKey, the vmbackup tool requires the inclusion of sensitive credentials directly within the URL. For instance:
-snapshot.createURL=http://<user>:<pass>@localhost:8428/snapshot/create?authKey=<snapshotAuthKey>

However, this practice exposes credentials, which is against our organizational security policies.

Describe the solution you'd like

The proposed resolution involves the introduction of three new flags: snapshot.basicAuthUsername, snapshot.basicAuthPassword, and snapshot.authKey. These flags would be generated using flagutil.NewPassword(), enabling users to provide them via hardcoding, file paths, or HTTP URLs. This approach is particularly advantageous in Kubernetes environments, where such credentials can be supplied via Secrets and mounted within the respective pods and allows them to be dynamically updated.

Describe alternatives you've considered

No response

Additional information

If the community believes this feature would be useful, I would be happy to open a PR for this.

[hagen1778]

Hello @wasim-nihal!

However, this practice exposes credentials, which is against our organizational security policies.

The suggested solution is to introduce new flags to specify the secrets. But isn't it the same? The auth key will be still exposed via snapshot.authKey.

vmbackup supposed to be a sidecar, application tied together with vmstorage process on the same pod. Hence, it is okay to disallow access to snapshot API for everything outside the pod. This makes it easier to protect the sensitive API.

Regarding secrets, all cmd-line flags in VM can be set via ENV variables - see https://docs.victoriametrics.com/#environment-variables. The ENV variable can be populated from k8s secrets.

[wasim-nihal]

Hi @hagen1778, thanks for your reply!!

The suggested solution is to introduce new flags to specify the secrets. But isn't it the same? The auth key will be still exposed via snapshot.authKey.

The proposed solution would segregate basic auth credentials and snapshot auth key from the URL and allow them to be specified as a file path (ex --snapshot.authKey=file://). This file path could be the path of the mounted secret containing the credential.

Also, with the existing approach of providing basic auth credentials and snapshot key, there is a chance of exposing snapshotAuthKey. For instance, if the authKey is configured correctly and basic auth credential is wrong, in that case the below log will expose the valid authKey. So, I believe the segregation of credentials from URL would be more generic and cleaner approach. (Note: The above PR dose not cover masking of authKey for existing format -snapshot.createURL=http://<user>:<pass>@localhost:8428/snapshot/create?authKey=<snapshotAuthKey>. Please let me know if this should be included as well.)

VictoriaMetrics/lib/snapshot/snapshot.go

Line 54 in 21d9393

return "", fmt.Errorf("unexpected status code returned from %q: %d; expecting %d; response body: %q", u.Redacted(), resp.StatusCode, http.StatusOK, body)

Regarding secrets, all cmd-line flags in VM can be set via ENV variables - see https://docs.victoriametrics.com/#environment-variables. The ENV variable can be populated from k8s secrets.

Yes, in this way one could configure the create/deleteURL(with credentials) as k8s secret and feed it via an environment variable. But we cannot use this approach because our organization mandates that sensitive info is not provided via env variables (this is not specific to Victoria Metrics but in general some applications/processes tend to dump the env variables in logs in case of exceptions)

Please do let me know your thoughts on this. You can refer the above PR and provide your comments.

[hagen1778]

@wasim-nihal wdyt about changing -snapshot.createURL type to flagutil.NewPassword?
https://github.com/VictoriaMetrics/VictoriaMetrics/blob/master/lib/flagutil/password.go

With this change, -snapshot.createURL will get the following abilities:

1. Its content will be secured in log outputs
2. It will get the ability to be read from file, which I assume is aligned with your requirements

[wasim-nihal]

@wasim-nihal wdyt about changing -snapshot.createURL type to flagutil.NewPassword?

@hagen1778 , thanks for your suggestion. This is simple and would fulfill our requirements.

Is it okay if I open a PR with this change?

[hagen1778]

Is it okay if I open a PR with this change?

Sure! Will appreciate it!

[hagen1778]

As discussed here, auth token still can be exposed in logs even if set via cmd-line flag. The proper solution would be to make vmstorage and vmsingle to read auth token from HTTP headers. Th ability to read token from GET params should remain for backward compatibility.