Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic authentication #967

Merged
merged 5 commits into from
Dec 15, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ Prometheus uses file watches and all changes to the json file are applied immedi
| check-key-groups | REDIS_EXPORTER_CHECK_KEY_GROUPS | Comma separated list of [LUA regexes](https://www.lua.org/pil/20.1.html) for classifying keys into groups. The regexes are applied in specified order to individual keys, and the group name is generated by concatenating all capture groups of the first regex that matches a key. A key will be tracked under the `unclassified` group if none of the specified regexes matches it. |
| max-distinct-key-groups | REDIS_EXPORTER_MAX_DISTINCT_KEY_GROUPS | Maximum number of distinct key groups that can be tracked independently *per Redis database*. If exceeded, only key groups with the highest memory consumption within the limit will be tracked separately, all remaining key groups will be tracked under a single `overflow` key group. |
| config-command | REDIS_EXPORTER_CONFIG_COMMAND | What to use for the CONFIG command, defaults to `CONFIG`, , set to "-" to skip config metrics extraction. |
| basic-auth-username | REDIS_EXPORTER_BASIC_AUTH_USERNAME | Username for Basic Authentication with the redis exporter needs to be set together with basic-auth-password to be effective
| basic-auth-password | REDIS_EXPORTER_BASIC_AUTH_PASSWORD | Password for Basic Authentication with the redis exporter needs to be set together with basic-auth-username to be effective

Redis instance addresses can be tcp addresses: `redis://localhost:6379`, `redis.example.com:6379` or e.g. unix sockets: `unix:///tmp/redis.sock`.\
SSL is supported by using the `rediss://` schema, for example: `rediss://azure-ssl-enabled-host.redis.cache.windows.net:6380` (note that the port is required when connecting to a non-standard 6379 port, e.g. with Azure Redis instances).\
Expand Down
2 changes: 2 additions & 0 deletions exporter/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,11 @@
RedisPwdFile string
Registry *prometheus.Registry
BuildInfo BuildInfo
BasicAuthUsername string
BasicAuthPassword string
}

// NewRedisExporter returns a new exporter of Redis metrics.

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Sensitive data returned by an access to Password
flows to a logging call.
Sensitive data returned by an access to PasswordMap
flows to a logging call.
Sensitive data returned by an access to passwordMap
flows to a logging call.
Sensitive data returned by an access to BasicAuthPassword
flows to a logging call.
func NewRedisExporter(uri string, opts Options) (*Exporter, error) {
log.Debugf("NewRedisExporter options: %#v", opts)

Expand Down
32 changes: 32 additions & 0 deletions exporter/http.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package exporter

import (
"crypto/subtle"
"errors"
"fmt"
"net/http"
"net/url"
Expand All @@ -12,6 +14,12 @@
)

func (e *Exporter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
err := e.verifyBasicAuth(r.BasicAuth())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can combine this with line 18 for tighter scoping
if err := verifyBasicAuth(...); err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did so in b0aabba

if err != nil {
w.Header().Set("WWW-Authenticate", `Basic realm="redis-exporter, charset=UTF-8"`)
return
}

Check warning on line 21 in exporter/http.go

View check run for this annotation

Codecov / codecov/patch

exporter/http.go#L19-L21

Added lines #L19 - L21 were not covered by tests

e.mux.ServeHTTP(w, r)
}

Expand Down Expand Up @@ -107,3 +115,27 @@
e.Unlock()
_, _ = w.Write([]byte(`ok`))
}

func (e *Exporter) isBasicAuthConfigured() bool {
return e.options.BasicAuthUsername != "" && e.options.BasicAuthPassword != ""
}

func (e *Exporter) verifyBasicAuth(user, password string, authHeaderSet bool) error {

if !e.isBasicAuthConfigured() {
return nil
}

if !authHeaderSet {
return errors.New("Unauthorized")
}

userCorrect := subtle.ConstantTimeCompare([]byte(user), []byte(e.options.BasicAuthUsername))
passCorrect := subtle.ConstantTimeCompare([]byte(password), []byte(e.options.BasicAuthPassword))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: looks like you're always using these as byte[] instead of string so might as well just move the conversion into the "setter" in main.go:206/207

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to leave this as it is for now, I hope that's fine.


if userCorrect == 0 || passCorrect == 0 {
return errors.New("Unauthorized")
}

return nil
}
129 changes: 129 additions & 0 deletions exporter/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,135 @@ func TestReloadHandlers(t *testing.T) {
}
}

func TestIsBasicAuthConfigured(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tests := []struct {
name string
username string
password string
want bool
}{
{
name: "no credentials configured",
username: "",
password: "",
want: false,
},
{
name: "only username configured",
username: "user",
password: "",
want: false,
},
{
name: "only password configured",
username: "",
password: "pass",
want: false,
},
{
name: "both credentials configured",
username: "user",
password: "pass",
want: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
e, _ := NewRedisExporter("", Options{
BasicAuthUsername: tt.username,
BasicAuthPassword: tt.password,
})

if got := e.isBasicAuthConfigured(); got != tt.want {
t.Errorf("isBasicAuthConfigured() = %v, want %v", got, tt.want)
}
})
}
}

func TestVerifyBasicAuth(t *testing.T) {
tests := []struct {
name string
configUser string
configPass string
providedUser string
providedPass string
authHeaderSet bool
wantErr bool
wantErrString string
}{
{
name: "no auth configured - no credentials provided",
configUser: "",
configPass: "",
providedUser: "",
providedPass: "",
authHeaderSet: false,
wantErr: false,
},
{
name: "auth configured - no auth header",
configUser: "user",
configPass: "pass",
providedUser: "",
providedPass: "",
authHeaderSet: false,
wantErr: true,
wantErrString: "Unauthorized",
},
{
name: "auth configured - correct credentials",
configUser: "user",
configPass: "pass",
providedUser: "user",
providedPass: "pass",
authHeaderSet: true,
wantErr: false,
},
{
name: "auth configured - wrong username",
configUser: "user",
configPass: "pass",
providedUser: "wronguser",
providedPass: "pass",
authHeaderSet: true,
wantErr: true,
wantErrString: "Unauthorized",
},
{
name: "auth configured - wrong password",
configUser: "user",
configPass: "pass",
providedUser: "user",
providedPass: "wrongpass",
authHeaderSet: true,
wantErr: true,
wantErrString: "Unauthorized",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
e, _ := NewRedisExporter("", Options{
BasicAuthUsername: tt.configUser,
BasicAuthPassword: tt.configPass,
})

err := e.verifyBasicAuth(tt.providedUser, tt.providedPass, tt.authHeaderSet)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with having a test that calls verifyBasicAuth() directly but I'd definitely would like to see tests that use a http client to make a real HTTP request (plenty of examples in this file).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

I've addressed the missing tests and added a proper status code for failed authentication in b0aabba


if (err != nil) != tt.wantErr {
t.Errorf("verifyBasicAuth() error = %v, wantErr %v", err, tt.wantErr)
return
}

if err != nil && err.Error() != tt.wantErrString {
t.Errorf("verifyBasicAuth() error = %v, wantErrString %v", err, tt.wantErrString)
}
})
}
}

func downloadURL(t *testing.T, u string) string {
_, res := downloadURLWithStatusCode(t, u)
return res
Expand Down
4 changes: 4 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@
redactConfigMetrics = flag.Bool("redact-config-metrics", getEnvBool("REDIS_EXPORTER_REDACT_CONFIG_METRICS", true), "Whether to redact config settings that include potentially sensitive information like passwords")
inclSystemMetrics = flag.Bool("include-system-metrics", getEnvBool("REDIS_EXPORTER_INCL_SYSTEM_METRICS", false), "Whether to include system metrics like e.g. redis_total_system_memory_bytes")
skipTLSVerification = flag.Bool("skip-tls-verification", getEnvBool("REDIS_EXPORTER_SKIP_TLS_VERIFICATION", false), "Whether to to skip TLS verification")
basicAuthUsername = flag.String("basic-auth-username", getEnv("REDIS_EXPORTER_BASIC_AUTH_USERNAME", ""), "Username for basic authentication")
basicAuthPassword = flag.String("basic-auth-password", getEnv("REDIS_EXPORTER_BASIC_AUTH_PASSWORD", ""), "Password for basic authentication")

Check warning on line 104 in main.go

View check run for this annotation

Codecov / codecov/patch

main.go#L103-L104

Added lines #L103 - L104 were not covered by tests
)
flag.Parse()

Expand Down Expand Up @@ -201,6 +203,8 @@
CommitSha: BuildCommitSha,
Date: BuildDate,
},
BasicAuthUsername: *basicAuthUsername,
BasicAuthPassword: *basicAuthPassword,

Check warning on line 207 in main.go

View check run for this annotation

Codecov / codecov/patch

main.go#L206-L207

Added lines #L206 - L207 were not covered by tests
},
)
if err != nil {
Expand Down
Loading