From cc626ae3b5e64ca7159229966ed201f6cc4cf205 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Wed, 31 Jan 2024 19:45:13 +0200 Subject: [PATCH] lib/promauth: follow-up for fca3b14b7b8ea8367603e02d497f5ae208a3fba2 - Simplify the code for handling BasicAuthConfig at lib/promauth/config.go - Move the description of the change into correct place at docs/CHANGELOG.md - Put tests for username in front of tests for password at lib/promauth/config_test.go Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/5720 Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5511 --- docs/CHANGELOG.md | 3 +- lib/promauth/config.go | 107 +++++++++++++++--------------------- lib/promauth/config_test.go | 56 +++++++++---------- 3 files changed, 72 insertions(+), 94 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 85ec7455b8..c851e0061d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -28,6 +28,8 @@ The sandbox cluster installation is running under the constant load generated by ## tip +* FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent.html): add support for `username_file` option at `basic_auth` section in [`scrape_configs`](https://docs.victoriametrics.com/sd_configs/#http-api-client-options). See [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5511). Thanks to @wasim-nihal for [the initial implementation](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/5720). + * BUGFIX: fix `runtime error: slice bounds out of range` panic, which can occur during query execution. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5733). The bug has been introduced in `v1.97.0`. * BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): properly handle `avg_over_time({some_filter}[d]) keep_metric_names` queries, where [`some_filter`](https://docs.victoriametrics.com/keyconcepts/#filtering) matches multiple time series with multiple names, while `d` is bigger or equal to `3h`. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5556). * BUGFIX: dashboards/single: fix typo in query for `version` annotation which falsely produced many version change events. @@ -39,7 +41,6 @@ Released at 2024-01-30 * SECURITY: upgrade base docker image (Alpine) from 3.19.0 to 3.19.1. See [alpine 3.19.1 release notes](https://www.alpinelinux.org/posts/Alpine-3.19.1-released.html). * SECURITY: upgrade Go builder from Go1.21.5 to Go1.21.6. See [the list of issues addressed in Go1.21.6](https://github.com/golang/go/issues?q=milestone%3AGo1.21.6+label%3ACherryPickApproved). -* FEATURE: [vmagent](https://docs.victoriametrics.com/vmagent.html) added support of `username_file` for `basic_auth` in scrape_config to have config compatibility with Prometheus. See [these docs](https://docs.victoriametrics.com/sd_configs/#http-api-client-options) and [issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5511). Thanks to @wasim-nihal for the [pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/5720). * FEATURE: improve new [time series](https://docs.victoriametrics.com/keyConcepts.html#time-series) registration speed on systems with high number of CPU cores. Thanks to @misutoth for the initial idea and [implementation](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/5212). * FEATURE: make [background merge](https://docs.victoriametrics.com/#storage) more responsive and scalable. This should help the following issues: [5190](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5190), [3425](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3425), [648](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/648). * FEATURE: [graphite](https://docs.victoriametrics.com/#graphite-render-api-usage): add support for negative index in `groupByNode` and `aliasByNode` functions. Thanks to @rbizos for [the pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/5581). diff --git a/lib/promauth/config.go b/lib/promauth/config.go index e7084e59bd..4fafb3d7a7 100644 --- a/lib/promauth/config.go +++ b/lib/promauth/config.go @@ -92,8 +92,8 @@ type Authorization struct { // BasicAuthConfig represents basic auth config. type BasicAuthConfig struct { - Username string `yaml:"username"` - UsernameFile string `yaml:"username_file"` + Username string `yaml:"username,omitempty"` + UsernameFile string `yaml:"username_file,omitempty"` Password *Secret `yaml:"password,omitempty"` PasswordFile string `yaml:"password_file,omitempty"` } @@ -644,76 +644,55 @@ func (actx *authContext) initFromAuthorization(baseDir string, az *Authorization } func (actx *authContext) initFromBasicAuthConfig(baseDir string, ba *BasicAuthConfig) error { - if ba.Username == "" && ba.UsernameFile == "" { - return fmt.Errorf("missing `username` and `username_file` in `basic_auth` section. either one should be configured") - } - if ba.Username != "" && ba.UsernameFile != "" { - return fmt.Errorf("both `username`=%q and `username_file`=%q are set in `basic_auth` section", ba.Username, ba.UsernameFile) - } - if ba.Password != nil && ba.PasswordFile != "" { - return fmt.Errorf("both `password`=%q and `password_file`=%q are set in `basic_auth` section", ba.Password, ba.PasswordFile) - } - actx.getAuthHeader, actx.authHeaderDigest = fetchBasicAuthHeaderAndDigest(baseDir, ba) - return nil -} - -func fetchBasicAuthHeaderAndDigest(baseDir string, ba *BasicAuthConfig) (func() (string, error), string) { - var getUsername, getPassword func() (string, error) - var usernameDigest, passwordDigest string - - if ba.Username != "" { - usernameDigest = fmt.Sprintf("username=%q", ba.Username) - getUsername = func() (string, error) { - return ba.Username, nil - } - } - if ba.UsernameFile != "" { - usernameDigest = fmt.Sprintf("usernameFile=%q", ba.UsernameFile) - getUsername = func() (string, error) { - username, err := fscore.ReadPasswordFromFileOrHTTP(fscore.GetFilepath(baseDir, ba.UsernameFile)) - if err != nil { - return "", fmt.Errorf("cannot read username from `username_file`=%q set in `basic_auth` section: %w", ba.UsernameFile, err) - } - return username, nil - } - } + username := ba.Username + usernameFile := ba.UsernameFile + password := "" if ba.Password != nil { - passwordDigest = fmt.Sprintf("password=%q", ba.Password.String()) - getPassword = func() (string, error) { - return ba.Password.String(), nil - } + password = ba.Password.S } - if ba.PasswordFile != "" { - passwordDigest = fmt.Sprintf("passwordFile=%q", ba.PasswordFile) - getPassword = func() (string, error) { - password, err := fscore.ReadPasswordFromFileOrHTTP(fscore.GetFilepath(baseDir, ba.PasswordFile)) + passwordFile := ba.PasswordFile + if username == "" && usernameFile == "" { + return fmt.Errorf("missing `username` and `username_file` in `basic_auth` section; please specify one; " + + "see https://docs.victoriametrics.com/sd_configs.html#http-api-client-options") + } + if username != "" && usernameFile != "" { + return fmt.Errorf("both `username` and `username_file` are set in `basic_auth` section; please specify only one; " + + "see https://docs.victoriametrics.com/sd_configs.html#http-api-client-options") + } + if password != "" && passwordFile != "" { + return fmt.Errorf("both `password` and `password_file` are set in `basic_auth` section; please specify only one; " + + "see https://docs.victoriametrics.com/sd_configs.html#http-api-client-options") + } + if usernameFile != "" { + usernameFile = fscore.GetFilepath(baseDir, usernameFile) + } + if passwordFile != "" { + passwordFile = fscore.GetFilepath(baseDir, passwordFile) + } + actx.getAuthHeader = func() (string, error) { + usernameLocal := username + if usernameFile != "" { + s, err := fscore.ReadPasswordFromFileOrHTTP(usernameFile) if err != nil { - return "", fmt.Errorf("cannot read password from `password_file`=%q set in `basic_auth` section: %w", ba.PasswordFile, err) + return "", fmt.Errorf("cannot read username from `username_file`=%q: %w", usernameFile, err) } - return password, nil + usernameLocal = s } - } - authHeader := func() (string, error) { - username, err := getUsername() - if err != nil { - return "", err + passwordLocal := password + if passwordFile != "" { + s, err := fscore.ReadPasswordFromFileOrHTTP(passwordFile) + if err != nil { + return "", fmt.Errorf("cannot read password from `password_file`=%q: %w", passwordFile, err) + } + passwordLocal = s } - password, err := getPassword() - if err != nil { - return "", err - } - token64 := generateBasicAuthEncodedToken(username, password) + // See https://en.wikipedia.org/wiki/Basic_access_authentication + token := usernameLocal + ":" + passwordLocal + token64 := base64.StdEncoding.EncodeToString([]byte(token)) return "Basic " + token64, nil } - authHeaderDigest := fmt.Sprintf("basic(%q, %q)", usernameDigest, passwordDigest) - return authHeader, authHeaderDigest -} - -func generateBasicAuthEncodedToken(username string, password string) string { - // See https://en.wikipedia.org/wiki/Basic_access_authentication - token := username + ":" + password - token64 := base64.StdEncoding.EncodeToString([]byte(token)) - return token64 + actx.authHeaderDigest = fmt.Sprintf("basic(username=%q, usernameFile=%q, password=%q, passwordFile=%q)", username, usernameFile, password, passwordFile) + return nil } func (actx *authContext) mustInitFromBearerTokenFile(baseDir string, bearerTokenFile string) { diff --git a/lib/promauth/config_test.go b/lib/promauth/config_test.go index c3e12ddc12..0f51a223c6 100644 --- a/lib/promauth/config_test.go +++ b/lib/promauth/config_test.go @@ -47,7 +47,14 @@ basic_auth: password: pass `) - // basic_auth: password and password_file are set + // basic_auth: both username and username_file are set + f(` +basic_auth: + username: foo + username_file: testdata/test_secretfile.txt +`) + + // basic_auth: both password and password_file are set f(` basic_auth: username: user @@ -55,14 +62,6 @@ basic_auth: password_file: testdata/test_secretfile.txt `) - // basic_auth: username and username_file are set - f(` -basic_auth: - username: user - username_file: testdata/test_secretfile.txt - password: pass -`) - // bearer_token: both authorization and bearer_token are set f(` authorization: @@ -353,20 +352,20 @@ oauth2: ca_file: non-existing-file `) - // basic auth via non-existing password_file + // basic auth via non-existing username file + f(` +basic_auth: + username_file: non-existing-file + password: foobar +`) + + // basic auth via non-existing password file f(` basic_auth: username: user password_file: non-existing-file `) - // basic auth via non-existing username_file - f(` -basic_auth: - username_file: non-existing-file - password: pass -`) - // bearer token via non-existing file f(` bearer_token_file: non-existing-file @@ -479,25 +478,24 @@ basic_auth: password: password `, "Basic dXNlcjpwYXNzd29yZA==") - // basic auth via file + // basic auth via username file + f(` +basic_auth: + username_file: testdata/test_secretfile.txt +`, "Basic c2VjcmV0LWNvbnRlbnQ6") + + // basic auth via password file f(` basic_auth: username: user password_file: testdata/test_secretfile.txt `, "Basic dXNlcjpzZWNyZXQtY29udGVudA==") - // basic auth username via file + // basic auth via username file and password file f(` - basic_auth: - username_file: testdata/test_secretfile.txt - password: password - `, "Basic c2VjcmV0LWNvbnRlbnQ6cGFzc3dvcmQ=") - - // basic auth username and password via file - f(` - basic_auth: - username_file: testdata/test_secretfile.txt - password_file: testdata/test_secretfile.txt +basic_auth: + username_file: testdata/test_secretfile.txt + password_file: testdata/test_secretfile.txt `, "Basic c2VjcmV0LWNvbnRlbnQ6c2VjcmV0LWNvbnRlbnQ=") // inline authorization config