From 4e01537fe79f8a63c0b578d6f40e6a7554c2c02e Mon Sep 17 00:00:00 2001 From: Philip Conrad Date: Thu, 27 Jun 2024 03:55:19 -0400 Subject: [PATCH] server/authorizer: Fix gzip payload handling. (#6825) This PR fixes an issue where an OPA running authorization policies would be unable to handle gzipped request bodies. Example OPA CLI setup: opa run -s --authorization=basic Example request: echo -n '{}' | gzip | curl -H "Content-Encoding: gzip" --data-binary @- http://127.0.0.1:8181/v1/data This would result in unhelpful error messages, like: ```json { "code": "invalid_parameter", "message": "invalid character '\\x1f' looking for beginning of value" } ``` The cause was that the request body handling system in the `server/authorizer` package did not take gzipped payloads into account. The fix was to borrow the gzip request body handling function from `server/server.go`, to transparently decompress the body when needed. Fixes: #6804 Signed-off-by: Philip Conrad --- server/authorizer/authorizer.go | 7 +- server/server.go | 23 +----- server/server_test.go | 141 ++++++++++++++++++++++++++++++++ util/read_gzip_body.go | 26 ++++++ 4 files changed, 175 insertions(+), 22 deletions(-) create mode 100644 util/read_gzip_body.go diff --git a/server/authorizer/authorizer.go b/server/authorizer/authorizer.go index 1f706c56d5..10d65ecd88 100644 --- a/server/authorizer/authorizer.go +++ b/server/authorizer/authorizer.go @@ -151,7 +151,6 @@ func (h *Basic) ServeHTTP(w http.ResponseWriter, r *http.Request) { } func makeInput(r *http.Request) (*http.Request, interface{}, error) { - path, err := parsePath(r.URL.Path) if err != nil { return r, nil, err @@ -164,7 +163,11 @@ func makeInput(r *http.Request) (*http.Request, interface{}, error) { if expectBody(r.Method, path) { var err error - rawBody, err = io.ReadAll(r.Body) + plaintextBody, err := util.ReadMaybeCompressedBody(r) + if err != nil { + return r, nil, err + } + rawBody, err = io.ReadAll(plaintextBody) if err != nil { return r, nil, err } diff --git a/server/server.go b/server/server.go index 7cdf3ab0fd..398126fac7 100644 --- a/server/server.go +++ b/server/server.go @@ -6,7 +6,6 @@ package server import ( "bytes" - "compress/gzip" "context" "crypto/tls" "crypto/x509" @@ -1337,7 +1336,7 @@ func (s *Server) v1CompilePost(w http.ResponseWriter, r *http.Request) { m.Timer(metrics.RegoQueryParse).Start() // decompress the input if sent as zip - body, err := readPlainBody(r) + body, err := util.ReadMaybeCompressedBody(r) if err != nil { writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, "could not decompress the body")) return @@ -2732,7 +2731,7 @@ func readInputV0(r *http.Request) (ast.Value, *interface{}, error) { } // decompress the input if sent as zip - body, err := readPlainBody(r) + body, err := util.ReadMaybeCompressedBody(r) if err != nil { return nil, nil, fmt.Errorf("could not decompress the body: %w", err) } @@ -2785,7 +2784,7 @@ func readInputPostV1(r *http.Request) (ast.Value, *interface{}, error) { var request types.DataRequestV1 // decompress the input if sent as zip - body, err := readPlainBody(r) + body, err := util.ReadMaybeCompressedBody(r) if err != nil { return nil, nil, fmt.Errorf("could not decompress the body: %w", err) } @@ -3023,22 +3022,6 @@ func annotateSpan(ctx context.Context, decisionID string) { SetAttributes(attribute.String(otelDecisionIDAttr, decisionID)) } -func readPlainBody(r *http.Request) (io.ReadCloser, error) { - if strings.Contains(r.Header.Get("Content-Encoding"), "gzip") { - gzReader, err := gzip.NewReader(r.Body) - if err != nil { - return nil, err - } - bytesBody, err := io.ReadAll(gzReader) - if err != nil { - return nil, err - } - defer gzReader.Close() - return io.NopCloser(bytes.NewReader(bytesBody)), err - } - return r.Body, nil -} - func pretty(r *http.Request) bool { return getBoolParam(r.URL, types.ParamPrettyV1, true) } diff --git a/server/server_test.go b/server/server_test.go index ea0e0fab1a..7e640678b9 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -15,6 +15,7 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" + "encoding/binary" "encoding/json" "encoding/pem" "errors" @@ -1682,6 +1683,146 @@ func TestDataPutV1IfNoneMatch(t *testing.T) { } } +// Ensure JSON payload is compressed with gzip. +func mustGZIPPayload(payload []byte) []byte { + var compressedPayload bytes.Buffer + gz := gzip.NewWriter(&compressedPayload) + if _, err := gz.Write(payload); err != nil { + panic(fmt.Errorf("Error writing to gzip writer: %w", err)) + } + if err := gz.Close(); err != nil { + panic(fmt.Errorf("Error closing gzip writer: %w", err)) + } + return compressedPayload.Bytes() +} + +// generateJSONBenchmarkData returns a map of `k` keys and `v` key/value pairs. +// Taken from topdown/topdown_bench_test.go +func generateJSONBenchmarkData(k, v int) map[string]interface{} { + // create array of null values that can be iterated over + keys := make([]interface{}, k) + for i := range keys { + keys[i] = nil + } + + // create large JSON object value (100,000 entries is about 2MB on disk) + values := map[string]interface{}{} + for i := 0; i < v; i++ { + values[fmt.Sprintf("key%d", i)] = fmt.Sprintf("value%d", i) + } + + return map[string]interface{}{ + "keys": keys, + "values": values, + } +} + +// Ref: https://github.com/open-policy-agent/opa/issues/6804 +func TestDataGetV1CompressedRequestWithAuthorizer(t *testing.T) { + tests := []struct { + note string + payload []byte + forcePayloadSizeField uint32 // Size to manually set the payload field for the gzip blob. + expRespHTTPStatus int + expErrorMsg string + }{ + { + note: "empty message", + payload: mustGZIPPayload([]byte{}), + expRespHTTPStatus: 401, + }, + { + note: "empty object", + payload: mustGZIPPayload([]byte(`{}`)), + expRespHTTPStatus: 401, + }, + { + note: "basic authz - fail", + payload: mustGZIPPayload([]byte(`{"user": "bob"}`)), + expRespHTTPStatus: 401, + }, + { + note: "basic authz - pass", + payload: mustGZIPPayload([]byte(`{"user": "alice"}`)), + expRespHTTPStatus: 200, + }, + { + note: "basic authz - malicious size field", + payload: mustGZIPPayload([]byte(`{"user": "alice"}`)), + expRespHTTPStatus: 400, + forcePayloadSizeField: 134217728, // 128 MB + expErrorMsg: "gzip: invalid checksum", + }, + { + note: "basic authz - huge zip", + payload: mustGZIPPayload(util.MustMarshalJSON(generateJSONBenchmarkData(100, 100))), + expRespHTTPStatus: 401, + }, + } + + for _, test := range tests { + t.Run(test.note, func(t *testing.T) { + ctx := context.Background() + store := inmem.New() + txn := storage.NewTransactionOrDie(ctx, store, storage.WriteParams) + authzPolicy := `package system.authz + +import rego.v1 + +default allow := false # Reject requests by default. + +allow if { + # Logic to authorize request goes here. + input.body.user == "alice" +} +` + + if err := store.UpsertPolicy(ctx, txn, "test", []byte(authzPolicy)); err != nil { + panic(err) + } + + if err := store.Commit(ctx, txn); err != nil { + panic(err) + } + + opts := [](func(*Server)){ + func(s *Server) { + s.WithStore(store) + }, + func(s *Server) { + s.WithAuthorization(AuthorizationBasic) + }, + } + + f := newFixtureWithConfig(t, fmt.Sprintf(`{"server":{"decision_logs": %t}}`, true), opts...) + + // Forcibly replace the size trailer field for the gzip blob. + // Byte order is little-endian, field is a uint32. + if test.forcePayloadSizeField != 0 { + binary.LittleEndian.PutUint32(test.payload[len(test.payload)-4:], test.forcePayloadSizeField) + } + + // execute the request + req := newReqV1(http.MethodPost, "/data/test", string(test.payload)) + req.Header.Set("Content-Encoding", "gzip") + f.reset() + f.server.Handler.ServeHTTP(f.recorder, req) + if f.recorder.Code != test.expRespHTTPStatus { + t.Fatalf("Unexpected HTTP status code, (exp,got): %d, %d", test.expRespHTTPStatus, f.recorder.Code) + } + if test.expErrorMsg != "" { + var serverErr types.ErrorV1 + if err := json.Unmarshal(f.recorder.Body.Bytes(), &serverErr); err != nil { + t.Fatalf("Could not deserialize error message: %s", err.Error()) + } + if serverErr.Message != test.expErrorMsg { + t.Fatalf("Expected error message to have message '%s', got message: '%s'", test.expErrorMsg, serverErr.Message) + } + } + }) + } +} + func TestDataPostV0CompressedResponse(t *testing.T) { tests := []struct { gzipMinLength int diff --git a/util/read_gzip_body.go b/util/read_gzip_body.go new file mode 100644 index 0000000000..2e33cae5fa --- /dev/null +++ b/util/read_gzip_body.go @@ -0,0 +1,26 @@ +package util + +import ( + "bytes" + "compress/gzip" + "io" + "net/http" + "strings" +) + +// Note(philipc): Originally taken from server/server.go +func ReadMaybeCompressedBody(r *http.Request) (io.ReadCloser, error) { + if strings.Contains(r.Header.Get("Content-Encoding"), "gzip") { + gzReader, err := gzip.NewReader(r.Body) + if err != nil { + return nil, err + } + defer gzReader.Close() + bytesBody, err := io.ReadAll(gzReader) + if err != nil { + return nil, err + } + return io.NopCloser(bytes.NewReader(bytesBody)), err + } + return r.Body, nil +}