From 5e2fc11641fb2f685d473629f70076fce28314a4 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Tue, 24 Mar 2026 10:51:19 +1100 Subject: [PATCH] feat: replace OPA deny-reasons with boolean allow rule Switch from a deny set to a simple boolean allow rule for OPA authorization. This follows OPA's "default deny" pattern where everything not explicitly permitted is rejected. The default policy now allows GET/HEAD from any source and all methods from localhost, fixing readiness probes from non-localhost. Co-Authored-By: Claude Opus 4.6 (1M context) --- README.md | 11 ++-- internal/opa/opa.go | 54 +++++++------------ internal/opa/opa_test.go | 111 +++++++++------------------------------ 3 files changed, 48 insertions(+), 128 deletions(-) diff --git a/README.md b/README.md index d9c5a20..9768bdd 100644 --- a/README.md +++ b/README.md @@ -143,16 +143,16 @@ s3 { ## Authorization (OPA) -Cachew uses [Open Policy Agent](https://www.openpolicyagent.org/) for request authorization. The default policy allows all methods from `127.0.0.1` and `GET`/`HEAD` from elsewhere. +Cachew uses [Open Policy Agent](https://www.openpolicyagent.org/) for request authorization. The default policy allows GET/HEAD from any source and all methods from `127.0.0.1`. -Policies must be in `package cachew.authz` and define a `deny` rule set. If the set is empty, the request is allowed; otherwise the reasons are returned to the client. +Policies must be in `package cachew.authz` and define an `allow` rule. If `allow` is true the request proceeds; otherwise it is rejected with 403. ```hcl opa { policy = < 0 { - logger.Warn("OPA denied request", "method", r.Method, "path", r.URL.Path, "remote_addr", r.RemoteAddr, "reasons", reasons) - http.Error(w, "forbidden: "+strings.Join(reasons, "; "), http.StatusForbidden) + if !allowed { + logger.Warn("OPA denied request", "method", r.Method, "path", r.URL.Path, "remote_addr", r.RemoteAddr) + http.Error(w, "forbidden", http.StatusForbidden) return } @@ -96,33 +94,17 @@ func dataOptions(cfg Config) ([]func(*rego.Rego), error) { return []func(*rego.Rego){rego.Data(opaData)}, nil } -// evalDeny evaluates the prepared deny query and returns any denial reason strings. -// If the policy produces no deny reasons, nil is returned. -func evalDeny(ctx context.Context, prepared rego.PreparedEvalQuery, input map[string]any) ([]string, error) { +// evalAllow evaluates the prepared allow query and returns whether the request is permitted. +func evalAllow(ctx context.Context, prepared rego.PreparedEvalQuery, input map[string]any) (bool, error) { results, err := prepared.Eval(ctx, rego.EvalInput(input)) if err != nil { - return nil, errors.Errorf("evaluate deny query: %w", err) + return false, errors.Errorf("evaluate allow query: %w", err) } if len(results) == 0 || len(results[0].Expressions) == 0 { - return nil, nil - } - val := results[0].Expressions[0].Value - // OPA represents sets as []interface{} in the Go bindings. - set, isSet := val.([]any) - if !isSet { - return nil, nil - } - if len(set) == 0 { - return nil, nil - } - reasons := make([]string, 0, len(set)) - for _, v := range set { - if s, isString := v.(string); isString { - reasons = append(reasons, s) - } + return false, nil } - sort.Strings(reasons) // deterministic order for logging/testing - return reasons, nil + allowed, ok := results[0].Expressions[0].Value.(bool) + return ok && allowed, nil } func loadPolicy(cfg Config) (string, error) { diff --git a/internal/opa/opa_test.go b/internal/opa/opa_test.go index 5e24bbd..ddd2dec 100644 --- a/internal/opa/opa_test.go +++ b/internal/opa/opa_test.go @@ -23,13 +23,15 @@ func TestMiddlewareDefaultPolicy(t *testing.T) { tests := []struct { Name string Method string + RemoteAddr string ExpectedStatus int }{ - {"GETAllowed", http.MethodGet, http.StatusOK}, - {"HEADAllowed", http.MethodHead, http.StatusOK}, - {"POSTDenied", http.MethodPost, http.StatusForbidden}, - {"PUTDenied", http.MethodPut, http.StatusForbidden}, - {"DELETEDenied", http.MethodDelete, http.StatusForbidden}, + {"GETFromAnywhere", http.MethodGet, "10.0.0.1:9999", http.StatusOK}, + {"HEADFromAnywhere", http.MethodHead, "10.0.0.1:9999", http.StatusOK}, + {"POSTFromLocalhost", http.MethodPost, "127.0.0.1:12345", http.StatusOK}, + {"PUTFromLocalhost", http.MethodPut, "127.0.0.1:12345", http.StatusOK}, + {"POSTFromRemote", http.MethodPost, "10.0.0.1:9999", http.StatusForbidden}, + {"DELETEFromRemote", http.MethodDelete, "10.0.0.1:9999", http.StatusForbidden}, } next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) @@ -39,10 +41,7 @@ func TestMiddlewareDefaultPolicy(t *testing.T) { for _, test := range tests { t.Run(test.Name, func(t *testing.T) { r := newRequest(test.Method, "/some/path") - // Default policy requires localhost; httptest uses 192.0.2.1, so - // non-localhost requests that are also non-GET/HEAD get two reasons. - // Override RemoteAddr so we only test the method rule. - r.RemoteAddr = "127.0.0.1:12345" + r.RemoteAddr = test.RemoteAddr w := httptest.NewRecorder() handler.ServeHTTP(w, r) assert.Equal(t, test.ExpectedStatus, w.Code) @@ -50,22 +49,10 @@ func TestMiddlewareDefaultPolicy(t *testing.T) { } } -func TestMiddlewareDefaultPolicyDeniesNonLocalhost(t *testing.T) { - next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) - handler, err := opa.Middleware(t.Context(), opa.Config{}, next) - assert.NoError(t, err) - - r := newRequest(http.MethodGet, "/some/path") - r.RemoteAddr = "10.0.0.1:9999" - w := httptest.NewRecorder() - handler.ServeHTTP(w, r) - assert.Equal(t, http.StatusForbidden, w.Code) - assert.Contains(t, w.Body.String(), "remote address not allowed") -} - func TestMiddlewareInlinePolicy(t *testing.T) { policy := `package cachew.authz -deny contains "only POST allowed" if input.method != "POST" +default allow := false +allow if input.method == "POST" ` next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) handler, err := opa.Middleware(t.Context(), opa.Config{Policy: policy}, next) @@ -91,7 +78,8 @@ deny contains "only POST allowed" if input.method != "POST" func TestMiddlewarePolicyFile(t *testing.T) { policy := `package cachew.authz -deny contains "private path" if input.path[0] != "public" +default allow := false +allow if input.path[0] == "public" ` dir := t.TempDir() path := filepath.Join(dir, "policy.rego") @@ -121,10 +109,9 @@ deny contains "private path" if input.path[0] != "public" func TestMiddlewarePathBasedPolicy(t *testing.T) { policy := `package cachew.authz -deny contains "path not allowed" if { - not input.path[0] == "api" - not input.path[0] == "_liveness" -} +default allow := false +allow if input.path[0] == "api" +allow if input.path[0] == "_liveness" ` next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) handler, err := opa.Middleware(t.Context(), opa.Config{Policy: policy}, next) @@ -151,7 +138,8 @@ deny contains "path not allowed" if { func TestMiddlewareInlineData(t *testing.T) { policy := `package cachew.authz -deny contains "method not in allowed set" if not data.allowed_methods[input.method] +default allow := false +allow if data.allowed_methods[input.method] ` next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) handler, err := opa.Middleware(t.Context(), opa.Config{ @@ -193,7 +181,8 @@ func TestMiddlewareInlineDataInvalidJSON(t *testing.T) { func TestMiddlewareDataFile(t *testing.T) { policy := `package cachew.authz -deny contains "method not in allowed set" if not data.allowed_methods[input.method] +default allow := false +allow if data.allowed_methods[input.method] ` dataJSON := `{"allowed_methods": {"POST": true, "PUT": true}}` @@ -261,54 +250,20 @@ func TestMiddlewareInvalidPolicy(t *testing.T) { assert.Error(t, err) } -func TestMiddlewareDenyReasons(t *testing.T) { +func TestMiddlewareHeaderBasedPolicy(t *testing.T) { policy := `package cachew.authz -deny contains "writes are not allowed" if input.method == "PUT" -deny contains "deletes are not allowed" if input.method == "DELETE" +default allow := false +allow if input.headers["authorization"] ` next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) handler, err := opa.Middleware(t.Context(), opa.Config{Policy: policy}, next) assert.NoError(t, err) - tests := []struct { - Name string - Method string - ExpectedStatus int - ExpectedBody string - }{ - {"GETAllowed", http.MethodGet, http.StatusOK, ""}, - {"PUTDenied", http.MethodPut, http.StatusForbidden, "forbidden: writes are not allowed\n"}, - {"DELETEDenied", http.MethodDelete, http.StatusForbidden, "forbidden: deletes are not allowed\n"}, - } - for _, test := range tests { - t.Run(test.Name, func(t *testing.T) { - r := newRequest(test.Method, "/") - w := httptest.NewRecorder() - handler.ServeHTTP(w, r) - assert.Equal(t, test.ExpectedStatus, w.Code) - if test.ExpectedBody != "" { - assert.Equal(t, test.ExpectedBody, w.Body.String()) - } - }) - } -} - -func TestMiddlewareDenyUnauthenticated(t *testing.T) { - policy := `package cachew.authz -deny contains "unauthenticated" if not input.headers["authorization"] -` - next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) - handler, err := opa.Middleware(t.Context(), opa.Config{Policy: policy}, next) - assert.NoError(t, err) - - // Without Authorization header: denied. r := newRequest(http.MethodGet, "/") w := httptest.NewRecorder() handler.ServeHTTP(w, r) assert.Equal(t, http.StatusForbidden, w.Code) - assert.Equal(t, "forbidden: unauthenticated\n", w.Body.String()) - // With Authorization header: allowed. r = newRequest(http.MethodGet, "/") r.Header.Set("Authorization", "Bearer token") w = httptest.NewRecorder() @@ -316,25 +271,7 @@ deny contains "unauthenticated" if not input.headers["authorization"] assert.Equal(t, http.StatusOK, w.Code) } -func TestMiddlewareDenyMultipleReasons(t *testing.T) { - policy := `package cachew.authz -deny contains "reason-a" if input.method == "POST" -deny contains "reason-b" if input.method == "POST" -` - next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) - handler, err := opa.Middleware(t.Context(), opa.Config{Policy: policy}, next) - assert.NoError(t, err) - - r := newRequest(http.MethodPost, "/") - w := httptest.NewRecorder() - handler.ServeHTTP(w, r) - assert.Equal(t, http.StatusForbidden, w.Code) - // Reasons are sorted deterministically. - assert.Equal(t, "forbidden: reason-a; reason-b\n", w.Body.String()) -} - -func TestMiddlewareEmptyDenyAllowsAll(t *testing.T) { - // A policy with no deny rules allows everything. +func TestMiddlewareEmptyPolicyDeniesAll(t *testing.T) { policy := `package cachew.authz ` next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}) @@ -345,6 +282,6 @@ func TestMiddlewareEmptyDenyAllowsAll(t *testing.T) { r := newRequest(method, "/any/path") w := httptest.NewRecorder() handler.ServeHTTP(w, r) - assert.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, http.StatusForbidden, w.Code) } }