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 all 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,6 +83,8 @@ type Options struct {
RedisPwdFile string
Registry *prometheus.Registry
BuildInfo BuildInfo
BasicAuthUsername string
BasicAuthPassword string
}

// NewRedisExporter returns a new exporter of Redis metrics.
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 @@ import (
)

func (e *Exporter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
if err := e.verifyBasicAuth(r.BasicAuth()); err != nil {
w.Header().Set("WWW-Authenticate", `Basic realm="redis-exporter, charset=UTF-8"`)
http.Error(w, err.Error(), http.StatusUnauthorized)
return
}

e.mux.ServeHTTP(w, r)
}

Expand Down Expand Up @@ -107,3 +115,27 @@ func (e *Exporter) reloadPwdFile(w http.ResponseWriter, r *http.Request) {
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
}
233 changes: 233 additions & 0 deletions exporter/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,239 @@ 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 TestBasicAuth(t *testing.T) {
if os.Getenv("TEST_REDIS_URI") == "" {
t.Skipf("TEST_REDIS_URI not set - skipping")
}

tests := []struct {
name string
username string
password string
configUsername string
configPassword string
wantStatusCode int
}{
{
name: "No auth configured - no credentials provided",
username: "",
password: "",
configUsername: "",
configPassword: "",
wantStatusCode: http.StatusOK,
},
{
name: "Auth configured - correct credentials",
username: "testuser",
password: "testpass",
configUsername: "testuser",
configPassword: "testpass",
wantStatusCode: http.StatusOK,
},
{
name: "Auth configured - wrong username",
username: "wronguser",
password: "testpass",
configUsername: "testuser",
configPassword: "testpass",
wantStatusCode: http.StatusUnauthorized,
},
{
name: "Auth configured - wrong password",
username: "testuser",
password: "wrongpass",
configUsername: "testuser",
configPassword: "testpass",
wantStatusCode: http.StatusUnauthorized,
},
{
name: "Auth configured - no credentials provided",
username: "",
password: "",
configUsername: "testuser",
configPassword: "testpass",
wantStatusCode: http.StatusUnauthorized,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
e, _ := NewRedisExporter(os.Getenv("TEST_REDIS_URI"), Options{
Namespace: "test",
Registry: prometheus.NewRegistry(),
BasicAuthUsername: tt.configUsername,
BasicAuthPassword: tt.configPassword,
})
ts := httptest.NewServer(e)
defer ts.Close()

client := &http.Client{}
req, err := http.NewRequest("GET", ts.URL+"/metrics", nil)
if err != nil {
t.Fatalf("Failed to create request: %v", err)
}

if tt.username != "" || tt.password != "" {
req.SetBasicAuth(tt.username, tt.password)
}

resp, err := client.Do(req)
if err != nil {
t.Fatalf("Failed to send request: %v", err)
}
defer resp.Body.Close()

if resp.StatusCode != tt.wantStatusCode {
t.Errorf("Expected status code %d, got %d", tt.wantStatusCode, resp.StatusCode)
}

body, err := io.ReadAll(resp.Body)
if err != nil {
t.Fatalf("Failed to read response body: %v", err)
}

if tt.wantStatusCode == http.StatusOK {
if !strings.Contains(string(body), "test_up") {
t.Errorf("Expected body to contain 'test_up', got: %s", string(body))
}
} else {
if !strings.Contains(resp.Header.Get("WWW-Authenticate"), "Basic realm=\"redis-exporter") {
t.Errorf("Expected WWW-Authenticate header, got: %s", resp.Header.Get("WWW-Authenticate"))
}
}
})
}
}

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