From 201fd6de1eb0dd03cf5c24787d6a550d07b1390a Mon Sep 17 00:00:00 2001 From: Zakhar Bessarab Date: Wed, 19 Jun 2024 12:31:48 +0400 Subject: [PATCH] lib/fs/fscore: do not trim content from path (#6503) ### Describe Your Changes Trimming content which is loaded from an external pass leads to obscure issues in case user-defined input contained trimmed chars. For example. user-defined password "foo\n" will become "foo" while user will expect it to contain a new line. --- For example, a user defines a password which ends with `\n`. This often happens when user Kubernetes secrets and manually encodes value as base64-encoded string. In this case vmauth configuration might look like: ``` users: - url_prefix: - http://vminsert:8480/insert/0/prometheus/api/v1/write name: foo username: foo password: "foobar\n" ``` vmagent configuration for this setup will use the following flags: ``` -remoteWrite.url=http://vmauth:8427/ -remoteWrite.basicAuth.passwordFile=/tmp/vmagent-password -remoteWrite.basicAuth.username="foo" ``` Where `/tmp/vmagent-password` is a file with `foobar\n` password. Before this change such configuration will result in `401 Unauthorized` response received by vmagent since after file content will become `foobar`. --- An example with Kubernetes operator which uses a secret to reference the same password in multiple configurations.
See full manifests `Secret`: ``` apiVersion: v1 data: name: Zm9v # foo password: Zm9vYmFy # foobar\n username: Zm9v= # foo kind: Secret metadata: name: vmuser ``` `VMUser`: ``` apiVersion: operator.victoriametrics.com/v1beta1 kind: VMUser metadata: name: vmagents spec: generatePassword: false name: vmagents targetRefs: - crd: kind: VMAgent name: some-other-agent namespace: example username: foo # note - the secret above is referenced to provide password passwordRef: name: vmagent key: password ``` `VMAgent`: ``` apiVersion: operator.victoriametrics.com/v1beta1 kind: VMAgent metadata: name: example spec: selectAllByDefault: true scrapeInterval: 5s replicaCount: 1 remoteWrite: - url: "http://vmauth-vmauth-example:8427/api/v1/write" # note - the secret above is referenced as well basicAuth: username: name: vmagent key: username password: name: vmagent key: password ```
Since both config target exactly the same `Secret` object it is expected to work, but apparently the result will be `401 Unauthrized` error. ### Checklist The following checks are **mandatory**: - [x] My change adheres [VictoriaMetrics contributing guidelines](https://docs.victoriametrics.com/contributing/). --------- Signed-off-by: Zakhar Bessarab Signed-off-by: hagen1778 Co-authored-by: hagen1778 --- docs/CHANGELOG.md | 3 +++ lib/flagutil/password_test.go | 4 ++-- lib/fs/fscore/fscore.go | 5 +---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 1d5408e12..75289344f 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -32,6 +32,8 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/). **Update note 1: the `--vm-disable-progress-bar` command-line flag at `vmctl` was deprecated. Use `--disable-progress-bar` instead.** +**Update note 2: `*.passwordFile` and similar flags are no longer trimming trailing whitespaces at the end of content. Make sure to update the templating of password files or HTTP endpoints to not include trailing whitespaces before the upgrade. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6503) PR for the details.** + * FEATURE: all VictoriaMetrics components: use constant-time comparison for comparing HTTP basic auth credentials and auth keys. This should prevent timing attacks when comparing these credentials. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6392) for details. Thanks to @wasim-nihal for [the pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6423). * FEATURE: [alerts-vmagent](https://github.com/VictoriaMetrics/VictoriaMetrics/blob/master/deployment/docker/alerts-vmagent.yml): add new alerting rules `StreamAggrFlushTimeout` and `StreamAggrDedupFlushTimeout` to notify about issues during stream aggregation. * FEATURE: [dashboards/vmagent](https://grafana.com/grafana/dashboards/12683): add row `Streaming aggregation` with panels related to [streaming aggregation](https://docs.victoriametrics.com/stream-aggregation/) process. @@ -43,6 +45,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/). * FEATURE: [vmalert-tool](https://docs.victoriametrics.com/vmalert-tool/): support file path with hierarchical patterns and regexpes, and http url in unittest cmd-line flag `-files`, e.g. `-files="http:///path/to/rules"` or `-files="dir/**/*.yaml"`. * BUGFIX: all VictoriaMetrics components: prioritize `-configAuthKey` and `-reloadAuthKey` over `-httpAuth.*` settings. This change aligns behavior of mentioned flags with other auth flags like `-metricsAuthKey`, `-flagsAuthKey`, `-pprofAuthKey`. Check [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6329). +* BUGFIX: all VictoriaMetrics components: do not trim trailing spaces when reading content from `*.passwordFile` and similar flags. Previously, trailing spaces were trimmed from the content of the password file, which could lead to unexpected authentication errors. * BUGFIX: [vmctl](https://docs.victoriametrics.com/vmctl/): add `--disable-progress-bar` global command-line flag. It can be used for disabling dynamic progress bar for all migration modes. `--vm-disable-progress-bar` command-line flag is deprecated and will be removed in the future releases. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6367). * BUGFIX: [stream aggregation](https://docs.victoriametrics.com/stream-aggregation/): prevent [rate_sum](https://docs.victoriametrics.com/stream-aggregation/#rate_sum) and [rate_avg](https://docs.victoriametrics.com/stream-aggregation/#rate_avg) producing `NaN` results for stale time series. Before, when series matched for aggregation became stale or weren't updated during aggregation interval, the `rate_sum` or `rate_avg` could produce data point with `NaN` value. During visualization, such aggregation results would be displayed as gaps in time series. * BUGFIX: [vmalert](https://docs.victoriametrics.com/vmalert/): fix path for system links printed on default vmalert's UI page when `-http.pathPrefix` is set. diff --git a/lib/flagutil/password_test.go b/lib/flagutil/password_test.go index 8a9583406..e7a03c2a1 100644 --- a/lib/flagutil/password_test.go +++ b/lib/flagutil/password_test.go @@ -32,7 +32,7 @@ func TestPassword(t *testing.T) { // read the password from file by relative path localPassFile := "testdata/password.txt" - expectedPassword = "foo-bar-baz" + expectedPassword = "foo-bar-baz\n\n\n" path := "file://" + localPassFile if err := p.Set(path); err != nil { t.Fatalf("cannot set password to file: %s", err) @@ -52,7 +52,7 @@ func TestPassword(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %s", err) } - expectedPassword = "foo-bar-baz" + expectedPassword = "foo-bar-baz\n\n\n" path = "file://" + localPassFile if err := p.Set(path); err != nil { t.Fatalf("unexpected error: %s", err) diff --git a/lib/fs/fscore/fscore.go b/lib/fs/fscore/fscore.go index b67028203..03cc37562 100644 --- a/lib/fs/fscore/fscore.go +++ b/lib/fs/fscore/fscore.go @@ -7,8 +7,6 @@ import ( "net/url" "os" "path/filepath" - "strings" - "unicode" ) // ReadPasswordFromFileOrHTTP reads password for the give path. @@ -19,8 +17,7 @@ func ReadPasswordFromFileOrHTTP(path string) (string, error) { if err != nil { return "", err } - pass := strings.TrimRightFunc(string(data), unicode.IsSpace) - return pass, nil + return string(data), nil } // ReadFileOrHTTP reads path either from local filesystem or from http if path starts with http or https.