Surface unauthorized response count from OPA API authz handler

Currently when OPA's HTTP server rejects requests per
the authz policy, this is not accounted for via the management APIs.
This change adds that count in the metric registry that is
part of the Status API for more visibility.

Fixes: #3378

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>

docs/content/security.md

@@ -154,7 +154,8 @@ packages however it is recommended that administrators keep the policy under the
 
 If the document produced by the ``allow`` rule is ``true``, the request is
 processed normally. If the document is undefined or **not** ``true``, the
-request is rejected immediately.
+request is rejected immediately. The count of requests rejected by an OPA instance
+are surfaced via the performance metrics in the [Status](../management-status) information.
 
 OPA provides the following `input` document when executing the authorization
 policy:

server/server.go

@@ -95,6 +95,7 @@ const (
 	PromHandlerIndex      = "index"
 	PromHandlerCatch      = "catchall"
 	PromHandlerHealth     = "health"
+	PromHandlerAPIAuthz   = "authz"
 )
 
 const pqMaxCacheSize = 100
@@ -183,18 +184,16 @@ func (s *Server) Init(ctx context.Context) (*Server, error) {
 	s.partials = map[string]rego.PartialResult{}
 	s.preparedEvalQueries = newCache(pqMaxCacheSize)
 	s.defaultDecisionPath = s.generateDefaultDecisionPath()
-	s.interQueryBuiltinCache = iCache.NewInterQueryCache(s.manager.InterQueryBuiltinCacheConfig())
-	s.manager.RegisterCacheTrigger(s.updateCacheConfig)
 	s.manager.RegisterNDCacheTrigger(s.updateNDCache)
 
-	// authorizer, if configured, needs the iCache to be set up already
-	s.Handler = s.initHandlerAuth(s.Handler)
+	s.Handler = s.initHandlerAuthn(s.Handler)
+
 	// compression handler
 	s.Handler, err = s.initHandlerCompression(s.Handler)
 	if err != nil {
 		return nil, err
 	}
-	s.DiagnosticHandler = s.initHandlerAuth(s.DiagnosticHandler)
+	s.DiagnosticHandler = s.initHandlerAuthn(s.DiagnosticHandler)
 
 	return s, s.store.Commit(ctx, txn)
 }
@@ -641,9 +640,18 @@ func (s *Server) getListenerForUNIXSocket(u *url.URL, h http.Handler, t httpList
 	return domainSocketLoop, l, nil
 }
 
-func (s *Server) initHandlerAuth(handler http.Handler) http.Handler {
-	// Add authorization handler. This must come BEFORE authentication handler
-	// so that the latter can run first.
+func (s *Server) initHandlerAuthn(handler http.Handler) http.Handler {
+	switch s.authentication {
+	case AuthenticationToken:
+		handler = identifier.NewTokenBased(handler)
+	case AuthenticationTLS:
+		handler = identifier.NewTLSBased(handler)
+	}
+
+	return handler
+}
+
+func (s *Server) initHandlerAuthz(handler http.Handler) http.Handler {
 	switch s.authorization {
 	case AuthorizationBasic:
 		handler = authorizer.NewBasic(
@@ -655,13 +663,10 @@ func (s *Server) initHandlerAuth(handler http.Handler) http.Handler {
 			authorizer.PrintHook(s.manager.PrintHook()),
 			authorizer.EnablePrintStatements(s.manager.EnablePrintStatements()),
 			authorizer.InterQueryCache(s.interQueryBuiltinCache))
-	}
 
-	switch s.authentication {
-	case AuthenticationToken:
-		handler = identifier.NewTokenBased(handler)
-	case AuthenticationTLS:
-		handler = identifier.NewTLSBased(handler)
+		if s.metrics != nil {
+			handler = s.instrumentHandler(handler.ServeHTTP, PromHandlerAPIAuthz)
+		}
 	}
 
 	return handler
@@ -690,6 +695,16 @@ func (s *Server) initRouters() {
 
 	diagRouter := mux.NewRouter()
 
+	// authorizer, if configured, needs the iCache to be set up already
+	s.interQueryBuiltinCache = iCache.NewInterQueryCache(s.manager.InterQueryBuiltinCacheConfig())
+	s.manager.RegisterCacheTrigger(s.updateCacheConfig)
+
+	// Add authorization handler. This must come BEFORE authentication handler
+	// so that the latter can run first.
+	handlerAuthz := s.initHandlerAuthz(mainRouter)
+
+	handlerAuthzDiag := s.initHandlerAuthz(diagRouter)
+
 	// All routers get the same base configuration *and* diagnostic API's
 	for _, router := range []*mux.Router{mainRouter, diagRouter} {
 		router.StrictSlash(true)
@@ -771,6 +786,10 @@ func (s *Server) initRouters() {
 
 	s.Handler = mainRouter
 	s.DiagnosticHandler = diagRouter
+
+	// Add authorization handler in the end so that it can run first
+	s.Handler = handlerAuthz
+	s.DiagnosticHandler = handlerAuthzDiag
 }
 
 func (s *Server) instrumentHandler(handler func(http.ResponseWriter, *http.Request), label string) http.Handler {

server/server_test.go

@@ -25,6 +25,7 @@ import (
 	"time"
 
 	"github.com/gorilla/mux"
+	"github.com/open-policy-agent/opa/internal/prometheus"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 	"go.opentelemetry.io/otel/propagation"
 
@@ -3348,6 +3349,166 @@ func TestStatusV1(t *testing.T) {
 	}
 }
 
+func TestStatusV1MetricsWithSystemAuthzPolicy(t *testing.T) {
+
+	ctx := context.Background()
+
+	// Add the authz policy
+	store := inmem.New()
+	txn := storage.NewTransactionOrDie(ctx, store, storage.WriteParams)
+	authzPolicy := `package system.authz
+	default allow = false
+	allow {
+		input.path = ["v1", "status"]
+	}`
+
+	if err := store.UpsertPolicy(ctx, txn, "test", []byte(authzPolicy)); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := store.Commit(ctx, txn); err != nil {
+		t.Fatal(err)
+	}
+
+	// Add Prometheus Registerer to be used by plugins
+	inner := metrics.New()
+
+	logger := func(logger logging.Logger) func(attrs map[string]interface{}, f string, a ...interface{}) {
+		return func(attrs map[string]interface{}, f string, a ...interface{}) {
+			logger.WithFields(attrs).Error(f, a...)
+		}
+	}(logging.NewNoOpLogger())
+
+	prom := prometheus.New(inner, logger)
+	serverOpts := []func(s *Server){func(s *Server) { s.WithAuthorization(AuthorizationBasic) }, func(s *Server) { s.WithMetrics(prom) }}
+
+	f := newFixtureWithStore(t, store, serverOpts...)
+
+	// Expect HTTP 500 before status plugin is registered
+	req := newReqV1(http.MethodGet, "/status", "")
+	f.server.Handler.ServeHTTP(f.recorder, req)
+
+	if f.recorder.Result().StatusCode != http.StatusInternalServerError {
+		t.Fatal("expected internal error")
+	}
+
+	// Register Status plugin
+	manual := plugins.TriggerManual
+	bs := pluginStatus.New(&pluginStatus.Config{Trigger: &manual, Prometheus: true}, f.server.manager).WithMetrics(prom)
+	err := bs.Start(context.Background())
+	if err != nil {
+		t.Fatal(err)
+	}
+	f.server.manager.Register(pluginStatus.Name, bs)
+
+	// Fetch the status info
+	req = newReqV1(http.MethodGet, "/status", "")
+	f.reset()
+	f.server.Handler.ServeHTTP(f.recorder, req)
+	if f.recorder.Result().StatusCode != http.StatusOK {
+		t.Fatal("expected ok")
+	}
+
+	var resp1 struct {
+		Result struct {
+			Plugins struct {
+				Status struct {
+					State string
+				}
+			}
+		}
+	}
+	if err := util.NewJSONDecoder(f.recorder.Body).Decode(&resp1); err != nil {
+		t.Fatal(err)
+	} else if resp1.Result.Plugins.Status.State != "OK" {
+		t.Fatal("expected plugin state for status to be 'OK' but got:", resp1)
+	}
+
+	// Make requests that should get denied
+	req = newReqV1(http.MethodGet, "/policies", "")
+	f.reset()
+	f.server.Handler.ServeHTTP(f.recorder, req)
+
+	if f.recorder.Result().StatusCode != http.StatusUnauthorized {
+		t.Fatalf("Expected success but got %v", f.recorder)
+	}
+
+	req = newReqV1(http.MethodGet, "/data", "")
+	f.reset()
+	f.server.Handler.ServeHTTP(f.recorder, req)
+
+	if f.recorder.Result().StatusCode != http.StatusUnauthorized {
+		t.Fatalf("Expected success but got %v", f.recorder)
+	}
+
+	// Check Prometheus status metrics in the Status API
+
+	req = newReqV1(http.MethodGet, "/status", "")
+	f.reset()
+	f.server.Handler.ServeHTTP(f.recorder, req)
+
+	if f.recorder.Result().StatusCode != http.StatusOK {
+		t.Fatal("expected ok")
+	}
+
+	var resp struct {
+		Result struct {
+			Plugins struct {
+				Status struct {
+					State string
+				}
+			}
+			Metrics map[string]interface{}
+		}
+	}
+	if err := util.NewJSONDecoder(f.recorder.Body).Decode(&resp); err != nil {
+		t.Fatal(err)
+	} else if resp.Result.Plugins.Status.State != "OK" {
+		t.Fatal("expected plugin state for status to be 'OK' but got:", resp)
+	}
+
+	met, ok := resp.Result.Metrics["prometheus"]
+	if !ok {
+		t.Fatal("expected prometheus metrics to be present in status")
+	}
+
+	promMet, ok := met.(map[string]interface{})
+	if !ok {
+		t.Fatal("expected prometheus metrics to be a map")
+	}
+
+	httpMet, ok := promMet["http_request_duration_seconds"].(map[string]interface{})
+	if !ok {
+		t.Fatal("expected http_request_duration_seconds metric to be a map")
+	}
+
+	innerMet, ok := httpMet["metric"].([]interface{})
+	if !ok {
+		t.Fatal("expected http_request_duration_seconds histogram metric to be a list")
+	}
+
+	expected := []interface{}{map[string]interface{}{"name": "code", "value": "401"},
+		map[string]interface{}{"name": "handler", "value": "authz"},
+		map[string]interface{}{"name": "method", "value": "get"}}
+
+	found := false
+	for _, m := range innerMet {
+		item, ok := m.(map[string]interface{})
+		if ok {
+			if reflect.DeepEqual(item["label"].([]interface{}), expected) {
+				found = true
+				break
+			}
+		} else {
+			t.Fatal("expected each http_request_duration_seconds histogram metric element to be a map")
+		}
+	}
+
+	if !found {
+		t.Fatalf("expected to find metrics %v but found no match", expected)
+	}
+}
+
 func TestQueryPostBasic(t *testing.T) {
 	f := newFixture(t)
 	f.server, _ = New().

test/e2e/distributedtracing/distributedtracing_test.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/open-policy-agent/opa/server"
 	"github.com/open-policy-agent/opa/test/e2e"
 	"github.com/open-policy-agent/opa/tracing"
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
@@ -329,6 +330,80 @@ func TestClientSpan(t *testing.T) {
 	})
 }
 
+func TestServerSpanWithSystemAuthzPolicy(t *testing.T) {
+
+	// setup
+	spanExp := tracetest.NewInMemoryExporter()
+	options := tracing.NewOptions(
+		otelhttp.WithTracerProvider(trace.NewTracerProvider(trace.WithSpanProcessor(trace.NewSimpleSpanProcessor(spanExp)))),
+	)
+
+	authzPolicy := []byte(`package system.authz
+default allow = false
+allow {
+	input.path = ["health"]
+}`)
+
+	tmpfile, err := os.CreateTemp("", "authz.*.rego")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(tmpfile.Name())
+
+	if _, err := tmpfile.Write(authzPolicy); err != nil {
+		t.Fatal(err)
+	}
+	if err := tmpfile.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	testServerParams := e2e.NewAPIServerTestParams()
+	testServerParams.DistributedTracingOpts = options
+	testServerParams.Authorization = server.AuthorizationBasic
+	testServerParams.Paths = []string{"system.authz:" + tmpfile.Name()}
+
+	e2e.WithRuntime(t, e2e.TestRuntimeOpts{}, testServerParams, func(rt *e2e.TestRuntime) {
+
+		spanExp.Reset()
+
+		mr, err := http.Post(rt.URL()+"/v1/data", "application/json", nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer mr.Body.Close()
+
+		if mr.StatusCode != http.StatusUnauthorized {
+			t.Fatalf("expected status %v but got %v", http.StatusUnauthorized, mr.StatusCode)
+		}
+
+		spans := spanExp.GetSpans()
+		if got, expected := len(spans), 1; got != expected {
+			t.Fatalf("got %d span(s), expected %d", got, expected)
+		}
+		if !spans[0].SpanContext.IsValid() {
+			t.Fatalf("invalid span created: %#v", spans[0].SpanContext)
+		}
+
+		if got, expected := spans[0].SpanKind.String(), "server"; got != expected {
+			t.Fatalf("Expected span kind to be %q but got %q", expected, got)
+		}
+
+		expected := []attribute.KeyValue{
+			attribute.String("http.host", strings.Replace(rt.URL(), "http://", "", 1)),
+			attribute.String("http.method", "POST"),
+			attribute.String("http.scheme", "http"),
+			attribute.String("http.server_name", server.PromHandlerAPIAuthz),
+			attribute.Int("http.status_code", 401),
+			attribute.String("http.target", "/v1/data"),
+			attribute.String("http.user_agent", "Go-http-client/1.1"),
+			attribute.Int("http.wrote_bytes", 87),
+			attribute.String("net.transport", "ip_tcp"),
+		}
+		compareSpanAttributes(t, expected, attribute.NewSet(spans[0].Attributes...))
+
+	})
+}
+
 func compareSpanAttributes(t *testing.T, expectedAttributes []attribute.KeyValue, spanAttributes attribute.Set) {
 	t.Helper()
 	for _, exp := range expectedAttributes {