mirror of
https://github.com/project-zot/zot.git
synced 2026-06-17 21:17:58 +08:00
fix(security): enhance timeout configurations and body size limits fo… (#3984)
* fix(security): enhance timeout configurations and body size limits for HTTP requests Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(tests): refactor backend result handling in proxyHTTPRequest test Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): preserve ContentLength in proxied requests to prevent server hang Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): preserve explicit zero-length request bodies in proxyHTTPRequest fix(tests): add test for normalizedTimeout function to ensure default fallback Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): prevent default HTTP timeout values from being set unless explicitly configured Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): refactor timeout handling to use explicit checks for nil and non-positive values Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(tests): add wait_for_event_count function to ensure expected event generation Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): improve timeout handling and update error responses for large requests Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): enhance HTTP timeout handling with explicit accessors and default values Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): increase default API key body size and timeout values for improved performance Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): unify timeout handling by replacing specific read/write timeouts with a single default timeout Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): consolidate HTTP timeout accessors and enhance timeout handling Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): simplify HTTP timeout accessors and set default values for read/write timeouts Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> --------- Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> Co-authored-by: Copilot <copilot@github.com>
This commit is contained in:
Ramkumar Chinchani
committed by
GitHub
GitHub
parent
8282aef12b
commit
934b22d124
@@ -376,10 +376,18 @@ type RatelimitConfig struct {
|
||||
|
||||
//nolint:maligned
|
||||
type HTTPConfig struct {
|
||||
Address string
|
||||
ExternalURL string `mapstructure:",omitempty"`
|
||||
Port string
|
||||
AllowOrigin string // comma separated
|
||||
Address string
|
||||
ExternalURL string `mapstructure:",omitempty"`
|
||||
Port string
|
||||
AllowOrigin string // comma separated
|
||||
// ReadTimeout controls maximum duration for reading the entire request (including body).
|
||||
// When unset (nil), server-level defaults may apply. When explicitly set to <= 0,
|
||||
// the HTTP server treats it as no timeout.
|
||||
ReadTimeout *time.Duration `mapstructure:"readTimeout,omitempty"`
|
||||
// WriteTimeout controls maximum duration before timing out response writes.
|
||||
// When unset (nil), server-level defaults may apply. When explicitly set to <= 0,
|
||||
// the HTTP server treats it as no timeout.
|
||||
WriteTimeout *time.Duration `mapstructure:"writeTimeout,omitempty"`
|
||||
TLS *TLSConfig
|
||||
Auth *AuthConfig
|
||||
AccessControl *AccessControlConfig `mapstructure:"accessControl,omitempty"`
|
||||
@@ -661,8 +669,12 @@ func New() *Config {
|
||||
Retention: ImageRetention{},
|
||||
},
|
||||
},
|
||||
HTTP: HTTPConfig{Address: "127.0.0.1", Port: "8080", Auth: &AuthConfig{FailDelay: 0}},
|
||||
Log: &LogConfig{Level: "debug"},
|
||||
HTTP: HTTPConfig{
|
||||
Address: "127.0.0.1",
|
||||
Port: "8080",
|
||||
Auth: &AuthConfig{FailDelay: 0},
|
||||
},
|
||||
Log: &LogConfig{Level: "debug"},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1117,6 +1129,38 @@ func (c *Config) GetHTTPPort() string {
|
||||
return c.HTTP.Port
|
||||
}
|
||||
|
||||
// GetHTTPReadTimeout returns the configured HTTP server read timeout.
|
||||
func (c *Config) GetHTTPReadTimeout() time.Duration {
|
||||
if c == nil {
|
||||
return 0
|
||||
}
|
||||
|
||||
c.mu.RLock()
|
||||
defer c.mu.RUnlock()
|
||||
|
||||
if c.HTTP.ReadTimeout == nil {
|
||||
return 0
|
||||
}
|
||||
|
||||
return *c.HTTP.ReadTimeout
|
||||
}
|
||||
|
||||
// GetHTTPWriteTimeout returns the configured HTTP server write timeout.
|
||||
func (c *Config) GetHTTPWriteTimeout() time.Duration {
|
||||
if c == nil {
|
||||
return 0
|
||||
}
|
||||
|
||||
c.mu.RLock()
|
||||
defer c.mu.RUnlock()
|
||||
|
||||
if c.HTTP.WriteTimeout == nil {
|
||||
return 0
|
||||
}
|
||||
|
||||
return *c.HTTP.WriteTimeout
|
||||
}
|
||||
|
||||
// GetAllowOrigin returns the CORS allow origin configuration.
|
||||
func (c *Config) GetAllowOrigin() string {
|
||||
if c == nil {
|
||||
|
||||
@@ -3364,3 +3364,41 @@ func TestConfig(t *testing.T) {
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
func TestHTTPTimeoutAccessors(t *testing.T) {
|
||||
Convey("GetHTTPReadTimeout returns configured values", t, func() {
|
||||
cfg := config.New()
|
||||
|
||||
So(cfg.GetHTTPReadTimeout(), ShouldEqual, 0)
|
||||
|
||||
zero := time.Duration(0)
|
||||
cfg.HTTP.ReadTimeout = &zero
|
||||
So(cfg.GetHTTPReadTimeout(), ShouldEqual, 0)
|
||||
|
||||
negative := -5 * time.Second
|
||||
cfg.HTTP.ReadTimeout = &negative
|
||||
So(cfg.GetHTTPReadTimeout(), ShouldEqual, negative)
|
||||
|
||||
positive := 45 * time.Second
|
||||
cfg.HTTP.ReadTimeout = &positive
|
||||
So(cfg.GetHTTPReadTimeout(), ShouldEqual, positive)
|
||||
})
|
||||
|
||||
Convey("GetHTTPWriteTimeout returns configured values", t, func() {
|
||||
cfg := config.New()
|
||||
|
||||
So(cfg.GetHTTPWriteTimeout(), ShouldEqual, 0)
|
||||
|
||||
zero := time.Duration(0)
|
||||
cfg.HTTP.WriteTimeout = &zero
|
||||
So(cfg.GetHTTPWriteTimeout(), ShouldEqual, 0)
|
||||
|
||||
negative := -5 * time.Second
|
||||
cfg.HTTP.WriteTimeout = &negative
|
||||
So(cfg.GetHTTPWriteTimeout(), ShouldEqual, negative)
|
||||
|
||||
positive := 1 * time.Minute
|
||||
cfg.HTTP.WriteTimeout = &positive
|
||||
So(cfg.GetHTTPWriteTimeout(), ShouldEqual, positive)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -24,7 +24,9 @@ const (
|
||||
// OCI manifest JSON is always small metadata; 4 MiB is well above any realistic manifest.
|
||||
MaxManifestBodySize = 4 * 1024 * 1024
|
||||
// MaxAPIKeyBodySize is the maximum number of bytes accepted for an API-key creation request body.
|
||||
MaxAPIKeyBodySize = 4 * 1024
|
||||
MaxAPIKeyBodySize = 8 * 1024
|
||||
// MaxImageTrustBodySize is the maximum number of bytes accepted for image-trust key/certificate uploads.
|
||||
MaxImageTrustBodySize = 8 * 1024 * 1024
|
||||
BlobUploadUUID = "Blob-Upload-UUID"
|
||||
DefaultMediaType = "application/json"
|
||||
BinaryMediaType = "application/octet-stream"
|
||||
|
||||
@@ -172,9 +172,12 @@ func (c *Controller) Run() error {
|
||||
|
||||
port := c.Config.GetHTTPPort()
|
||||
addr := fmt.Sprintf("%s:%s", c.Config.GetHTTPAddress(), port)
|
||||
|
||||
server := &http.Server{
|
||||
Addr: addr,
|
||||
Handler: c.Router,
|
||||
ReadTimeout: c.Config.GetHTTPReadTimeout(),
|
||||
WriteTimeout: c.Config.GetHTTPWriteTimeout(),
|
||||
IdleTimeout: idleTimeout,
|
||||
ReadHeaderTimeout: readHeaderTimeout,
|
||||
}
|
||||
|
||||
+15
-36
@@ -1,7 +1,6 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"fmt"
|
||||
"io"
|
||||
@@ -131,15 +130,28 @@ func proxyHTTPRequest(ctx context.Context, req *http.Request,
|
||||
cloneURL.Scheme = proxyQueryScheme
|
||||
cloneURL.Host = targetMember
|
||||
|
||||
clonedBody := cloneRequestBody(req)
|
||||
requestBody := io.Reader(http.NoBody)
|
||||
if req.Body != nil {
|
||||
requestBody = req.Body
|
||||
}
|
||||
|
||||
fwdRequest, err := http.NewRequestWithContext(ctx, req.Method, cloneURL.String(), clonedBody)
|
||||
fwdRequest, err := http.NewRequestWithContext(ctx, req.Method, cloneURL.String(), requestBody)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
copyHeader(fwdRequest.Header, req.Header)
|
||||
|
||||
// Preserve ContentLength from original request, including explicit zero-length
|
||||
// bodies, so empty requests are not forwarded as unknown-length chunked bodies.
|
||||
if req.ContentLength >= 0 {
|
||||
fwdRequest.ContentLength = req.ContentLength
|
||||
|
||||
if req.ContentLength == 0 {
|
||||
fwdRequest.Body = http.NoBody
|
||||
}
|
||||
}
|
||||
|
||||
// always set hop count to 1 for now.
|
||||
// the handler wrapper above will terminate the process if it sees a request that
|
||||
// already has a hop count but is due for proxying.
|
||||
@@ -171,42 +183,9 @@ func proxyHTTPRequest(ctx context.Context, req *http.Request,
|
||||
return nil, err
|
||||
}
|
||||
|
||||
var clonedRespBody bytes.Buffer
|
||||
|
||||
// copy out the contents into a new buffer as the response body
|
||||
// stream should be closed to get all the data out.
|
||||
_, _ = io.Copy(&clonedRespBody, resp.Body)
|
||||
resp.Body.Close()
|
||||
|
||||
// after closing the original body, substitute it with a new reader
|
||||
// using the buffer that was just created.
|
||||
// this buffer should be closed later by the consumer of the response.
|
||||
resp.Body = io.NopCloser(bytes.NewReader(clonedRespBody.Bytes()))
|
||||
|
||||
return resp, nil
|
||||
}
|
||||
|
||||
func cloneRequestBody(src *http.Request) io.Reader {
|
||||
var bCloneForOriginal, bCloneForCopy bytes.Buffer
|
||||
multiWriter := io.MultiWriter(&bCloneForOriginal, &bCloneForCopy)
|
||||
numBytesCopied, _ := io.Copy(multiWriter, src.Body)
|
||||
|
||||
// if the body is a type of io.NopCloser and length is 0,
|
||||
// the Content-Length header is not sent in the proxied request.
|
||||
// explicitly returning http.NoBody allows the implementation
|
||||
// to set the header.
|
||||
// ref: https://github.com/golang/go/issues/34295
|
||||
if numBytesCopied == 0 {
|
||||
src.Body = http.NoBody
|
||||
|
||||
return http.NoBody
|
||||
}
|
||||
|
||||
src.Body = io.NopCloser(&bCloneForOriginal)
|
||||
|
||||
return bytes.NewReader(bCloneForCopy.Bytes())
|
||||
}
|
||||
|
||||
func copyHeader(dst, src http.Header) {
|
||||
for k, vv := range src {
|
||||
for _, v := range vv {
|
||||
|
||||
@@ -0,0 +1,113 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"context"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
. "github.com/smartystreets/goconvey/convey"
|
||||
|
||||
"zotregistry.dev/zot/v2/pkg/api/config"
|
||||
"zotregistry.dev/zot/v2/pkg/api/constants"
|
||||
)
|
||||
|
||||
func TestProxyHTTPRequestStreamsBodyAndResponse(t *testing.T) {
|
||||
Convey("proxyHTTPRequest forwards request body/headers and returns streamed response", t, func() {
|
||||
requestPayload := strings.Repeat("payload-", 1024)
|
||||
responsePayload := strings.Repeat("response-", 2048)
|
||||
|
||||
type backendResult struct {
|
||||
body string
|
||||
hopCount string
|
||||
err error
|
||||
}
|
||||
|
||||
resultCh := make(chan backendResult, 1)
|
||||
|
||||
backend := httptest.NewServer(http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
|
||||
body, err := io.ReadAll(request.Body)
|
||||
resultCh <- backendResult{
|
||||
body: string(body),
|
||||
hopCount: request.Header.Get(constants.ScaleOutHopCountHeader),
|
||||
err: err,
|
||||
}
|
||||
|
||||
response.WriteHeader(http.StatusCreated)
|
||||
_, _ = io.WriteString(response, responsePayload)
|
||||
}))
|
||||
defer backend.Close()
|
||||
|
||||
backendURL, err := url.Parse(backend.URL)
|
||||
So(err, ShouldBeNil)
|
||||
|
||||
conf := config.New()
|
||||
conf.Cluster = &config.ClusterConfig{Members: []string{backendURL.Host}, HashKey: "loremipsumdolors"}
|
||||
|
||||
ctrlr := &Controller{Config: conf}
|
||||
|
||||
req, err := http.NewRequestWithContext(context.Background(), http.MethodPut,
|
||||
"http://example.com/v2/repo/manifests/latest", strings.NewReader(requestPayload))
|
||||
So(err, ShouldBeNil)
|
||||
|
||||
resp, err := proxyHTTPRequest(context.Background(), req, backendURL.Host, ctrlr)
|
||||
So(err, ShouldBeNil)
|
||||
So(resp, ShouldNotBeNil)
|
||||
defer resp.Body.Close()
|
||||
|
||||
respBody, err := io.ReadAll(resp.Body)
|
||||
So(err, ShouldBeNil)
|
||||
|
||||
result := <-resultCh
|
||||
So(result.err, ShouldBeNil)
|
||||
|
||||
remainingReqBody, err := io.ReadAll(req.Body)
|
||||
So(err, ShouldBeNil)
|
||||
|
||||
So(resp.StatusCode, ShouldEqual, http.StatusCreated)
|
||||
So(string(respBody), ShouldEqual, responsePayload)
|
||||
So(result.body, ShouldEqual, requestPayload)
|
||||
So(result.hopCount, ShouldEqual, "1")
|
||||
So(len(remainingReqBody), ShouldEqual, 0)
|
||||
})
|
||||
}
|
||||
|
||||
func TestProxyHTTPRequestPreservesExplicitEmptyBody(t *testing.T) {
|
||||
Convey("proxyHTTPRequest preserves explicit zero-length request bodies", t, func() {
|
||||
resultCh := make(chan *http.Request, 1)
|
||||
|
||||
backend := httptest.NewServer(http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
|
||||
resultCh <- request
|
||||
response.WriteHeader(http.StatusNoContent)
|
||||
}))
|
||||
defer backend.Close()
|
||||
|
||||
backendURL, err := url.Parse(backend.URL)
|
||||
So(err, ShouldBeNil)
|
||||
|
||||
conf := config.New()
|
||||
conf.Cluster = &config.ClusterConfig{Members: []string{backendURL.Host}, HashKey: "loremipsumdolors"}
|
||||
|
||||
ctrlr := &Controller{Config: conf}
|
||||
|
||||
req, err := http.NewRequestWithContext(context.Background(), http.MethodPost,
|
||||
"http://example.com/v2/repo/manifests/latest", http.NoBody)
|
||||
So(err, ShouldBeNil)
|
||||
So(req.ContentLength, ShouldEqual, 0)
|
||||
|
||||
resp, err := proxyHTTPRequest(context.Background(), req, backendURL.Host, ctrlr)
|
||||
So(err, ShouldBeNil)
|
||||
So(resp, ShouldNotBeNil)
|
||||
defer resp.Body.Close()
|
||||
|
||||
backendReq := <-resultCh
|
||||
|
||||
So(resp.StatusCode, ShouldEqual, http.StatusNoContent)
|
||||
So(backendReq.ContentLength, ShouldEqual, 0)
|
||||
So(backendReq.Body, ShouldEqual, http.NoBody)
|
||||
So(backendReq.TransferEncoding, ShouldBeEmpty)
|
||||
})
|
||||
}
|
||||
Reference in New Issue
Block a user