From 135407ba7c234f46fe4b30cfbaf29073ba09781d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 21 May 2026 19:13:10 -0500 Subject: [PATCH] Implemented a centralized secret-backed object-store helper --- docs/development.md | 2 + docs/internal/adapters.md | 1 + docs/internal/storage.md | 3 + internal/app/config_loader.go | 2 +- internal/app/object_store.go | 21 ++++ internal/app/object_store_test.go | 165 ++++++++++++++++++++++++++ internal/app/operator_helpers.go | 6 +- internal/app/operator_helpers_test.go | 44 +++++++ internal/app/remote_session_test.go | 58 +++++++++ internal/app/restore.go | 6 +- internal/app/runner.go | 4 +- 11 files changed, 301 insertions(+), 11 deletions(-) create mode 100644 internal/app/object_store.go create mode 100644 internal/app/object_store_test.go diff --git a/docs/development.md b/docs/development.md index af93974..c06d8d0 100644 --- a/docs/development.md +++ b/docs/development.md @@ -69,6 +69,8 @@ For design principles and invariants, see [docs/architecture.md](./architecture. 2. Add or update command tests (`TestExecute` and command-specific tests). 3. Update [docs/cli.md](./cli.md) and, if operator workflow changes, [docs/operations.md](./operations.md). +Remote-storage commands must obtain object storage through the app-level command object-store helper. Do not call `storage.NewObjectStoreFromConfig` directly from command handlers; the helper loads configured filesystem secrets before constructing the storage adapter. + ### Add or modify stages/adapters 1. Implement stage behavior in `internal/stage` with clear input/output boundaries. diff --git a/docs/internal/adapters.md b/docs/internal/adapters.md index 797c3eb..2874fb0 100644 --- a/docs/internal/adapters.md +++ b/docs/internal/adapters.md @@ -47,6 +47,7 @@ Current execution usage: Default construction in app runner: - Auto-constructed when not injected: WhisperX HTTP client, Seriatim subprocess runner, Audita subprocess runner, Scriptorium subprocess runner, object store (only when needed), and `notify.NoopSender`. +- Object-store construction goes through app command orchestration so configured filesystem secrets are loaded before the storage adapter is initialized. - Callers can inject test/fake implementations through `app.RunOptions.Env`. ## State and manifest behavior diff --git a/docs/internal/storage.md b/docs/internal/storage.md index ceafa87..9e8e4e2 100644 --- a/docs/internal/storage.md +++ b/docs/internal/storage.md @@ -6,6 +6,7 @@ Document Narratio's remote storage backend contracts and implementations under ` ## Inputs and outputs Inputs: - Resolved storage config (`pipeline.storage.*`). +- Already-loaded environment variables for configured S3 credentials. - Bucket-relative object keys and local file paths from app/stage orchestration. Outputs: @@ -22,6 +23,7 @@ Does not own: - Session/run prefix semantics. - Archive commit order semantics. - Manifest updates. +- Filesystem secret loading from `pipeline.secrets.env_dir`. ## Config fields used - `pipeline.storage.backend` @@ -55,6 +57,7 @@ Implementations: ## Failure behavior - `NewObjectStoreFromConfig` fails when no remote backend is configured or required S3 config is missing. - `S3Backend` constructor fails when required bucket is missing or AWS client setup fails. +- App command orchestration loads configured filesystem secrets before calling the object-store factory. - CRUD operations return contextual errors (including not-found behavior via `Exists`). - Key normalization is applied before operations (`\\` to `/`, leading slash trimmed). - Remote session loading uses `List` to find the exact `session.yml` key and `Download` to materialize it to a local temp file. diff --git a/internal/app/config_loader.go b/internal/app/config_loader.go index f2ae819..a7f1396 100644 --- a/internal/app/config_loader.go +++ b/internal/app/config_loader.go @@ -56,7 +56,7 @@ func loadCommandConfig(ctx context.Context, pipelineFlag, campaignFlag, sessionF PipelinePath: resolvedPipelinePath, CampaignPath: resolvedCampaignPath, } - store, err := newObjectStoreFromConfigFn(ctx, partialCfg) + store, err := newCommandObjectStore(ctx, partialCfg, nil) if err != nil { return nil, missingSessionConfigError(discoveredSession.Searched, fmt.Sprintf("remote session %q unavailable: %v", remoteKey, err)) } diff --git a/internal/app/object_store.go b/internal/app/object_store.go new file mode 100644 index 0000000..0c23b3a --- /dev/null +++ b/internal/app/object_store.go @@ -0,0 +1,21 @@ +package app + +import ( + "context" + "fmt" + "log/slog" + + "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" + "gitea.maximumdirect.net/eric/narratio/internal/config" +) + +func newCommandObjectStore(ctx context.Context, cfg *config.Config, logger *slog.Logger) (storage.ObjectStore, error) { + if _, err := loadSecretsFromConfig(cfg, logger); err != nil { + return nil, fmt.Errorf("load secrets from files: %w", err) + } + store, err := newObjectStoreFromConfigFn(ctx, cfg) + if err != nil { + return nil, fmt.Errorf("initialize object store backend: %w", err) + } + return store, nil +} diff --git a/internal/app/object_store_test.go b/internal/app/object_store_test.go new file mode 100644 index 0000000..5a845d9 --- /dev/null +++ b/internal/app/object_store_test.go @@ -0,0 +1,165 @@ +package app + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" + "gitea.maximumdirect.net/eric/narratio/internal/config" +) + +func TestNewCommandObjectStoreLoadsSecretsBeforeFactory(t *testing.T) { + accessKeyEnv := "NARRATIO_TEST_COMMAND_STORE_KEY_ID" + secretKeyEnv := "NARRATIO_TEST_COMMAND_STORE_SECRET" + restoreEnvAfterTest(t, accessKeyEnv, secretKeyEnv) + + secretsDir := t.TempDir() + mustWriteSecretFile(t, filepath.Join(secretsDir, accessKeyEnv), "loaded-key-id\n") + mustWriteSecretFile(t, filepath.Join(secretsDir, secretKeyEnv), "loaded-secret\n") + + cfg := commandObjectStoreTestConfig(secretsDir) + fake := &storage.FakeBackend{} + called := false + origStoreFn := newObjectStoreFromConfigFn + newObjectStoreFromConfigFn = func(context.Context, *config.Config) (storage.ObjectStore, error) { + called = true + if got := os.Getenv(accessKeyEnv); got != "loaded-key-id" { + return nil, errors.New("access key was not loaded before object store init") + } + if got := os.Getenv(secretKeyEnv); got != "loaded-secret" { + return nil, errors.New("secret key was not loaded before object store init") + } + return fake, nil + } + t.Cleanup(func() { + newObjectStoreFromConfigFn = origStoreFn + }) + + store, err := newCommandObjectStore(context.Background(), cfg, nil) + if err != nil { + t.Fatalf("newCommandObjectStore() error = %v", err) + } + if store != fake { + t.Fatalf("store = %#v, want fake backend", store) + } + if !called { + t.Fatal("object store factory was not called") + } +} + +func TestNewCommandObjectStorePreservesExistingEnv(t *testing.T) { + accessKeyEnv := "NARRATIO_TEST_COMMAND_STORE_EXISTING_KEY_ID" + secretKeyEnv := "NARRATIO_TEST_COMMAND_STORE_EXISTING_SECRET" + t.Setenv(accessKeyEnv, "existing-key-id") + t.Setenv(secretKeyEnv, "existing-secret") + + secretsDir := t.TempDir() + mustWriteSecretFile(t, filepath.Join(secretsDir, accessKeyEnv), "file-key-id\n") + mustWriteSecretFile(t, filepath.Join(secretsDir, secretKeyEnv), "file-secret\n") + + cfg := commandObjectStoreTestConfig(secretsDir) + origStoreFn := newObjectStoreFromConfigFn + newObjectStoreFromConfigFn = func(context.Context, *config.Config) (storage.ObjectStore, error) { + if got := os.Getenv(accessKeyEnv); got != "existing-key-id" { + return nil, errors.New("existing access key was overwritten") + } + if got := os.Getenv(secretKeyEnv); got != "existing-secret" { + return nil, errors.New("existing secret key was overwritten") + } + return &storage.FakeBackend{}, nil + } + t.Cleanup(func() { + newObjectStoreFromConfigFn = origStoreFn + }) + + if _, err := newCommandObjectStore(context.Background(), cfg, nil); err != nil { + t.Fatalf("newCommandObjectStore() error = %v", err) + } +} + +func TestNewCommandObjectStoreSecretErrorStopsFactory(t *testing.T) { + cfg := commandObjectStoreTestConfig(filepath.Join(t.TempDir(), "missing")) + called := false + origStoreFn := newObjectStoreFromConfigFn + newObjectStoreFromConfigFn = func(context.Context, *config.Config) (storage.ObjectStore, error) { + called = true + return &storage.FakeBackend{}, nil + } + t.Cleanup(func() { + newObjectStoreFromConfigFn = origStoreFn + }) + + _, err := newCommandObjectStore(context.Background(), cfg, nil) + if err == nil { + t.Fatal("expected error, got nil") + } + if called { + t.Fatal("object store factory was called after secret load failure") + } + if !strings.Contains(err.Error(), "load secrets from files") { + t.Fatalf("error = %q, want secret loading context", err.Error()) + } +} + +func TestNewCommandObjectStoreFactoryErrorIsContextual(t *testing.T) { + cfg := commandObjectStoreTestConfig("") + origStoreFn := newObjectStoreFromConfigFn + newObjectStoreFromConfigFn = func(context.Context, *config.Config) (storage.ObjectStore, error) { + return nil, errors.New("factory boom") + } + t.Cleanup(func() { + newObjectStoreFromConfigFn = origStoreFn + }) + + _, err := newCommandObjectStore(context.Background(), cfg, nil) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "initialize object store backend") || !strings.Contains(err.Error(), "factory boom") { + t.Fatalf("error = %q, want factory context", err.Error()) + } +} + +func commandObjectStoreTestConfig(secretsDir string) *config.Config { + cfg := &config.Config{ + Pipeline: &config.PipelineConfig{ + Storage: config.StorageConfig{ + Backend: "s3", + S3: &config.StorageS3Config{ + Bucket: "test-bucket", + AccessKeyIDEnv: "NARRATIO_TEST_COMMAND_STORE_KEY_ID", + SecretKeyEnv: "NARRATIO_TEST_COMMAND_STORE_SECRET", + }, + }, + }, + } + if strings.TrimSpace(secretsDir) != "" { + cfg.Pipeline.Secrets = &config.SecretsConfig{EnvDir: secretsDir} + } + return cfg +} + +func restoreEnvAfterTest(t *testing.T, names ...string) { + t.Helper() + originals := make(map[string]string, len(names)) + present := make(map[string]bool, len(names)) + for _, name := range names { + value, ok := os.LookupEnv(name) + originals[name] = value + present[name] = ok + _ = os.Unsetenv(name) + } + t.Cleanup(func() { + for _, name := range names { + if present[name] { + _ = os.Setenv(name, originals[name]) + } else { + _ = os.Unsetenv(name) + } + } + }) +} diff --git a/internal/app/operator_helpers.go b/internal/app/operator_helpers.go index 369cd35..879e213 100644 --- a/internal/app/operator_helpers.go +++ b/internal/app/operator_helpers.go @@ -316,7 +316,7 @@ func SessionInit(ctx context.Context, args []string, out io.Writer) error { return err } - store, err := newObjectStoreFromConfigFn(ctx, cfg) + store, err := newCommandObjectStore(ctx, cfg, nil) if err != nil { return fmt.Errorf("session init: %w", err) } @@ -488,7 +488,7 @@ func loadHelperContext(ctx context.Context, flags commonConfigFlags, needStore b } var store storage.ObjectStore if needStore { - store, err = newObjectStoreFromConfigFn(ctx, cfg) + store, err = newCommandObjectStore(ctx, cfg, nil) if err != nil { return nil, nil, nil, nil, err } @@ -511,7 +511,7 @@ func objectStoreIfConfigured(ctx context.Context, cfg *config.Config) (storage.O if cfg == nil || cfg.Pipeline == nil || cfg.Pipeline.Storage.S3 == nil || strings.TrimSpace(cfg.Pipeline.Storage.S3.Bucket) == "" { return nil, nil } - store, err := newObjectStoreFromConfigFn(ctx, cfg) + store, err := newCommandObjectStore(ctx, cfg, nil) if err != nil { return nil, err } diff --git a/internal/app/operator_helpers_test.go b/internal/app/operator_helpers_test.go index 767d2a6..6b8087d 100644 --- a/internal/app/operator_helpers_test.go +++ b/internal/app/operator_helpers_test.go @@ -3,6 +3,7 @@ package app import ( "bytes" "context" + "fmt" "os" "path/filepath" "strings" @@ -47,6 +48,49 @@ func TestExecuteSessionInitRemoteWritesCanonicalSessionConfig(t *testing.T) { } } +func TestExecuteSessionValidateLoadsSecretsBeforeObjectStoreInit(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot) + accessKeyEnv := "NARRATIO_TEST_VALIDATE_OBJECT_KEY_ID" + secretKeyEnv := "NARRATIO_TEST_VALIDATE_OBJECT_SECRET" + restoreEnvAfterTest(t, accessKeyEnv, secretKeyEnv) + secretsDir := t.TempDir() + mustWriteTestFile(t, filepath.Join(secretsDir, accessKeyEnv), "test-key-id\n") + mustWriteTestFile(t, filepath.Join(secretsDir, secretKeyEnv), "test-secret\n") + addSecretsToPipelineConfig(t, pipelinePath, secretsDir, accessKeyEnv, secretKeyEnv) + if err := os.WriteFile(sessionPath, []byte(`session_id: 2026-05-03 +inputs: + audio_s3: + prefix: audio/ +`), 0o644); err != nil { + t.Fatalf("write session: %v", err) + } + + fake := &storage.FakeBackend{} + audioKey := artifacts.S3PromotedArtifactKey(artifacts.S3AudioPrefix(artifacts.S3SessionPrefix("dnd", "sample-campaign", "2026-05-03"), "audio/"), "alice.flac") + fake.SeedObject(storage.FakeObject{Key: audioKey, Data: []byte("audio")}) + origStoreFn := newObjectStoreFromConfigFn + newObjectStoreFromConfigFn = func(context.Context, *config.Config) (storage.ObjectStore, error) { + if os.Getenv(accessKeyEnv) != "test-key-id" || os.Getenv(secretKeyEnv) != "test-secret" { + return nil, fmt.Errorf("secrets were not loaded before object store init") + } + return fake, nil + } + t.Cleanup(func() { + newObjectStoreFromConfigFn = origStoreFn + }) + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute([]string{"session", "validate", "--config", pipelinePath, "--campaign", campaignPath, "--session", sessionPath}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit code = %d, want 0; stdout=%q stderr=%q", code, stdout.String(), stderr.String()) + } + if !strings.Contains(stdout.String(), "OK audio") { + t.Fatalf("stdout = %q, want OK audio", stdout.String()) + } +} + func TestExecuteLockAndUnlockUseRemoteLockStore(t *testing.T) { workspaceRoot := t.TempDir() pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot) diff --git a/internal/app/remote_session_test.go b/internal/app/remote_session_test.go index fcca606..d08e69c 100644 --- a/internal/app/remote_session_test.go +++ b/internal/app/remote_session_test.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "errors" + "fmt" + "os" "path/filepath" "strings" "testing" @@ -42,6 +44,45 @@ inputs: } } +func TestExecuteRemoteSessionFallbackLoadsSecretsBeforeObjectStoreInit(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, campaignPath, _ := writeValidConfigFiles(t, workspaceRoot) + accessKeyEnv := "NARRATIO_TEST_REMOTE_SESSION_KEY_ID" + secretKeyEnv := "NARRATIO_TEST_REMOTE_SESSION_SECRET" + restoreEnvAfterTest(t, accessKeyEnv, secretKeyEnv) + secretsDir := t.TempDir() + mustWriteTestFile(t, filepath.Join(secretsDir, accessKeyEnv), "remote-session-key-id\n") + mustWriteTestFile(t, filepath.Join(secretsDir, secretKeyEnv), "remote-session-secret\n") + addSecretsToPipelineConfig(t, pipelinePath, secretsDir, accessKeyEnv, secretKeyEnv) + + fake := &storage.FakeBackend{} + seedRemoteSessionConfig(t, fake, "2026-05-03", `session_id: "{{ session_id }}" +inputs: + audio_s3: + prefix: audio/ +`) + origStoreFn := newObjectStoreFromConfigFn + origSessionDefaults := append([]string(nil), config.DefaultSessionConfigSearchPaths...) + config.DefaultSessionConfigSearchPaths = []string{filepath.Join(t.TempDir(), "session.yml")} + newObjectStoreFromConfigFn = func(context.Context, *config.Config) (storage.ObjectStore, error) { + if os.Getenv(accessKeyEnv) != "remote-session-key-id" || os.Getenv(secretKeyEnv) != "remote-session-secret" { + return nil, fmt.Errorf("secrets were not loaded before remote session object store init") + } + return fake, nil + } + t.Cleanup(func() { + newObjectStoreFromConfigFn = origStoreFn + config.DefaultSessionConfigSearchPaths = origSessionDefaults + }) + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute([]string{"plan", "--config", pipelinePath, "--campaign", campaignPath, "--session-id", "2026-05-03"}, &stdout, &stderr) + if code != 0 { + t.Fatalf("exit code = %d, want 0; stdout=%q stderr=%q", code, stdout.String(), stderr.String()) + } +} + func TestExecuteExplicitLocalSessionPrecedenceSkipsRemote(t *testing.T) { workspaceRoot := t.TempDir() pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot) @@ -192,3 +233,20 @@ func seedRemoteSessionConfig(t *testing.T, fake *storage.FakeBackend, sessionID, }) return remoteKey } + +func addSecretsToPipelineConfig(t *testing.T, pipelinePath, secretsDir, accessKeyEnv, secretKeyEnv string) { + t.Helper() + pipelineData, err := os.ReadFile(pipelinePath) + if err != nil { + t.Fatalf("read pipeline: %v", err) + } + pipelineYAML := strings.Replace( + string(pipelineData), + "storage:\n backend: s3\n s3:\n bucket: test-bucket\n", + "storage:\n backend: s3\n s3:\n bucket: test-bucket\n access_key_id_env: "+accessKeyEnv+"\n secret_access_key_env: "+secretKeyEnv+"\nsecrets:\n env_dir: "+secretsDir+"\n", + 1, + ) + if err := os.WriteFile(pipelinePath, []byte(pipelineYAML), 0o644); err != nil { + t.Fatalf("write pipeline: %v", err) + } +} diff --git a/internal/app/restore.go b/internal/app/restore.go index 402d7ed..8408b91 100644 --- a/internal/app/restore.go +++ b/internal/app/restore.go @@ -67,11 +67,7 @@ func Restore(ctx context.Context, args []string, out io.Writer) error { if err := config.Validate(cfg); err != nil { return fmt.Errorf("restore: %w", err) } - if _, err := loadSecretsFromConfig(cfg, logging.NewLogger(os.Stderr, slog.LevelInfo)); err != nil { - return fmt.Errorf("restore: %w", err) - } - - objectStore, err := newObjectStoreFromConfigFn(ctx, cfg) + objectStore, err := newCommandObjectStore(ctx, cfg, logging.NewLogger(os.Stderr, slog.LevelInfo)) if err != nil { return fmt.Errorf("restore: %w", err) } diff --git a/internal/app/runner.go b/internal/app/runner.go index 805ffa1..bbd270f 100644 --- a/internal/app/runner.go +++ b/internal/app/runner.go @@ -87,9 +87,9 @@ func executeStages(ctx context.Context, cfg *config.Config, stages []stage.Stage env.Storage = &storage.NoopBackend{} } if env.ObjectStore == nil && needsObjectStoreForRun(env.Config, stages) { - objectStore, err := newObjectStoreFromConfigFn(ctx, env.Config) + objectStore, err := newCommandObjectStore(ctx, env.Config, nil) if err != nil { - return nil, fmt.Errorf("initialize object store backend: %w", err) + return nil, err } env.ObjectStore = objectStore }