Harden API key file loading

This commit is contained in:
2026-08-10 18:24:29 +00:00
parent 363313d99c
commit 99b2e1cd81
13 changed files with 370 additions and 32 deletions

View File

@@ -94,7 +94,13 @@ inputs:
- Do not place raw secrets in YAML.
- Use env var names in config (for example `pipeline.audita.llm_api_key_env`).
- Optionally load env files from `pipeline.secrets.env_dir`.
- Optionally load credential files from `pipeline.secrets.env_dir`. Each valid
environment-variable filename supplies one value; trailing CR/LF is removed.
- An existing process environment value takes precedence over a credential file.
- Credential directories and files must not be symlinks and must be regular,
bounded files (at most 8 KiB per value). On POSIX, provision the directory
with no group/other access (normally `0700`) and files with no group/other
access (normally `0600`).
- Commands that need storage/auth load filesystem secrets before constructing adapters.
## Publish Configuration Summary

View File

@@ -31,6 +31,14 @@ tree through those directory handles. It rejects root deletion and any symlink
encountered in the target path or tree; repeated removal of a missing target is
successful. Command and post-publish policy remains owned by `internal/app`.
## Confined Reads
`ReadRegularFileUnderRoot` is the no-follow, bounded read primitive for a
caller-selected root and relative file path. It verifies the root and every
ancestor through directory handles, admits only a stable regular-file handle,
and lets the caller enforce its own byte limit and access policy. Credential
mode policy and environment precedence remain owned by `internal/app`.
## Replacement Contract
`ReplaceFileAtomic` requires an existing destination directory. It creates a

View File

@@ -303,6 +303,9 @@ Windows access control.
API keys are credentials, not ordinary workspace data. Store them outside the
shared workspace or in a separately restricted credential location; ordinary
workspace group access must never be treated as authorization to read keys.
On POSIX, provision a credential directory as `0700` and credential files as
`0600`; Narratio rejects group- or other-readable configured credential paths.
On Windows, restrict the directory and files with ACLs to the credential owner.
## Cleanup

View File

@@ -21,7 +21,7 @@ All stages are pending when this plan is created.
| 3 | Consolidate crash-durable atomic file replacement | RSK-002, DUP-001, DUP-005 | Completed |
| 4 | Add confined destination and download/install capabilities | COR-003, DUP-003, TST-003 | Completed |
| 5 | Confine recursive cleanup and replace sentinel locks | RSK-003 | Completed |
| 6 | Harden API-key file acquisition | RSK-010 | Pending |
| 6 | Harden API-key file acquisition | RSK-010 | Completed |
| 7 | Bound and verify external result acquisition | RSK-013, TST-007 | Pending |
| 8 | Terminate owned subprocess trees | RSK-011 | Pending |
| 9 | Redact and cap subprocess diagnostics | RSK-012 | Pending |

View File

@@ -189,7 +189,7 @@ func TestExecuteRunStagePolishLoadsCredentialFromSecretsDir(t *testing.T) {
configDir := t.TempDir()
sessionID := "2026-05-03"
secretsDir := filepath.Join(configDir, "secrets")
if err := os.MkdirAll(secretsDir, 0o755); err != nil {
if err := os.MkdirAll(secretsDir, secretDirectoryPrivateMode); err != nil {
t.Fatalf("MkdirAll(%q): %v", secretsDir, err)
}
if err := os.WriteFile(filepath.Join(secretsDir, "OPENROUTER_API_KEY"), []byte("from-secret-file\n"), 0o600); err != nil {

View File

@@ -189,8 +189,8 @@ func TestExecuteSessionInitRemoteLoadsSecretsBeforeObjectStoreInit(t *testing.T)
secretKeyEnv := "NARRATIO_TEST_SESSION_INIT_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")
mustWriteSecretFile(t, filepath.Join(secretsDir, accessKeyEnv), "test-key-id\n")
mustWriteSecretFile(t, filepath.Join(secretsDir, secretKeyEnv), "test-secret\n")
addSecretsToPipelineConfig(t, pipelinePath, secretsDir, accessKeyEnv, secretKeyEnv)
fake := &storage.FakeBackend{}
@@ -417,8 +417,8 @@ func TestExecuteSessionValidateLoadsSecretsBeforeObjectStoreInit(t *testing.T) {
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")
mustWriteSecretFile(t, filepath.Join(secretsDir, accessKeyEnv), "test-key-id\n")
mustWriteSecretFile(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:

View File

@@ -51,8 +51,8 @@ func TestExecuteRemoteSessionFallbackLoadsSecretsBeforeObjectStoreInit(t *testin
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")
mustWriteSecretFile(t, filepath.Join(secretsDir, accessKeyEnv), "remote-session-key-id\n")
mustWriteSecretFile(t, filepath.Join(secretsDir, secretKeyEnv), "remote-session-secret\n")
addSecretsToPipelineConfig(t, pipelinePath, secretsDir, accessKeyEnv, secretKeyEnv)
fake := &storage.FakeBackend{}

View File

@@ -213,8 +213,8 @@ func TestExecuteRestoreLoadsSecretsBeforeObjectStoreInit(t *testing.T) {
workspaceRoot := t.TempDir()
pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot)
secretsDir := filepath.Join(t.TempDir(), "secrets")
mustWriteTestFile(t, filepath.Join(secretsDir, accessKeyEnv), "test-access-key-id\n")
mustWriteTestFile(t, filepath.Join(secretsDir, secretKeyEnv), "test-secret-key\n")
mustWriteSecretFile(t, filepath.Join(secretsDir, accessKeyEnv), "test-access-key-id\n")
mustWriteSecretFile(t, filepath.Join(secretsDir, secretKeyEnv), "test-secret-key\n")
f, err := os.OpenFile(pipelinePath, os.O_APPEND|os.O_WRONLY, 0)
if err != nil {
t.Fatalf("open pipeline config for append: %v", err)

View File

@@ -9,9 +9,18 @@ import (
"strings"
"gitea.maximumdirect.net/eric/narratio/internal/config"
"gitea.maximumdirect.net/eric/narratio/internal/fileops"
)
var envVarNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
var readSecretsDirectoryEntries = os.ReadDir
// APIKeyFileByteLimit bounds one credential value read from the configured
// secrets directory. API keys are expected to be short single-line values.
const APIKeyFileByteLimit int64 = 8 * 1024
const secretDirectoryPrivateMode os.FileMode = 0o700
const secretFilePrivateMode os.FileMode = 0o600
type secretsLoadStats struct {
Dir string
@@ -40,9 +49,13 @@ func loadSecretsFromConfig(cfg *config.Config, logger *slog.Logger) (*secretsLoa
}
resolvedDir = filepath.Clean(resolvedDir)
entries, err := os.ReadDir(resolvedDir)
if err := fileops.ValidateConfinedDirectory(resolvedDir, validateSecretDirectory); err != nil {
return nil, fmt.Errorf("API-key loader: validate secrets env_dir: %w", err)
}
entries, err := readSecretsDirectoryEntries(resolvedDir)
if err != nil {
return nil, fmt.Errorf("read secrets env_dir %q: %w", resolvedDir, err)
return nil, fmt.Errorf("API-key loader: read secrets env_dir %q: %w", resolvedDir, err)
}
stats := &secretsLoadStats{Dir: resolvedDir}
@@ -52,24 +65,16 @@ func loadSecretsFromConfig(cfg *config.Config, logger *slog.Logger) (*secretsLoa
stats.Skipped++
continue
}
if entry.IsDir() {
stats.Skipped++
continue
}
secretPath := filepath.Join(resolvedDir, name)
bytes, err := os.ReadFile(secretPath)
if err != nil {
return nil, fmt.Errorf("read secret file %q: %w", secretPath, err)
}
value := strings.TrimRight(string(bytes), "\r\n")
if _, exists := os.LookupEnv(name); exists {
stats.PreservedExisting++
continue
}
value, err := readAPIKeyFile(resolvedDir, name)
if err != nil {
return nil, err
}
if err := os.Setenv(name, value); err != nil {
return nil, fmt.Errorf("set environment variable %q from %q: %w", name, secretPath, err)
return nil, fmt.Errorf("API-key loader: set environment variable %q: %w", name, err)
}
stats.Loaded++
}
@@ -86,3 +91,31 @@ func loadSecretsFromConfig(cfg *config.Config, logger *slog.Logger) (*secretsLoa
return stats, nil
}
func validateSecretDirectory(info os.FileInfo) error {
if !info.IsDir() {
return fmt.Errorf("secrets env_dir is not a directory")
}
return validateSecretDirectoryPrivacy(info)
}
func validateSecretFile(info os.FileInfo) error {
if !info.Mode().IsRegular() {
return fmt.Errorf("secret entry is not a regular file")
}
return validateSecretFilePrivacy(info)
}
func readAPIKeyFile(directory, name string) (string, error) {
content, err := fileops.ReadRegularFileUnderRoot(
directory,
name,
APIKeyFileByteLimit,
validateSecretDirectory,
validateSecretFile,
)
if err != nil {
return "", fmt.Errorf("API-key loader: read secret file %q with %d-byte limit: %w", name, APIKeyFileByteLimit, err)
}
return strings.TrimRight(string(content), "\r\n"), nil
}

View File

@@ -0,0 +1,22 @@
//go:build !windows
package app
import (
"fmt"
"os"
)
func validateSecretDirectoryPrivacy(info os.FileInfo) error {
if info.Mode().Perm()&^secretDirectoryPrivateMode != 0 {
return fmt.Errorf("secret directory mode %04o grants group or other access; require %04o", info.Mode().Perm(), secretDirectoryPrivateMode)
}
return nil
}
func validateSecretFilePrivacy(info os.FileInfo) error {
if info.Mode().Perm()&^secretFilePrivateMode != 0 {
return fmt.Errorf("secret file mode %04o grants group or other access; require %04o", info.Mode().Perm(), secretFilePrivateMode)
}
return nil
}

View File

@@ -12,6 +12,8 @@ import (
func TestLoadSecretsFromConfigLoadsValidFiles(t *testing.T) {
dir := t.TempDir()
unsetSecretEnvironment(t, "NARRATIO_TEST_SECRET_A")
unsetSecretEnvironment(t, "NARRATIO_TEST_SECRET_B")
mustWriteSecretFile(t, filepath.Join(dir, "NARRATIO_TEST_SECRET_A"), "value-1\n")
mustWriteSecretFile(t, filepath.Join(dir, "NARRATIO_TEST_SECRET_B"), "value-2\r\n")
mustWriteSecretFile(t, filepath.Join(dir, "not-valid-name.txt"), "ignored")
@@ -73,7 +75,7 @@ func TestLoadSecretsFromConfigPreservesExistingEnv(t *testing.T) {
func TestLoadSecretsFromConfigRelativeDirUsesCWD(t *testing.T) {
cwd := t.TempDir()
secretsDir := filepath.Join(cwd, "secrets")
if err := os.MkdirAll(secretsDir, 0o755); err != nil {
if err := os.MkdirAll(secretsDir, secretDirectoryPrivateMode); err != nil {
t.Fatalf("MkdirAll(%q): %v", secretsDir, err)
}
mustWriteSecretFile(t, filepath.Join(secretsDir, "OBJECT_STORAGE_KEY_ID"), "id-123\n")
@@ -113,8 +115,8 @@ func TestLoadSecretsFromConfigMissingDirFails(t *testing.T) {
if err == nil {
t.Fatal("expected error, got nil")
}
if !strings.Contains(err.Error(), "read secrets env_dir") {
t.Fatalf("error = %q, want read-dir context", err.Error())
if !strings.Contains(err.Error(), "API-key loader") {
t.Fatalf("error = %q, want API-key loader context", err.Error())
}
}
@@ -124,6 +126,7 @@ func TestLoadSecretsFromConfigUnreadableValidEntryFails(t *testing.T) {
}
dir := t.TempDir()
ensureSecretDirectory(t, dir)
broken := filepath.Join(dir, "OPENROUTER_API_KEY")
if err := os.Symlink(filepath.Join(dir, "does-not-exist"), broken); err != nil {
t.Fatalf("Symlink(%q): %v", broken, err)
@@ -139,17 +142,159 @@ func TestLoadSecretsFromConfigUnreadableValidEntryFails(t *testing.T) {
if err == nil {
t.Fatal("expected error, got nil")
}
if !strings.Contains(err.Error(), "read secret file") {
t.Fatalf("error = %q, want read secret file context", err.Error())
if !strings.Contains(err.Error(), "API-key loader") {
t.Fatalf("error = %q, want API-key loader context", err.Error())
}
}
func TestLoadSecretsFromConfigRejectsUnsafeModes(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("Windows access control is verified with ACLs")
}
for _, tc := range []struct {
name string
directory os.FileMode
file os.FileMode
wantSecret string
}{
{name: "directory", directory: 0o750, file: secretFilePrivateMode, wantSecret: "directory-secret"},
{name: "file", directory: secretDirectoryPrivateMode, file: 0o640, wantSecret: "file-secret"},
} {
t.Run(tc.name, func(t *testing.T) {
dir := t.TempDir()
unsetSecretEnvironment(t, "OPENROUTER_API_KEY")
path := filepath.Join(dir, "OPENROUTER_API_KEY")
mustWriteSecretFile(t, path, tc.wantSecret)
if err := os.Chmod(dir, tc.directory); err != nil {
t.Fatalf("Chmod(directory) error = %v", err)
}
if err := os.Chmod(path, tc.file); err != nil {
t.Fatalf("Chmod(file) error = %v", err)
}
_, err := loadSecretsFromConfig(secretConfig(dir), nil)
if err == nil || !strings.Contains(err.Error(), "API-key loader") {
t.Fatalf("loadSecretsFromConfig() error = %v, want API-key loader failure", err)
}
if strings.Contains(err.Error(), tc.wantSecret) {
t.Fatalf("error leaked secret content: %q", err)
}
})
}
}
func TestLoadSecretsFromConfigRejectsNonRegularAndOversizedEntries(t *testing.T) {
dir := t.TempDir()
ensureSecretDirectory(t, dir)
unsetSecretEnvironment(t, "OPENROUTER_API_KEY")
if err := os.Mkdir(filepath.Join(dir, "OPENROUTER_API_KEY"), secretDirectoryPrivateMode); err != nil {
t.Fatalf("Mkdir(non-regular entry) error = %v", err)
}
if _, err := loadSecretsFromConfig(secretConfig(dir), nil); err == nil || !strings.Contains(err.Error(), "not a regular file") {
t.Fatalf("loadSecretsFromConfig(non-regular) error = %v, want regular-file failure", err)
}
if err := os.Remove(filepath.Join(dir, "OPENROUTER_API_KEY")); err != nil {
t.Fatalf("Remove(non-regular entry) error = %v", err)
}
overLimit := strings.Repeat("credential-value-", int(APIKeyFileByteLimit/17)+1)
mustWriteSecretFile(t, filepath.Join(dir, "OPENROUTER_API_KEY"), overLimit)
_, err := loadSecretsFromConfig(secretConfig(dir), nil)
if err == nil || !strings.Contains(err.Error(), "API-key loader") || !strings.Contains(err.Error(), "8192-byte limit") {
t.Fatalf("loadSecretsFromConfig(over-limit) error = %v, want bounded API-key loader failure", err)
}
if strings.Contains(err.Error(), overLimit[:32]) {
t.Fatalf("error leaked secret content: %q", err)
}
}
func TestLoadSecretsFromConfigRejectsSymlinkAndAncestorReplacement(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("symlink behavior differs on windows")
}
dir := t.TempDir()
outside := t.TempDir()
ensureSecretDirectory(t, dir)
unsetSecretEnvironment(t, "OPENROUTER_API_KEY")
outsideValue := "outside-secret-value"
mustWriteSecretFile(t, filepath.Join(outside, "OPENROUTER_API_KEY"), outsideValue)
if err := os.Symlink(filepath.Join(outside, "OPENROUTER_API_KEY"), filepath.Join(dir, "OPENROUTER_API_KEY")); err != nil {
t.Fatalf("Symlink(leaf) error = %v", err)
}
_, err := loadSecretsFromConfig(secretConfig(dir), nil)
if err == nil || !strings.Contains(err.Error(), "API-key loader") {
t.Fatalf("loadSecretsFromConfig(symlink) error = %v, want failure", err)
}
if strings.Contains(err.Error(), outsideValue) {
t.Fatalf("error leaked secret content: %q", err)
}
if err := os.Remove(filepath.Join(dir, "OPENROUTER_API_KEY")); err != nil {
t.Fatalf("Remove(leaf symlink) error = %v", err)
}
mustWriteSecretFile(t, filepath.Join(dir, "OPENROUTER_API_KEY"), "inside-secret")
original := readSecretsDirectoryEntries
readSecretsDirectoryEntries = func(path string) ([]os.DirEntry, error) {
entries, err := original(path)
if err != nil {
return nil, err
}
if err := os.Remove(filepath.Join(dir, "OPENROUTER_API_KEY")); err != nil {
return nil, err
}
if err := os.Remove(dir); err != nil {
return nil, err
}
if err := os.Symlink(outside, dir); err != nil {
return nil, err
}
return entries, nil
}
t.Cleanup(func() { readSecretsDirectoryEntries = original })
_, err = loadSecretsFromConfig(secretConfig(dir), nil)
if err == nil || !strings.Contains(err.Error(), "API-key loader") {
t.Fatalf("loadSecretsFromConfig(ancestor replacement) error = %v, want failure", err)
}
if _, exists := os.LookupEnv("OPENROUTER_API_KEY"); exists {
t.Fatal("ancestor replacement loaded a secret value")
}
}
func mustWriteSecretFile(t *testing.T, path, contents string) {
t.Helper()
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
if err := os.MkdirAll(filepath.Dir(path), secretDirectoryPrivateMode); err != nil {
t.Fatalf("MkdirAll(%q): %v", path, err)
}
if err := os.Chmod(filepath.Dir(path), secretDirectoryPrivateMode); err != nil {
t.Fatalf("Chmod(%q): %v", filepath.Dir(path), err)
}
if err := os.WriteFile(path, []byte(contents), 0o600); err != nil {
t.Fatalf("WriteFile(%q): %v", path, err)
}
}
func secretConfig(dir string) *config.Config {
return &config.Config{Pipeline: &config.PipelineConfig{Secrets: &config.SecretsConfig{EnvDir: dir}}}
}
func unsetSecretEnvironment(t *testing.T, name string) {
t.Helper()
previous, existed := os.LookupEnv(name)
if err := os.Unsetenv(name); err != nil {
t.Fatalf("Unsetenv(%q): %v", name, err)
}
t.Cleanup(func() {
if existed {
_ = os.Setenv(name, previous)
return
}
_ = os.Unsetenv(name)
})
}
func ensureSecretDirectory(t *testing.T, directory string) {
t.Helper()
if err := os.Chmod(directory, secretDirectoryPrivateMode); err != nil {
t.Fatalf("Chmod(%q): %v", directory, err)
}
}

View File

@@ -0,0 +1,11 @@
//go:build windows
package app
import "os"
// Windows access control is determined by ACLs rather than POSIX mode bits.
func validateSecretDirectoryPrivacy(_ os.FileInfo) error { return nil }
// Windows access control is determined by ACLs rather than POSIX mode bits.
func validateSecretFilePrivacy(_ os.FileInfo) error { return nil }

View File

@@ -0,0 +1,110 @@
package fileops
import (
"fmt"
"io"
"os"
"path/filepath"
)
// ValidateConfinedDirectory opens path without following symbolic links and
// applies validate to the opened directory's metadata.
func ValidateConfinedDirectory(path string, validate func(os.FileInfo) error) error {
root, err := openConfinedDirectory(path)
if err != nil {
return err
}
defer func() { _ = root.Close() }()
info, err := root.Stat(".")
if err != nil {
return fmt.Errorf("inspect root directory: %w", err)
}
if !info.IsDir() {
return fmt.Errorf("root path is not a directory")
}
if validate != nil {
return validate(info)
}
return nil
}
// ReadRegularFileUnderRoot reads a bounded regular file after opening root and
// every relative-path ancestor without following symbolic links. validateRoot
// and validateFile may enforce caller-owned access policy while their handles
// are still verified.
func ReadRegularFileUnderRoot(
rootPath, relativePath string,
maxBytes int64,
validateRoot, validateFile func(os.FileInfo) error,
) ([]byte, error) {
if maxBytes < 0 {
return nil, fmt.Errorf("maximum byte count must not be negative")
}
if filepath.IsAbs(relativePath) {
return nil, fmt.Errorf("relative file path must not be absolute")
}
parts, err := relativePathParts(relativePath)
if err != nil {
return nil, fmt.Errorf("invalid relative file path: %w", err)
}
root, err := openConfinedDirectory(rootPath)
if err != nil {
return nil, err
}
defer func() { _ = root.Close() }()
rootInfo, err := root.Stat(".")
if err != nil {
return nil, fmt.Errorf("inspect root directory: %w", err)
}
if validateRoot != nil {
if err := validateRoot(rootInfo); err != nil {
return nil, err
}
}
for _, part := range parts[:len(parts)-1] {
child, err := openConfinedChild(root, part, false, 0)
if err != nil {
return nil, err
}
_ = root.Close()
root = child
}
name := parts[len(parts)-1]
inspected, err := root.Lstat(name)
if err != nil {
return nil, fmt.Errorf("inspect file %q: %w", name, err)
}
if inspected.Mode()&os.ModeSymlink != 0 || !inspected.Mode().IsRegular() {
return nil, fmt.Errorf("file %q is not a regular file", name)
}
file, err := root.Open(name)
if err != nil {
return nil, fmt.Errorf("open file %q: %w", name, err)
}
defer func() { _ = file.Close() }()
opened, err := file.Stat()
if err != nil {
return nil, fmt.Errorf("inspect opened file %q: %w", name, err)
}
current, err := root.Lstat(name)
if err != nil || current.Mode()&os.ModeSymlink != 0 || !current.Mode().IsRegular() || !os.SameFile(opened, current) {
if err != nil {
return nil, fmt.Errorf("reinspect file %q: %w", name, err)
}
return nil, fmt.Errorf("file %q changed while being opened", name)
}
if validateFile != nil {
if err := validateFile(opened); err != nil {
return nil, err
}
}
content, err := io.ReadAll(io.LimitReader(file, maxBytes+1))
if err != nil {
return nil, fmt.Errorf("read file %q: %w", name, err)
}
if int64(len(content)) > maxBytes {
return nil, fmt.Errorf("file %q exceeds %d-byte limit", name, maxBytes)
}
return content, nil
}