fix(security): add HTTP timeout to native registry push client
Set a default 5-minute timeout on the HTTP client used for registry operations. Previously, a slow or malicious registry could stall push operations indefinitely. Configurable via WithTimeout option.
- commit
- ff97ba90941d701c5661e64493ee4824bef24540
- author
- brice · · 2026-03-14 22:16 UTC
- parent
- 658f480
- signature
- unsigned
2 files · +29 -2
M pkg/registrypush/push.go +11 -2
| @@ -13,6 +13,7 @@ import ( | |||
| 13 | 13 | "net/url" | |
| 14 | 14 | "os" | |
| 15 | 15 | "strings" | |
| 16 | + | "time" | |
| 16 | 17 | ||
| 17 | 18 | "github.com/docker/docker/client" | |
| 18 | 19 | "github.com/google/go-containerregistry/pkg/authn" | |
| @@ -25,6 +26,8 @@ import ( | |||
| 25 | 26 | // and Gordon's default max_blob_chunk_size of 95MB. | |
| 26 | 27 | const DefaultChunkSize = 50 * 1024 * 1024 | |
| 27 | 28 | ||
| 29 | + | const defaultHTTPTimeout = 5 * time.Minute | |
| 30 | + | ||
| 28 | 31 | // ImageSource provides a v1.Image for a given ref. | |
| 29 | 32 | // The default reads from the local Docker/Podman daemon. | |
| 30 | 33 | // Override with WithImageSource for testing. | |
| @@ -33,6 +36,7 @@ type ImageSource func(ctx context.Context, ref string) (v1.Image, error) | |||
| 33 | 36 | // Pusher uploads container images to a registry using chunked blob uploads. | |
| 34 | 37 | type Pusher struct { | |
| 35 | 38 | chunkSize int64 | |
| 39 | + | timeout time.Duration | |
| 36 | 40 | transport http.RoundTripper | |
| 37 | 41 | insecureTLS bool | |
| 38 | 42 | imageSource ImageSource | |
| @@ -53,6 +57,11 @@ func WithTransport(t http.RoundTripper) Option { | |||
| 53 | 57 | return func(p *Pusher) { p.transport = t } | |
| 54 | 58 | } | |
| 55 | 59 | ||
| 60 | + | // WithTimeout sets the HTTP client timeout for registry operations. | |
| 61 | + | func WithTimeout(d time.Duration) Option { | |
| 62 | + | return func(p *Pusher) { p.timeout = d } | |
| 63 | + | } | |
| 64 | + | ||
| 56 | 65 | // WithInsecureTLS disables TLS certificate verification for registry requests. | |
| 57 | 66 | // This is ignored when a custom transport is provided via WithTransport. | |
| 58 | 67 | func WithInsecureTLS(insecure bool) Option { | |
| @@ -77,7 +86,7 @@ func WithProgress(w io.Writer) Option { | |||
| 77 | 86 | ||
| 78 | 87 | // New creates a Pusher with the given options. | |
| 79 | 88 | func New(opts ...Option) *Pusher { | |
| 80 | - | p := &Pusher{chunkSize: DefaultChunkSize} | |
| 89 | + | p := &Pusher{chunkSize: DefaultChunkSize, timeout: defaultHTTPTimeout} | |
| 81 | 90 | for _, o := range opts { | |
| 82 | 91 | o(p) | |
| 83 | 92 | } | |
| @@ -502,7 +511,7 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, baseURL, uploadURL, dig | |||
| 502 | 511 | } | |
| 503 | 512 | ||
| 504 | 513 | func (p *Pusher) httpClient() *http.Client { | |
| 505 | - | return &http.Client{Transport: p.transport} | |
| 514 | + | return &http.Client{Transport: p.transport, Timeout: p.timeout} | |
| 506 | 515 | } | |
| 507 | 516 | ||
| 508 | 517 | // setScopedAuth attaches the auth header only if the request URL matches the | |
| @@ -13,6 +13,7 @@ import ( | |||||
| 13 | "net/url" | 13 | "net/url" | ||
| 14 | "os" | 14 | "os" | ||
| 15 | "strings" | 15 | "strings" | ||
| 16 | + | "time" | |||
| 16 | 17 | ||||
| 17 | "github.com/docker/docker/client" | 18 | "github.com/docker/docker/client" | ||
| 18 | "github.com/google/go-containerregistry/pkg/authn" | 19 | "github.com/google/go-containerregistry/pkg/authn" | ||
| @@ -25,6 +26,8 @@ import ( | |||||
| 25 | // and Gordon's default max_blob_chunk_size of 95MB. | 26 | // and Gordon's default max_blob_chunk_size of 95MB. | ||
| 26 | const DefaultChunkSize = 50 * 1024 * 1024 | 27 | const DefaultChunkSize = 50 * 1024 * 1024 | ||
| 27 | 28 | ||||
| 29 | + | const defaultHTTPTimeout = 5 * time.Minute | |||
| 30 | + | ||||
| 28 | // ImageSource provides a v1.Image for a given ref. | 31 | // ImageSource provides a v1.Image for a given ref. | ||
| 29 | // The default reads from the local Docker/Podman daemon. | 32 | // The default reads from the local Docker/Podman daemon. | ||
| 30 | // Override with WithImageSource for testing. | 33 | // Override with WithImageSource for testing. | ||
| @@ -33,6 +36,7 @@ type ImageSource func(ctx context.Context, ref string) (v1.Image, error) | |||||
| 33 | // Pusher uploads container images to a registry using chunked blob uploads. | 36 | // Pusher uploads container images to a registry using chunked blob uploads. | ||
| 34 | type Pusher struct { | 37 | type Pusher struct { | ||
| 35 | chunkSize int64 | 38 | chunkSize int64 | ||
| 39 | + | timeout time.Duration | |||
| 36 | transport http.RoundTripper | 40 | transport http.RoundTripper | ||
| 37 | insecureTLS bool | 41 | insecureTLS bool | ||
| 38 | imageSource ImageSource | 42 | imageSource ImageSource | ||
| @@ -53,6 +57,11 @@ func WithTransport(t http.RoundTripper) Option { | |||||
| 53 | return func(p *Pusher) { p.transport = t } | 57 | return func(p *Pusher) { p.transport = t } | ||
| 54 | } | 58 | } | ||
| 55 | 59 | ||||
| 60 | + | // WithTimeout sets the HTTP client timeout for registry operations. | |||
| 61 | + | func WithTimeout(d time.Duration) Option { | |||
| 62 | + | return func(p *Pusher) { p.timeout = d } | |||
| 63 | + | } | |||
| 64 | + | ||||
| 56 | // WithInsecureTLS disables TLS certificate verification for registry requests. | 65 | // WithInsecureTLS disables TLS certificate verification for registry requests. | ||
| 57 | // This is ignored when a custom transport is provided via WithTransport. | 66 | // This is ignored when a custom transport is provided via WithTransport. | ||
| 58 | func WithInsecureTLS(insecure bool) Option { | 67 | func WithInsecureTLS(insecure bool) Option { | ||
| @@ -77,7 +86,7 @@ func WithProgress(w io.Writer) Option { | |||||
| 77 | 86 | ||||
| 78 | // New creates a Pusher with the given options. | 87 | // New creates a Pusher with the given options. | ||
| 79 | func New(opts ...Option) *Pusher { | 88 | func New(opts ...Option) *Pusher { | ||
| 80 | - | p := &Pusher{chunkSize: DefaultChunkSize} | 89 | + | p := &Pusher{chunkSize: DefaultChunkSize, timeout: defaultHTTPTimeout} |
| 81 | for _, o := range opts { | 90 | for _, o := range opts { | ||
| 82 | o(p) | 91 | o(p) | ||
| 83 | } | 92 | } | ||
| @@ -502,7 +511,7 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, baseURL, uploadURL, dig | |||||
| 502 | } | 511 | } | ||
| 503 | 512 | ||||
| 504 | func (p *Pusher) httpClient() *http.Client { | 513 | func (p *Pusher) httpClient() *http.Client { | ||
| 505 | - | return &http.Client{Transport: p.transport} | 514 | + | return &http.Client{Transport: p.transport, Timeout: p.timeout} |
| 506 | } | 515 | } | ||
| 507 | 516 | ||||
| 508 | // setScopedAuth attaches the auth header only if the request URL matches the | 517 | // setScopedAuth attaches the auth header only if the request URL matches the | ||
M pkg/registrypush/push_test.go +18 -0
| @@ -12,6 +12,7 @@ import ( | |||
| 12 | 12 | "strconv" | |
| 13 | 13 | "strings" | |
| 14 | 14 | "testing" | |
| 15 | + | "time" | |
| 15 | 16 | ||
| 16 | 17 | "github.com/google/go-containerregistry/pkg/v1" | |
| 17 | 18 | "github.com/google/go-containerregistry/pkg/v1/random" | |
| @@ -281,6 +282,23 @@ func TestPusher_WithInsecureTLS_AllowsSelfSignedCertificate(t *testing.T) { | |||
| 281 | 282 | require.NoError(t, err) | |
| 282 | 283 | } | |
| 283 | 284 | ||
| 285 | + | func TestPusher_WithTimeout_RespectsDeadline(t *testing.T) { | |
| 286 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 287 | + | <-r.Context().Done() | |
| 288 | + | })) | |
| 289 | + | defer srv.Close() | |
| 290 | + | ||
| 291 | + | p := registrypush.New( | |
| 292 | + | registrypush.WithChunkSize(1024), | |
| 293 | + | registrypush.WithTimeout(100*time.Millisecond), | |
| 294 | + | ) | |
| 295 | + | ||
| 296 | + | ctx := context.Background() | |
| 297 | + | err := p.UploadBlob(ctx, srv.URL, "test/repo", "sha256:abc123", 10, strings.NewReader("0123456789"), "") | |
| 298 | + | ||
| 299 | + | require.Error(t, err) | |
| 300 | + | } | |
| 301 | + | ||
| 284 | 302 | func TestUploadBlob_RejectsCrossOriginRedirect(t *testing.T) { | |
| 285 | 303 | attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 286 | 304 | t.Errorf("request leaked to attacker: %s %s (Authorization: %s)", r.Method, r.URL.Path, r.Header.Get("Authorization")) | |
| @@ -12,6 +12,7 @@ import ( | |||||
| 12 | "strconv" | 12 | "strconv" | ||
| 13 | "strings" | 13 | "strings" | ||
| 14 | "testing" | 14 | "testing" | ||
| 15 | + | "time" | |||
| 15 | 16 | ||||
| 16 | "github.com/google/go-containerregistry/pkg/v1" | 17 | "github.com/google/go-containerregistry/pkg/v1" | ||
| 17 | "github.com/google/go-containerregistry/pkg/v1/random" | 18 | "github.com/google/go-containerregistry/pkg/v1/random" | ||
| @@ -281,6 +282,23 @@ func TestPusher_WithInsecureTLS_AllowsSelfSignedCertificate(t *testing.T) { | |||||
| 281 | require.NoError(t, err) | 282 | require.NoError(t, err) | ||
| 282 | } | 283 | } | ||
| 283 | 284 | ||||
| 285 | + | func TestPusher_WithTimeout_RespectsDeadline(t *testing.T) { | |||
| 286 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 287 | + | <-r.Context().Done() | |||
| 288 | + | })) | |||
| 289 | + | defer srv.Close() | |||
| 290 | + | ||||
| 291 | + | p := registrypush.New( | |||
| 292 | + | registrypush.WithChunkSize(1024), | |||
| 293 | + | registrypush.WithTimeout(100*time.Millisecond), | |||
| 294 | + | ) | |||
| 295 | + | ||||
| 296 | + | ctx := context.Background() | |||
| 297 | + | err := p.UploadBlob(ctx, srv.URL, "test/repo", "sha256:abc123", 10, strings.NewReader("0123456789"), "") | |||
| 298 | + | ||||
| 299 | + | require.Error(t, err) | |||
| 300 | + | } | |||
| 301 | + | ||||
| 284 | func TestUploadBlob_RejectsCrossOriginRedirect(t *testing.T) { | 302 | func TestUploadBlob_RejectsCrossOriginRedirect(t *testing.T) { | ||
| 285 | attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | 303 | attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 286 | t.Errorf("request leaked to attacker: %s %s (Authorization: %s)", r.Method, r.URL.Path, r.Header.Get("Authorization")) | 304 | t.Errorf("request leaked to attacker: %s %s (Authorization: %s)", r.Method, r.URL.Path, r.Header.Get("Authorization")) | ||