From d40ff6f79b24647f6fa86a6790988ded39f345de Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Mon, 7 Dec 2020 15:59:31 -0500 Subject: [PATCH] server: Add message body to authorization policy input This commit updates the server's basic authorizer to include the deserialized message body in the input to the authorization policy so that the latter can make decisions based on policy query input documents. The authorizer caches the parsed message body on the request context and the server retrieves the value to avoid parsing twice. Signed-off-by: Torin Sandall --- docs/content/security.md | 26 +++++- server/authorizer/authorizer.go | 103 +++++++++++++++++++++- server/authorizer/authorizer_test.go | 123 ++++++++++++++++++++++++++- server/server.go | 18 ++++ server/server_test.go | 70 +++++++++++++++ 5 files changed, 334 insertions(+), 6 deletions(-) diff --git a/docs/content/security.md b/docs/content/security.md index aa8b3c1fa6..8c0c7e4c90 100644 --- a/docs/content/security.md +++ b/docs/content/security.md @@ -191,7 +191,31 @@ policy: # characters following a hyphen are uppercase. The rest are lowercase. # If the header key contains space or invalid header field bytes, # no conversion is performed. - "headers": {"...": [...]} + "headers": {"...": [...]}, + + # Request message body if present for applicable APIs. + # + # Example Request: + # + # POST v1/data HTTP/1.1 + # Content-Type: application/json + # + # {"input": {"action": "trade", "stock": "ACME"}} + # + # Example input.body Value: + # + # {"input": {"action": "trade", "stock": "ACME"}} + # + # Example body check: + # + # input.body.input.stock == "ACME" + # + # The 'body' field is provided for the following APIs: + # + # * POST v1/data + # * POST v0/data + # * POST / + "body": ..., } ``` diff --git a/server/authorizer/authorizer.go b/server/authorizer/authorizer.go index 7d0ae4f801..e47f6565d3 100644 --- a/server/authorizer/authorizer.go +++ b/server/authorizer/authorizer.go @@ -6,6 +6,8 @@ package authorizer import ( + "context" + "io/ioutil" "net/http" "net/url" "strings" @@ -16,6 +18,7 @@ import ( "github.com/open-policy-agent/opa/server/types" "github.com/open-policy-agent/opa/server/writer" "github.com/open-policy-agent/opa/storage" + "github.com/open-policy-agent/opa/util" ) // Basic provides policy-based authorization over incoming requests. @@ -59,7 +62,9 @@ func NewBasic(inner http.Handler, compiler func() *ast.Compiler, store storage.S func (h *Basic) ServeHTTP(w http.ResponseWriter, r *http.Request) { - input, err := makeInput(r) + // TODO(tsandall): Pass AST value as input instead of Go value to avoid unnecessary + // conversions. + r, input, err := makeInput(r) if err != nil { writer.ErrorString(w, http.StatusBadRequest, types.CodeInvalidParameter, err) return @@ -97,15 +102,25 @@ func (h *Basic) ServeHTTP(w http.ResponseWriter, r *http.Request) { writer.Error(w, http.StatusUnauthorized, types.NewErrorV1(types.CodeUnauthorized, types.MsgUnauthorizedError)) } -func makeInput(r *http.Request) (interface{}, error) { +func makeInput(r *http.Request) (*http.Request, interface{}, error) { + path, err := parsePath(r.URL.Path) if err != nil { - return nil, err + return r, nil, err } method := strings.ToUpper(r.Method) query := r.URL.Query() + var rawBody []byte + + if expectBody(r.Method, path) { + rawBody, err = readBody(r) + if err != nil { + return r, nil, err + } + } + input := map[string]interface{}{ "path": path, "method": method, @@ -113,12 +128,66 @@ func makeInput(r *http.Request) (interface{}, error) { "headers": r.Header, } + if len(rawBody) > 0 { + var body interface{} + if expectYAML(r) { + if err := util.Unmarshal(rawBody, &body); err != nil { + return r, nil, err + } + } else if err := util.UnmarshalJSON(rawBody, &body); err != nil { + return r, nil, err + } + + // We cache the parsed body on the context so the server does not have + // to parse the input document twice. + input["body"] = body + ctx := SetBodyOnContext(r.Context(), body) + r = r.WithContext(ctx) + } + identity, ok := identifier.Identity(r) if ok { input["identity"] = identity } - return input, nil + return r, input, nil +} + +var dataAPIVersions = map[string]bool{ + "v0": true, + "v1": true, +} + +func expectBody(method string, path []interface{}) bool { + if method == http.MethodPost { + if len(path) == 1 { + s := path[0].(string) + return s == "" + } else if len(path) >= 2 { + s1 := path[0].(string) + s2 := path[1].(string) + return dataAPIVersions[s1] && s2 == "data" + } + } + return false +} + +func expectYAML(r *http.Request) bool { + // NOTE(tsandall): This check comes from the server's HTTP handler code. The docs + // are a bit more strict, but the authorizer should be consistent w/ the original + // server handler implementation. + return strings.Contains(r.Header.Get("Content-Type"), "yaml") +} + +func readBody(r *http.Request) ([]byte, error) { + + bs, err := ioutil.ReadAll(r.Body) + + if err != nil { + return nil, err + } + + return bs, nil } func parsePath(path string) ([]interface{}, error) { @@ -139,3 +208,29 @@ func parsePath(path string) ([]interface{}, error) { } return sl, nil } + +type authorizerCachedBody struct { + parsed interface{} +} + +type authorizerCachedBodyKey string + +const ctxkey authorizerCachedBodyKey = "authorizerCachedBodyKey" + +// SetBodyOnContext adds the parsed input value to the context. This function is only +// exposed for test purposes. +func SetBodyOnContext(ctx context.Context, x interface{}) context.Context { + return context.WithValue(ctx, ctxkey, authorizerCachedBody{ + parsed: x, + }) +} + +// GetBodyOnContext returns the parsed input from the request context if it exists. +// The authorizer saves the parsed input on the context when it runs. +func GetBodyOnContext(ctx context.Context) (interface{}, bool) { + input, ok := ctx.Value(ctxkey).(authorizerCachedBody) + if !ok { + return nil, false + } + return input.parsed, true +} diff --git a/server/authorizer/authorizer_test.go b/server/authorizer/authorizer_test.go index b45cda4ad5..e33df72ed4 100644 --- a/server/authorizer/authorizer_test.go +++ b/server/authorizer/authorizer_test.go @@ -5,6 +5,7 @@ package authorizer import ( + "bytes" "encoding/json" "net/http" "reflect" @@ -251,7 +252,7 @@ func TestMakeInput(t *testing.T) { req = identifier.SetIdentity(req, "bob") - result, err := makeInput(req) + req, result, err := makeInput(req) if err != nil { panic(err) } @@ -275,3 +276,123 @@ func TestMakeInput(t *testing.T) { } } + +func TestMakeInputWithBody(t *testing.T) { + + reqs := []struct { + method string + path string + headers map[string]string + body string + useYAML bool + assertBodyExists bool + assertBodyDoesNotExist bool + }{ + { + method: "POST", + path: "/", + body: `{"foo": "bar"}`, + assertBodyExists: true, + }, + { + method: "POST", + path: "/", + body: `foo: bar`, + useYAML: true, + assertBodyExists: true, + }, + { + method: "POST", + path: "/v0/data", + body: `{"foo": "bar"}`, + assertBodyExists: true, + }, + { + method: "POST", + path: "/v1/data", + body: `{"foo": "bar"}`, + assertBodyExists: true, + }, + { + method: "PUT", + path: "/v1/data", + body: `{"foo": "bar"}`, + assertBodyDoesNotExist: true, + }, + { + method: "PATCH", + path: "/v1/data", + body: `{"foo": "bar"}`, + assertBodyDoesNotExist: true, + }, + { + method: "GET", + path: "/v1/data", + assertBodyDoesNotExist: true, + }, + { + method: "PUT", + path: "/v1/policies/test", + body: "package test\np = 7", + assertBodyDoesNotExist: true, + }, + } + + for _, tc := range reqs { + + t.Run(tc.method+"_"+tc.path, func(t *testing.T) { + + req, err := http.NewRequest(tc.method, "http://localhost:8181"+tc.path, bytes.NewBufferString(tc.body)) + if err != nil { + t.Fatal(err) + } + + if tc.useYAML { + req.Header.Set("Content-Type", "application/x-yaml") + } + + req, input, err := makeInput(req) + if err != nil { + t.Fatal(err) + } + + if tc.assertBodyExists { + + var want interface{} + + if tc.useYAML { + if err := util.Unmarshal([]byte(tc.body), &want); err != nil { + t.Fatal(err) + } + } else { + want = util.MustUnmarshalJSON([]byte(tc.body)) + } + + body := input.(map[string]interface{})["body"] + + if !reflect.DeepEqual(body, want) { + t.Fatalf("expected parsed bodies to be equal but got %v and want %v", body, want) + } + + body, ok := GetBodyOnContext(req.Context()) + if !ok || !reflect.DeepEqual(body, want) { + t.Fatalf("expected parsed body to be cached on context but got %v and want %v", body, want) + } + } + + if tc.assertBodyDoesNotExist { + _, ok := input.(map[string]interface{})["body"] + if ok { + t.Fatal("expected no parsed body in input") + } + _, ok = GetBodyOnContext(req.Context()) + if ok { + t.Fatal("expected no parsed body to be cached on context") + } + } + + }) + + } + +} diff --git a/server/server.go b/server/server.go index 4f1c459eb6..ca59d5e278 100644 --- a/server/server.go +++ b/server/server.go @@ -2313,14 +2313,22 @@ func getExplain(p []string, zero types.ExplainModeV1) types.ExplainModeV1 { } func readInputV0(r *http.Request) (ast.Value, error) { + + parsed, ok := authorizer.GetBodyOnContext(r.Context()) + if ok { + return ast.InterfaceToValue(parsed) + } + bs, err := ioutil.ReadAll(r.Body) if err != nil { return nil, err } + bs = bytes.TrimSpace(bs) if len(bs) == 0 { return nil, nil } + var x interface{} if strings.Contains(r.Header.Get("Content-Type"), "yaml") { @@ -2344,6 +2352,16 @@ func readInputGetV1(str string) (ast.Value, error) { func readInputPostV1(r *http.Request) (ast.Value, error) { + parsed, ok := authorizer.GetBodyOnContext(r.Context()) + if ok { + if obj, ok := parsed.(map[string]interface{}); ok { + if input, ok := obj["input"]; ok { + return ast.InterfaceToValue(input) + } + } + return nil, nil + } + bs, err := ioutil.ReadAll(r.Body) if err != nil { diff --git a/server/server_test.go b/server/server_test.go index 367b03a398..2c7e548a7b 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -5,6 +5,7 @@ package server import ( + "bytes" "context" "encoding/json" "fmt" @@ -23,6 +24,7 @@ import ( "github.com/open-policy-agent/opa/metrics" "github.com/open-policy-agent/opa/plugins" pluginBundle "github.com/open-policy-agent/opa/plugins/bundle" + "github.com/open-policy-agent/opa/server/authorizer" "github.com/open-policy-agent/opa/server/identifier" "github.com/open-policy-agent/opa/server/types" "github.com/open-policy-agent/opa/storage" @@ -2782,6 +2784,32 @@ func TestAuthorization(t *testing.T) { // Try bob again. server.Handler.ServeHTTP(recorder, req1) validateAuthorizedRequest(t, server, req1, http.StatusUnauthorized) + + // Try to query for "data" as alice (allowed) + req3, err := http.NewRequest(http.MethodPost, "http://localhost:8182/v1/data", bytes.NewBufferString(`{"input": {"foo": "bar"}}`)) + if err != nil { + panic(err) + } + + req3 = identifier.SetIdentity(req3, "alice") + recorder = httptest.NewRecorder() + server.Handler.ServeHTTP(recorder, req3) + if recorder.Code != http.StatusOK { + t.Fatal("expected successful response for data") + } + + // Try to query for "data" as bob (denied) + req4, err := http.NewRequest(http.MethodPost, "http://localhost:8182/v1/data", bytes.NewBufferString(`{"input": {"foo": "bar"}}`)) + if err != nil { + panic(err) + } + + req4 = identifier.SetIdentity(req4, "bob") + recorder = httptest.NewRecorder() + server.Handler.ServeHTTP(recorder, req4) + if recorder.Code != http.StatusUnauthorized { + t.Fatal("expected unauthorized response for data") + } } func validateAuthorizedRequest(t *testing.T, s *Server, req *http.Request, exp int) { @@ -2804,6 +2832,48 @@ func validateAuthorizedRequest(t *testing.T, s *Server, req *http.Request, exp i } } +func TestServerUsesAuthorizerParsedBody(t *testing.T) { + + // Construct a request w/ a different message body (this should never happen.) + req, err := http.NewRequest(http.MethodPost, "http://localhost:8182/v1/data/test/echo", bytes.NewBufferString(`{"foo": "bad"}`)) + if err != nil { + t.Fatal(err) + } + + // Set the authorizer's parsed input to the expected message body. + ctx := authorizer.SetBodyOnContext(req.Context(), map[string]interface{}{ + "input": map[string]interface{}{ + "foo": "good", + }, + }) + + // Check that v1 reader function behaves correctly. + inp, err := readInputPostV1(req.WithContext(ctx)) + if err != nil { + t.Fatal(err) + } + + exp := ast.MustParseTerm(`{"foo": "good"}`) + + if exp.Value.Compare(inp) != 0 { + t.Fatalf("expected %v but got %v", exp, inp) + } + + // Check that v0 reader function behaves correctly. + ctx = authorizer.SetBodyOnContext(req.Context(), map[string]interface{}{ + "foo": "good", + }) + + inp, err = readInputV0(req.WithContext(ctx)) + if err != nil { + t.Fatal(err) + } + + if exp.Value.Compare(inp) != 0 { + t.Fatalf("expected %v but got %v", exp, inp) + } +} + func TestServerReloadTrigger(t *testing.T) { f := newFixture(t) store := f.server.store