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 <[email protected]>
open-policy-agent · Jun 27, 2024 · 4e01537 · 4e01537
1 parent c2cede7
commit 4e01537
Show file tree

Hide file tree

Showing 4 changed files with 175 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
+}