lib/promauth: follow-up for fca3b14b7b

- 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

@@ -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).

lib/promauth/config.go

@@ -92,8 +92,8 @@ type Authorization struct {
 
 // BasicAuthConfig represents basic auth config.
 type BasicAuthConfig struct {
-	Username     string  `yaml:"username"`
+	Username     string  `yaml:"username,omitempty"`
-	UsernameFile string  `yaml:"username_file"`
+	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 == "" {
+	username := ba.Username
-		return fmt.Errorf("missing `username` and `username_file` in `basic_auth` section. either one should be configured")
+	usernameFile := ba.UsernameFile
-	}
+	password := ""
-	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
-		}
-	}
 	if ba.Password != nil {
-		passwordDigest = fmt.Sprintf("password=%q", ba.Password.String())
+		password = ba.Password.S
-		getPassword = func() (string, error) {
-			return ba.Password.String(), nil
-		}
 	}
-	if ba.PasswordFile != "" {
+	passwordFile := ba.PasswordFile
-		passwordDigest = fmt.Sprintf("passwordFile=%q", ba.PasswordFile)
+	if username == "" && usernameFile == "" {
-		getPassword = func() (string, error) {
+		return fmt.Errorf("missing `username` and `username_file` in `basic_auth` section; please specify one; " +
-			password, err := fscore.ReadPasswordFromFileOrHTTP(fscore.GetFilepath(baseDir, ba.PasswordFile))
+			"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
 		}
-	}
+		passwordLocal := password
-	authHeader := func() (string, error) {
+		if passwordFile != "" {
-		username, err := getUsername()
+			s, err := fscore.ReadPasswordFromFileOrHTTP(passwordFile)
-		if err != nil {
+			if err != nil {
-			return "", err
+				return "", fmt.Errorf("cannot read password from `password_file`=%q: %w", passwordFile, err)
+			}
+			passwordLocal = s
 		}
-		password, err := getPassword()
+		// See https://en.wikipedia.org/wiki/Basic_access_authentication
-		if err != nil {
+		token := usernameLocal + ":" + passwordLocal
-			return "", err
+		token64 := base64.StdEncoding.EncodeToString([]byte(token))
-		}
-		token64 := generateBasicAuthEncodedToken(username, password)
 		return "Basic " + token64, nil
 	}
-	authHeaderDigest := fmt.Sprintf("basic(%q, %q)", usernameDigest, passwordDigest)
+	actx.authHeaderDigest = fmt.Sprintf("basic(username=%q, usernameFile=%q, password=%q, passwordFile=%q)", username, usernameFile, password, passwordFile)
-	return authHeader, authHeaderDigest
+	return nil
-}
-
-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
 }
 
 func (actx *authContext) mustInitFromBearerTokenFile(baseDir string, bearerTokenFile string) {

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:
+basic_auth:
-     username_file: testdata/test_secretfile.txt
+  username_file: testdata/test_secretfile.txt
-     password: password
+  password_file: testdata/test_secretfile.txt
-   `, "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 c2VjcmV0LWNvbnRlbnQ6c2VjcmV0LWNvbnRlbnQ=")
 
 	// inline authorization config