Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server+util: Limit max request sizes, prealloc request buffers #6868

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,29 @@ project adheres to [Semantic Versioning](http://semver.org/).

## Unreleased

### Request Body Size Limits

OPA now rejects requests with request bodies larger than a preset maximum size. To control this behavior, two new configuration keys are available: `server.decoding.max_length` and `server.decoding.gzip.max_length`. These control the max size in bytes to allow for an incoming request payload, and the maximum size in bytes to allow for a decompressed gzip request payload, respectively.

Here's an example OPA configuration using the new keys:

```yaml
# Set max request size to 64 MB and max gzip size (decompressed) to be 128 MB.
server:
decoding:
max_length: 67108864
gzip:
max_length: 134217728
```

These changes allow improvements in memory usage for the OPA HTTP server, and help OPA deployments avoid some accidental out-of-memory situations.

### Breaking Changes

OPA now automatically rejects very large requests. Requests with a `Content-Length` larger than 128 MB uncompressed, and gzipped requests with payloads that decompress to larger than 256 MB will be rejected, as part of hardening OPA against denial-of-service attacks. Previously, a large enough request could cause an OPA instance to run out of memory in low-memory sidecar deployment scenarios, just from attempting to read the request body into memory.

For most users, no changes will be needed to continue using OPA. However, for those who need to override the default limits, the new `server.decoding.max_length` and `server.decoding.gzip.max_length` configuration fields allow setting higher request size limits.

## 0.66.0

This release contains a mix of features, performance improvements, and bugfixes.
Expand Down
1 change: 1 addition & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type Config struct {
DistributedTracing json.RawMessage `json:"distributed_tracing,omitempty"`
Server *struct {
Encoding json.RawMessage `json:"encoding,omitempty"`
Decoding json.RawMessage `json:"decoding,omitempty"`
Metrics json.RawMessage `json:"metrics,omitempty"`
} `json:"server,omitempty"`
Storage *struct {
Expand Down
12 changes: 12 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,12 @@ func TestActiveConfig(t *testing.T) {
"some-plugin": {}
},
"server": {
"decoding": {
"max_length": 134217728,
"gzip": {
"max_length": 268435456
}
},
"encoding": {
"gzip": {
"min_length": 1024,
Expand Down Expand Up @@ -265,6 +271,12 @@ func TestActiveConfig(t *testing.T) {
"some-plugin": {}
},
"server": {
"decoding": {
"max_length": 134217728,
"gzip": {
"max_length": 268435456
}
},
"encoding": {
"gzip": {
"min_length": 1024,
Expand Down
21 changes: 15 additions & 6 deletions docs/content/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,13 @@ distributed_tracing:
encryption: "off"

server:
decoding:
max_length: 134217728
gzip:
max_length: 268435456
encoding:
gzip:
min_length: 1024,
min_length: 1024
compression_level: 9
```

Expand Down Expand Up @@ -886,14 +890,19 @@ See [the docs on disk storage](../storage/) for details about the settings.
## Server

The `server` configuration sets:
- the gzip compression settings for `/v0/data`, `/v1/data` and `/v1/compile` HTTP `POST` endpoints
- for all incoming requests:
- maximum allowed request size
- maximum decompressed gzip payload size
- the gzip compression settings for responses from the `/v0/data`, `/v1/data` and `/v1/compile` HTTP `POST` endpoints
The gzip compression settings are used when the client sends `Accept-Encoding: gzip`
- buckets for `http_request_duration_seconds` histogram

| Field | Type | Required | Description |
|-------------------------------------------------------------|-------------|---------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `server.encoding.gzip.min_length` | `int` | No, (default: 1024) | Specifies the minimum length of the response to compress |
| `server.encoding.gzip.compression_level` | `int` | No, (default: 9) | Specifies the compression level. Accepted values: a value of either 0 (no compression), 1 (best speed, lowest compression) or 9 (slowest, best compression). See https://pkg.go.dev/compress/flate#pkg-constants |
| Field | Type| Required | Description |
| --- | --- | --- | --- |
| `server.decoding.max_length` | `int` | No, (default: 268435456) | Specifies the maximum allowed number of bytes to read from a request body. |
| `server.decoding.gzip.max_length` | `int` | No, (default: 536870912) | Specifies the maximum allowed number of bytes to read from the gzip decompressor for gzip-encoded requests. |
philipaconrad marked this conversation as resolved.
Show resolved Hide resolved
| `server.encoding.gzip.min_length` | `int` | No, (default: 1024) | Specifies the minimum length of the response to compress. |
| `server.encoding.gzip.compression_level` | `int` | No, (default: 9) | Specifies the compression level. Accepted values: a value of either 0 (no compression), 1 (best speed, lowest compression) or 9 (slowest, best compression). See https://pkg.go.dev/compress/flate#pkg-constants |
| `server.metrics.prom.http_request_duration_seconds.buckets` | `[]float64` | No, (default: [1e-6, 5e-6, 1e-5, 5e-5, 1e-4, 5e-4, 1e-3, 0.01, 0.1, 1 ]) | Specifies the buckets for the `http_request_duration_seconds` metric. Each value is a float, it is expressed in seconds and subdivisions of it. E.g `1e-6` is 1 microsecond, `1e-3` 1 millisecond, `0.01` 10 milliseconds |

## Miscellaneous
Expand Down
102 changes: 102 additions & 0 deletions plugins/server/decoding/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// Package decoding implements the configuration side of the upgraded gzip
// decompression framework. The original work only enabled gzip decoding for
// a few endpoints-- here we enable if for all of OPA. Additionally, we provide
// some new defensive configuration options: max_length, and gzip.max_length.
// These allow rejecting requests that indicate their contents are larger than
// the size limits.
//
// The request handling pipeline now looks roughly like this:
//
// Request -> MaxBytesReader(Config.MaxLength) -> ir.CopyN(dest, req, Gzip.MaxLength)
//
// The intent behind this design is to improve how OPA handles large and/or
// malicious requests, compressed or otherwise. The benefit of being a little
// more strict in what we allow is that we can now use "riskier", but
// dramatically more performant techniques, like preallocating content buffers
// for gzipped data. This also should help OPAs in limited memory situations.
package decoding

import (
"fmt"

"github.com/open-policy-agent/opa/util"
)

var (
defaultMaxRequestLength = int64(268435456) // 256 MB
defaultGzipMaxContentLength = int64(536870912) // 512 MB
)

// Config represents the configuration for the Server.Decoding settings
type Config struct {
MaxLength *int64 `json:"max_length,omitempty"` // maximum request size that will be read, regardless of compression.
Gzip *Gzip `json:"gzip,omitempty"`
}

// Gzip represents the configuration for the Server.Decoding.Gzip settings
type Gzip struct {
MaxLength *int64 `json:"max_length,omitempty"` // Max number of bytes allowed to be read from the decompressor.
}

// ConfigBuilder assists in the construction of the plugin configuration.
type ConfigBuilder struct {
raw []byte
}

// NewConfigBuilder returns a new ConfigBuilder to build and parse the server config
func NewConfigBuilder() *ConfigBuilder {
return &ConfigBuilder{}
}

// WithBytes sets the raw server config
func (b *ConfigBuilder) WithBytes(config []byte) *ConfigBuilder {
b.raw = config
return b
}

// Parse returns a valid Config object with defaults injected.
func (b *ConfigBuilder) Parse() (*Config, error) {
if b.raw == nil {
defaultConfig := &Config{
MaxLength: &defaultMaxRequestLength,
Gzip: &Gzip{
MaxLength: &defaultGzipMaxContentLength,
},
}
return defaultConfig, nil
}

var result Config

if err := util.Unmarshal(b.raw, &result); err != nil {
return nil, err
}

return &result, result.validateAndInjectDefaults()
}

// validateAndInjectDefaults populates defaults if the fields are nil, then
// validates the config values.
func (c *Config) validateAndInjectDefaults() error {
if c.MaxLength == nil {
c.MaxLength = &defaultMaxRequestLength
}

if c.Gzip == nil {
c.Gzip = &Gzip{
MaxLength: &defaultGzipMaxContentLength,
}
}
if c.Gzip.MaxLength == nil {
c.Gzip.MaxLength = &defaultGzipMaxContentLength
}

if *c.MaxLength <= 0 {
return fmt.Errorf("invalid value for server.decoding.max_length field, should be a positive number")
}
if *c.Gzip.MaxLength <= 0 {
return fmt.Errorf("invalid value for server.decoding.gzip.max_length field, should be a positive number")
}

return nil
}
108 changes: 108 additions & 0 deletions plugins/server/decoding/config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package decoding

import (
"fmt"
"testing"
)

func TestConfigValidation(t *testing.T) {
tests := []struct {
input string
wantErr bool
}{
{
input: `{}`,
wantErr: false,
},
{
input: `{"gzip": {"max_length": "not-a-number"}}`,
wantErr: true,
},
{
input: `{"gzip": {max_length": 42}}`,
wantErr: false,
},
{
input: `{"gzip":{"max_length": "42"}}`,
wantErr: true,
},
{
input: `{"gzip":{"max_length": 0}}`,
wantErr: true,
},
{
input: `{"gzip":{"max_length": -10}}`,
wantErr: true,
},
{
input: `{"gzip":{"random_key": 0}}`,
wantErr: false,
},
{
input: `{"gzip": {"max_length": -10}}`,
wantErr: true,
},
{
input: `{"max_length": "not-a-number"}`,
wantErr: true,
},
{
input: `{"gzip":{}}`,
wantErr: false,
},
{
input: `{"max_length": "not-a-number", "gzip":{}}`,
wantErr: true,
},
{
input: `{"max_length": 42, "gzip":{"max_length": 42}}`,
wantErr: false,
},
}

for i, test := range tests {
t.Run(fmt.Sprintf("TestConfigValidation_case_%d", i), func(t *testing.T) {
_, err := NewConfigBuilder().WithBytes([]byte(test.input)).Parse()
if err != nil && !test.wantErr {
t.Fatalf("Unexpected error: %s", err.Error())
}
if err == nil && test.wantErr {
t.Fail()
}
})
}
}

func TestConfigValue(t *testing.T) {
tests := []struct {
input string
maxLengthExpectedValue int64
gzipMaxLengthExpectedValue int64
}{
{
input: `{}`,
maxLengthExpectedValue: 268435456,
gzipMaxLengthExpectedValue: 536870912,
},
{
input: `{"max_length": 5, "gzip":{"max_length": 42}}`,
maxLengthExpectedValue: 5,
gzipMaxLengthExpectedValue: 42,
},
}

for i, test := range tests {
t.Run(fmt.Sprintf("TestConfigValue_case_%d", i), func(t *testing.T) {
config, err := NewConfigBuilder().WithBytes([]byte(test.input)).Parse()
if err != nil {
t.Fatalf("Error building configuration: %s", err.Error())
}
if *config.MaxLength != test.maxLengthExpectedValue {
t.Fatalf("Unexpected config value for max_length (exp/actual): %d, %d", test.maxLengthExpectedValue, *config.MaxLength)
}
if *config.Gzip.MaxLength != test.gzipMaxLengthExpectedValue {
t.Fatalf("Unexpected config value for gzip.max_length (exp/actual): %d, %d", test.gzipMaxLengthExpectedValue, *config.Gzip.MaxLength)
}
})
}
}
7 changes: 1 addition & 6 deletions server/authorizer/authorizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package authorizer

import (
"context"
"io"
"net/http"
"net/url"
"strings"
Expand Down Expand Up @@ -163,11 +162,7 @@ func makeInput(r *http.Request) (*http.Request, interface{}, error) {

if expectBody(r.Method, path) {
var err error
plaintextBody, err := util.ReadMaybeCompressedBody(r)
if err != nil {
return r, nil, err
}
rawBody, err = io.ReadAll(plaintextBody)
rawBody, err = util.ReadMaybeCompressedBody(r)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be backwards-incompatible. It would be good to avoid this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we can define a new method.

Copy link
Contributor Author

@philipaconrad philipaconrad Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashutosh-narkar Could you elaborate on the backwards-incompatible bits?

As far as I can tell, the updated util.ReadMaybeCompressedBody method produces the same end result types as before, and only generates new errors if the gzip payload is larger than the configured size limit. 🤔

if err != nil {
return r, nil, err
}
Expand Down
40 changes: 40 additions & 0 deletions server/handlers/decoding.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package handlers

import (
"net/http"
"strings"

"github.com/open-policy-agent/opa/server/types"
"github.com/open-policy-agent/opa/server/writer"
util_decoding "github.com/open-policy-agent/opa/util/decoding"
)

// This handler provides hard limits on the size of the request body, for both
// the raw body content, and also for the decompressed size when gzip
// compression is used.
//
// The Content-Length restriction happens here in the handler, but the
// decompressed size limit is enforced later, in `util.ReadMaybeCompressedBody`.
// The handler passes the gzip size limits down to that function through the
// request context whenever gzip encoding is present.
func DecodingLimitsHandler(handler http.Handler, maxLength, gzipMaxLength int64) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Reject too-large requests before doing any further processing.
// Note(philipc): This likely does nothing in the case of "chunked"
// requests, since those should report a ContentLength of -1.
if r.ContentLength > maxLength {
writer.Error(w, http.StatusBadRequest, types.NewErrorV1(types.CodeInvalidParameter, types.MsgDecodingLimitError))
return
}
// Pass server.decoding.gzip.max_length down, using the request context.
if strings.Contains(r.Header.Get("Content-Encoding"), "gzip") {
ctx := util_decoding.AddServerDecodingGzipMaxLen(r.Context(), gzipMaxLength)
r = r.WithContext(ctx)
}

// Copied over from the net/http package; enforces max body read limits.
r2 := *r
r2.Body = http.MaxBytesReader(w, r.Body, maxLength)
handler.ServeHTTP(w, &r2)
})
}
Loading