fix: apply CodeRabbit auto-fixes
Fixed 7 file(s) based on 8 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
- commit
- a40ece32f19753378504a389e46b57143f0a0879
- author
- coderabbitai[bot] · · 2026-04-26 06:41 UTC
- committer
- GitHub · · 2026-04-26 06:41 UTC
- parent
- 225cbb6
- signature
- GPG Unverified
B5690EEEBB952194The key is unknown
7 files · +105 -39
M internal/adapters/in/http/registry/handler.go +21 -9
| @@ -32,12 +32,17 @@ const ( | |||
| 32 | 32 | DefaultMaxBlobSize = 1024 * 1024 * 1024 | |
| 33 | 33 | ) | |
| 34 | 34 | ||
| 35 | + | // blobLimits holds consistent blob size limits. | |
| 36 | + | type blobLimits struct { | |
| 37 | + | MaxChunkSize int64 | |
| 38 | + | MaxBlobSize int64 | |
| 39 | + | } | |
| 40 | + | ||
| 35 | 41 | // Handler implements the HTTP handler for Docker Registry API v2. | |
| 36 | 42 | type Handler struct { | |
| 37 | - | registrySvc in.RegistryService | |
| 38 | - | log zerowrap.Logger | |
| 39 | - | maxBlobChunkSize atomic.Int64 | |
| 40 | - | maxBlobSize atomic.Int64 | |
| 43 | + | registrySvc in.RegistryService | |
| 44 | + | log zerowrap.Logger | |
| 45 | + | blobLimits atomic.Value // stores *blobLimits | |
| 41 | 46 | } | |
| 42 | 47 | ||
| 43 | 48 | // NewHandler creates a new registry HTTP handler. | |
| @@ -59,7 +64,10 @@ func NewHandler( | |||
| 59 | 64 | registrySvc: registrySvc, | |
| 60 | 65 | log: log, | |
| 61 | 66 | } | |
| 62 | - | h.UpdateBlobLimits(maxBlobChunkSize, blobSizeLimit) | |
| 67 | + | h.blobLimits.Store(&blobLimits{ | |
| 68 | + | MaxChunkSize: maxBlobChunkSize, | |
| 69 | + | MaxBlobSize: blobSizeLimit, | |
| 70 | + | }) | |
| 63 | 71 | return h | |
| 64 | 72 | } | |
| 65 | 73 | ||
| @@ -71,8 +79,10 @@ func (h *Handler) UpdateBlobLimits(maxBlobChunkSize, maxBlobSize int64) { | |||
| 71 | 79 | if maxBlobSize <= 0 { | |
| 72 | 80 | maxBlobSize = DefaultMaxBlobSize | |
| 73 | 81 | } | |
| 74 | - | h.maxBlobChunkSize.Store(maxBlobChunkSize) | |
| 75 | - | h.maxBlobSize.Store(maxBlobSize) | |
| 82 | + | h.blobLimits.Store(&blobLimits{ | |
| 83 | + | MaxChunkSize: maxBlobChunkSize, | |
| 84 | + | MaxBlobSize: maxBlobSize, | |
| 85 | + | }) | |
| 76 | 86 | } | |
| 77 | 87 | ||
| 78 | 88 | // RegisterRoutes registers the registry routes on the given mux. | |
| @@ -430,8 +440,10 @@ func (h *Handler) handleBlobUpload(w http.ResponseWriter, r *http.Request) { | |||
| 430 | 440 | } | |
| 431 | 441 | } | |
| 432 | 442 | ||
| 433 | - | maxBlobChunkSize := h.maxBlobChunkSize.Load() | |
| 434 | - | maxBlobSize := h.maxBlobSize.Load() | |
| 443 | + | // Load a consistent snapshot of both blob limits atomically | |
| 444 | + | limits := h.blobLimits.Load().(*blobLimits) | |
| 445 | + | maxBlobChunkSize := limits.MaxChunkSize | |
| 446 | + | maxBlobSize := limits.MaxBlobSize | |
| 435 | 447 | ||
| 436 | 448 | // Limit blob chunk size to prevent excessive uploads. | |
| 437 | 449 | // MaxBytesReader wraps the body so the downstream io.Copy stops at the limit. | |
| @@ -32,12 +32,17 @@ const ( | |||||
| 32 | DefaultMaxBlobSize = 1024 * 1024 * 1024 | 32 | DefaultMaxBlobSize = 1024 * 1024 * 1024 | ||
| 33 | ) | 33 | ) | ||
| 34 | 34 | ||||
| 35 | + | // blobLimits holds consistent blob size limits. | |||
| 36 | + | type blobLimits struct { | |||
| 37 | + | MaxChunkSize int64 | |||
| 38 | + | MaxBlobSize int64 | |||
| 39 | + | } | |||
| 40 | + | ||||
| 35 | // Handler implements the HTTP handler for Docker Registry API v2. | 41 | // Handler implements the HTTP handler for Docker Registry API v2. | ||
| 36 | type Handler struct { | 42 | type Handler struct { | ||
| 37 | - | registrySvc in.RegistryService | 43 | + | registrySvc in.RegistryService |
| 38 | - | log zerowrap.Logger | 44 | + | log zerowrap.Logger |
| 39 | - | maxBlobChunkSize atomic.Int64 | 45 | + | blobLimits atomic.Value // stores *blobLimits |
| 40 | - | maxBlobSize atomic.Int64 | |||
| 41 | } | 46 | } | ||
| 42 | 47 | ||||
| 43 | // NewHandler creates a new registry HTTP handler. | 48 | // NewHandler creates a new registry HTTP handler. | ||
| @@ -59,7 +64,10 @@ func NewHandler( | |||||
| 59 | registrySvc: registrySvc, | 64 | registrySvc: registrySvc, | ||
| 60 | log: log, | 65 | log: log, | ||
| 61 | } | 66 | } | ||
| 62 | - | h.UpdateBlobLimits(maxBlobChunkSize, blobSizeLimit) | 67 | + | h.blobLimits.Store(&blobLimits{ |
| 68 | + | MaxChunkSize: maxBlobChunkSize, | |||
| 69 | + | MaxBlobSize: blobSizeLimit, | |||
| 70 | + | }) | |||
| 63 | return h | 71 | return h | ||
| 64 | } | 72 | } | ||
| 65 | 73 | ||||
| @@ -71,8 +79,10 @@ func (h *Handler) UpdateBlobLimits(maxBlobChunkSize, maxBlobSize int64) { | |||||
| 71 | if maxBlobSize <= 0 { | 79 | if maxBlobSize <= 0 { | ||
| 72 | maxBlobSize = DefaultMaxBlobSize | 80 | maxBlobSize = DefaultMaxBlobSize | ||
| 73 | } | 81 | } | ||
| 74 | - | h.maxBlobChunkSize.Store(maxBlobChunkSize) | 82 | + | h.blobLimits.Store(&blobLimits{ |
| 75 | - | h.maxBlobSize.Store(maxBlobSize) | 83 | + | MaxChunkSize: maxBlobChunkSize, |
| 84 | + | MaxBlobSize: maxBlobSize, | |||
| 85 | + | }) | |||
| 76 | } | 86 | } | ||
| 77 | 87 | ||||
| 78 | // RegisterRoutes registers the registry routes on the given mux. | 88 | // RegisterRoutes registers the registry routes on the given mux. | ||
| @@ -430,8 +440,10 @@ func (h *Handler) handleBlobUpload(w http.ResponseWriter, r *http.Request) { | |||||
| 430 | } | 440 | } | ||
| 431 | } | 441 | } | ||
| 432 | 442 | ||||
| 433 | - | maxBlobChunkSize := h.maxBlobChunkSize.Load() | 443 | + | // Load a consistent snapshot of both blob limits atomically |
| 434 | - | maxBlobSize := h.maxBlobSize.Load() | 444 | + | limits := h.blobLimits.Load().(*blobLimits) |
| 445 | + | maxBlobChunkSize := limits.MaxChunkSize | |||
| 446 | + | maxBlobSize := limits.MaxBlobSize | |||
| 435 | 447 | ||||
| 436 | // Limit blob chunk size to prevent excessive uploads. | 448 | // Limit blob chunk size to prevent excessive uploads. | ||
| 437 | // MaxBytesReader wraps the body so the downstream io.Copy stops at the limit. | 449 | // MaxBytesReader wraps the body so the downstream io.Copy stops at the limit. | ||
M internal/app/migrate_env.go +49 -2
| @@ -10,6 +10,7 @@ import ( | |||
| 10 | 10 | ||
| 11 | 11 | "github.com/bnema/gordon/internal/adapters/out/domainsecrets" | |
| 12 | 12 | "github.com/bnema/gordon/internal/domain" | |
| 13 | + | domainerrors "github.com/bnema/gordon/internal/domain" | |
| 13 | 14 | ) | |
| 14 | 15 | ||
| 15 | 16 | func migrateEnvFilesToPass(envDir string, passStore *domainsecrets.PassStore, log zerowrap.Logger) error { | |
| @@ -85,13 +86,30 @@ func migrateEnvFile(envDir, name string, passStore *domainsecrets.PassStore, log | |||
| 85 | 86 | } | |
| 86 | 87 | ||
| 87 | 88 | if len(existingKeys) > 0 { | |
| 88 | - | return fmt.Errorf("pass secrets already exist for %s; refusing to delete plaintext env file automatically", domainName) | |
| 89 | + | return fmt.Errorf("%w: pass secrets already exist for %s (found %d keys); refusing to delete plaintext env file automatically", domainerrors.ErrSecretNotFound, domainName, len(existingKeys)) | |
| 89 | 90 | } | |
| 90 | 91 | if err := passStore.Set(domainName, secrets); err != nil { | |
| 91 | 92 | return fmt.Errorf("failed to migrate secrets for %s: %w", domainName, err) | |
| 92 | 93 | } | |
| 93 | 94 | ||
| 94 | 95 | if err := os.Remove(filePath); err != nil { | |
| 96 | + | // Check if the stored secrets match what we just wrote | |
| 97 | + | storedSecrets, getErr := passStore.Get(domainName) | |
| 98 | + | if getErr == nil && secretsMatch(storedSecrets, secrets) { | |
| 99 | + | // Migration was successful, removal failed but secrets are stored | |
| 100 | + | log.Warn(). | |
| 101 | + | Str("file", filePath). | |
| 102 | + | Str("domain", domainName). | |
| 103 | + | Msg("secrets migrated successfully but failed to remove original file; treating as success") | |
| 104 | + | return nil | |
| 105 | + | } | |
| 106 | + | // Attempt rollback: remove the pass entry we just wrote | |
| 107 | + | if delErr := passStore.Delete(domainName); delErr != nil { | |
| 108 | + | log.Error(). | |
| 109 | + | Err(delErr). | |
| 110 | + | Str("domain", domainName). | |
| 111 | + | Msg("failed to rollback pass entry after file removal failure") | |
| 112 | + | } | |
| 95 | 113 | return fmt.Errorf("failed to remove migrated env file %s: %w", filePath, err) | |
| 96 | 114 | } | |
| 97 | 115 | ||
| @@ -162,13 +180,30 @@ func migrateAttachmentEnvFile(envDir, name string, passStore *domainsecrets.Pass | |||
| 162 | 180 | } | |
| 163 | 181 | ||
| 164 | 182 | if len(existingKeys) > 0 { | |
| 165 | - | return fmt.Errorf("pass secrets already exist for attachment %s; refusing to delete plaintext env file automatically", containerName) | |
| 183 | + | return fmt.Errorf("%w: pass secrets already exist for attachment %s (found %d keys); refusing to delete plaintext env file automatically", domainerrors.ErrSecretNotFound, containerName, len(existingKeys)) | |
| 166 | 184 | } | |
| 167 | 185 | if err := passStore.SetAttachment(containerName, secrets); err != nil { | |
| 168 | 186 | return fmt.Errorf("failed to migrate attachment secrets for %s: %w", containerName, err) | |
| 169 | 187 | } | |
| 170 | 188 | ||
| 171 | 189 | if err := os.Remove(filePath); err != nil { | |
| 190 | + | // Check if the stored secrets match what we just wrote | |
| 191 | + | storedSecrets, getErr := passStore.GetAllAttachment(containerName) | |
| 192 | + | if getErr == nil && secretsMatch(storedSecrets, secrets) { | |
| 193 | + | // Migration was successful, removal failed but secrets are stored | |
| 194 | + | log.Warn(). | |
| 195 | + | Str("file", filePath). | |
| 196 | + | Str("container", containerName). | |
| 197 | + | Msg("attachment secrets migrated successfully but failed to remove original file; treating as success") | |
| 198 | + | return nil | |
| 199 | + | } | |
| 200 | + | // Attempt rollback: remove the pass entry we just wrote | |
| 201 | + | if delErr := passStore.DeleteAttachment(containerName); delErr != nil { | |
| 202 | + | log.Error(). | |
| 203 | + | Err(delErr). | |
| 204 | + | Str("container", containerName). | |
| 205 | + | Msg("failed to rollback pass entry after file removal failure") | |
| 206 | + | } | |
| 172 | 207 | return fmt.Errorf("failed to remove migrated attachment env file %s: %w", filePath, err) | |
| 173 | 208 | } | |
| 174 | 209 | ||
| @@ -185,3 +220,15 @@ func extractContainerNameFromAttachmentFile(filename string) string { | |||
| 185 | 220 | name := strings.TrimSuffix(filename, ".env") | |
| 186 | 221 | return name | |
| 187 | 222 | } | |
| 223 | + | ||
| 224 | + | func secretsMatch(a, b map[string]string) bool { | |
| 225 | + | if len(a) != len(b) { | |
| 226 | + | return false | |
| 227 | + | } | |
| 228 | + | for k, v := range a { | |
| 229 | + | if b[k] != v { | |
| 230 | + | return false | |
| 231 | + | } | |
| 232 | + | } | |
| 233 | + | return true | |
| 234 | + | } | |
| @@ -10,6 +10,7 @@ import ( | |||||
| 10 | 10 | ||||
| 11 | "github.com/bnema/gordon/internal/adapters/out/domainsecrets" | 11 | "github.com/bnema/gordon/internal/adapters/out/domainsecrets" | ||
| 12 | "github.com/bnema/gordon/internal/domain" | 12 | "github.com/bnema/gordon/internal/domain" | ||
| 13 | + | domainerrors "github.com/bnema/gordon/internal/domain" | |||
| 13 | ) | 14 | ) | ||
| 14 | 15 | ||||
| 15 | func migrateEnvFilesToPass(envDir string, passStore *domainsecrets.PassStore, log zerowrap.Logger) error { | 16 | func migrateEnvFilesToPass(envDir string, passStore *domainsecrets.PassStore, log zerowrap.Logger) error { | ||
| @@ -85,13 +86,30 @@ func migrateEnvFile(envDir, name string, passStore *domainsecrets.PassStore, log | |||||
| 85 | } | 86 | } | ||
| 86 | 87 | ||||
| 87 | if len(existingKeys) > 0 { | 88 | if len(existingKeys) > 0 { | ||
| 88 | - | return fmt.Errorf("pass secrets already exist for %s; refusing to delete plaintext env file automatically", domainName) | 89 | + | return fmt.Errorf("%w: pass secrets already exist for %s (found %d keys); refusing to delete plaintext env file automatically", domainerrors.ErrSecretNotFound, domainName, len(existingKeys)) |
| 89 | } | 90 | } | ||
| 90 | if err := passStore.Set(domainName, secrets); err != nil { | 91 | if err := passStore.Set(domainName, secrets); err != nil { | ||
| 91 | return fmt.Errorf("failed to migrate secrets for %s: %w", domainName, err) | 92 | return fmt.Errorf("failed to migrate secrets for %s: %w", domainName, err) | ||
| 92 | } | 93 | } | ||
| 93 | 94 | ||||
| 94 | if err := os.Remove(filePath); err != nil { | 95 | if err := os.Remove(filePath); err != nil { | ||
| 96 | + | // Check if the stored secrets match what we just wrote | |||
| 97 | + | storedSecrets, getErr := passStore.Get(domainName) | |||
| 98 | + | if getErr == nil && secretsMatch(storedSecrets, secrets) { | |||
| 99 | + | // Migration was successful, removal failed but secrets are stored | |||
| 100 | + | log.Warn(). | |||
| 101 | + | Str("file", filePath). | |||
| 102 | + | Str("domain", domainName). | |||
| 103 | + | Msg("secrets migrated successfully but failed to remove original file; treating as success") | |||
| 104 | + | return nil | |||
| 105 | + | } | |||
| 106 | + | // Attempt rollback: remove the pass entry we just wrote | |||
| 107 | + | if delErr := passStore.Delete(domainName); delErr != nil { | |||
| 108 | + | log.Error(). | |||
| 109 | + | Err(delErr). | |||
| 110 | + | Str("domain", domainName). | |||
| 111 | + | Msg("failed to rollback pass entry after file removal failure") | |||
| 112 | + | } | |||
| 95 | return fmt.Errorf("failed to remove migrated env file %s: %w", filePath, err) | 113 | return fmt.Errorf("failed to remove migrated env file %s: %w", filePath, err) | ||
| 96 | } | 114 | } | ||
| 97 | 115 | ||||
| @@ -162,13 +180,30 @@ func migrateAttachmentEnvFile(envDir, name string, passStore *domainsecrets.Pass | |||||
| 162 | } | 180 | } | ||
| 163 | 181 | ||||
| 164 | if len(existingKeys) > 0 { | 182 | if len(existingKeys) > 0 { | ||
| 165 | - | return fmt.Errorf("pass secrets already exist for attachment %s; refusing to delete plaintext env file automatically", containerName) | 183 | + | return fmt.Errorf("%w: pass secrets already exist for attachment %s (found %d keys); refusing to delete plaintext env file automatically", domainerrors.ErrSecretNotFound, containerName, len(existingKeys)) |
| 166 | } | 184 | } | ||
| 167 | if err := passStore.SetAttachment(containerName, secrets); err != nil { | 185 | if err := passStore.SetAttachment(containerName, secrets); err != nil { | ||
| 168 | return fmt.Errorf("failed to migrate attachment secrets for %s: %w", containerName, err) | 186 | return fmt.Errorf("failed to migrate attachment secrets for %s: %w", containerName, err) | ||
| 169 | } | 187 | } | ||
| 170 | 188 | ||||
| 171 | if err := os.Remove(filePath); err != nil { | 189 | if err := os.Remove(filePath); err != nil { | ||
| 190 | + | // Check if the stored secrets match what we just wrote | |||
| 191 | + | storedSecrets, getErr := passStore.GetAllAttachment(containerName) | |||
| 192 | + | if getErr == nil && secretsMatch(storedSecrets, secrets) { | |||
| 193 | + | // Migration was successful, removal failed but secrets are stored | |||
| 194 | + | log.Warn(). | |||
| 195 | + | Str("file", filePath). | |||
| 196 | + | Str("container", containerName). | |||
| 197 | + | Msg("attachment secrets migrated successfully but failed to remove original file; treating as success") | |||
| 198 | + | return nil | |||
| 199 | + | } | |||
| 200 | + | // Attempt rollback: remove the pass entry we just wrote | |||
| 201 | + | if delErr := passStore.DeleteAttachment(containerName); delErr != nil { | |||
| 202 | + | log.Error(). | |||
| 203 | + | Err(delErr). | |||
| 204 | + | Str("container", containerName). | |||
| 205 | + | Msg("failed to rollback pass entry after file removal failure") | |||
| 206 | + | } | |||
| 172 | return fmt.Errorf("failed to remove migrated attachment env file %s: %w", filePath, err) | 207 | return fmt.Errorf("failed to remove migrated attachment env file %s: %w", filePath, err) | ||
| 173 | } | 208 | } | ||
| 174 | 209 | ||||
| @@ -185,3 +220,15 @@ func extractContainerNameFromAttachmentFile(filename string) string { | |||||
| 185 | name := strings.TrimSuffix(filename, ".env") | 220 | name := strings.TrimSuffix(filename, ".env") | ||
| 186 | return name | 221 | return name | ||
| 187 | } | 222 | } | ||
| 223 | + | ||||
| 224 | + | func secretsMatch(a, b map[string]string) bool { | |||
| 225 | + | if len(a) != len(b) { | |||
| 226 | + | return false | |||
| 227 | + | } | |||
| 228 | + | for k, v := range a { | |||
| 229 | + | if b[k] != v { | |||
| 230 | + | return false | |||
| 231 | + | } | |||
| 232 | + | } | |||
| 233 | + | return true | |||
| 234 | + | } | |||
M internal/app/run.go +5 -1
| @@ -1384,7 +1384,11 @@ func readFileBeneath(root, cleanedRelPath string) ([]byte, error) { | |||
| 1384 | 1384 | if st.Mode&unix.S_IFMT != unix.S_IFREG { | |
| 1385 | 1385 | return nil, fmt.Errorf("invalid secret path: secret must be a regular file") | |
| 1386 | 1386 | } | |
| 1387 | - | return os.ReadFile(fmt.Sprintf("/proc/self/fd/%d", fd)) | |
| 1387 | + | data, err := os.ReadFile(fmt.Sprintf("/proc/self/fd/%d", fd)) | |
| 1388 | + | if err != nil { | |
| 1389 | + | return nil, fmt.Errorf("failed to read secret file: %w", err) | |
| 1390 | + | } | |
| 1391 | + | return data, nil | |
| 1388 | 1392 | } | |
| 1389 | 1393 | ||
| 1390 | 1394 | nextFD, err := unix.Openat(dirFD, part, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) | |
| @@ -1384,7 +1384,11 @@ func readFileBeneath(root, cleanedRelPath string) ([]byte, error) { | |||||
| 1384 | if st.Mode&unix.S_IFMT != unix.S_IFREG { | 1384 | if st.Mode&unix.S_IFMT != unix.S_IFREG { | ||
| 1385 | return nil, fmt.Errorf("invalid secret path: secret must be a regular file") | 1385 | return nil, fmt.Errorf("invalid secret path: secret must be a regular file") | ||
| 1386 | } | 1386 | } | ||
| 1387 | - | return os.ReadFile(fmt.Sprintf("/proc/self/fd/%d", fd)) | 1387 | + | data, err := os.ReadFile(fmt.Sprintf("/proc/self/fd/%d", fd)) |
| 1388 | + | if err != nil { | |||
| 1389 | + | return nil, fmt.Errorf("failed to read secret file: %w", err) | |||
| 1390 | + | } | |||
| 1391 | + | return data, nil | |||
| 1388 | } | 1392 | } | ||
| 1389 | 1393 | ||||
| 1390 | nextFD, err := unix.Openat(dirFD, part, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) | 1394 | nextFD, err := unix.Openat(dirFD, part, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0) | ||
M internal/app/run_auth_config_test.go +2 -1
| @@ -7,12 +7,13 @@ import ( | |||
| 7 | 7 | "runtime" | |
| 8 | 8 | "testing" | |
| 9 | 9 | ||
| 10 | - | "github.com/bnema/gordon/internal/domain" | |
| 11 | 10 | "github.com/bnema/zerowrap" | |
| 12 | 11 | "github.com/spf13/viper" | |
| 13 | 12 | "github.com/stretchr/testify/assert" | |
| 14 | 13 | "github.com/stretchr/testify/require" | |
| 15 | 14 | "golang.org/x/sys/unix" | |
| 15 | + | ||
| 16 | + | "github.com/bnema/gordon/internal/domain" | |
| 16 | 17 | ) | |
| 17 | 18 | ||
| 18 | 19 | func TestBuildAuthConfig_UsesConfigEnabledFlag(t *testing.T) { | |
| @@ -7,12 +7,13 @@ import ( | |||||
| 7 | "runtime" | 7 | "runtime" | ||
| 8 | "testing" | 8 | "testing" | ||
| 9 | 9 | ||||
| 10 | - | "github.com/bnema/gordon/internal/domain" | |||
| 11 | "github.com/bnema/zerowrap" | 10 | "github.com/bnema/zerowrap" | ||
| 12 | "github.com/spf13/viper" | 11 | "github.com/spf13/viper" | ||
| 13 | "github.com/stretchr/testify/assert" | 12 | "github.com/stretchr/testify/assert" | ||
| 14 | "github.com/stretchr/testify/require" | 13 | "github.com/stretchr/testify/require" | ||
| 15 | "golang.org/x/sys/unix" | 14 | "golang.org/x/sys/unix" | ||
| 15 | + | ||||
| 16 | + | "github.com/bnema/gordon/internal/domain" | |||
| 16 | ) | 17 | ) | ||
| 17 | 18 | ||||
| 18 | func TestBuildAuthConfig_UsesConfigEnabledFlag(t *testing.T) { | 19 | func TestBuildAuthConfig_UsesConfigEnabledFlag(t *testing.T) { | ||
M internal/domain/redact.go +2 -2
| @@ -11,11 +11,11 @@ const secretKeyPattern = `[A-Z0-9_]*(?:PASSWORD|TOKEN|SECRET|API[_-]?KEY|DATABAS | |||
| 11 | 11 | ||
| 12 | 12 | var secretValuePatterns = []secretRedactionPattern{ | |
| 13 | 13 | { | |
| 14 | - | re: regexp.MustCompile(`(?i)(\b` + secretKeyPattern + `\b\s*=\s*)(?:"[^"]*"|'[^']*'|[^\s]+)`), | |
| 14 | + | re: regexp.MustCompile(`(?i)(\b` + secretKeyPattern + `\b\s*=\s*)(?:"(?:\\.|[^"])*"|'(?:\\.|[^'])*'|[^\s]+)`), | |
| 15 | 15 | repl: `${1}[REDACTED]`, | |
| 16 | 16 | }, | |
| 17 | 17 | { | |
| 18 | - | re: regexp.MustCompile(`(?i)("` + secretKeyPattern + `"\s*:\s*")([^"]*)(")`), | |
| 18 | + | re: regexp.MustCompile(`(?i)("` + secretKeyPattern + `"\s*:\s*")((?:\\.|[^"])*)(")`), | |
| 19 | 19 | repl: `${1}[REDACTED]${3}`, | |
| 20 | 20 | }, | |
| 21 | 21 | { | |
| @@ -11,11 +11,11 @@ const secretKeyPattern = `[A-Z0-9_]*(?:PASSWORD|TOKEN|SECRET|API[_-]?KEY|DATABAS | |||||
| 11 | 11 | ||||
| 12 | var secretValuePatterns = []secretRedactionPattern{ | 12 | var secretValuePatterns = []secretRedactionPattern{ | ||
| 13 | { | 13 | { | ||
| 14 | - | re: regexp.MustCompile(`(?i)(\b` + secretKeyPattern + `\b\s*=\s*)(?:"[^"]*"|'[^']*'|[^\s]+)`), | 14 | + | re: regexp.MustCompile(`(?i)(\b` + secretKeyPattern + `\b\s*=\s*)(?:"(?:\\.|[^"])*"|'(?:\\.|[^'])*'|[^\s]+)`), |
| 15 | repl: `${1}[REDACTED]`, | 15 | repl: `${1}[REDACTED]`, | ||
| 16 | }, | 16 | }, | ||
| 17 | { | 17 | { | ||
| 18 | - | re: regexp.MustCompile(`(?i)("` + secretKeyPattern + `"\s*:\s*")([^"]*)(")`), | 18 | + | re: regexp.MustCompile(`(?i)("` + secretKeyPattern + `"\s*:\s*")((?:\\.|[^"])*)(")`), |
| 19 | repl: `${1}[REDACTED]${3}`, | 19 | repl: `${1}[REDACTED]${3}`, | ||
| 20 | }, | 20 | }, | ||
| 21 | { | 21 | { | ||
M internal/usecase/container/service.go +24 -22
| @@ -1766,14 +1766,14 @@ func (s *Service) validateImageRef(imageRef string) error { | |||
| 1766 | 1766 | if !hasExplicitRegistry(imageRef) { | |
| 1767 | 1767 | return nil | |
| 1768 | 1768 | } | |
| 1769 | - | return s.validateExternalImageRef(imageRef, imageRegistry(imageRef)) | |
| 1769 | + | return s.validateExternalImageRef(context.Background(), imageRef, imageRegistry(imageRef)) | |
| 1770 | 1770 | } | |
| 1771 | 1771 | ||
| 1772 | - | func (s *Service) validateImagePullRef(imageRef string) error { | |
| 1773 | - | return s.validateExternalImageRef(imageRef, imageRegistryForPolicy(imageRef)) | |
| 1772 | + | func (s *Service) validateImagePullRef(ctx context.Context, imageRef string) error { | |
| 1773 | + | return s.validateExternalImageRef(ctx, imageRef, imageRegistryForPolicy(imageRef)) | |
| 1774 | 1774 | } | |
| 1775 | 1775 | ||
| 1776 | - | func (s *Service) validateExternalImageRef(imageRef, registry string) error { | |
| 1776 | + | func (s *Service) validateExternalImageRef(ctx context.Context, imageRef, registry string) error { | |
| 1777 | 1777 | s.mu.RLock() | |
| 1778 | 1778 | cfg := s.config | |
| 1779 | 1779 | s.mu.RUnlock() | |
| @@ -1785,7 +1785,7 @@ func (s *Service) validateExternalImageRef(imageRef, registry string) error { | |||
| 1785 | 1785 | if sameRegistry(registry, cfg.RegistryDomain) { | |
| 1786 | 1786 | return nil | |
| 1787 | 1787 | } | |
| 1788 | - | dangerous, err := isDangerousRegistryHost(imageRegistryHost(registry)) | |
| 1788 | + | dangerous, err := isDangerousRegistryHost(ctx, imageRegistryHost(registry)) | |
| 1789 | 1789 | if err != nil { | |
| 1790 | 1790 | return fmt.Errorf("image %q rejected: registry %q could not be verified: %w", imageRef, registry, err) | |
| 1791 | 1791 | } | |
| @@ -1853,7 +1853,7 @@ func sameRegistry(a, b string) bool { | |||
| 1853 | 1853 | return normalizeRegistry(a) == normalizeRegistry(b) | |
| 1854 | 1854 | } | |
| 1855 | 1855 | ||
| 1856 | - | func isDangerousRegistryHost(host string) (bool, error) { | |
| 1856 | + | func isDangerousRegistryHost(ctx context.Context, host string) (bool, error) { | |
| 1857 | 1857 | host = strings.ToLower(strings.TrimSuffix(host, ".")) | |
| 1858 | 1858 | if host == "localhost" || host == "metadata.google.internal" { | |
| 1859 | 1859 | return true, nil | |
| @@ -1861,12 +1861,12 @@ func isDangerousRegistryHost(host string) (bool, error) { | |||
| 1861 | 1861 | if ip := net.ParseIP(host); ip != nil { | |
| 1862 | 1862 | return isDangerousRegistryIP(ip), nil | |
| 1863 | 1863 | } | |
| 1864 | - | ips, err := net.LookupIP(host) | |
| 1864 | + | addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host) | |
| 1865 | 1865 | if err != nil { | |
| 1866 | 1866 | return false, err | |
| 1867 | 1867 | } | |
| 1868 | - | for _, ip := range ips { | |
| 1869 | - | if isDangerousRegistryIP(ip) { | |
| 1868 | + | for _, addr := range addrs { | |
| 1869 | + | if isDangerousRegistryIP(addr.IP) { | |
| 1870 | 1870 | return true, nil | |
| 1871 | 1871 | } | |
| 1872 | 1872 | } | |
| @@ -1945,7 +1945,7 @@ func (s *Service) ensureImage(ctx context.Context, imageRef string) (string, err | |||
| 1945 | 1945 | ||
| 1946 | 1946 | // Determine if this is an internal deploy and what reference to use for pulls. | |
| 1947 | 1947 | pullRef, isInternal := s.pullRefForDeploy(ctx, imageRef) | |
| 1948 | - | if err := s.validateImagePullRef(imageRef); err != nil { | |
| 1948 | + | if err := s.validateImagePullRef(ctx, imageRef); err != nil { | |
| 1949 | 1949 | return "", err | |
| 1950 | 1950 | } | |
| 1951 | 1951 | if pullRef != imageRef { | |
| @@ -2582,8 +2582,14 @@ func (s *Service) deployAttachedService(ctx context.Context, ownerDomain, servic | |||
| 2582 | 2582 | } | |
| 2583 | 2583 | ||
| 2584 | 2584 | exposedPorts := s.attachmentExposedPorts(ctx, actualImageRef) | |
| 2585 | - | volumes := s.attachmentVolumes(ctx, containerName, actualImageRef) | |
| 2586 | - | envVars := s.attachmentEnv(ctx, containerName, actualImageRef) | |
| 2585 | + | volumes, err := s.attachmentVolumes(ctx, containerName, actualImageRef) | |
| 2586 | + | if err != nil { | |
| 2587 | + | return err | |
| 2588 | + | } | |
| 2589 | + | envVars, err := s.attachmentEnv(ctx, containerName, actualImageRef) | |
| 2590 | + | if err != nil { | |
| 2591 | + | return err | |
| 2592 | + | } | |
| 2587 | 2593 | envHash := hashEnvironment(envVars) | |
| 2588 | 2594 | ||
| 2589 | 2595 | s.mu.RLock() | |
| @@ -2656,24 +2662,20 @@ func (s *Service) attachmentExposedPorts(ctx context.Context, imageRef string) [ | |||
| 2656 | 2662 | return exposedPorts | |
| 2657 | 2663 | } | |
| 2658 | 2664 | ||
| 2659 | - | func (s *Service) attachmentVolumes(ctx context.Context, containerName, imageRef string) map[string]string { | |
| 2660 | - | log := zerowrap.FromCtx(ctx) | |
| 2665 | + | func (s *Service) attachmentVolumes(ctx context.Context, containerName, imageRef string) (map[string]string, error) { | |
| 2661 | 2666 | volumes, err := s.setupVolumes(ctx, containerName, imageRef) | |
| 2662 | 2667 | if err != nil { | |
| 2663 | - | log.WrapErr(err, "failed to setup volumes for attachment") | |
| 2664 | - | return make(map[string]string) | |
| 2668 | + | return nil, fmt.Errorf("failed to setup volumes for attachment %s with image %s: %w", containerName, imageRef, err) | |
| 2665 | 2669 | } | |
| 2666 | - | return volumes | |
| 2670 | + | return volumes, nil | |
| 2667 | 2671 | } | |
| 2668 | 2672 | ||
| 2669 | - | func (s *Service) attachmentEnv(ctx context.Context, containerName, imageRef string) []string { | |
| 2670 | - | log := zerowrap.FromCtx(ctx) | |
| 2673 | + | func (s *Service) attachmentEnv(ctx context.Context, containerName, imageRef string) ([]string, error) { | |
| 2671 | 2674 | envVars, err := s.loadEnvironment(ctx, nil, containerName, imageRef) | |
| 2672 | 2675 | if err != nil { | |
| 2673 | - | log.WrapErr(err, "failed to load environment for attachment") | |
| 2674 | - | return []string{} | |
| 2676 | + | return nil, fmt.Errorf("failed to load environment for attachment %s with image %s: %w", containerName, imageRef, err) | |
| 2675 | 2677 | } | |
| 2676 | - | return envVars | |
| 2678 | + | return envVars, nil | |
| 2677 | 2679 | } | |
| 2678 | 2680 | ||
| 2679 | 2681 | func (s *Service) attachmentEnvDrifted(ctx context.Context, existing *domain.Container, containerName, serviceImage string) (bool, error) { | |
| @@ -1766,14 +1766,14 @@ func (s *Service) validateImageRef(imageRef string) error { | |||||
| 1766 | if !hasExplicitRegistry(imageRef) { | 1766 | if !hasExplicitRegistry(imageRef) { | ||
| 1767 | return nil | 1767 | return nil | ||
| 1768 | } | 1768 | } | ||
| 1769 | - | return s.validateExternalImageRef(imageRef, imageRegistry(imageRef)) | 1769 | + | return s.validateExternalImageRef(context.Background(), imageRef, imageRegistry(imageRef)) |
| 1770 | } | 1770 | } | ||
| 1771 | 1771 | ||||
| 1772 | - | func (s *Service) validateImagePullRef(imageRef string) error { | 1772 | + | func (s *Service) validateImagePullRef(ctx context.Context, imageRef string) error { |
| 1773 | - | return s.validateExternalImageRef(imageRef, imageRegistryForPolicy(imageRef)) | 1773 | + | return s.validateExternalImageRef(ctx, imageRef, imageRegistryForPolicy(imageRef)) |
| 1774 | } | 1774 | } | ||
| 1775 | 1775 | ||||
| 1776 | - | func (s *Service) validateExternalImageRef(imageRef, registry string) error { | 1776 | + | func (s *Service) validateExternalImageRef(ctx context.Context, imageRef, registry string) error { |
| 1777 | s.mu.RLock() | 1777 | s.mu.RLock() | ||
| 1778 | cfg := s.config | 1778 | cfg := s.config | ||
| 1779 | s.mu.RUnlock() | 1779 | s.mu.RUnlock() | ||
| @@ -1785,7 +1785,7 @@ func (s *Service) validateExternalImageRef(imageRef, registry string) error { | |||||
| 1785 | if sameRegistry(registry, cfg.RegistryDomain) { | 1785 | if sameRegistry(registry, cfg.RegistryDomain) { | ||
| 1786 | return nil | 1786 | return nil | ||
| 1787 | } | 1787 | } | ||
| 1788 | - | dangerous, err := isDangerousRegistryHost(imageRegistryHost(registry)) | 1788 | + | dangerous, err := isDangerousRegistryHost(ctx, imageRegistryHost(registry)) |
| 1789 | if err != nil { | 1789 | if err != nil { | ||
| 1790 | return fmt.Errorf("image %q rejected: registry %q could not be verified: %w", imageRef, registry, err) | 1790 | return fmt.Errorf("image %q rejected: registry %q could not be verified: %w", imageRef, registry, err) | ||
| 1791 | } | 1791 | } | ||
| @@ -1853,7 +1853,7 @@ func sameRegistry(a, b string) bool { | |||||
| 1853 | return normalizeRegistry(a) == normalizeRegistry(b) | 1853 | return normalizeRegistry(a) == normalizeRegistry(b) | ||
| 1854 | } | 1854 | } | ||
| 1855 | 1855 | ||||
| 1856 | - | func isDangerousRegistryHost(host string) (bool, error) { | 1856 | + | func isDangerousRegistryHost(ctx context.Context, host string) (bool, error) { |
| 1857 | host = strings.ToLower(strings.TrimSuffix(host, ".")) | 1857 | host = strings.ToLower(strings.TrimSuffix(host, ".")) | ||
| 1858 | if host == "localhost" || host == "metadata.google.internal" { | 1858 | if host == "localhost" || host == "metadata.google.internal" { | ||
| 1859 | return true, nil | 1859 | return true, nil | ||
| @@ -1861,12 +1861,12 @@ func isDangerousRegistryHost(host string) (bool, error) { | |||||
| 1861 | if ip := net.ParseIP(host); ip != nil { | 1861 | if ip := net.ParseIP(host); ip != nil { | ||
| 1862 | return isDangerousRegistryIP(ip), nil | 1862 | return isDangerousRegistryIP(ip), nil | ||
| 1863 | } | 1863 | } | ||
| 1864 | - | ips, err := net.LookupIP(host) | 1864 | + | addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host) |
| 1865 | if err != nil { | 1865 | if err != nil { | ||
| 1866 | return false, err | 1866 | return false, err | ||
| 1867 | } | 1867 | } | ||
| 1868 | - | for _, ip := range ips { | 1868 | + | for _, addr := range addrs { |
| 1869 | - | if isDangerousRegistryIP(ip) { | 1869 | + | if isDangerousRegistryIP(addr.IP) { |
| 1870 | return true, nil | 1870 | return true, nil | ||
| 1871 | } | 1871 | } | ||
| 1872 | } | 1872 | } | ||
| @@ -1945,7 +1945,7 @@ func (s *Service) ensureImage(ctx context.Context, imageRef string) (string, err | |||||
| 1945 | 1945 | ||||
| 1946 | // Determine if this is an internal deploy and what reference to use for pulls. | 1946 | // Determine if this is an internal deploy and what reference to use for pulls. | ||
| 1947 | pullRef, isInternal := s.pullRefForDeploy(ctx, imageRef) | 1947 | pullRef, isInternal := s.pullRefForDeploy(ctx, imageRef) | ||
| 1948 | - | if err := s.validateImagePullRef(imageRef); err != nil { | 1948 | + | if err := s.validateImagePullRef(ctx, imageRef); err != nil { |
| 1949 | return "", err | 1949 | return "", err | ||
| 1950 | } | 1950 | } | ||
| 1951 | if pullRef != imageRef { | 1951 | if pullRef != imageRef { | ||
| @@ -2582,8 +2582,14 @@ func (s *Service) deployAttachedService(ctx context.Context, ownerDomain, servic | |||||
| 2582 | } | 2582 | } | ||
| 2583 | 2583 | ||||
| 2584 | exposedPorts := s.attachmentExposedPorts(ctx, actualImageRef) | 2584 | exposedPorts := s.attachmentExposedPorts(ctx, actualImageRef) | ||
| 2585 | - | volumes := s.attachmentVolumes(ctx, containerName, actualImageRef) | 2585 | + | volumes, err := s.attachmentVolumes(ctx, containerName, actualImageRef) |
| 2586 | - | envVars := s.attachmentEnv(ctx, containerName, actualImageRef) | 2586 | + | if err != nil { |
| 2587 | + | return err | |||
| 2588 | + | } | |||
| 2589 | + | envVars, err := s.attachmentEnv(ctx, containerName, actualImageRef) | |||
| 2590 | + | if err != nil { | |||
| 2591 | + | return err | |||
| 2592 | + | } | |||
| 2587 | envHash := hashEnvironment(envVars) | 2593 | envHash := hashEnvironment(envVars) | ||
| 2588 | 2594 | ||||
| 2589 | s.mu.RLock() | 2595 | s.mu.RLock() | ||
| @@ -2656,24 +2662,20 @@ func (s *Service) attachmentExposedPorts(ctx context.Context, imageRef string) [ | |||||
| 2656 | return exposedPorts | 2662 | return exposedPorts | ||
| 2657 | } | 2663 | } | ||
| 2658 | 2664 | ||||
| 2659 | - | func (s *Service) attachmentVolumes(ctx context.Context, containerName, imageRef string) map[string]string { | 2665 | + | func (s *Service) attachmentVolumes(ctx context.Context, containerName, imageRef string) (map[string]string, error) { |
| 2660 | - | log := zerowrap.FromCtx(ctx) | |||
| 2661 | volumes, err := s.setupVolumes(ctx, containerName, imageRef) | 2666 | volumes, err := s.setupVolumes(ctx, containerName, imageRef) | ||
| 2662 | if err != nil { | 2667 | if err != nil { | ||
| 2663 | - | log.WrapErr(err, "failed to setup volumes for attachment") | 2668 | + | return nil, fmt.Errorf("failed to setup volumes for attachment %s with image %s: %w", containerName, imageRef, err) |
| 2664 | - | return make(map[string]string) | |||
| 2665 | } | 2669 | } | ||
| 2666 | - | return volumes | 2670 | + | return volumes, nil |
| 2667 | } | 2671 | } | ||
| 2668 | 2672 | ||||
| 2669 | - | func (s *Service) attachmentEnv(ctx context.Context, containerName, imageRef string) []string { | 2673 | + | func (s *Service) attachmentEnv(ctx context.Context, containerName, imageRef string) ([]string, error) { |
| 2670 | - | log := zerowrap.FromCtx(ctx) | |||
| 2671 | envVars, err := s.loadEnvironment(ctx, nil, containerName, imageRef) | 2674 | envVars, err := s.loadEnvironment(ctx, nil, containerName, imageRef) | ||
| 2672 | if err != nil { | 2675 | if err != nil { | ||
| 2673 | - | log.WrapErr(err, "failed to load environment for attachment") | 2676 | + | return nil, fmt.Errorf("failed to load environment for attachment %s with image %s: %w", containerName, imageRef, err) |
| 2674 | - | return []string{} | |||
| 2675 | } | 2677 | } | ||
| 2676 | - | return envVars | 2678 | + | return envVars, nil |
| 2677 | } | 2679 | } | ||
| 2678 | 2680 | ||||
| 2679 | func (s *Service) attachmentEnvDrifted(ctx context.Context, existing *domain.Container, containerName, serviceImage string) (bool, error) { | 2681 | func (s *Service) attachmentEnvDrifted(ctx context.Context, existing *domain.Container, containerName, serviceImage string) (bool, error) { | ||
M internal/usecase/container/service_test.go +2 -2
| @@ -4235,7 +4235,7 @@ func TestService_BuildValidatedImageRef_RejectsExternalRegistryWhenAllowlistEmpt | |||
| 4235 | 4235 | require.Error(t, err) | |
| 4236 | 4236 | assert.Contains(t, err.Error(), "images.allowed_registries") | |
| 4237 | 4237 | ||
| 4238 | - | err = svc.validateImagePullRef("nginx:latest") | |
| 4238 | + | err = svc.validateImagePullRef(context.Background(), "nginx:latest") | |
| 4239 | 4239 | require.Error(t, err) | |
| 4240 | 4240 | assert.Contains(t, err.Error(), "docker.io") | |
| 4241 | 4241 | } | |
| @@ -4251,7 +4251,7 @@ func TestService_BuildValidatedImageRef_RequireDigestOnlyForExternalAllowedRegis | |||
| 4251 | 4251 | _, err := svc.buildValidatedImageRef("docker.io/library/nginx:latest") | |
| 4252 | 4252 | require.Error(t, err) | |
| 4253 | 4253 | ||
| 4254 | - | err = svc.validateImagePullRef("nginx:latest") | |
| 4254 | + | err = svc.validateImagePullRef(context.Background(), "nginx:latest") | |
| 4255 | 4255 | require.Error(t, err) | |
| 4256 | 4256 | ||
| 4257 | 4257 | _, err = svc.buildValidatedImageRef("docker.io/library/nginx@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") | |
| @@ -4235,7 +4235,7 @@ func TestService_BuildValidatedImageRef_RejectsExternalRegistryWhenAllowlistEmpt | |||||
| 4235 | require.Error(t, err) | 4235 | require.Error(t, err) | ||
| 4236 | assert.Contains(t, err.Error(), "images.allowed_registries") | 4236 | assert.Contains(t, err.Error(), "images.allowed_registries") | ||
| 4237 | 4237 | ||||
| 4238 | - | err = svc.validateImagePullRef("nginx:latest") | 4238 | + | err = svc.validateImagePullRef(context.Background(), "nginx:latest") |
| 4239 | require.Error(t, err) | 4239 | require.Error(t, err) | ||
| 4240 | assert.Contains(t, err.Error(), "docker.io") | 4240 | assert.Contains(t, err.Error(), "docker.io") | ||
| 4241 | } | 4241 | } | ||
| @@ -4251,7 +4251,7 @@ func TestService_BuildValidatedImageRef_RequireDigestOnlyForExternalAllowedRegis | |||||
| 4251 | _, err := svc.buildValidatedImageRef("docker.io/library/nginx:latest") | 4251 | _, err := svc.buildValidatedImageRef("docker.io/library/nginx:latest") | ||
| 4252 | require.Error(t, err) | 4252 | require.Error(t, err) | ||
| 4253 | 4253 | ||||
| 4254 | - | err = svc.validateImagePullRef("nginx:latest") | 4254 | + | err = svc.validateImagePullRef(context.Background(), "nginx:latest") |
| 4255 | require.Error(t, err) | 4255 | require.Error(t, err) | ||
| 4256 | 4256 | ||||
| 4257 | _, err = svc.buildValidatedImageRef("docker.io/library/nginx@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") | 4257 | _, err = svc.buildValidatedImageRef("docker.io/library/nginx@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") | ||