Consider only Authorization Bearer (#104)

- Key changes:
  - From now on, `Authorization` header will be considered only if it's of `Bearer` type:
    - In practical terms, it means that requests in a misconfigured setup will fail earlier than before;
    - There's no change for properly configured setups.
- Runtime and dependencies:
  - Go: `1.19.3` -> `1.19.5`.

CHANGELOG.md

@@ -1,5 +1,21 @@
 # CHANGELOG
 
+## 0.12.4
+
+- Key changes:
+  - From now on, `Authorization` header will be considered only if it's of `Bearer` type:
+    - In practical terms, it means that requests in a misconfigured setup will fail earlier than before;
+    - There's no change for properly configured setups.
+- Runtime and dependencies:
+  - Go: `1.18.2` -> `1.19.5`;
+  - Alpine: `3.15.4` -> `3.17.1`;
+  - `VictoriaMetrics/metrics`: `v1.18.1` -> `v1.23.0`;
+  - `VictoriaMetrics/metricsql` `v0.43.0` -> `v0.51.1`;
+  - `coreos/go-oidc/v3`: `v3.2.0` -> `v3.5.0`;
+  - `rs/zerolog v1.26.1`: -> `v1.28.0`;
+  - `stretchr/testify`: `v1.7.1` -> `v1.8.1`;
+  - `urfave/cli/v2`: `v2.6.0` -> `v2.23.7`.
+
 ## 0.12.3
 
 - Key changes:

Dockerfile

@@ -1,4 +1,4 @@
-FROM golang:1.19.3-alpine3.15 as builder
+FROM golang:1.19.5-alpine3.17 as builder
 
 ARG COMMIT
 ARG VERSION

README.md

@@ -36,13 +36,22 @@ Example of `keycloak + grafana + lfgw` setup is described [here](docs/oidc.md).
 
 ### Environment variables
 
+#### Basic settings
+
 | Variable                    | Default Value | Description                                                  |
 | --------------------------- | ------------- | ------------------------------------------------------------ |
 | `UPSTREAM_URL`              |               | Prometheus URL, e.g. `http://prometheus.localhost`.          |
 | `OIDC_REALM_URL`            |               | OIDC Realm URL, e.g. `https://keycloak.localhost/auth/realms/monitoring` |
 | `OIDC_CLIENT_ID`            |               | OIDC Client ID (1*)                                          |
 | `ACL_PATH`                  | `./acl.yaml`  | Path to a file with ACL definitions (OIDC role to namespace bindings). Skipped if `ACL_PATH` is empty (might be useful when autoconfiguration is enabled through `ASSUMED_ROLES=true`). |
 | `ASSUMED_ROLES`             | `false`       | In environments, where OIDC-role names match names of namespaces, ACLs can be constructed on the fly (e.g. `["role1", "role2"]` will give access to metrics from namespaces `role1` and `role2`). The roles specified in `acl.yaml` are still considered and get merged with assumed roles. Role names may contain regular expressions, including the admin definition `.*`. |
+
+(1*): since it's grafana who obtains jwt-tokens in the first place, the specified client id must also be present in the forwarded token (the `aud` claim).
+
+#### Additional settings
+
+| Variable                    | Default Value | Description                                                  |
+| --------------------------- | ------------- | ------------------------------------------------------------ |
 | `ENABLE_DEDUPLICATION`      | `true`        | Whether to enable deduplication, which leaves some of the requests unmodified if they match the target policy. Examples can be found in the "acl.yaml syntax" section. |
 | `OPTIMIZE_EXPRESSIONS`      | `true`        | Whether to automatically optimize expressions for non-full access requests. [More details](https://pkg.go.dev/github.com/VictoriaMetrics/metricsql#Optimize) |
 | `SAFE_MODE`                 | `true`        | Whether to block requests to sensitive endpoints like `/api/v1/admin/tsdb`, `/api/v1/insert`. |
@@ -57,9 +66,7 @@ Example of `keycloak + grafana + lfgw` setup is described [here](docs/oidc.md).
 | `WRITE_TIMEOUT`             | `10s`         | `WriteTimeout` normally covers the time from the end of the request header read to the end of the response write (a.k.a. the lifetime of the ServeHTTP). [More details](https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/) |
 | `GRACEFUL_SHUTDOWN_TIMEOUT` | `20s`         | Maximum amount of time to wait for all connections to be closed. [More details](https://pkg.go.dev/net/http#Server.Shutdown) |
 
-(1*): since it's grafana who obtains jwt-tokens in the first place, the specified client id must also be present in the forwarded token (the `aud` claim).
-
-### ACL
+### ACL syntax
 
 The file with ACL definitions (`./acl.yaml` by default) has a simple structure:
 
@@ -106,11 +113,3 @@ Note: a user is free to have multiple roles matching the contents of `acl.yaml`.
 ## Licensing
 
 lfgw code is licensed under MIT, though its dependencies might have other licenses. Please, inspect the modules listed in [go.mod](go.mod) if needed.
-
-## TODO
-
-* improve naming;
-* log slow requests;
-* metrics;
-* OIDC callback to support for proxying Prometheus web-interface itself;
-* add a helm chart.

internal/lfgw/helpers.go

@@ -35,6 +35,11 @@ func (app *application) getRawAccessToken(r *http.Request) (string, error) {
 		t := r.Header.Get(h)
 
 		if h == "Authorization" {
+			// Consider only Bearer tokens
+			if !strings.HasPrefix(t, "Bearer ") {
+				continue
+			}
+
 			t = strings.TrimPrefix(t, "Bearer ")
 		}
 

internal/lfgw/helpers_test.go

@@ -1,7 +1,6 @@
 package lfgw
 
 import (
-	"fmt"
 	"net/http"
 	"testing"
 
@@ -15,76 +14,83 @@ func TestGetRawAccessToken(t *testing.T) {
 		logger: &logger,
 	}
 
-	testsWithToken := []struct {
-		name   string
-		header string
-		want   string
+	userAgentGrafana := "Grafana/8.5.0"
+	userAgentCurl := "curl/7.81.0"
+
+	tests := []struct {
+		name        string
+		userAgent   string
+		header      string
+		headerValue string
+		want        string
+		wantErr     error
 	}{
 		{
-			name:   "X-Forwarded-Access-Token",
-			header: "X-Forwarded-Access-Token",
-			want:   "FAKE_TOKEN",
+			name:        "X-Forwarded-Access-Token",
+			userAgent:   userAgentGrafana,
+			header:      "X-Forwarded-Access-Token",
+			headerValue: "FAKE_TOKEN",
+			want:        "FAKE_TOKEN",
+			wantErr:     nil,
 		},
 		{
-			name:   "X-Auth-Request-Access-Token",
-			header: "X-Auth-Request-Access-Token",
-			want:   "FAKE_TOKEN",
+			name:        "X-Auth-Request-Access-Token",
+			userAgent:   userAgentGrafana,
+			header:      "X-Auth-Request-Access-Token",
+			headerValue: "FAKE_TOKEN",
+			want:        "FAKE_TOKEN",
+			wantErr:     nil,
 		},
 		{
-			name:   "Authorization",
-			header: "Authorization",
-			want:   "FAKE_TOKEN",
+			name:        "Authorization Bearer",
+			userAgent:   userAgentGrafana,
+			header:      "Authorization",
+			headerValue: "Bearer FAKE_TOKEN",
+			want:        "FAKE_TOKEN",
+			wantErr:     nil,
+		},
+		{
+			name:        "No token: Authorization Basic",
+			userAgent:   userAgentGrafana,
+			header:      "Authorization",
+			headerValue: "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==",
+			want:        "",
+			wantErr:     errNoTokenGrafana,
 		},
-	}
-
-	for _, tt := range testsWithToken {
-		t.Run(tt.name, func(t *testing.T) {
-			r, err := http.NewRequest(http.MethodGet, "/", nil)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			switch tt.header {
-			case "Authorization":
-				r.Header.Set(tt.header, fmt.Sprintf("Bearer %s", tt.want))
-			default:
-				r.Header.Set(tt.header, tt.want)
-			}
-
-			got, err := app.getRawAccessToken(r)
-			assert.Nil(t, err)
-			assert.Equal(t, tt.want, got)
-		})
-	}
-
-	testsNoToken := []struct {
-		name      string
-		userAgent string
-		want      error
-	}{
 		{
-			name:      "Request from Grafana",
-			userAgent: "Grafana/8.5.0",
-			want:      errNoTokenGrafana,
+			name:        "No token: request from grafana",
+			userAgent:   userAgentGrafana,
+			header:      "",
+			headerValue: "",
+			want:        "",
+			wantErr:     errNoTokenGrafana,
 		},
 		{
-			name:      "Request from another client",
-			userAgent: "curl/7.81.0",
-			want:      errNoToken,
+			name:        "No token: request from curl",
+			userAgent:   userAgentCurl,
+			header:      "",
+			headerValue: "",
+			want:        "",
+			wantErr:     errNoToken,
 		},
 	}
 
-	for _, tt := range testsNoToken {
+	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			r, err := http.NewRequest(http.MethodGet, "/", nil)
 			if err != nil {
 				t.Fatal(err)
 			}
 
+			if tt.header != "" && tt.headerValue != "" {
+				r.Header.Set(tt.header, tt.headerValue)
+			}
+
 			r.Header.Set("User-Agent", tt.userAgent)
+
 			got, err := app.getRawAccessToken(r)
-			assert.Empty(t, got)
-			assert.ErrorIs(t, err, tt.want)
+			assert.ErrorIs(t, err, tt.wantErr)
+			assert.Equal(t, tt.want, got)
 		})
 	}
 }