Merge pull request #112 from bnema/fix/security-hardening
fix(security): harden registry push, remote client, and config writes
6 files · +342 -45
M internal/adapters/in/cli/remote/client.go +5 -3
| @@ -33,6 +33,8 @@ var ( | |||
| 33 | 33 | retryBaseDelay = 250 * time.Millisecond | |
| 34 | 34 | ) | |
| 35 | 35 | ||
| 36 | + | const maxErrorBodySize int64 = 1024 // cap error response reads at 1 KB | |
| 37 | + | ||
| 36 | 38 | // ClientOption configures the Client. | |
| 37 | 39 | type ClientOption func(*Client) | |
| 38 | 40 | ||
| @@ -188,7 +190,7 @@ func parseResponse(resp *http.Response, target any) error { | |||
| 188 | 190 | defer resp.Body.Close() | |
| 189 | 191 | ||
| 190 | 192 | if resp.StatusCode >= 400 { | |
| 191 | - | body, _ := io.ReadAll(resp.Body) | |
| 193 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) | |
| 192 | 194 | return parseErrorResponse(resp, body) | |
| 193 | 195 | } | |
| 194 | 196 | ||
| @@ -245,7 +247,7 @@ func (c *Client) requestWithRetry(ctx context.Context, method, path string, body | |||
| 245 | 247 | return resp, nil | |
| 246 | 248 | } | |
| 247 | 249 | ||
| 248 | - | respBody, _ := io.ReadAll(resp.Body) | |
| 250 | + | respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) | |
| 249 | 251 | _ = resp.Body.Close() | |
| 250 | 252 | lastErr = parseErrorResponse(resp, respBody) | |
| 251 | 253 | if attempt == retryMaxAttempts { | |
| @@ -972,7 +974,7 @@ func (c *Client) streamLogs(ctx context.Context, path string) (<-chan string, er | |||
| 972 | 974 | c.observeTokenRotation(resp) | |
| 973 | 975 | ||
| 974 | 976 | if resp.StatusCode >= 400 { | |
| 975 | - | body, _ := io.ReadAll(resp.Body) | |
| 977 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) | |
| 976 | 978 | resp.Body.Close() | |
| 977 | 979 | return nil, fmt.Errorf("%s: %s", resp.Status, string(body)) | |
| 978 | 980 | } | |
| @@ -33,6 +33,8 @@ var ( | |||||
| 33 | retryBaseDelay = 250 * time.Millisecond | 33 | retryBaseDelay = 250 * time.Millisecond | ||
| 34 | ) | 34 | ) | ||
| 35 | 35 | ||||
| 36 | + | const maxErrorBodySize int64 = 1024 // cap error response reads at 1 KB | |||
| 37 | + | ||||
| 36 | // ClientOption configures the Client. | 38 | // ClientOption configures the Client. | ||
| 37 | type ClientOption func(*Client) | 39 | type ClientOption func(*Client) | ||
| 38 | 40 | ||||
| @@ -188,7 +190,7 @@ func parseResponse(resp *http.Response, target any) error { | |||||
| 188 | defer resp.Body.Close() | 190 | defer resp.Body.Close() | ||
| 189 | 191 | ||||
| 190 | if resp.StatusCode >= 400 { | 192 | if resp.StatusCode >= 400 { | ||
| 191 | - | body, _ := io.ReadAll(resp.Body) | 193 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) |
| 192 | return parseErrorResponse(resp, body) | 194 | return parseErrorResponse(resp, body) | ||
| 193 | } | 195 | } | ||
| 194 | 196 | ||||
| @@ -245,7 +247,7 @@ func (c *Client) requestWithRetry(ctx context.Context, method, path string, body | |||||
| 245 | return resp, nil | 247 | return resp, nil | ||
| 246 | } | 248 | } | ||
| 247 | 249 | ||||
| 248 | - | respBody, _ := io.ReadAll(resp.Body) | 250 | + | respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) |
| 249 | _ = resp.Body.Close() | 251 | _ = resp.Body.Close() | ||
| 250 | lastErr = parseErrorResponse(resp, respBody) | 252 | lastErr = parseErrorResponse(resp, respBody) | ||
| 251 | if attempt == retryMaxAttempts { | 253 | if attempt == retryMaxAttempts { | ||
| @@ -972,7 +974,7 @@ func (c *Client) streamLogs(ctx context.Context, path string) (<-chan string, er | |||||
| 972 | c.observeTokenRotation(resp) | 974 | c.observeTokenRotation(resp) | ||
| 973 | 975 | ||||
| 974 | if resp.StatusCode >= 400 { | 976 | if resp.StatusCode >= 400 { | ||
| 975 | - | body, _ := io.ReadAll(resp.Body) | 977 | + | body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) |
| 976 | resp.Body.Close() | 978 | resp.Body.Close() | ||
| 977 | return nil, fmt.Errorf("%s: %s", resp.Status, string(body)) | 979 | return nil, fmt.Errorf("%s: %s", resp.Status, string(body)) | ||
| 978 | } | 980 | } | ||
M internal/adapters/in/cli/remote/client_test.go +57 -0
| @@ -3,8 +3,10 @@ package remote | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "encoding/json" | |
| 6 | + | "io" | |
| 6 | 7 | "net/http" | |
| 7 | 8 | "net/http/httptest" | |
| 9 | + | "strings" | |
| 8 | 10 | "sync/atomic" | |
| 9 | 11 | "testing" | |
| 10 | 12 | "time" | |
| @@ -193,3 +195,58 @@ func TestClientFindAttachmentTargetsByImage_WithSlashContainingImage(t *testing. | |||
| 193 | 195 | require.NoError(t, err) | |
| 194 | 196 | assert.Equal(t, []string{"workers"}, targets) | |
| 195 | 197 | } | |
| 198 | + | ||
| 199 | + | func TestParseResponse_CapsErrorBodySize(t *testing.T) { | |
| 200 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |
| 201 | + | resp := &http.Response{ | |
| 202 | + | StatusCode: 500, | |
| 203 | + | Status: "500 Internal Server Error", | |
| 204 | + | Body: io.NopCloser(strings.NewReader(largeBody)), | |
| 205 | + | Header: make(http.Header), | |
| 206 | + | } | |
| 207 | + | ||
| 208 | + | err := parseResponse(resp, nil) | |
| 209 | + | ||
| 210 | + | require.Error(t, err) | |
| 211 | + | assert.Less(t, len(err.Error()), 2*1024, "error body should be capped to ~1KB") | |
| 212 | + | } | |
| 213 | + | ||
| 214 | + | func TestStreamLogs_CapsErrorBodySize(t *testing.T) { | |
| 215 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |
| 216 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 217 | + | w.WriteHeader(http.StatusInternalServerError) | |
| 218 | + | _, _ = w.Write([]byte(largeBody)) | |
| 219 | + | })) | |
| 220 | + | defer srv.Close() | |
| 221 | + | ||
| 222 | + | c := &Client{ | |
| 223 | + | baseURL: srv.URL, | |
| 224 | + | httpClient: srv.Client(), | |
| 225 | + | } | |
| 226 | + | ||
| 227 | + | _, err := c.StreamProcessLogs(context.Background(), 100) | |
| 228 | + | ||
| 229 | + | require.Error(t, err) | |
| 230 | + | assert.Less(t, len(err.Error()), 2*1024, "streaming error body should be capped to ~1KB") | |
| 231 | + | } | |
| 232 | + | ||
| 233 | + | func TestRequestWithRetry_CapsErrorBodySize(t *testing.T) { | |
| 234 | + | withFastRetry(t) | |
| 235 | + | ||
| 236 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |
| 237 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 238 | + | w.WriteHeader(http.StatusBadGateway) | |
| 239 | + | _, _ = w.Write([]byte(largeBody)) | |
| 240 | + | })) | |
| 241 | + | defer srv.Close() | |
| 242 | + | ||
| 243 | + | c := &Client{ | |
| 244 | + | baseURL: srv.URL, | |
| 245 | + | httpClient: srv.Client(), | |
| 246 | + | } | |
| 247 | + | ||
| 248 | + | _, err := c.requestWithRetry(context.Background(), http.MethodPost, "/test", nil) | |
| 249 | + | ||
| 250 | + | require.Error(t, err) | |
| 251 | + | assert.Less(t, len(err.Error()), 2*1024, "retry error body should be capped to ~1KB") | |
| 252 | + | } | |
| @@ -3,8 +3,10 @@ package remote | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "encoding/json" | 5 | "encoding/json" | ||
| 6 | + | "io" | |||
| 6 | "net/http" | 7 | "net/http" | ||
| 7 | "net/http/httptest" | 8 | "net/http/httptest" | ||
| 9 | + | "strings" | |||
| 8 | "sync/atomic" | 10 | "sync/atomic" | ||
| 9 | "testing" | 11 | "testing" | ||
| 10 | "time" | 12 | "time" | ||
| @@ -193,3 +195,58 @@ func TestClientFindAttachmentTargetsByImage_WithSlashContainingImage(t *testing. | |||||
| 193 | require.NoError(t, err) | 195 | require.NoError(t, err) | ||
| 194 | assert.Equal(t, []string{"workers"}, targets) | 196 | assert.Equal(t, []string{"workers"}, targets) | ||
| 195 | } | 197 | } | ||
| 198 | + | ||||
| 199 | + | func TestParseResponse_CapsErrorBodySize(t *testing.T) { | |||
| 200 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |||
| 201 | + | resp := &http.Response{ | |||
| 202 | + | StatusCode: 500, | |||
| 203 | + | Status: "500 Internal Server Error", | |||
| 204 | + | Body: io.NopCloser(strings.NewReader(largeBody)), | |||
| 205 | + | Header: make(http.Header), | |||
| 206 | + | } | |||
| 207 | + | ||||
| 208 | + | err := parseResponse(resp, nil) | |||
| 209 | + | ||||
| 210 | + | require.Error(t, err) | |||
| 211 | + | assert.Less(t, len(err.Error()), 2*1024, "error body should be capped to ~1KB") | |||
| 212 | + | } | |||
| 213 | + | ||||
| 214 | + | func TestStreamLogs_CapsErrorBodySize(t *testing.T) { | |||
| 215 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |||
| 216 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 217 | + | w.WriteHeader(http.StatusInternalServerError) | |||
| 218 | + | _, _ = w.Write([]byte(largeBody)) | |||
| 219 | + | })) | |||
| 220 | + | defer srv.Close() | |||
| 221 | + | ||||
| 222 | + | c := &Client{ | |||
| 223 | + | baseURL: srv.URL, | |||
| 224 | + | httpClient: srv.Client(), | |||
| 225 | + | } | |||
| 226 | + | ||||
| 227 | + | _, err := c.StreamProcessLogs(context.Background(), 100) | |||
| 228 | + | ||||
| 229 | + | require.Error(t, err) | |||
| 230 | + | assert.Less(t, len(err.Error()), 2*1024, "streaming error body should be capped to ~1KB") | |||
| 231 | + | } | |||
| 232 | + | ||||
| 233 | + | func TestRequestWithRetry_CapsErrorBodySize(t *testing.T) { | |||
| 234 | + | withFastRetry(t) | |||
| 235 | + | ||||
| 236 | + | largeBody := strings.Repeat("x", 10*1024*1024) | |||
| 237 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 238 | + | w.WriteHeader(http.StatusBadGateway) | |||
| 239 | + | _, _ = w.Write([]byte(largeBody)) | |||
| 240 | + | })) | |||
| 241 | + | defer srv.Close() | |||
| 242 | + | ||||
| 243 | + | c := &Client{ | |||
| 244 | + | baseURL: srv.URL, | |||
| 245 | + | httpClient: srv.Client(), | |||
| 246 | + | } | |||
| 247 | + | ||||
| 248 | + | _, err := c.requestWithRetry(context.Background(), http.MethodPost, "/test", nil) | |||
| 249 | + | ||||
| 250 | + | require.Error(t, err) | |||
| 251 | + | assert.Less(t, len(err.Error()), 2*1024, "retry error body should be capped to ~1KB") | |||
| 252 | + | } | |||
M internal/usecase/config/service.go +23 -6
| @@ -591,13 +591,30 @@ func (s *Service) writeConfigSurgical(configFile string) error { | |||
| 591 | 591 | return fmt.Errorf("failed to marshal config: %w", err) | |
| 592 | 592 | } | |
| 593 | 593 | ||
| 594 | - | tmpFile := configFile + ".tmp" | |
| 595 | - | if err := os.WriteFile(tmpFile, out, 0600); err != nil { | |
| 596 | - | return fmt.Errorf("failed to write temp config: %w", err) | |
| 594 | + | configDir := filepath.Dir(configFile) | |
| 595 | + | tmpFile, err := os.CreateTemp(configDir, ".gordon-config-*.tmp") | |
| 596 | + | if err != nil { | |
| 597 | + | return fmt.Errorf("failed to create temp config file: %w", err) | |
| 598 | + | } | |
| 599 | + | tmpPath := tmpFile.Name() | |
| 600 | + | ||
| 601 | + | if _, writeErr := tmpFile.Write(out); writeErr != nil { | |
| 602 | + | _ = tmpFile.Close() | |
| 603 | + | _ = os.Remove(tmpPath) | |
| 604 | + | return fmt.Errorf("failed to write temp config: %w", writeErr) | |
| 605 | + | } | |
| 606 | + | if chmodErr := tmpFile.Chmod(0600); chmodErr != nil { | |
| 607 | + | _ = tmpFile.Close() | |
| 608 | + | _ = os.Remove(tmpPath) | |
| 609 | + | return fmt.Errorf("failed to set temp config permissions: %w", chmodErr) | |
| 610 | + | } | |
| 611 | + | if closeErr := tmpFile.Close(); closeErr != nil { | |
| 612 | + | _ = os.Remove(tmpPath) | |
| 613 | + | return fmt.Errorf("failed to close temp config: %w", closeErr) | |
| 597 | 614 | } | |
| 598 | - | if err := os.Rename(tmpFile, configFile); err != nil { | |
| 599 | - | _ = os.Remove(tmpFile) | |
| 600 | - | return fmt.Errorf("failed to rename temp config: %w", err) | |
| 615 | + | if renameErr := os.Rename(tmpPath, configFile); renameErr != nil { | |
| 616 | + | _ = os.Remove(tmpPath) | |
| 617 | + | return fmt.Errorf("failed to rename temp config: %w", renameErr) | |
| 601 | 618 | } | |
| 602 | 619 | ||
| 603 | 620 | return nil | |
| @@ -591,13 +591,30 @@ func (s *Service) writeConfigSurgical(configFile string) error { | |||||
| 591 | return fmt.Errorf("failed to marshal config: %w", err) | 591 | return fmt.Errorf("failed to marshal config: %w", err) | ||
| 592 | } | 592 | } | ||
| 593 | 593 | ||||
| 594 | - | tmpFile := configFile + ".tmp" | 594 | + | configDir := filepath.Dir(configFile) |
| 595 | - | if err := os.WriteFile(tmpFile, out, 0600); err != nil { | 595 | + | tmpFile, err := os.CreateTemp(configDir, ".gordon-config-*.tmp") |
| 596 | - | return fmt.Errorf("failed to write temp config: %w", err) | 596 | + | if err != nil { |
| 597 | + | return fmt.Errorf("failed to create temp config file: %w", err) | |||
| 598 | + | } | |||
| 599 | + | tmpPath := tmpFile.Name() | |||
| 600 | + | ||||
| 601 | + | if _, writeErr := tmpFile.Write(out); writeErr != nil { | |||
| 602 | + | _ = tmpFile.Close() | |||
| 603 | + | _ = os.Remove(tmpPath) | |||
| 604 | + | return fmt.Errorf("failed to write temp config: %w", writeErr) | |||
| 605 | + | } | |||
| 606 | + | if chmodErr := tmpFile.Chmod(0600); chmodErr != nil { | |||
| 607 | + | _ = tmpFile.Close() | |||
| 608 | + | _ = os.Remove(tmpPath) | |||
| 609 | + | return fmt.Errorf("failed to set temp config permissions: %w", chmodErr) | |||
| 610 | + | } | |||
| 611 | + | if closeErr := tmpFile.Close(); closeErr != nil { | |||
| 612 | + | _ = os.Remove(tmpPath) | |||
| 613 | + | return fmt.Errorf("failed to close temp config: %w", closeErr) | |||
| 597 | } | 614 | } | ||
| 598 | - | if err := os.Rename(tmpFile, configFile); err != nil { | 615 | + | if renameErr := os.Rename(tmpPath, configFile); renameErr != nil { |
| 599 | - | _ = os.Remove(tmpFile) | 616 | + | _ = os.Remove(tmpPath) |
| 600 | - | return fmt.Errorf("failed to rename temp config: %w", err) | 617 | + | return fmt.Errorf("failed to rename temp config: %w", renameErr) |
| 601 | } | 618 | } | ||
| 602 | 619 | ||||
| 603 | return nil | 620 | return nil | ||
M internal/usecase/config/service_test.go +33 -0
| @@ -1236,6 +1236,39 @@ func TestService_FindAttachmentTargetsByImage(t *testing.T) { | |||
| 1236 | 1236 | } | |
| 1237 | 1237 | ||
| 1238 | 1238 | func TestSavePreservesAllConfigFields(t *testing.T) { | |
| 1239 | + | t.Run("does not write predictable tmp path", func(t *testing.T) { | |
| 1240 | + | dir := t.TempDir() | |
| 1241 | + | configFile := filepath.Join(dir, "gordon.toml") | |
| 1242 | + | err := os.WriteFile(configFile, []byte("[server]\nport = 8080\n\n[routes]\n\"app.example.com\" = \"myapp:latest\"\n"), 0600) | |
| 1243 | + | require.NoError(t, err) | |
| 1244 | + | ||
| 1245 | + | v := viper.New() | |
| 1246 | + | v.SetConfigFile(configFile) | |
| 1247 | + | err = v.ReadInConfig() | |
| 1248 | + | require.NoError(t, err) | |
| 1249 | + | ||
| 1250 | + | eventBus := mocks.NewMockEventPublisher(t) | |
| 1251 | + | svc := NewService(v, eventBus) | |
| 1252 | + | ctx := testContext() | |
| 1253 | + | err = svc.Load(ctx) | |
| 1254 | + | require.NoError(t, err) | |
| 1255 | + | ||
| 1256 | + | maliciousTarget := filepath.Join(dir, "pwned.txt") | |
| 1257 | + | predictableTmp := configFile + ".tmp" | |
| 1258 | + | err = os.Symlink(maliciousTarget, predictableTmp) | |
| 1259 | + | require.NoError(t, err) | |
| 1260 | + | ||
| 1261 | + | err = svc.Save(ctx) | |
| 1262 | + | require.NoError(t, err) | |
| 1263 | + | ||
| 1264 | + | _, statErr := os.Stat(maliciousTarget) | |
| 1265 | + | assert.True(t, os.IsNotExist(statErr), "symlink target should not exist - temp file should use unpredictable name") | |
| 1266 | + | ||
| 1267 | + | info, err := os.Lstat(predictableTmp) | |
| 1268 | + | require.NoError(t, err) | |
| 1269 | + | assert.NotZero(t, info.Mode()&os.ModeSymlink, "predictable .tmp path should still be the original symlink") | |
| 1270 | + | }) | |
| 1271 | + | ||
| 1239 | 1272 | t.Run("with defaults does not corrupt config", func(t *testing.T) { | |
| 1240 | 1273 | tmpDir := t.TempDir() | |
| 1241 | 1274 | configFile := filepath.Join(tmpDir, "gordon.toml") | |
| @@ -1236,6 +1236,39 @@ func TestService_FindAttachmentTargetsByImage(t *testing.T) { | |||||
| 1236 | } | 1236 | } | ||
| 1237 | 1237 | ||||
| 1238 | func TestSavePreservesAllConfigFields(t *testing.T) { | 1238 | func TestSavePreservesAllConfigFields(t *testing.T) { | ||
| 1239 | + | t.Run("does not write predictable tmp path", func(t *testing.T) { | |||
| 1240 | + | dir := t.TempDir() | |||
| 1241 | + | configFile := filepath.Join(dir, "gordon.toml") | |||
| 1242 | + | err := os.WriteFile(configFile, []byte("[server]\nport = 8080\n\n[routes]\n\"app.example.com\" = \"myapp:latest\"\n"), 0600) | |||
| 1243 | + | require.NoError(t, err) | |||
| 1244 | + | ||||
| 1245 | + | v := viper.New() | |||
| 1246 | + | v.SetConfigFile(configFile) | |||
| 1247 | + | err = v.ReadInConfig() | |||
| 1248 | + | require.NoError(t, err) | |||
| 1249 | + | ||||
| 1250 | + | eventBus := mocks.NewMockEventPublisher(t) | |||
| 1251 | + | svc := NewService(v, eventBus) | |||
| 1252 | + | ctx := testContext() | |||
| 1253 | + | err = svc.Load(ctx) | |||
| 1254 | + | require.NoError(t, err) | |||
| 1255 | + | ||||
| 1256 | + | maliciousTarget := filepath.Join(dir, "pwned.txt") | |||
| 1257 | + | predictableTmp := configFile + ".tmp" | |||
| 1258 | + | err = os.Symlink(maliciousTarget, predictableTmp) | |||
| 1259 | + | require.NoError(t, err) | |||
| 1260 | + | ||||
| 1261 | + | err = svc.Save(ctx) | |||
| 1262 | + | require.NoError(t, err) | |||
| 1263 | + | ||||
| 1264 | + | _, statErr := os.Stat(maliciousTarget) | |||
| 1265 | + | assert.True(t, os.IsNotExist(statErr), "symlink target should not exist - temp file should use unpredictable name") | |||
| 1266 | + | ||||
| 1267 | + | info, err := os.Lstat(predictableTmp) | |||
| 1268 | + | require.NoError(t, err) | |||
| 1269 | + | assert.NotZero(t, info.Mode()&os.ModeSymlink, "predictable .tmp path should still be the original symlink") | |||
| 1270 | + | }) | |||
| 1271 | + | ||||
| 1239 | t.Run("with defaults does not corrupt config", func(t *testing.T) { | 1272 | t.Run("with defaults does not corrupt config", func(t *testing.T) { | ||
| 1240 | tmpDir := t.TempDir() | 1273 | tmpDir := t.TempDir() | ||
| 1241 | configFile := filepath.Join(tmpDir, "gordon.toml") | 1274 | configFile := filepath.Join(tmpDir, "gordon.toml") | ||
M pkg/registrypush/push.go +89 -36
| @@ -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,9 +36,11 @@ 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 | - | imageSource ImageSource | |
| 42 | + | imageSource func(ctx context.Context, ref string) (v1.Image, func(), error) | |
| 43 | + | cleanupFn func() | |
| 39 | 44 | progress io.Writer | |
| 40 | 45 | } | |
| 41 | 46 | ||
| @@ -52,6 +57,11 @@ func WithTransport(t http.RoundTripper) Option { | |||
| 52 | 57 | return func(p *Pusher) { p.transport = t } | |
| 53 | 58 | } | |
| 54 | 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 | + | ||
| 55 | 65 | // WithInsecureTLS disables TLS certificate verification for registry requests. | |
| 56 | 66 | // This is ignored when a custom transport is provided via WithTransport. | |
| 57 | 67 | func WithInsecureTLS(insecure bool) Option { | |
| @@ -60,7 +70,18 @@ func WithInsecureTLS(insecure bool) Option { | |||
| 60 | 70 | ||
| 61 | 71 | // WithImageSource overrides the default daemon-based image reader. | |
| 62 | 72 | func WithImageSource(src ImageSource) Option { | |
| 63 | - | return func(p *Pusher) { p.imageSource = src } | |
| 73 | + | return func(p *Pusher) { | |
| 74 | + | p.imageSource = func(ctx context.Context, ref string) (v1.Image, func(), error) { | |
| 75 | + | img, err := src(ctx, ref) | |
| 76 | + | return img, nil, err | |
| 77 | + | } | |
| 78 | + | } | |
| 79 | + | } | |
| 80 | + | ||
| 81 | + | // WithImageCleanup sets a function called after Push completes to clean up | |
| 82 | + | // resources created by the image source (e.g., temp tar files). | |
| 83 | + | func WithImageCleanup(fn func()) Option { | |
| 84 | + | return func(p *Pusher) { p.cleanupFn = fn } | |
| 64 | 85 | } | |
| 65 | 86 | ||
| 66 | 87 | // WithProgress sets a writer for progress messages. | |
| @@ -70,7 +91,7 @@ func WithProgress(w io.Writer) Option { | |||
| 70 | 91 | ||
| 71 | 92 | // New creates a Pusher with the given options. | |
| 72 | 93 | func New(opts ...Option) *Pusher { | |
| 73 | - | p := &Pusher{chunkSize: DefaultChunkSize} | |
| 94 | + | p := &Pusher{chunkSize: DefaultChunkSize, timeout: defaultHTTPTimeout} | |
| 74 | 95 | for _, o := range opts { | |
| 75 | 96 | o(p) | |
| 76 | 97 | } | |
| @@ -82,7 +103,9 @@ func New(opts ...Option) *Pusher { | |||
| 82 | 103 | } | |
| 83 | 104 | } | |
| 84 | 105 | if p.imageSource == nil { | |
| 85 | - | p.imageSource = defaultImageSource | |
| 106 | + | p.imageSource = func(ctx context.Context, ref string) (v1.Image, func(), error) { | |
| 107 | + | return defaultImageSourceWithCleanup(ctx, ref) | |
| 108 | + | } | |
| 86 | 109 | } | |
| 87 | 110 | return p | |
| 88 | 111 | } | |
| @@ -107,10 +130,22 @@ func (p *Pusher) Push(ctx context.Context, ref string) error { | |||
| 107 | 130 | return err | |
| 108 | 131 | } | |
| 109 | 132 | ||
| 110 | - | img, err := p.imageSource(ctx, ref) | |
| 133 | + | img, sourceCleanup, err := p.imageSource(ctx, ref) | |
| 111 | 134 | if err != nil { | |
| 112 | 135 | return err | |
| 113 | 136 | } | |
| 137 | + | userCleanup := p.cleanupFn | |
| 138 | + | switch { | |
| 139 | + | case sourceCleanup != nil && userCleanup != nil: | |
| 140 | + | defer func() { | |
| 141 | + | sourceCleanup() | |
| 142 | + | userCleanup() | |
| 143 | + | }() | |
| 144 | + | case sourceCleanup != nil: | |
| 145 | + | defer sourceCleanup() | |
| 146 | + | case userCleanup != nil: | |
| 147 | + | defer userCleanup() | |
| 148 | + | } | |
| 114 | 149 | ||
| 115 | 150 | baseURL := registryBaseURL(parsedRef.Context().RegistryStr()) | |
| 116 | 151 | repo := parsedRef.Context().RepositoryStr() | |
| @@ -147,38 +182,38 @@ func (p *Pusher) UploadBlob(ctx context.Context, baseURL, repo, digest string, s | |||
| 147 | 182 | } | |
| 148 | 183 | } | |
| 149 | 184 | ||
| 150 | - | uploadURL, err = p.uploadBlobChunks(ctx, uploadURL, content, auth) | |
| 185 | + | uploadURL, err = p.uploadBlobChunks(ctx, baseURL, uploadURL, content, auth) | |
| 151 | 186 | if err != nil { | |
| 152 | 187 | return err | |
| 153 | 188 | } | |
| 154 | 189 | ||
| 155 | - | return p.finalizeBlobUpload(ctx, uploadURL, digest, auth) | |
| 190 | + | return p.finalizeBlobUpload(ctx, baseURL, uploadURL, digest, auth) | |
| 156 | 191 | } | |
| 157 | 192 | ||
| 158 | - | // defaultImageSource reads an image from the local Docker/Podman daemon. | |
| 159 | - | func defaultImageSource(ctx context.Context, ref string) (v1.Image, error) { | |
| 193 | + | // defaultImageSourceWithCleanup reads an image from the local Docker/Podman daemon. | |
| 194 | + | func defaultImageSourceWithCleanup(ctx context.Context, ref string) (v1.Image, func(), error) { | |
| 160 | 195 | cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) | |
| 161 | 196 | if err != nil { | |
| 162 | - | return nil, fmt.Errorf("failed to create Docker client: %w", err) | |
| 197 | + | return nil, nil, fmt.Errorf("failed to create Docker client: %w", err) | |
| 163 | 198 | } | |
| 164 | 199 | defer cli.Close() | |
| 165 | 200 | ||
| 166 | 201 | tag, err := name.NewTag(ref) | |
| 167 | 202 | if err != nil { | |
| 168 | - | return nil, fmt.Errorf("failed to parse image ref %s: %w", ref, err) | |
| 203 | + | return nil, nil, fmt.Errorf("failed to parse image ref %s: %w", ref, err) | |
| 169 | 204 | } | |
| 170 | 205 | ||
| 171 | 206 | rc, err := cli.ImageSave(ctx, []string{ref}) | |
| 172 | 207 | if err != nil { | |
| 173 | - | return nil, fmt.Errorf("failed to export image %s: %w", ref, err) | |
| 208 | + | return nil, nil, fmt.Errorf("failed to export image %s: %w", ref, err) | |
| 174 | 209 | } | |
| 175 | 210 | ||
| 176 | 211 | tmpFile, err := os.CreateTemp("", "gordon-push-*.tar") | |
| 177 | 212 | if err != nil { | |
| 178 | 213 | if closeErr := rc.Close(); closeErr != nil { | |
| 179 | - | return nil, fmt.Errorf("failed to create temp file for image export: %w (also failed to close image export stream: %v)", err, closeErr) | |
| 214 | + | return nil, nil, fmt.Errorf("failed to create temp file for image export: %w (also failed to close image export stream: %v)", err, closeErr) | |
| 180 | 215 | } | |
| 181 | - | return nil, fmt.Errorf("failed to create temp file for image export: %w", err) | |
| 216 | + | return nil, nil, fmt.Errorf("failed to create temp file for image export: %w", err) | |
| 182 | 217 | } | |
| 183 | 218 | tmpPath := tmpFile.Name() | |
| 184 | 219 | ||
| @@ -186,33 +221,34 @@ func defaultImageSource(ctx context.Context, ref string) (v1.Image, error) { | |||
| 186 | 221 | if closeErr := rc.Close(); closeErr != nil { | |
| 187 | 222 | _ = tmpFile.Close() | |
| 188 | 223 | _ = os.Remove(tmpPath) | |
| 189 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close image export stream: %v)", err, closeErr) | |
| 224 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close image export stream: %v)", err, closeErr) | |
| 190 | 225 | } | |
| 191 | 226 | if closeErr := tmpFile.Close(); closeErr != nil { | |
| 192 | 227 | _ = os.Remove(tmpPath) | |
| 193 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close temp file: %v)", err, closeErr) | |
| 228 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close temp file: %v)", err, closeErr) | |
| 194 | 229 | } | |
| 195 | 230 | if removeErr := os.Remove(tmpPath); removeErr != nil { | |
| 196 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to remove temp file %s: %v)", err, tmpPath, removeErr) | |
| 231 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to remove temp file %s: %v)", err, tmpPath, removeErr) | |
| 197 | 232 | } | |
| 198 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w", err) | |
| 233 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w", err) | |
| 199 | 234 | } | |
| 200 | 235 | if err := rc.Close(); err != nil { | |
| 201 | 236 | _ = tmpFile.Close() | |
| 202 | 237 | _ = os.Remove(tmpPath) | |
| 203 | - | return nil, fmt.Errorf("failed to close image export stream: %w", err) | |
| 238 | + | return nil, nil, fmt.Errorf("failed to close image export stream: %w", err) | |
| 204 | 239 | } | |
| 205 | 240 | if err := tmpFile.Close(); err != nil { | |
| 206 | 241 | _ = os.Remove(tmpPath) | |
| 207 | - | return nil, fmt.Errorf("failed to close temp file %s: %w", tmpPath, err) | |
| 242 | + | return nil, nil, fmt.Errorf("failed to close temp file %s: %w", tmpPath, err) | |
| 208 | 243 | } | |
| 209 | 244 | ||
| 210 | 245 | img, err := tarball.ImageFromPath(tmpPath, &tag) | |
| 211 | 246 | if err != nil { | |
| 212 | 247 | _ = os.Remove(tmpPath) | |
| 213 | - | return nil, fmt.Errorf("failed to parse image tar for %s: %w", ref, err) | |
| 248 | + | return nil, nil, fmt.Errorf("failed to parse image tar for %s: %w", ref, err) | |
| 214 | 249 | } | |
| 215 | - | return img, nil | |
| 250 | + | cleanup := func() { _ = os.Remove(tmpPath) } | |
| 251 | + | return img, cleanup, nil | |
| 216 | 252 | } | |
| 217 | 253 | ||
| 218 | 254 | func (p *Pusher) uploadImageLayers(ctx context.Context, img v1.Image, baseURL, repo, auth string) error { | |
| @@ -283,7 +319,7 @@ func (p *Pusher) uploadManifest(ctx context.Context, baseURL, repo, tag string, | |||
| 283 | 319 | if err != nil { | |
| 284 | 320 | return fmt.Errorf("failed to create manifest upload request: %w", err) | |
| 285 | 321 | } | |
| 286 | - | setAuthHeader(req, auth) | |
| 322 | + | setScopedAuth(req, auth, baseURL) | |
| 287 | 323 | req.Header.Set("Content-Type", string(mediaType)) | |
| 288 | 324 | req.ContentLength = int64(len(manifest)) | |
| 289 | 325 | ||
| @@ -346,7 +382,7 @@ func (p *Pusher) checkBlobExists(ctx context.Context, baseURL, repo, digest, aut | |||
| 346 | 382 | if err != nil { | |
| 347 | 383 | return fmt.Errorf("failed to create blob HEAD request: %w", err) | |
| 348 | 384 | } | |
| 349 | - | setAuthHeader(req, auth) | |
| 385 | + | setScopedAuth(req, auth, baseURL) | |
| 350 | 386 | ||
| 351 | 387 | resp, err := p.httpClient().Do(req) | |
| 352 | 388 | if err != nil { | |
| @@ -370,7 +406,7 @@ func (p *Pusher) startBlobUpload(ctx context.Context, baseURL, repo, auth string | |||
| 370 | 406 | if err != nil { | |
| 371 | 407 | return "", fmt.Errorf("failed to create blob upload start request: %w", err) | |
| 372 | 408 | } | |
| 373 | - | setAuthHeader(req, auth) | |
| 409 | + | setScopedAuth(req, auth, baseURL) | |
| 374 | 410 | ||
| 375 | 411 | resp, err := p.httpClient().Do(req) | |
| 376 | 412 | if err != nil { | |
| @@ -391,7 +427,7 @@ func (p *Pusher) startBlobUpload(ctx context.Context, baseURL, repo, auth string | |||
| 391 | 427 | return location, nil | |
| 392 | 428 | } | |
| 393 | 429 | ||
| 394 | - | func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content io.Reader, auth string) (string, error) { | |
| 430 | + | func (p *Pusher) uploadBlobChunks(ctx context.Context, baseURL, uploadURL string, content io.Reader, auth string) (string, error) { | |
| 395 | 431 | buffer := make([]byte, p.chunkSize) | |
| 396 | 432 | offset := int64(0) | |
| 397 | 433 | ||
| @@ -404,7 +440,7 @@ func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content | |||
| 404 | 440 | return "", fmt.Errorf("failed to read blob chunk: %w", err) | |
| 405 | 441 | } | |
| 406 | 442 | ||
| 407 | - | nextURL, patchErr := p.uploadBlobChunk(ctx, uploadURL, buffer[:readBytes], offset, auth) | |
| 443 | + | nextURL, patchErr := p.uploadBlobChunk(ctx, baseURL, uploadURL, buffer[:readBytes], offset, auth) | |
| 408 | 444 | if patchErr != nil { | |
| 409 | 445 | return "", patchErr | |
| 410 | 446 | } | |
| @@ -418,12 +454,12 @@ func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content | |||
| 418 | 454 | } | |
| 419 | 455 | } | |
| 420 | 456 | ||
| 421 | - | func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk []byte, offset int64, auth string) (string, error) { | |
| 457 | + | func (p *Pusher) uploadBlobChunk(ctx context.Context, baseURL, uploadURL string, chunk []byte, offset int64, auth string) (string, error) { | |
| 422 | 458 | req, err := http.NewRequestWithContext(ctx, http.MethodPatch, uploadURL, bytesReader(chunk)) | |
| 423 | 459 | if err != nil { | |
| 424 | 460 | return "", fmt.Errorf("failed to create blob chunk request: %w", err) | |
| 425 | 461 | } | |
| 426 | - | setAuthHeader(req, auth) | |
| 462 | + | setScopedAuth(req, auth, baseURL) | |
| 427 | 463 | req.Header.Set("Content-Type", "application/octet-stream") | |
| 428 | 464 | req.Header.Set("Content-Range", fmt.Sprintf("%d-%d", offset, offset+int64(len(chunk))-1)) | |
| 429 | 465 | req.ContentLength = int64(len(chunk)) | |
| @@ -447,7 +483,7 @@ func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk [] | |||
| 447 | 483 | return nextURL, nil | |
| 448 | 484 | } | |
| 449 | 485 | ||
| 450 | - | func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth string) error { | |
| 486 | + | func (p *Pusher) finalizeBlobUpload(ctx context.Context, baseURL, uploadURL, digest, auth string) error { | |
| 451 | 487 | parsedURL, err := url.Parse(uploadURL) | |
| 452 | 488 | if err != nil { | |
| 453 | 489 | return fmt.Errorf("failed to parse blob upload location: %w", err) | |
| @@ -461,7 +497,7 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth | |||
| 461 | 497 | if err != nil { | |
| 462 | 498 | return fmt.Errorf("failed to create blob finalize request: %w", err) | |
| 463 | 499 | } | |
| 464 | - | setAuthHeader(req, auth) | |
| 500 | + | setScopedAuth(req, auth, baseURL) | |
| 465 | 501 | ||
| 466 | 502 | resp, err := p.httpClient().Do(req) | |
| 467 | 503 | if err != nil { | |
| @@ -478,13 +514,24 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth | |||
| 478 | 514 | } | |
| 479 | 515 | ||
| 480 | 516 | func (p *Pusher) httpClient() *http.Client { | |
| 481 | - | return &http.Client{Transport: p.transport} | |
| 517 | + | return &http.Client{Transport: p.transport, Timeout: p.timeout} | |
| 482 | 518 | } | |
| 483 | 519 | ||
| 484 | - | func setAuthHeader(req *http.Request, auth string) { | |
| 485 | - | if auth != "" { | |
| 486 | - | req.Header.Set("Authorization", auth) | |
| 520 | + | // setScopedAuth attaches the auth header only if the request URL matches the | |
| 521 | + | // registry origin (scheme + host). This prevents credential leakage if a | |
| 522 | + | // redirect somehow escapes origin pinning. | |
| 523 | + | func setScopedAuth(req *http.Request, auth, registryOrigin string) { | |
| 524 | + | if auth == "" { | |
| 525 | + | return | |
| 526 | + | } | |
| 527 | + | origin, err := url.Parse(registryOrigin) | |
| 528 | + | if err != nil { | |
| 529 | + | return | |
| 530 | + | } | |
| 531 | + | if req.URL.Scheme != origin.Scheme || req.URL.Host != origin.Host { | |
| 532 | + | return | |
| 487 | 533 | } | |
| 534 | + | req.Header.Set("Authorization", auth) | |
| 488 | 535 | } | |
| 489 | 536 | ||
| 490 | 537 | func resolveLocation(baseURL, location string) (string, error) { | |
| @@ -502,7 +549,13 @@ func resolveLocation(baseURL, location string) (string, error) { | |||
| 502 | 549 | return "", err | |
| 503 | 550 | } | |
| 504 | 551 | ||
| 505 | - | return base.ResolveReference(ref).String(), nil | |
| 552 | + | resolved := base.ResolveReference(ref) | |
| 553 | + | ||
| 554 | + | if resolved.Scheme != base.Scheme || resolved.Host != base.Host { | |
| 555 | + | return "", fmt.Errorf("registry returned cross-origin Location %q (expected %s://%s)", location, base.Scheme, base.Host) | |
| 556 | + | } | |
| 557 | + | ||
| 558 | + | return resolved.String(), nil | |
| 506 | 559 | } | |
| 507 | 560 | ||
| 508 | 561 | func chunkCount(size, chunkSize int64) int64 { | |
| @@ -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,9 +36,11 @@ 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 func(ctx context.Context, ref string) (v1.Image, func(), error) |
| 43 | + | cleanupFn func() | |||
| 39 | progress io.Writer | 44 | progress io.Writer | ||
| 40 | } | 45 | } | ||
| 41 | 46 | ||||
| @@ -52,6 +57,11 @@ func WithTransport(t http.RoundTripper) Option { | |||||
| 52 | return func(p *Pusher) { p.transport = t } | 57 | return func(p *Pusher) { p.transport = t } | ||
| 53 | } | 58 | } | ||
| 54 | 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 | + | ||||
| 55 | // WithInsecureTLS disables TLS certificate verification for registry requests. | 65 | // WithInsecureTLS disables TLS certificate verification for registry requests. | ||
| 56 | // This is ignored when a custom transport is provided via WithTransport. | 66 | // This is ignored when a custom transport is provided via WithTransport. | ||
| 57 | func WithInsecureTLS(insecure bool) Option { | 67 | func WithInsecureTLS(insecure bool) Option { | ||
| @@ -60,7 +70,18 @@ func WithInsecureTLS(insecure bool) Option { | |||||
| 60 | 70 | ||||
| 61 | // WithImageSource overrides the default daemon-based image reader. | 71 | // WithImageSource overrides the default daemon-based image reader. | ||
| 62 | func WithImageSource(src ImageSource) Option { | 72 | func WithImageSource(src ImageSource) Option { | ||
| 63 | - | return func(p *Pusher) { p.imageSource = src } | 73 | + | return func(p *Pusher) { |
| 74 | + | p.imageSource = func(ctx context.Context, ref string) (v1.Image, func(), error) { | |||
| 75 | + | img, err := src(ctx, ref) | |||
| 76 | + | return img, nil, err | |||
| 77 | + | } | |||
| 78 | + | } | |||
| 79 | + | } | |||
| 80 | + | ||||
| 81 | + | // WithImageCleanup sets a function called after Push completes to clean up | |||
| 82 | + | // resources created by the image source (e.g., temp tar files). | |||
| 83 | + | func WithImageCleanup(fn func()) Option { | |||
| 84 | + | return func(p *Pusher) { p.cleanupFn = fn } | |||
| 64 | } | 85 | } | ||
| 65 | 86 | ||||
| 66 | // WithProgress sets a writer for progress messages. | 87 | // WithProgress sets a writer for progress messages. | ||
| @@ -70,7 +91,7 @@ func WithProgress(w io.Writer) Option { | |||||
| 70 | 91 | ||||
| 71 | // New creates a Pusher with the given options. | 92 | // New creates a Pusher with the given options. | ||
| 72 | func New(opts ...Option) *Pusher { | 93 | func New(opts ...Option) *Pusher { | ||
| 73 | - | p := &Pusher{chunkSize: DefaultChunkSize} | 94 | + | p := &Pusher{chunkSize: DefaultChunkSize, timeout: defaultHTTPTimeout} |
| 74 | for _, o := range opts { | 95 | for _, o := range opts { | ||
| 75 | o(p) | 96 | o(p) | ||
| 76 | } | 97 | } | ||
| @@ -82,7 +103,9 @@ func New(opts ...Option) *Pusher { | |||||
| 82 | } | 103 | } | ||
| 83 | } | 104 | } | ||
| 84 | if p.imageSource == nil { | 105 | if p.imageSource == nil { | ||
| 85 | - | p.imageSource = defaultImageSource | 106 | + | p.imageSource = func(ctx context.Context, ref string) (v1.Image, func(), error) { |
| 107 | + | return defaultImageSourceWithCleanup(ctx, ref) | |||
| 108 | + | } | |||
| 86 | } | 109 | } | ||
| 87 | return p | 110 | return p | ||
| 88 | } | 111 | } | ||
| @@ -107,10 +130,22 @@ func (p *Pusher) Push(ctx context.Context, ref string) error { | |||||
| 107 | return err | 130 | return err | ||
| 108 | } | 131 | } | ||
| 109 | 132 | ||||
| 110 | - | img, err := p.imageSource(ctx, ref) | 133 | + | img, sourceCleanup, err := p.imageSource(ctx, ref) |
| 111 | if err != nil { | 134 | if err != nil { | ||
| 112 | return err | 135 | return err | ||
| 113 | } | 136 | } | ||
| 137 | + | userCleanup := p.cleanupFn | |||
| 138 | + | switch { | |||
| 139 | + | case sourceCleanup != nil && userCleanup != nil: | |||
| 140 | + | defer func() { | |||
| 141 | + | sourceCleanup() | |||
| 142 | + | userCleanup() | |||
| 143 | + | }() | |||
| 144 | + | case sourceCleanup != nil: | |||
| 145 | + | defer sourceCleanup() | |||
| 146 | + | case userCleanup != nil: | |||
| 147 | + | defer userCleanup() | |||
| 148 | + | } | |||
| 114 | 149 | ||||
| 115 | baseURL := registryBaseURL(parsedRef.Context().RegistryStr()) | 150 | baseURL := registryBaseURL(parsedRef.Context().RegistryStr()) | ||
| 116 | repo := parsedRef.Context().RepositoryStr() | 151 | repo := parsedRef.Context().RepositoryStr() | ||
| @@ -147,38 +182,38 @@ func (p *Pusher) UploadBlob(ctx context.Context, baseURL, repo, digest string, s | |||||
| 147 | } | 182 | } | ||
| 148 | } | 183 | } | ||
| 149 | 184 | ||||
| 150 | - | uploadURL, err = p.uploadBlobChunks(ctx, uploadURL, content, auth) | 185 | + | uploadURL, err = p.uploadBlobChunks(ctx, baseURL, uploadURL, content, auth) |
| 151 | if err != nil { | 186 | if err != nil { | ||
| 152 | return err | 187 | return err | ||
| 153 | } | 188 | } | ||
| 154 | 189 | ||||
| 155 | - | return p.finalizeBlobUpload(ctx, uploadURL, digest, auth) | 190 | + | return p.finalizeBlobUpload(ctx, baseURL, uploadURL, digest, auth) |
| 156 | } | 191 | } | ||
| 157 | 192 | ||||
| 158 | - | // defaultImageSource reads an image from the local Docker/Podman daemon. | 193 | + | // defaultImageSourceWithCleanup reads an image from the local Docker/Podman daemon. |
| 159 | - | func defaultImageSource(ctx context.Context, ref string) (v1.Image, error) { | 194 | + | func defaultImageSourceWithCleanup(ctx context.Context, ref string) (v1.Image, func(), error) { |
| 160 | cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) | 195 | cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation()) | ||
| 161 | if err != nil { | 196 | if err != nil { | ||
| 162 | - | return nil, fmt.Errorf("failed to create Docker client: %w", err) | 197 | + | return nil, nil, fmt.Errorf("failed to create Docker client: %w", err) |
| 163 | } | 198 | } | ||
| 164 | defer cli.Close() | 199 | defer cli.Close() | ||
| 165 | 200 | ||||
| 166 | tag, err := name.NewTag(ref) | 201 | tag, err := name.NewTag(ref) | ||
| 167 | if err != nil { | 202 | if err != nil { | ||
| 168 | - | return nil, fmt.Errorf("failed to parse image ref %s: %w", ref, err) | 203 | + | return nil, nil, fmt.Errorf("failed to parse image ref %s: %w", ref, err) |
| 169 | } | 204 | } | ||
| 170 | 205 | ||||
| 171 | rc, err := cli.ImageSave(ctx, []string{ref}) | 206 | rc, err := cli.ImageSave(ctx, []string{ref}) | ||
| 172 | if err != nil { | 207 | if err != nil { | ||
| 173 | - | return nil, fmt.Errorf("failed to export image %s: %w", ref, err) | 208 | + | return nil, nil, fmt.Errorf("failed to export image %s: %w", ref, err) |
| 174 | } | 209 | } | ||
| 175 | 210 | ||||
| 176 | tmpFile, err := os.CreateTemp("", "gordon-push-*.tar") | 211 | tmpFile, err := os.CreateTemp("", "gordon-push-*.tar") | ||
| 177 | if err != nil { | 212 | if err != nil { | ||
| 178 | if closeErr := rc.Close(); closeErr != nil { | 213 | if closeErr := rc.Close(); closeErr != nil { | ||
| 179 | - | return nil, fmt.Errorf("failed to create temp file for image export: %w (also failed to close image export stream: %v)", err, closeErr) | 214 | + | return nil, nil, fmt.Errorf("failed to create temp file for image export: %w (also failed to close image export stream: %v)", err, closeErr) |
| 180 | } | 215 | } | ||
| 181 | - | return nil, fmt.Errorf("failed to create temp file for image export: %w", err) | 216 | + | return nil, nil, fmt.Errorf("failed to create temp file for image export: %w", err) |
| 182 | } | 217 | } | ||
| 183 | tmpPath := tmpFile.Name() | 218 | tmpPath := tmpFile.Name() | ||
| 184 | 219 | ||||
| @@ -186,33 +221,34 @@ func defaultImageSource(ctx context.Context, ref string) (v1.Image, error) { | |||||
| 186 | if closeErr := rc.Close(); closeErr != nil { | 221 | if closeErr := rc.Close(); closeErr != nil { | ||
| 187 | _ = tmpFile.Close() | 222 | _ = tmpFile.Close() | ||
| 188 | _ = os.Remove(tmpPath) | 223 | _ = os.Remove(tmpPath) | ||
| 189 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close image export stream: %v)", err, closeErr) | 224 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close image export stream: %v)", err, closeErr) |
| 190 | } | 225 | } | ||
| 191 | if closeErr := tmpFile.Close(); closeErr != nil { | 226 | if closeErr := tmpFile.Close(); closeErr != nil { | ||
| 192 | _ = os.Remove(tmpPath) | 227 | _ = os.Remove(tmpPath) | ||
| 193 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close temp file: %v)", err, closeErr) | 228 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to close temp file: %v)", err, closeErr) |
| 194 | } | 229 | } | ||
| 195 | if removeErr := os.Remove(tmpPath); removeErr != nil { | 230 | if removeErr := os.Remove(tmpPath); removeErr != nil { | ||
| 196 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to remove temp file %s: %v)", err, tmpPath, removeErr) | 231 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w (also failed to remove temp file %s: %v)", err, tmpPath, removeErr) |
| 197 | } | 232 | } | ||
| 198 | - | return nil, fmt.Errorf("failed to write image tar to temp file: %w", err) | 233 | + | return nil, nil, fmt.Errorf("failed to write image tar to temp file: %w", err) |
| 199 | } | 234 | } | ||
| 200 | if err := rc.Close(); err != nil { | 235 | if err := rc.Close(); err != nil { | ||
| 201 | _ = tmpFile.Close() | 236 | _ = tmpFile.Close() | ||
| 202 | _ = os.Remove(tmpPath) | 237 | _ = os.Remove(tmpPath) | ||
| 203 | - | return nil, fmt.Errorf("failed to close image export stream: %w", err) | 238 | + | return nil, nil, fmt.Errorf("failed to close image export stream: %w", err) |
| 204 | } | 239 | } | ||
| 205 | if err := tmpFile.Close(); err != nil { | 240 | if err := tmpFile.Close(); err != nil { | ||
| 206 | _ = os.Remove(tmpPath) | 241 | _ = os.Remove(tmpPath) | ||
| 207 | - | return nil, fmt.Errorf("failed to close temp file %s: %w", tmpPath, err) | 242 | + | return nil, nil, fmt.Errorf("failed to close temp file %s: %w", tmpPath, err) |
| 208 | } | 243 | } | ||
| 209 | 244 | ||||
| 210 | img, err := tarball.ImageFromPath(tmpPath, &tag) | 245 | img, err := tarball.ImageFromPath(tmpPath, &tag) | ||
| 211 | if err != nil { | 246 | if err != nil { | ||
| 212 | _ = os.Remove(tmpPath) | 247 | _ = os.Remove(tmpPath) | ||
| 213 | - | return nil, fmt.Errorf("failed to parse image tar for %s: %w", ref, err) | 248 | + | return nil, nil, fmt.Errorf("failed to parse image tar for %s: %w", ref, err) |
| 214 | } | 249 | } | ||
| 215 | - | return img, nil | 250 | + | cleanup := func() { _ = os.Remove(tmpPath) } |
| 251 | + | return img, cleanup, nil | |||
| 216 | } | 252 | } | ||
| 217 | 253 | ||||
| 218 | func (p *Pusher) uploadImageLayers(ctx context.Context, img v1.Image, baseURL, repo, auth string) error { | 254 | func (p *Pusher) uploadImageLayers(ctx context.Context, img v1.Image, baseURL, repo, auth string) error { | ||
| @@ -283,7 +319,7 @@ func (p *Pusher) uploadManifest(ctx context.Context, baseURL, repo, tag string, | |||||
| 283 | if err != nil { | 319 | if err != nil { | ||
| 284 | return fmt.Errorf("failed to create manifest upload request: %w", err) | 320 | return fmt.Errorf("failed to create manifest upload request: %w", err) | ||
| 285 | } | 321 | } | ||
| 286 | - | setAuthHeader(req, auth) | 322 | + | setScopedAuth(req, auth, baseURL) |
| 287 | req.Header.Set("Content-Type", string(mediaType)) | 323 | req.Header.Set("Content-Type", string(mediaType)) | ||
| 288 | req.ContentLength = int64(len(manifest)) | 324 | req.ContentLength = int64(len(manifest)) | ||
| 289 | 325 | ||||
| @@ -346,7 +382,7 @@ func (p *Pusher) checkBlobExists(ctx context.Context, baseURL, repo, digest, aut | |||||
| 346 | if err != nil { | 382 | if err != nil { | ||
| 347 | return fmt.Errorf("failed to create blob HEAD request: %w", err) | 383 | return fmt.Errorf("failed to create blob HEAD request: %w", err) | ||
| 348 | } | 384 | } | ||
| 349 | - | setAuthHeader(req, auth) | 385 | + | setScopedAuth(req, auth, baseURL) |
| 350 | 386 | ||||
| 351 | resp, err := p.httpClient().Do(req) | 387 | resp, err := p.httpClient().Do(req) | ||
| 352 | if err != nil { | 388 | if err != nil { | ||
| @@ -370,7 +406,7 @@ func (p *Pusher) startBlobUpload(ctx context.Context, baseURL, repo, auth string | |||||
| 370 | if err != nil { | 406 | if err != nil { | ||
| 371 | return "", fmt.Errorf("failed to create blob upload start request: %w", err) | 407 | return "", fmt.Errorf("failed to create blob upload start request: %w", err) | ||
| 372 | } | 408 | } | ||
| 373 | - | setAuthHeader(req, auth) | 409 | + | setScopedAuth(req, auth, baseURL) |
| 374 | 410 | ||||
| 375 | resp, err := p.httpClient().Do(req) | 411 | resp, err := p.httpClient().Do(req) | ||
| 376 | if err != nil { | 412 | if err != nil { | ||
| @@ -391,7 +427,7 @@ func (p *Pusher) startBlobUpload(ctx context.Context, baseURL, repo, auth string | |||||
| 391 | return location, nil | 427 | return location, nil | ||
| 392 | } | 428 | } | ||
| 393 | 429 | ||||
| 394 | - | func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content io.Reader, auth string) (string, error) { | 430 | + | func (p *Pusher) uploadBlobChunks(ctx context.Context, baseURL, uploadURL string, content io.Reader, auth string) (string, error) { |
| 395 | buffer := make([]byte, p.chunkSize) | 431 | buffer := make([]byte, p.chunkSize) | ||
| 396 | offset := int64(0) | 432 | offset := int64(0) | ||
| 397 | 433 | ||||
| @@ -404,7 +440,7 @@ func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content | |||||
| 404 | return "", fmt.Errorf("failed to read blob chunk: %w", err) | 440 | return "", fmt.Errorf("failed to read blob chunk: %w", err) | ||
| 405 | } | 441 | } | ||
| 406 | 442 | ||||
| 407 | - | nextURL, patchErr := p.uploadBlobChunk(ctx, uploadURL, buffer[:readBytes], offset, auth) | 443 | + | nextURL, patchErr := p.uploadBlobChunk(ctx, baseURL, uploadURL, buffer[:readBytes], offset, auth) |
| 408 | if patchErr != nil { | 444 | if patchErr != nil { | ||
| 409 | return "", patchErr | 445 | return "", patchErr | ||
| 410 | } | 446 | } | ||
| @@ -418,12 +454,12 @@ func (p *Pusher) uploadBlobChunks(ctx context.Context, uploadURL string, content | |||||
| 418 | } | 454 | } | ||
| 419 | } | 455 | } | ||
| 420 | 456 | ||||
| 421 | - | func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk []byte, offset int64, auth string) (string, error) { | 457 | + | func (p *Pusher) uploadBlobChunk(ctx context.Context, baseURL, uploadURL string, chunk []byte, offset int64, auth string) (string, error) { |
| 422 | req, err := http.NewRequestWithContext(ctx, http.MethodPatch, uploadURL, bytesReader(chunk)) | 458 | req, err := http.NewRequestWithContext(ctx, http.MethodPatch, uploadURL, bytesReader(chunk)) | ||
| 423 | if err != nil { | 459 | if err != nil { | ||
| 424 | return "", fmt.Errorf("failed to create blob chunk request: %w", err) | 460 | return "", fmt.Errorf("failed to create blob chunk request: %w", err) | ||
| 425 | } | 461 | } | ||
| 426 | - | setAuthHeader(req, auth) | 462 | + | setScopedAuth(req, auth, baseURL) |
| 427 | req.Header.Set("Content-Type", "application/octet-stream") | 463 | req.Header.Set("Content-Type", "application/octet-stream") | ||
| 428 | req.Header.Set("Content-Range", fmt.Sprintf("%d-%d", offset, offset+int64(len(chunk))-1)) | 464 | req.Header.Set("Content-Range", fmt.Sprintf("%d-%d", offset, offset+int64(len(chunk))-1)) | ||
| 429 | req.ContentLength = int64(len(chunk)) | 465 | req.ContentLength = int64(len(chunk)) | ||
| @@ -447,7 +483,7 @@ func (p *Pusher) uploadBlobChunk(ctx context.Context, uploadURL string, chunk [] | |||||
| 447 | return nextURL, nil | 483 | return nextURL, nil | ||
| 448 | } | 484 | } | ||
| 449 | 485 | ||||
| 450 | - | func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth string) error { | 486 | + | func (p *Pusher) finalizeBlobUpload(ctx context.Context, baseURL, uploadURL, digest, auth string) error { |
| 451 | parsedURL, err := url.Parse(uploadURL) | 487 | parsedURL, err := url.Parse(uploadURL) | ||
| 452 | if err != nil { | 488 | if err != nil { | ||
| 453 | return fmt.Errorf("failed to parse blob upload location: %w", err) | 489 | return fmt.Errorf("failed to parse blob upload location: %w", err) | ||
| @@ -461,7 +497,7 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth | |||||
| 461 | if err != nil { | 497 | if err != nil { | ||
| 462 | return fmt.Errorf("failed to create blob finalize request: %w", err) | 498 | return fmt.Errorf("failed to create blob finalize request: %w", err) | ||
| 463 | } | 499 | } | ||
| 464 | - | setAuthHeader(req, auth) | 500 | + | setScopedAuth(req, auth, baseURL) |
| 465 | 501 | ||||
| 466 | resp, err := p.httpClient().Do(req) | 502 | resp, err := p.httpClient().Do(req) | ||
| 467 | if err != nil { | 503 | if err != nil { | ||
| @@ -478,13 +514,24 @@ func (p *Pusher) finalizeBlobUpload(ctx context.Context, uploadURL, digest, auth | |||||
| 478 | } | 514 | } | ||
| 479 | 515 | ||||
| 480 | func (p *Pusher) httpClient() *http.Client { | 516 | func (p *Pusher) httpClient() *http.Client { | ||
| 481 | - | return &http.Client{Transport: p.transport} | 517 | + | return &http.Client{Transport: p.transport, Timeout: p.timeout} |
| 482 | } | 518 | } | ||
| 483 | 519 | ||||
| 484 | - | func setAuthHeader(req *http.Request, auth string) { | 520 | + | // setScopedAuth attaches the auth header only if the request URL matches the |
| 485 | - | if auth != "" { | 521 | + | // registry origin (scheme + host). This prevents credential leakage if a |
| 486 | - | req.Header.Set("Authorization", auth) | 522 | + | // redirect somehow escapes origin pinning. |
| 523 | + | func setScopedAuth(req *http.Request, auth, registryOrigin string) { | |||
| 524 | + | if auth == "" { | |||
| 525 | + | return | |||
| 526 | + | } | |||
| 527 | + | origin, err := url.Parse(registryOrigin) | |||
| 528 | + | if err != nil { | |||
| 529 | + | return | |||
| 530 | + | } | |||
| 531 | + | if req.URL.Scheme != origin.Scheme || req.URL.Host != origin.Host { | |||
| 532 | + | return | |||
| 487 | } | 533 | } | ||
| 534 | + | req.Header.Set("Authorization", auth) | |||
| 488 | } | 535 | } | ||
| 489 | 536 | ||||
| 490 | func resolveLocation(baseURL, location string) (string, error) { | 537 | func resolveLocation(baseURL, location string) (string, error) { | ||
| @@ -502,7 +549,13 @@ func resolveLocation(baseURL, location string) (string, error) { | |||||
| 502 | return "", err | 549 | return "", err | ||
| 503 | } | 550 | } | ||
| 504 | 551 | ||||
| 505 | - | return base.ResolveReference(ref).String(), nil | 552 | + | resolved := base.ResolveReference(ref) |
| 553 | + | ||||
| 554 | + | if resolved.Scheme != base.Scheme || resolved.Host != base.Host { | |||
| 555 | + | return "", fmt.Errorf("registry returned cross-origin Location %q (expected %s://%s)", location, base.Scheme, base.Host) | |||
| 556 | + | } | |||
| 557 | + | ||||
| 558 | + | return resolved.String(), nil | |||
| 506 | } | 559 | } | ||
| 507 | 560 | ||||
| 508 | func chunkCount(size, chunkSize int64) int64 { | 561 | func chunkCount(size, chunkSize int64) int64 { | ||
M pkg/registrypush/push_test.go +135 -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" | |
| @@ -217,6 +218,34 @@ func TestPusher_Push(t *testing.T) { | |||
| 217 | 218 | } | |
| 218 | 219 | } | |
| 219 | 220 | ||
| 221 | + | func TestPusher_Push_InvokesCleanup(t *testing.T) { | |
| 222 | + | var cleanupCount int | |
| 223 | + | img, err := random.Image(1024, 1) | |
| 224 | + | require.NoError(t, err) | |
| 225 | + | ||
| 226 | + | registry := httptest.NewServer(newFakePushRegistry(t, img, map[string]bool{}).handler()) | |
| 227 | + | defer registry.Close() | |
| 228 | + | ||
| 229 | + | p := registrypush.New( | |
| 230 | + | registrypush.WithChunkSize(1024), | |
| 231 | + | registrypush.WithImageSource(func(ctx context.Context, ref string) (v1.Image, error) { | |
| 232 | + | return img, nil | |
| 233 | + | }), | |
| 234 | + | registrypush.WithImageCleanup(func() { | |
| 235 | + | cleanupCount++ | |
| 236 | + | }), | |
| 237 | + | ) | |
| 238 | + | ||
| 239 | + | registryHost := strings.TrimPrefix(registry.URL, "http://") | |
| 240 | + | err = p.Push(context.Background(), registryHost+"/demo/app:v1") | |
| 241 | + | require.NoError(t, err) | |
| 242 | + | assert.Equal(t, 1, cleanupCount, "cleanup callback should have been invoked after first push") | |
| 243 | + | ||
| 244 | + | err = p.Push(context.Background(), registryHost+"/demo/app:v1") | |
| 245 | + | require.NoError(t, err) | |
| 246 | + | assert.Equal(t, 2, cleanupCount, "cleanup callback should have been invoked after second push") | |
| 247 | + | } | |
| 248 | + | ||
| 220 | 249 | func TestPusher_WithInsecureTLS_AllowsSelfSignedCertificate(t *testing.T) { | |
| 221 | 250 | t.Parallel() | |
| 222 | 251 | ||
| @@ -256,6 +285,112 @@ func TestPusher_WithInsecureTLS_AllowsSelfSignedCertificate(t *testing.T) { | |||
| 256 | 285 | require.NoError(t, err) | |
| 257 | 286 | } | |
| 258 | 287 | ||
| 288 | + | func TestPusher_WithTimeout_RespectsDeadline(t *testing.T) { | |
| 289 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 290 | + | <-r.Context().Done() | |
| 291 | + | })) | |
| 292 | + | defer srv.Close() | |
| 293 | + | ||
| 294 | + | p := registrypush.New( | |
| 295 | + | registrypush.WithChunkSize(1024), | |
| 296 | + | registrypush.WithTimeout(100*time.Millisecond), | |
| 297 | + | ) | |
| 298 | + | ||
| 299 | + | ctx := context.Background() | |
| 300 | + | err := p.UploadBlob(ctx, srv.URL, "test/repo", "sha256:abc123", 10, strings.NewReader("0123456789"), "") | |
| 301 | + | ||
| 302 | + | require.Error(t, err) | |
| 303 | + | } | |
| 304 | + | ||
| 305 | + | func TestUploadBlob_RejectsCrossOriginRedirect(t *testing.T) { | |
| 306 | + | attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 307 | + | t.Errorf("request leaked to attacker: %s %s (Authorization: %s)", r.Method, r.URL.Path, r.Header.Get("Authorization")) | |
| 308 | + | w.WriteHeader(http.StatusOK) | |
| 309 | + | })) | |
| 310 | + | defer attacker.Close() | |
| 311 | + | ||
| 312 | + | registry := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 313 | + | switch r.Method { | |
| 314 | + | case http.MethodHead: | |
| 315 | + | w.WriteHeader(http.StatusNotFound) | |
| 316 | + | case http.MethodPost: | |
| 317 | + | w.Header().Set("Location", attacker.URL+"/v2/repo/blobs/uploads/evil") | |
| 318 | + | w.WriteHeader(http.StatusAccepted) | |
| 319 | + | default: | |
| 320 | + | w.WriteHeader(http.StatusInternalServerError) | |
| 321 | + | } | |
| 322 | + | })) | |
| 323 | + | defer registry.Close() | |
| 324 | + | ||
| 325 | + | p := registrypush.New(registrypush.WithChunkSize(1024)) | |
| 326 | + | ctx := context.Background() | |
| 327 | + | err := p.UploadBlob(ctx, registry.URL, "test/repo", "sha256:abc", 5, strings.NewReader("hello"), "Basic secret") | |
| 328 | + | ||
| 329 | + | require.Error(t, err) | |
| 330 | + | assert.Contains(t, err.Error(), "cross-origin") | |
| 331 | + | } | |
| 332 | + | ||
| 333 | + | // authRecordingTransport records Authorization headers per request URL. | |
| 334 | + | type authRecordingTransport struct { | |
| 335 | + | inner http.RoundTripper | |
| 336 | + | records []authRecord | |
| 337 | + | } | |
| 338 | + | ||
| 339 | + | type authRecord struct { | |
| 340 | + | URL string | |
| 341 | + | Auth string | |
| 342 | + | } | |
| 343 | + | ||
| 344 | + | func (t *authRecordingTransport) RoundTrip(req *http.Request) (*http.Response, error) { | |
| 345 | + | t.records = append(t.records, authRecord{ | |
| 346 | + | URL: req.URL.String(), | |
| 347 | + | Auth: req.Header.Get("Authorization"), | |
| 348 | + | }) | |
| 349 | + | return t.inner.RoundTrip(req) | |
| 350 | + | } | |
| 351 | + | ||
| 352 | + | func TestUploadBlob_AuthHeaderOnlySentToSameOrigin(t *testing.T) { | |
| 353 | + | registry := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 354 | + | switch r.Method { | |
| 355 | + | case http.MethodHead: | |
| 356 | + | w.WriteHeader(http.StatusNotFound) | |
| 357 | + | case http.MethodPost: | |
| 358 | + | w.Header().Set("Location", "/v2/repo/blobs/uploads/abc?chunk=0") | |
| 359 | + | w.WriteHeader(http.StatusAccepted) | |
| 360 | + | case http.MethodPatch: | |
| 361 | + | w.Header().Set("Location", "/v2/repo/blobs/uploads/abc?chunk=1") | |
| 362 | + | w.WriteHeader(http.StatusAccepted) | |
| 363 | + | case http.MethodPut: | |
| 364 | + | w.WriteHeader(http.StatusCreated) | |
| 365 | + | default: | |
| 366 | + | w.WriteHeader(http.StatusInternalServerError) | |
| 367 | + | } | |
| 368 | + | })) | |
| 369 | + | defer registry.Close() | |
| 370 | + | ||
| 371 | + | recorder := &authRecordingTransport{inner: http.DefaultTransport} | |
| 372 | + | p := registrypush.New( | |
| 373 | + | registrypush.WithChunkSize(1024), | |
| 374 | + | registrypush.WithTransport(recorder), | |
| 375 | + | ) | |
| 376 | + | ctx := context.Background() | |
| 377 | + | secret := "Basic dXNlcjpwYXNz" | |
| 378 | + | err := p.UploadBlob(ctx, registry.URL, "test/repo", "sha256:abc", 5, strings.NewReader("hello"), secret) | |
| 379 | + | ||
| 380 | + | require.NoError(t, err) | |
| 381 | + | require.NotEmpty(t, recorder.records) | |
| 382 | + | ||
| 383 | + | registryURL, _ := url.Parse(registry.URL) | |
| 384 | + | for _, rec := range recorder.records { | |
| 385 | + | reqURL, _ := url.Parse(rec.URL) | |
| 386 | + | if reqURL.Host == registryURL.Host { | |
| 387 | + | assert.Equal(t, secret, rec.Auth, "same-origin request to %s should have auth", rec.URL) | |
| 388 | + | } else { | |
| 389 | + | assert.Empty(t, rec.Auth, "cross-origin request to %s should NOT have auth", rec.URL) | |
| 390 | + | } | |
| 391 | + | } | |
| 392 | + | } | |
| 393 | + | ||
| 259 | 394 | type fakeBlobUploadRegistry struct { | |
| 260 | 395 | t *testing.T | |
| 261 | 396 | expectedAuth string | |
| @@ -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" | ||
| @@ -217,6 +218,34 @@ func TestPusher_Push(t *testing.T) { | |||||
| 217 | } | 218 | } | ||
| 218 | } | 219 | } | ||
| 219 | 220 | ||||
| 221 | + | func TestPusher_Push_InvokesCleanup(t *testing.T) { | |||
| 222 | + | var cleanupCount int | |||
| 223 | + | img, err := random.Image(1024, 1) | |||
| 224 | + | require.NoError(t, err) | |||
| 225 | + | ||||
| 226 | + | registry := httptest.NewServer(newFakePushRegistry(t, img, map[string]bool{}).handler()) | |||
| 227 | + | defer registry.Close() | |||
| 228 | + | ||||
| 229 | + | p := registrypush.New( | |||
| 230 | + | registrypush.WithChunkSize(1024), | |||
| 231 | + | registrypush.WithImageSource(func(ctx context.Context, ref string) (v1.Image, error) { | |||
| 232 | + | return img, nil | |||
| 233 | + | }), | |||
| 234 | + | registrypush.WithImageCleanup(func() { | |||
| 235 | + | cleanupCount++ | |||
| 236 | + | }), | |||
| 237 | + | ) | |||
| 238 | + | ||||
| 239 | + | registryHost := strings.TrimPrefix(registry.URL, "http://") | |||
| 240 | + | err = p.Push(context.Background(), registryHost+"/demo/app:v1") | |||
| 241 | + | require.NoError(t, err) | |||
| 242 | + | assert.Equal(t, 1, cleanupCount, "cleanup callback should have been invoked after first push") | |||
| 243 | + | ||||
| 244 | + | err = p.Push(context.Background(), registryHost+"/demo/app:v1") | |||
| 245 | + | require.NoError(t, err) | |||
| 246 | + | assert.Equal(t, 2, cleanupCount, "cleanup callback should have been invoked after second push") | |||
| 247 | + | } | |||
| 248 | + | ||||
| 220 | func TestPusher_WithInsecureTLS_AllowsSelfSignedCertificate(t *testing.T) { | 249 | func TestPusher_WithInsecureTLS_AllowsSelfSignedCertificate(t *testing.T) { | ||
| 221 | t.Parallel() | 250 | t.Parallel() | ||
| 222 | 251 | ||||
| @@ -256,6 +285,112 @@ func TestPusher_WithInsecureTLS_AllowsSelfSignedCertificate(t *testing.T) { | |||||
| 256 | require.NoError(t, err) | 285 | require.NoError(t, err) | ||
| 257 | } | 286 | } | ||
| 258 | 287 | ||||
| 288 | + | func TestPusher_WithTimeout_RespectsDeadline(t *testing.T) { | |||
| 289 | + | srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 290 | + | <-r.Context().Done() | |||
| 291 | + | })) | |||
| 292 | + | defer srv.Close() | |||
| 293 | + | ||||
| 294 | + | p := registrypush.New( | |||
| 295 | + | registrypush.WithChunkSize(1024), | |||
| 296 | + | registrypush.WithTimeout(100*time.Millisecond), | |||
| 297 | + | ) | |||
| 298 | + | ||||
| 299 | + | ctx := context.Background() | |||
| 300 | + | err := p.UploadBlob(ctx, srv.URL, "test/repo", "sha256:abc123", 10, strings.NewReader("0123456789"), "") | |||
| 301 | + | ||||
| 302 | + | require.Error(t, err) | |||
| 303 | + | } | |||
| 304 | + | ||||
| 305 | + | func TestUploadBlob_RejectsCrossOriginRedirect(t *testing.T) { | |||
| 306 | + | attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 307 | + | t.Errorf("request leaked to attacker: %s %s (Authorization: %s)", r.Method, r.URL.Path, r.Header.Get("Authorization")) | |||
| 308 | + | w.WriteHeader(http.StatusOK) | |||
| 309 | + | })) | |||
| 310 | + | defer attacker.Close() | |||
| 311 | + | ||||
| 312 | + | registry := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 313 | + | switch r.Method { | |||
| 314 | + | case http.MethodHead: | |||
| 315 | + | w.WriteHeader(http.StatusNotFound) | |||
| 316 | + | case http.MethodPost: | |||
| 317 | + | w.Header().Set("Location", attacker.URL+"/v2/repo/blobs/uploads/evil") | |||
| 318 | + | w.WriteHeader(http.StatusAccepted) | |||
| 319 | + | default: | |||
| 320 | + | w.WriteHeader(http.StatusInternalServerError) | |||
| 321 | + | } | |||
| 322 | + | })) | |||
| 323 | + | defer registry.Close() | |||
| 324 | + | ||||
| 325 | + | p := registrypush.New(registrypush.WithChunkSize(1024)) | |||
| 326 | + | ctx := context.Background() | |||
| 327 | + | err := p.UploadBlob(ctx, registry.URL, "test/repo", "sha256:abc", 5, strings.NewReader("hello"), "Basic secret") | |||
| 328 | + | ||||
| 329 | + | require.Error(t, err) | |||
| 330 | + | assert.Contains(t, err.Error(), "cross-origin") | |||
| 331 | + | } | |||
| 332 | + | ||||
| 333 | + | // authRecordingTransport records Authorization headers per request URL. | |||
| 334 | + | type authRecordingTransport struct { | |||
| 335 | + | inner http.RoundTripper | |||
| 336 | + | records []authRecord | |||
| 337 | + | } | |||
| 338 | + | ||||
| 339 | + | type authRecord struct { | |||
| 340 | + | URL string | |||
| 341 | + | Auth string | |||
| 342 | + | } | |||
| 343 | + | ||||
| 344 | + | func (t *authRecordingTransport) RoundTrip(req *http.Request) (*http.Response, error) { | |||
| 345 | + | t.records = append(t.records, authRecord{ | |||
| 346 | + | URL: req.URL.String(), | |||
| 347 | + | Auth: req.Header.Get("Authorization"), | |||
| 348 | + | }) | |||
| 349 | + | return t.inner.RoundTrip(req) | |||
| 350 | + | } | |||
| 351 | + | ||||
| 352 | + | func TestUploadBlob_AuthHeaderOnlySentToSameOrigin(t *testing.T) { | |||
| 353 | + | registry := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| 354 | + | switch r.Method { | |||
| 355 | + | case http.MethodHead: | |||
| 356 | + | w.WriteHeader(http.StatusNotFound) | |||
| 357 | + | case http.MethodPost: | |||
| 358 | + | w.Header().Set("Location", "/v2/repo/blobs/uploads/abc?chunk=0") | |||
| 359 | + | w.WriteHeader(http.StatusAccepted) | |||
| 360 | + | case http.MethodPatch: | |||
| 361 | + | w.Header().Set("Location", "/v2/repo/blobs/uploads/abc?chunk=1") | |||
| 362 | + | w.WriteHeader(http.StatusAccepted) | |||
| 363 | + | case http.MethodPut: | |||
| 364 | + | w.WriteHeader(http.StatusCreated) | |||
| 365 | + | default: | |||
| 366 | + | w.WriteHeader(http.StatusInternalServerError) | |||
| 367 | + | } | |||
| 368 | + | })) | |||
| 369 | + | defer registry.Close() | |||
| 370 | + | ||||
| 371 | + | recorder := &authRecordingTransport{inner: http.DefaultTransport} | |||
| 372 | + | p := registrypush.New( | |||
| 373 | + | registrypush.WithChunkSize(1024), | |||
| 374 | + | registrypush.WithTransport(recorder), | |||
| 375 | + | ) | |||
| 376 | + | ctx := context.Background() | |||
| 377 | + | secret := "Basic dXNlcjpwYXNz" | |||
| 378 | + | err := p.UploadBlob(ctx, registry.URL, "test/repo", "sha256:abc", 5, strings.NewReader("hello"), secret) | |||
| 379 | + | ||||
| 380 | + | require.NoError(t, err) | |||
| 381 | + | require.NotEmpty(t, recorder.records) | |||
| 382 | + | ||||
| 383 | + | registryURL, _ := url.Parse(registry.URL) | |||
| 384 | + | for _, rec := range recorder.records { | |||
| 385 | + | reqURL, _ := url.Parse(rec.URL) | |||
| 386 | + | if reqURL.Host == registryURL.Host { | |||
| 387 | + | assert.Equal(t, secret, rec.Auth, "same-origin request to %s should have auth", rec.URL) | |||
| 388 | + | } else { | |||
| 389 | + | assert.Empty(t, rec.Auth, "cross-origin request to %s should NOT have auth", rec.URL) | |||
| 390 | + | } | |||
| 391 | + | } | |||
| 392 | + | } | |||
| 393 | + | ||||
| 259 | type fakeBlobUploadRegistry struct { | 394 | type fakeBlobUploadRegistry struct { | ||
| 260 | t *testing.T | 395 | t *testing.T | ||
| 261 | expectedAuth string | 396 | expectedAuth string | ||