diff --git a/tools/knowledge-adapter-v2/adapter.go b/tools/knowledge-adapter-v2/adapter.go index 189c3d1..483f59e 100644 --- a/tools/knowledge-adapter-v2/adapter.go +++ b/tools/knowledge-adapter-v2/adapter.go @@ -8,6 +8,7 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "os" "os/exec" "path/filepath" @@ -47,7 +48,7 @@ func marshalDocument(value any) ([]byte, error) { func loadAcquisition(path string) (acquisitionDocument, error) { var doc acquisitionDocument - data, err := os.ReadFile(path) + data, err := os.ReadFile(path) // #nosec G304 -- Path is the operator-selected acquisition input or compiler-produced manifest path. if err != nil { return doc, fmt.Errorf("read acquisition document %s: %w", path, err) } @@ -170,14 +171,21 @@ func buildReceipt(specBytes []byte, proposal InventoryProposal) SourceSpecReview // layMirror materializes the reviewed files into a private mirror, reading each // blob from the content-addressed store and re-verifying its digest and size // against the reviewed (and therefore acquisition-bound) expectation. +func validMirrorPath(relative string) bool { + return fs.ValidPath(relative) && relative != "." +} + func layMirror(mirrorRoot string, spec SourceSpec, casRoot string) error { for _, file := range spec.Files { + if !validMirrorPath(file.RelativePath) { + return fmt.Errorf("invalid mirror relative path %q", file.RelativePath) + } hexDigest := strings.TrimPrefix(file.SHA256, "sha256:") - if len(hexDigest) != 64 { + if decoded, err := hex.DecodeString(hexDigest); err != nil || len(decoded) != 32 { return fmt.Errorf("malformed digest for %s", file.RelativePath) } blobPath := filepath.Join(casRoot, "sha256", hexDigest[:2], hexDigest) - data, err := os.ReadFile(blobPath) + data, err := os.ReadFile(blobPath) // #nosec G304 -- Operator-selected CAS root plus a validated SHA-256 hex digest, verified below. if err != nil { return fmt.Errorf("read CAS blob for %s: %w", file.RelativePath, err) } @@ -188,10 +196,10 @@ func layMirror(mirrorRoot string, spec SourceSpec, casRoot string) error { return fmt.Errorf("CAS digest mismatch for %s", file.RelativePath) } destination := filepath.Join(mirrorRoot, filepath.FromSlash(file.RelativePath)) - if err := os.MkdirAll(filepath.Dir(destination), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(destination), 0o700); err != nil { return fmt.Errorf("create mirror directory for %s: %w", file.RelativePath, err) } - if err := os.WriteFile(destination, data, 0o644); err != nil { + if err := os.WriteFile(destination, data, 0o600); err != nil { // #nosec G703 -- Operator-selected output root; suffix is validated relative path or SHA-256 hex digest. return fmt.Errorf("write mirror file %s: %w", file.RelativePath, err) } } @@ -244,7 +252,19 @@ func prepareBundle(cfg Config, target acquisitionTarget, receiptID, acquisitionP return bundle, err } - bundle = authorityBundle{ + bundle = bundlePaths(root, target, spec, proposal) + if err := writeBundleFiles(bundle, specBytes, proposalBytes, receiptBytes, signature); err != nil { + return bundle, err + } + mirrorRoot := filepath.Join(bundle.mirrorBase, filepath.FromSlash(spec.MirrorPath)) + if err := layMirror(mirrorRoot, spec, cfg.CASRoot); err != nil { + return bundle, err + } + return bundle, nil +} + +func bundlePaths(root string, target acquisitionTarget, spec SourceSpec, proposal InventoryProposal) authorityBundle { + return authorityBundle{ target: target, spec: spec, proposal: proposal, @@ -257,6 +277,9 @@ func prepareBundle(cfg Config, target acquisitionTarget, receiptID, acquisitionP snapshotStore: filepath.Join(root, "snapshot-store"), evidenceStore: filepath.Join(root, "evidence-store"), } +} + +func writeBundleFiles(bundle authorityBundle, specBytes, proposalBytes, receiptBytes, signature []byte) error { writes := []struct { path string data []byte @@ -270,20 +293,16 @@ func prepareBundle(cfg Config, target acquisitionTarget, receiptID, acquisitionP } for _, write := range writes { if err := os.WriteFile(write.path, write.data, write.mode); err != nil { - return bundle, fmt.Errorf("write %s: %w", filepath.Base(write.path), err) + return fmt.Errorf("write %s: %w", filepath.Base(write.path), err) } } - mirrorRoot := filepath.Join(bundle.mirrorBase, filepath.FromSlash(spec.MirrorPath)) - if err := layMirror(mirrorRoot, spec, cfg.CASRoot); err != nil { - return bundle, err - } - return bundle, nil + return nil } func runTool(bin string, args ...string) ([]byte, error) { ctx, cancel := context.WithTimeout(context.Background(), toolTimeout) defer cancel() - command := exec.CommandContext(ctx, bin, args...) + command := exec.CommandContext(ctx, bin, args...) // #nosec G204 -- Operator explicitly supplies the snapshot/evidence executable through CLI flags; arguments bypass a shell. var stdout, stderr bytes.Buffer command.Stdout = &stdout command.Stderr = &stderr @@ -350,7 +369,7 @@ func runEvidenceCompile(bin string, bundle authorityBundle, snapshotManifestPath func readSnapshotManifestHead(path string) (snapshotManifestHead, error) { var head snapshotManifestHead - data, err := os.ReadFile(path) + data, err := os.ReadFile(path) // #nosec G304 -- Path is the operator-selected acquisition input or compiler-produced manifest path. if err != nil { return head, fmt.Errorf("read snapshot manifest: %w", err) } @@ -366,7 +385,7 @@ func manifestHex(manifestID string) (string, error) { return "", fmt.Errorf("malformed manifest id %q", manifestID) } hexDigest := manifestID[index+1:] - if len(hexDigest) != 64 { + if decoded, err := hex.DecodeString(hexDigest); err != nil || len(decoded) != 32 { return "", fmt.Errorf("manifest id %q is not content-addressed", manifestID) } return hexDigest, nil @@ -377,15 +396,15 @@ func copyRetained(sourcePath, destDir, manifestID string) (string, error) { if err != nil { return "", err } - if err := os.MkdirAll(destDir, 0o755); err != nil { + if err := os.MkdirAll(destDir, 0o700); err != nil { return "", fmt.Errorf("create retained-evidence directory: %w", err) } - data, err := os.ReadFile(sourcePath) + data, err := os.ReadFile(sourcePath) // #nosec G304 -- Source path is emitted by the operator-selected snapshot/evidence compiler. if err != nil { return "", fmt.Errorf("read compiled manifest: %w", err) } destination := filepath.Join(destDir, hexDigest+".json") - if err := os.WriteFile(destination, data, 0o644); err != nil { + if err := os.WriteFile(destination, data, 0o600); err != nil { // #nosec G703 -- Operator-selected output root; suffix is validated relative path or SHA-256 hex digest. return "", fmt.Errorf("write retained manifest: %w", err) } return destination, nil @@ -432,14 +451,8 @@ func adaptAuthority(cfg Config, target acquisitionTarget, receiptID, acquisition // a reviewed source bundle and drives the frozen snapshot and evidence // compilers, returning one distinct snapshot and evidence manifest per target. func Adapt(cfg Config) ([]AuthorityResult, error) { - if strings.TrimSpace(cfg.SnapshotBin) == "" || strings.TrimSpace(cfg.EvidenceBin) == "" { - return nil, errors.New("snapshot and evidence compiler binaries are required") - } - if strings.TrimSpace(cfg.CASRoot) == "" { - return nil, errors.New("content-addressed source-blob root is required") - } - if strings.TrimSpace(cfg.WorkRoot) == "" { - return nil, errors.New("work root is required") + if err := validateAdapterConfig(cfg); err != nil { + return nil, err } receipt, err := loadAcquisition(cfg.ReceiptPath) if err != nil { @@ -474,6 +487,19 @@ func Adapt(cfg Config) ([]AuthorityResult, error) { return results, nil } +func validateAdapterConfig(cfg Config) error { + if strings.TrimSpace(cfg.SnapshotBin) == "" || strings.TrimSpace(cfg.EvidenceBin) == "" { + return errors.New("snapshot and evidence compiler binaries are required") + } + if strings.TrimSpace(cfg.CASRoot) == "" { + return errors.New("content-addressed source-blob root is required") + } + if strings.TrimSpace(cfg.WorkRoot) == "" { + return errors.New("work root is required") + } + return nil +} + // crossCheckAcquisition confirms the receipt and inventory proposal describe the // same non-aliasing target set before any authority is adapted. func crossCheckAcquisition(receipt, proposal acquisitionDocument) error { diff --git a/tools/knowledge-adapter-v2/security_test.go b/tools/knowledge-adapter-v2/security_test.go new file mode 100644 index 0000000..333c08d --- /dev/null +++ b/tools/knowledge-adapter-v2/security_test.go @@ -0,0 +1,64 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestManifestHexRejectsNonHex(t *testing.T) { + for _, value := range []string{"sha256:" + strings.Repeat("a", 63) + "/", "sha256:" + strings.Repeat("g", 64), "sha256:" + strings.Repeat("a", 63)} { + if _, err := manifestHex(value); err == nil { + t.Errorf("accepted malformed manifest ID %q", value) + } + } + if _, err := manifestHex("sha256:" + strings.Repeat("a", 64)); err != nil { + t.Fatal(err) + } +} + +func TestMirrorRejectsTraversalAndUsesPrivateModes(t *testing.T) { + root := t.TempDir() + data := []byte("source") + digest := digestBytes(data) + hex := strings.TrimPrefix(digest, "sha256:") + blob := filepath.Join(root, "cas", "sha256", hex[:2], hex) + if err := os.MkdirAll(filepath.Dir(blob), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(blob, data, 0o600); err != nil { + t.Fatal(err) + } + for _, name := range []string{"../escape", "/absolute", "."} { + spec := SourceSpec{Files: []ExpectedFile{{RelativePath: name, SHA256: digest, Size: int64(len(data))}}} + if err := layMirror(filepath.Join(root, "mirror"), spec, filepath.Join(root, "cas")); err == nil { + t.Errorf("accepted unsafe path %q", name) + } + } + spec := SourceSpec{Files: []ExpectedFile{{RelativePath: "dir/source", SHA256: digest, Size: int64(len(data))}}} + if err := layMirror(filepath.Join(root, "mirror"), spec, filepath.Join(root, "cas")); err != nil { + t.Fatal(err) + } + info, err := os.Stat(filepath.Join(root, "mirror", "dir", "source")) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm()&0o077 != 0 { + t.Errorf("mirror readable outside owner: %o", info.Mode().Perm()) + } +} + +func TestSSHStringLengthBounds(t *testing.T) { + for _, value := range []int{-1, 1 << 32} { + if _, err := sshStringLength(value); err == nil { + t.Errorf("accepted length %d", value) + } + } + for _, value := range []int{0, 32, 1<<32 - 1} { + got, err := sshStringLength(value) + if err != nil || uint64(got) != uint64(value) { + t.Errorf("length %d: %d %v", value, got, err) + } + } +} diff --git a/tools/knowledge-adapter-v2/sshsig.go b/tools/knowledge-adapter-v2/sshsig.go index a269b70..2f9fc64 100644 --- a/tools/knowledge-adapter-v2/sshsig.go +++ b/tools/knowledge-adapter-v2/sshsig.go @@ -9,6 +9,7 @@ import ( "encoding/pem" "errors" "fmt" + "math" "os" "os/exec" "path/filepath" @@ -38,12 +39,23 @@ func devPrivateKey() ed25519.PrivateKey { } func sshString(value []byte) []byte { + length, err := sshStringLength(len(value)) + if err != nil { + panic(err) + } out := make([]byte, 4+len(value)) - binary.BigEndian.PutUint32(out, uint32(len(value))) + binary.BigEndian.PutUint32(out, length) copy(out[4:], value) return out } +func sshStringLength(length int) (uint32, error) { + if length < 0 || length > math.MaxUint32 { + return 0, errors.New("SSH string exceeds uint32 length") + } + return uint32(length), nil +} + func ed25519PublicBlob(pub ed25519.PublicKey) []byte { var blob []byte blob = append(blob, sshString([]byte("ssh-ed25519"))...) @@ -109,11 +121,11 @@ func signReviewReceipt(receiptBytes []byte, workDir string) ([]byte, error) { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() command := exec.CommandContext(ctx, sshKeygenPath, "-Y", "sign", "-q", "-f", keyPath, - "-n", reviewSignatureNamespace, receiptPath) + "-n", reviewSignatureNamespace, receiptPath) // #nosec G204 -- Fixed /usr/bin/ssh-keygen binary; paths are arguments without a shell. if output, err := command.CombinedOutput(); err != nil || ctx.Err() != nil { return nil, fmt.Errorf("ssh-keygen review signing failed: %v: %s", err, output) } - signature, err := os.ReadFile(receiptPath + ".sig") + signature, err := os.ReadFile(receiptPath + ".sig") // #nosec G304 -- Signature is emitted by fixed ssh-keygen in the operator-selected private signing directory. if err != nil { return nil, fmt.Errorf("read detached review signature: %w", err) } diff --git a/tools/knowledge-evidence/compiler.go b/tools/knowledge-evidence/compiler.go index ce98cca..528d616 100644 --- a/tools/knowledge-evidence/compiler.go +++ b/tools/knowledge-evidence/compiler.go @@ -413,14 +413,13 @@ func readManifestDocument(filePath string) ([]byte, []QuarantineIssue) { if info.Size() > maxManifestBytes { return nil, []QuarantineIssue{issue("manifest_size_limit_exceeded", "", "snapshot manifest exceeds the fixed input limit")} } - handle, err := os.Open(filePath) + handle, err := os.Open(filePath) // #nosec G304 -- Operator-selected manifest/review path passes no-symlink, regular-file, size and identity checks. if err != nil { return nil, []QuarantineIssue{issue("manifest_read_failed", "", "snapshot manifest could not be read")} } opened, err := handle.Stat() if err != nil || !opened.Mode().IsRegular() || !os.SameFile(info, opened) { - handle.Close() - return nil, []QuarantineIssue{issue("manifest_replaced", "", "snapshot manifest identity changed before read")} + return nil, []QuarantineIssue{closeWithIssue(handle, "manifest_replaced", "", "snapshot manifest identity changed before read")} } data, readErr := io.ReadAll(io.LimitReader(handle, maxManifestBytes+1)) closeErr := handle.Close() @@ -926,7 +925,7 @@ func verifySSHSIG(reviewer string, receipt, signature, policy []byte) []Quaranti defer signatureFile.Close() ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - command := exec.CommandContext(ctx, sshKeygenPath, "-Y", "verify", "-f", "/proc/self/fd/3", "-I", reviewer, "-n", reviewSignatureNamespace, "-s", "/proc/self/fd/4") + command := exec.CommandContext(ctx, sshKeygenPath, "-Y", "verify", "-f", "/proc/self/fd/3", "-I", reviewer, "-n", reviewSignatureNamespace, "-s", "/proc/self/fd/4") // #nosec G204 -- Fixed /usr/bin/ssh-keygen binary; isolated input descriptors and principal bypass a shell. command.ExtraFiles = []*os.File{policyFile, signatureFile} command.Stdin = bytes.NewReader(receipt) command.Stdout = io.Discard @@ -944,24 +943,19 @@ func immutableExecInput(prefix string, data []byte) (*os.File, error) { } name := file.Name() if removeErr := os.Remove(name); removeErr != nil { - file.Close() - return nil, removeErr + return nil, closeWithError(removeErr, file) } if chmodErr := file.Chmod(0o400); chmodErr != nil { - file.Close() - return nil, chmodErr + return nil, closeWithError(chmodErr, file) } if _, writeErr := file.Write(data); writeErr != nil { - file.Close() - return nil, writeErr + return nil, closeWithError(writeErr, file) } if syncErr := file.Sync(); syncErr != nil { - file.Close() - return nil, syncErr + return nil, closeWithError(syncErr, file) } if _, seekErr := file.Seek(0, io.SeekStart); seekErr != nil { - file.Close() - return nil, seekErr + return nil, closeWithError(seekErr, file) } return file, nil } @@ -1029,7 +1023,7 @@ func readSSHString(reader *bytes.Reader) ([]byte, error) { if err := binary.Read(reader, binary.BigEndian, &length); err != nil { return nil, err } - if uint64(length) > uint64(reader.Len()) { + if !sshLengthFits(length, reader.Len()) { return nil, errors.New("SSH string exceeds envelope") } value := make([]byte, int(length)) @@ -1037,6 +1031,10 @@ func readSSHString(reader *bytes.Reader) ([]byte, error) { return value, err } +func sshLengthFits(length uint32, remaining int) bool { + return remaining >= 0 && int64(length) <= int64(remaining) +} + func readStableBoundedFile(filePath, label string, maxBytes int64) ([]byte, []QuarantineIssue) { if issues := validateNoSymlinkPath(filePath, label); len(issues) != 0 { return nil, issues @@ -1045,14 +1043,13 @@ func readStableBoundedFile(filePath, label string, maxBytes int64) ([]byte, []Qu if err != nil || !before.Mode().IsRegular() || before.Mode()&os.ModeSymlink != 0 || before.Size() > maxBytes { return nil, []QuarantineIssue{issue("review_artifact_unavailable", label, "review artifact is not a bounded regular file")} } - handle, err := os.Open(filePath) + handle, err := os.Open(filePath) // #nosec G304 -- Operator-selected manifest/review path passes no-symlink, regular-file, size and identity checks. if err != nil { return nil, []QuarantineIssue{issue("review_artifact_unavailable", label, "review artifact could not be opened")} } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return nil, []QuarantineIssue{issue("review_artifact_replaced", label, "review artifact identity changed before read")} + return nil, []QuarantineIssue{closeWithIssue(handle, "review_artifact_replaced", label, "review artifact identity changed before read")} } data, readErr := io.ReadAll(io.LimitReader(handle, maxBytes+1)) closeErr := handle.Close() @@ -1121,14 +1118,13 @@ func readVerifiedRaw(rawRoot string, file ManifestFile, maxBytes int64) ([]byte, if before.Size() != file.Size || before.Size() > maxBytes { return nil, []QuarantineIssue{issue("raw_object_size_mismatch", file.RelativePath, "raw object size differs from the complete snapshot")} } - handle, err := os.Open(objectPath) + handle, err := os.Open(objectPath) // #nosec G304 -- Operator-selected raw root plus digest-bound relative object path passes containment, symlink and identity checks. if err != nil { return nil, []QuarantineIssue{issue("raw_object_read_failed", file.RelativePath, "raw object could not be opened")} } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return nil, []QuarantineIssue{issue("raw_object_replaced", file.RelativePath, "raw object identity changed before read")} + return nil, []QuarantineIssue{closeWithIssue(handle, "raw_object_replaced", file.RelativePath, "raw object identity changed before read")} } data, readErr := io.ReadAll(io.LimitReader(handle, maxBytes+1)) closeErr := handle.Close() @@ -1293,12 +1289,10 @@ func openSafeOutputRoot(storeRoot string) (*os.Root, fs.FileInfo, error) { } pathInfo, err := os.Lstat(abs) if err != nil || pathInfo.Mode()&os.ModeSymlink != 0 || !pathInfo.IsDir() || !os.SameFile(opened, pathInfo) { - root.Close() - return nil, nil, errors.New("output store root path changed while opening") + return nil, nil, closeWithError(errors.New("output store root path changed while opening"), root) } if err := verifyRootStillBound(abs, root, opened); err != nil { - root.Close() - return nil, nil, err + return nil, nil, closeWithError(err, root) } return root, opened, nil } @@ -1322,33 +1316,29 @@ func openAbsoluteRootNoSymlinks(directory string, mode fs.FileMode) (*os.Root, f info, statErr := root.Lstat(part) if errors.Is(statErr, fs.ErrNotExist) { if err := root.Mkdir(part, mode); err != nil && !errors.Is(err, fs.ErrExist) { - root.Close() - return nil, nil, err + return nil, nil, closeWithError(err, root) } info, statErr = root.Lstat(part) } if statErr != nil || info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { - root.Close() - return nil, nil, errors.New("output store path contains a symlink or non-directory component") + return nil, nil, closeWithError(errors.New("output store path contains a symlink or non-directory component"), root) } child, err := root.OpenRoot(part) if err != nil { - root.Close() - return nil, nil, err + return nil, nil, closeWithError(err, root) } opened, err := child.Stat(".") if err != nil || !os.SameFile(info, opened) { - child.Close() - root.Close() - return nil, nil, errors.New("output store component changed while opening") + return nil, nil, closeWithError(closeWithError(errors.New("output store component changed while opening"), child), root) + } + if err := root.Close(); err != nil { + return nil, nil, closeWithError(fmt.Errorf("close output store parent: %w", err), child) } - root.Close() root = child } opened, err := root.Stat(".") if err != nil { - root.Close() - return nil, nil, err + return nil, nil, closeWithError(err, root) } return root, opened, nil } @@ -1455,12 +1445,10 @@ func writeRootSyncedFile(root *os.Root, storeRoot string, rootInfo fs.FileInfo, } written, writeErr := handle.Write(data) if writeErr != nil || written != len(data) { - handle.Close() - return errors.New("staged object write was incomplete") + return closeWithError(errors.New("staged object write was incomplete"), handle) } if err := handle.Sync(); err != nil { - handle.Close() - return err + return closeWithError(err, handle) } if err := handle.Close(); err != nil { return err @@ -1488,8 +1476,7 @@ func verifyRootFileDigest(root *os.Root, relative, expectedDigest string, expect } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return errors.New("object identity changed before read") + return closeWithError(errors.New("object identity changed before read"), handle) } hash := sha256.New() readLimit := expectedSize @@ -1525,8 +1512,7 @@ func syncRootFile(root *os.Root, relative string) error { } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return errors.New("rooted sync target changed before sync") + return closeWithError(errors.New("rooted sync target changed before sync"), handle) } syncErr := handle.Sync() closeErr := handle.Close() @@ -1652,12 +1638,10 @@ func writeImmutableAtRoot(root *os.Root, storeRoot string, rootInfo fs.FileInfo, defer root.Remove(tempRelative) written, writeErr := temp.Write(data) if writeErr != nil || written != len(data) { - temp.Close() - return errors.New("immutable staged object write was incomplete") + return closeWithError(errors.New("immutable staged object write was incomplete"), temp) } if err := temp.Sync(); err != nil { - temp.Close() - return err + return closeWithError(err, temp) } if err := temp.Close(); err != nil { return err @@ -1707,8 +1691,7 @@ func readRootStableFile(root *os.Root, relative string, maxBytes int64) ([]byte, } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return nil, errors.New("rooted object changed before read") + return nil, closeWithError(errors.New("rooted object changed before read"), handle) } data, readErr := io.ReadAll(io.LimitReader(handle, maxBytes+1)) closeErr := handle.Close() diff --git a/tools/knowledge-evidence/security_cleanup.go b/tools/knowledge-evidence/security_cleanup.go new file mode 100644 index 0000000..1e6adce --- /dev/null +++ b/tools/knowledge-evidence/security_cleanup.go @@ -0,0 +1,14 @@ +package main + +import ( + "errors" + "io" +) + +func closeWithError(primary error, closer io.Closer) error { + return errors.Join(primary, closer.Close()) +} + +func closeWithIssue(closer io.Closer, code, path, message string) QuarantineIssue { + return issue(code, path, closeWithError(errors.New(message), closer).Error()) +} diff --git a/tools/knowledge-evidence/security_cleanup_test.go b/tools/knowledge-evidence/security_cleanup_test.go new file mode 100644 index 0000000..4277b9c --- /dev/null +++ b/tools/knowledge-evidence/security_cleanup_test.go @@ -0,0 +1,45 @@ +package main + +import ( + "bytes" + "encoding/binary" + "errors" + "testing" +) + +type failingCloser struct { + err error + calls int +} + +func (closer *failingCloser) Close() error { closer.calls++; return closer.err } +func TestCloseWithErrorPreservesBothFailures(t *testing.T) { + primary := errors.New("operation failed") + cleanup := errors.New("close failed") + for _, closeErr := range []error{nil, cleanup} { + closer := &failingCloser{err: closeErr} + got := closeWithError(primary, closer) + if !errors.Is(got, primary) || (closeErr != nil && !errors.Is(got, closeErr)) || closer.calls != 1 { + t.Fatalf("close result %v calls %d", got, closer.calls) + } + } + if got := closeWithError(nil, &failingCloser{}); got != nil { + t.Fatal(got) + } +} +func TestReadSSHStringEnvelopeBounds(t *testing.T) { + for _, length := range []uint32{0, 1, 2, 1<<32 - 1} { + var data bytes.Buffer + if err := binary.Write(&data, binary.BigEndian, length); err != nil { + t.Fatal(err) + } + data.WriteByte('x') + got, err := readSSHString(bytes.NewReader(data.Bytes())) + if length <= 1 && (err != nil || len(got) != int(length)) { + t.Fatalf("length %d: %q %v", length, got, err) + } + if length > 1 && err == nil { + t.Fatalf("accepted truncated length %d", length) + } + } +} diff --git a/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/main.go b/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/main.go index 6fff164..d8e10c9 100644 --- a/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/main.go +++ b/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/main.go @@ -180,7 +180,11 @@ func parseEndpoint(raw string) (*url.URL, error) { var remoteEndpointHost = "gateway.example.com" func readCredential(path string) (string, error) { - return readCredentialForUID(path, uint32(os.Getuid())) + uid, err := credentialUID(os.Getuid()) + if err != nil { + return "", err + } + return readCredentialForUID(path, uid) } func readCredentialForUID(path string, expectedUID uint32) (string, error) { diff --git a/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/uid.go b/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/uid.go new file mode 100644 index 0000000..a0948df --- /dev/null +++ b/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/uid.go @@ -0,0 +1,13 @@ +package main + +import ( + "fmt" + "math" +) + +func credentialUID(value int) (uint32, error) { + if value < 0 || uint64(value) > math.MaxUint32 { + return 0, fmt.Errorf("credential UID %d exceeds uint32 bounds", value) + } + return uint32(value), nil +} diff --git a/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/uid_test.go b/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/uid_test.go new file mode 100644 index 0000000..bb967a8 --- /dev/null +++ b/tools/knowledge-mcp/cmd/cordana-knowledge-mcp-bridge/uid_test.go @@ -0,0 +1,20 @@ +package main + +import ( + "math" + "testing" +) + +func TestCredentialUIDBounds(t *testing.T) { + for _, value := range []int{0, 1, math.MaxUint32} { + got, err := credentialUID(value) + if err != nil || uint64(got) != uint64(value) { + t.Fatalf("uid %d: %d %v", value, got, err) + } + } + for _, value := range []int{-1, math.MaxUint32 + 1} { + if _, err := credentialUID(value); err == nil { + t.Errorf("accepted uid %d", value) + } + } +} diff --git a/tools/knowledge-mcp/cmd/mcp01-acceptance/cases.go b/tools/knowledge-mcp/cmd/mcp01-acceptance/cases.go index 44bb1ab..4eca7e7 100644 --- a/tools/knowledge-mcp/cmd/mcp01-acceptance/cases.go +++ b/tools/knowledge-mcp/cmd/mcp01-acceptance/cases.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "fmt" + "io/fs" "net" "net/http" "os" @@ -708,31 +709,39 @@ func mustNormalize(result callResult, ctx *caseContext) []byte { // copySuiteTree clones the suite directory so tamper checks never touch the // frozen fixtures themselves. -func copySuiteTree(suiteDir string) (string, error) { - target, err := os.MkdirTemp("", "mcp01-tampered-suite-") +func copySuiteTree(suiteDir string) (target string, err error) { + source, err := os.OpenRoot(suiteDir) + if err != nil { + return "", fmt.Errorf("open suite root: %w", err) + } + defer func() { err = errors.Join(err, source.Close()) }() + target, err = os.MkdirTemp("", "mcp01-tampered-suite-") if err != nil { return "", err } - err = filepath.Walk(suiteDir, func(path string, info os.FileInfo, walkErr error) error { + destination, err := os.OpenRoot(target) + if err != nil { + return "", errors.Join(err, os.RemoveAll(target)) + } + defer func() { err = errors.Join(err, destination.Close()) }() + err = fs.WalkDir(source.FS(), ".", func(name string, item fs.DirEntry, walkErr error) error { if walkErr != nil { return walkErr } - relative, err := filepath.Rel(suiteDir, path) - if err != nil { - return err + if item.IsDir() { + return destination.MkdirAll(name, 0o700) } - destination := filepath.Join(target, relative) - if info.IsDir() { - return os.MkdirAll(destination, 0o755) + if !item.Type().IsRegular() { + return fmt.Errorf("suite entry %q is not regular", name) } - raw, err := os.ReadFile(path) - if err != nil { - return err + raw, readErr := source.ReadFile(name) + if readErr != nil { + return readErr } - return os.WriteFile(destination, raw, 0o644) + return destination.WriteFile(name, raw, 0o600) }) if err != nil { - return "", err + return "", errors.Join(err, os.RemoveAll(target)) } return target, nil } diff --git a/tools/knowledge-mcp/cmd/mcp01-acceptance/client.go b/tools/knowledge-mcp/cmd/mcp01-acceptance/client.go index 0408754..b028610 100644 --- a/tools/knowledge-mcp/cmd/mcp01-acceptance/client.go +++ b/tools/knowledge-mcp/cmd/mcp01-acceptance/client.go @@ -40,7 +40,7 @@ type stdioBroker struct { } func startStdioBroker(brokerPath string, arguments []string) (*stdioBroker, error) { - command := exec.Command(brokerPath, arguments...) + command := exec.Command(brokerPath, arguments...) // #nosec G204 -- acceptance harness executes the explicitly operator-selected broker binary with separate arguments. command.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} stdin, err := command.StdinPipe() if err != nil { diff --git a/tools/knowledge-mcp/cmd/mcp01-acceptance/main.go b/tools/knowledge-mcp/cmd/mcp01-acceptance/main.go index 8dcb8fa..68f1977 100644 --- a/tools/knowledge-mcp/cmd/mcp01-acceptance/main.go +++ b/tools/knowledge-mcp/cmd/mcp01-acceptance/main.go @@ -135,7 +135,7 @@ func runHermeticSuite(suiteDir, brokerPath, outDir string, freeze *suiteFreeze) verdict := runHermeticCase(suiteDir, brokerPath, freeze, ws, frozenCase) if verdict.transcriptBytes != nil { target := filepath.Join(outDir, frozenCase.CaseID+".transcript.json") - if err := os.WriteFile(target, verdict.transcriptBytes, 0o644); err != nil { + if err := os.WriteFile(target, verdict.transcriptBytes, 0o600); err != nil { return nil, err } } @@ -146,7 +146,7 @@ func runHermeticSuite(suiteDir, brokerPath, outDir string, freeze *suiteFreeze) } func runClusterSuite(endpoint, tokenFile, outDir string, freeze *suiteFreeze) (*suiteReport, error) { - rawToken, err := os.ReadFile(tokenFile) + rawToken, err := os.ReadFile(tokenFile) // #nosec G304 -- token path is the operator-selected acceptance credential. if err != nil { return nil, fmt.Errorf("read cluster bearer token file: %w", err) } @@ -163,7 +163,7 @@ func runClusterSuite(endpoint, tokenFile, outDir string, freeze *suiteFreeze) (* transcriptBytes := runClusterCase(client, freeze, frozenCase, &verdict) if transcriptBytes != nil { target := filepath.Join(outDir, frozenCase.CaseID+".transcript.json") - if err := os.WriteFile(target, transcriptBytes, 0o644); err != nil { + if err := os.WriteFile(target, transcriptBytes, 0o600); err != nil { return nil, err } digest := sha256.Sum256(transcriptBytes) @@ -223,7 +223,7 @@ func run() int { if err := verifyFixturePins(resolvedSuite, freeze); err != nil { return fail(err) } - if err := os.MkdirAll(*outDir, 0o755); err != nil { + if err := os.MkdirAll(*outDir, 0o700); err != nil { return fail(err) } var report *suiteReport diff --git a/tools/knowledge-mcp/cmd/mcp01-acceptance/security_cleanup_test.go b/tools/knowledge-mcp/cmd/mcp01-acceptance/security_cleanup_test.go new file mode 100644 index 0000000..4e245c0 --- /dev/null +++ b/tools/knowledge-mcp/cmd/mcp01-acceptance/security_cleanup_test.go @@ -0,0 +1,60 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func TestCopySuiteTreeRejectsSymlinks(t *testing.T) { + outside := filepath.Join(t.TempDir(), "outside") + if err := os.WriteFile(outside, []byte("outside"), 0o600); err != nil { + t.Fatal(err) + } + for _, external := range []bool{false, true} { + suite := t.TempDir() + target := outside + if !external { + target = "inside" + if err := os.WriteFile(filepath.Join(suite, target), []byte("inside"), 0o600); err != nil { + t.Fatal(err) + } + } + if err := os.Symlink(target, filepath.Join(suite, "link")); err != nil { + t.Fatal(err) + } + copied, err := copySuiteTree(suite) + if copied != "" { + t.Cleanup(func() { + if err := os.RemoveAll(copied); err != nil { + t.Error(err) + } + }) + } + if err == nil { + t.Errorf("accepted symlink (external=%v)", external) + } + } +} + +func TestCopySuiteTreeRegularAndEmpty(t *testing.T) { + for _, content := range []string{"", "fixture"} { + suite := t.TempDir() + if err := os.WriteFile(filepath.Join(suite, "file"), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + copied, err := copySuiteTree(suite) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := os.RemoveAll(copied); err != nil { + t.Error(err) + } + }) + raw, err := os.ReadFile(filepath.Join(copied, "file")) + if err != nil || string(raw) != content { + t.Fatalf("copy: %q %v", raw, err) + } + } +} diff --git a/tools/knowledge-mcp/cmd/mcp01-acceptance/workspace.go b/tools/knowledge-mcp/cmd/mcp01-acceptance/workspace.go index c9cb049..9a2af7d 100644 --- a/tools/knowledge-mcp/cmd/mcp01-acceptance/workspace.go +++ b/tools/knowledge-mcp/cmd/mcp01-acceptance/workspace.go @@ -59,7 +59,7 @@ var frozenCaseClasses = []string{ } func loadSuite(suiteDir string) (*suiteFreeze, error) { - raw, err := os.ReadFile(filepath.Join(suiteDir, "suite-freeze.json")) + raw, err := os.ReadFile(filepath.Join(suiteDir, "suite-freeze.json")) // #nosec G304 -- path names the operator-selected acceptance suite or its private temporary workspace. if err != nil { return nil, fmt.Errorf("read suite freeze: %w", err) } @@ -91,7 +91,7 @@ func loadSuite(suiteDir string) (*suiteFreeze, error) { } func hashFile(path string) (string, int64, error) { - raw, err := os.ReadFile(path) + raw, err := os.ReadFile(path) // #nosec G304 -- path names the operator-selected acceptance suite or its private temporary workspace. if err != nil { return "", 0, err } @@ -181,19 +181,19 @@ func materializeWorkspace(suiteDir string, freeze *suiteFreeze) (*workspace, err tokenDigest := sha256.Sum256([]byte(freeze.SuiteID + "/" + freeze.Revision)) ws.token = hex.EncodeToString(tokenDigest[:]) for _, directory := range []string{"repo", "bin", "policies", "registry", "catalogs", "http"} { - if err := os.MkdirAll(filepath.Join(root, directory), 0o755); err != nil { + if err := os.MkdirAll(filepath.Join(root, directory), 0o700); err != nil { return nil, err } } for _, lane := range []string{"solo", "alpha", "beta", "gamma"} { for _, leaf := range []string{"store", "state", "evidence"} { - if err := os.MkdirAll(filepath.Join(root, "lanes", lane, leaf), 0o755); err != nil { + if err := os.MkdirAll(filepath.Join(root, "lanes", lane, leaf), 0o700); err != nil { return nil, err } } } ws.scriptPath = filepath.Join(root, "bin", "lane-child.sh") - if err := ws.copyFixture("fixtures/lane-child.sh", ws.scriptPath, 0o555); err != nil { + if err := ws.copyFixture("fixtures/lane-child.sh", ws.scriptPath, 0o500); err != nil { return nil, err } ws.scriptPin, _, err = hashFile(ws.scriptPath) @@ -204,7 +204,7 @@ func materializeWorkspace(suiteDir string, freeze *suiteFreeze) (*workspace, err "alpha", "beta", "deny-retrieval", "deny-deleted", "deny-tombstone", "deny-absent", } { target := filepath.Join(root, "policies", policy+".json") - if err := ws.copyFixture("fixtures/policies/"+policy+".json", target, 0o444); err != nil { + if err := ws.copyFixture("fixtures/policies/"+policy+".json", target, 0o400); err != nil { return nil, err } pin, _, err := hashFile(target) @@ -214,7 +214,7 @@ func materializeWorkspace(suiteDir string, freeze *suiteFreeze) (*workspace, err ws.policyPins[policy] = pin } registryTarget := filepath.Join(root, "registry", "candidate-manifest.json") - if err := ws.copyFixture("fixtures/registry/candidate-manifest.json", registryTarget, 0o644); err != nil { + if err := ws.copyFixture("fixtures/registry/candidate-manifest.json", registryTarget, 0o600); err != nil { return nil, err } if err := os.WriteFile(filepath.Join(root, "http", "token"), []byte(ws.token+"\n"), 0o600); err != nil { @@ -244,7 +244,7 @@ func (ws *workspace) copyFixture(relative, target string, mode os.FileMode) erro if err != nil { return err } - if err := os.WriteFile(target, raw, mode); err != nil { + if err := os.WriteFile(target, raw, mode); err != nil { // #nosec G703 -- destination belongs to the private acceptance workspace built from fixed fixture recipes. return err } // An earlier run may have left a read-only file; enforce the exact mode. @@ -271,10 +271,10 @@ func (ws *workspace) buildCatalog(name string, laneIDs []string, policies map[st return err } path := filepath.Join(ws.root, "catalogs", name+".json") - if err := os.WriteFile(path, encoded, 0o644); err != nil { + if err := os.WriteFile(path, encoded, 0o600); err != nil { return err } - if err := os.Chmod(path, 0o444); err != nil { + if err := os.Chmod(path, 0o400); err != nil { return err } digest := sha256.Sum256(encoded) @@ -289,7 +289,7 @@ func (ws *workspace) restoreCatalog(name string) error { if !exists { return fmt.Errorf("unknown catalog %q", name) } - if err := os.Chmod(catalog.path, 0o644); err != nil { + if err := os.Chmod(catalog.path, 0o600); err != nil { return err } previous := catalog.sha256 @@ -310,13 +310,13 @@ func (ws *workspace) restoreCatalog(name string) error { // installResponse places one frozen response fixture as a lane's next // signed-current reply. func (ws *workspace) installResponse(lane, responseFixture string) error { - return ws.copyFixture("fixtures/responses/"+responseFixture, filepath.Join(ws.root, "lanes", lane, "store", "response.json"), 0o644) + return ws.copyFixture("fixtures/responses/"+responseFixture, filepath.Join(ws.root, "lanes", lane, "store", "response.json"), 0o600) } func (ws *workspace) setBlock(lane string, blocked bool) error { marker := filepath.Join(ws.root, "lanes", lane, "store", "block") if blocked { - return os.WriteFile(marker, []byte("block\n"), 0o644) + return os.WriteFile(marker, []byte("block\n"), 0o600) } err := os.Remove(marker) if errors.Is(err, os.ErrNotExist) { @@ -331,10 +331,10 @@ func tamperFile(path string) error { if err != nil { return err } - if err := os.Chmod(path, 0o644); err != nil { + if err := os.Chmod(path, 0o600); err != nil { return err } - raw, err := os.ReadFile(path) + raw, err := os.ReadFile(path) // #nosec G304 -- path names the operator-selected acceptance suite or its private temporary workspace. if err != nil { return err } @@ -342,7 +342,7 @@ func tamperFile(path string) error { return errors.New("cannot tamper an empty file") } raw[len(raw)-2] ^= 0x01 - if err := os.WriteFile(path, raw, 0o644); err != nil { + if err := os.WriteFile(path, raw, 0o600); err != nil { // #nosec G703 -- destination belongs to the private acceptance workspace built from fixed fixture recipes. return err } return os.Chmod(path, info.Mode()) @@ -350,7 +350,7 @@ func tamperFile(path string) error { // restoreFixtureFile rewrites a workspace file from its frozen fixture bytes. func (ws *workspace) restoreFixtureFile(relative, target string, mode os.FileMode) error { - if err := os.Chmod(target, 0o644); err != nil { + if err := os.Chmod(target, 0o600); err != nil { return err } return ws.copyFixture(relative, target, mode) @@ -387,13 +387,13 @@ func (ws *workspace) httpArguments(bind string) []string { } func (ws *workspace) remove() { - _ = os.Chmod(ws.scriptPath, 0o755) + _ = os.Chmod(ws.scriptPath, 0o600) for _, catalog := range ws.catalogs { - _ = os.Chmod(catalog.path, 0o644) + _ = os.Chmod(catalog.path, 0o600) } if entries, err := os.ReadDir(filepath.Join(ws.root, "policies")); err == nil { for _, entry := range entries { - _ = os.Chmod(filepath.Join(ws.root, "policies", entry.Name()), 0o644) + _ = os.Chmod(filepath.Join(ws.root, "policies", entry.Name()), 0o600) } } _ = os.RemoveAll(ws.root) diff --git a/tools/knowledge-mcp/git.go b/tools/knowledge-mcp/git.go index efe38e4..21ab4f2 100644 --- a/tools/knowledge-mcp/git.go +++ b/tools/knowledge-mcp/git.go @@ -74,7 +74,7 @@ func runGitCommandWithTimeout(root string, outputLimit int, timeout time.Duratio commandArguments = append(commandArguments, arguments...) ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - command := exec.CommandContext(ctx, gitExecutablePath, commandArguments...) + command := exec.CommandContext(ctx, gitExecutablePath, commandArguments...) // #nosec G204 -- executes fixed /usr/bin/git with a separate argument vector and validated binary identity. command.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} command.Cancel = func() error { if command.Process == nil { diff --git a/tools/knowledge-mcp/projection.go b/tools/knowledge-mcp/projection.go index ede31ee..6f6f5de 100644 --- a/tools/knowledge-mcp/projection.go +++ b/tools/knowledge-mcp/projection.go @@ -161,11 +161,11 @@ func compileRegistryProjectionWithPublishHook(manifestPath, outputPath string, m return projectionReceipt{}, fmt.Errorf("create private projection workspace: %w", err) } defer os.RemoveAll(privateDirectory) - if err := os.Chmod(privateDirectory, 0o700); err != nil { + if err := os.Chmod(privateDirectory, 0o700); err != nil { // #nosec G302 -- private workspace is a directory and requires owner traversal; no group or other access. return projectionReceipt{}, fmt.Errorf("protect private projection workspace: %w", err) } temporaryPath := filepath.Join(privateDirectory, "projection.sqlite") - temporary, err := os.OpenFile(temporaryPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + temporary, err := os.OpenFile(temporaryPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) // #nosec G304 -- path is an operator-selected projection artifact or a child of the private projection workspace. if err != nil { return projectionReceipt{}, fmt.Errorf("create private projection database: %w", err) } @@ -605,18 +605,15 @@ func openRegistryProjection(path string, maximum int64) (*registryProjection, er db.SetConnMaxLifetime(0) projection := ®istryProjection{db: db, path: path, receipt: receipt} if err := projection.validate(); err != nil { - db.Close() - return nil, err + return nil, joinCloseError(err, db) } projectionHashAfter, projectionSizeAfter, err := hashBoundFile(path, maximum) if err != nil || projectionHashAfter != projectionHash || projectionSizeAfter != projectionSize { - db.Close() - return nil, errors.New("projection changed while it was opened and validated") + return nil, joinCloseError(errors.Join(err, errors.New("projection changed while it was opened and validated")), db) } receiptRawAfter, err := readBoundRegular(path+projectionReceiptSuffix, maxControlObjectBytes, true) if err != nil || !stringBytesEqual(receiptRawAfter, receiptRaw) { - db.Close() - return nil, errors.New("projection receipt changed while the projection was opened") + return nil, joinCloseError(errors.Join(err, errors.New("projection receipt changed while the projection was opened")), db) } return projection, nil } @@ -650,6 +647,22 @@ func validateProjectionReceipt(receipt projectionReceipt) error { func (projection *registryProjection) validate() error { ctx, cancel := context.WithTimeout(context.Background(), projectionOpenTimeout) defer cancel() + for _, validate := range []func(context.Context) error{ + projection.validateDatabaseIdentity, + projection.validateTableSet, + projection.validateSchemaObjects, + projection.validateCellAndRowBounds, + projection.validateMetadata, + projection.validateReceiptTableCounts, + } { + if err := validate(ctx); err != nil { + return err + } + } + return nil +} + +func (projection *registryProjection) validateDatabaseIdentity(ctx context.Context) error { if _, err := projection.db.ExecContext(ctx, "PRAGMA query_only=ON"); err != nil { return fmt.Errorf("force projection query-only mode: %w", err) } @@ -664,6 +677,10 @@ func (projection *registryProjection) validate() error { if err := projection.db.QueryRowContext(ctx, "PRAGMA quick_check").Scan(&check); err != nil || check != "ok" { return fmt.Errorf("projection integrity check failed: %s", check) } + return nil +} + +func (projection *registryProjection) validateTableSet(ctx context.Context) error { rows, err := projection.db.QueryContext(ctx, `SELECT name FROM sqlite_schema WHERE type='table' AND name NOT LIKE 'sqlite_%' ORDER BY name`) if err != nil { @@ -673,8 +690,7 @@ WHERE type='table' AND name NOT LIKE 'sqlite_%' ORDER BY name`) for rows.Next() { var name string if err := rows.Scan(&name); err != nil { - rows.Close() - return err + return joinCloseError(err, rows) } tables = append(tables, name) } @@ -684,12 +700,10 @@ WHERE type='table' AND name NOT LIKE 'sqlite_%' ORDER BY name`) if strings.Join(tables, "\x00") != strings.Join(projectionTables, "\x00") { return fmt.Errorf("projection table set is invalid: %v", tables) } - if err := projection.validateSchemaObjects(ctx); err != nil { - return err - } - if err := projection.validateCellAndRowBounds(ctx); err != nil { - return err - } + return nil +} + +func (projection *registryProjection) validateMetadata(ctx context.Context) error { meta, err := projection.readMeta(ctx) if err != nil { return err @@ -712,6 +726,10 @@ WHERE type='table' AND name NOT LIKE 'sqlite_%' ORDER BY name`) if err := json.Unmarshal([]byte(meta["source_size_bytes"]), &sourceSize); err != nil || sourceSize != projection.receipt.SourceSizeBytes { return errors.New("projection source size binding is invalid") } + return projection.validateMetadataValues(meta) +} + +func (projection *registryProjection) validateMetadataValues(meta map[string]string) error { if err := decodeProjectionMeta(meta["counts"], &projection.counts); err != nil { return fmt.Errorf("decode projection counts: %w", err) } @@ -734,10 +752,20 @@ WHERE type='table' AND name NOT LIKE 'sqlite_%' ORDER BY name`) if err := validateReconciliationBounds(projection.reconcile); err != nil { return fmt.Errorf("validate projection reconciliation: %w", err) } + return nil +} + +func (projection *registryProjection) validateReceiptTableCounts(ctx context.Context) error { for table, expected := range projection.receipt.TableCounts { + if _, known := projectionColumns[table]; !known { + return fmt.Errorf("unknown projection table %q", table) + } var count int - query := "SELECT COUNT(*) FROM " + table - if err := projection.db.QueryRowContext(ctx, query).Scan(&count); err != nil || count != expected { + query := "SELECT COUNT(*) FROM " + table // #nosec G202 -- table is checked against the fixed projectionColumns allowlist above. + if err := projection.db.QueryRowContext(ctx, query).Scan(&count); err != nil { + return fmt.Errorf("count projection table %s: %w", table, err) + } + if count != expected { return fmt.Errorf("projection table count mismatch for %s", table) } } @@ -754,18 +782,15 @@ WHERE type IN ('table','index') AND name NOT LIKE 'sqlite_%' ORDER BY type,name` for rows.Next() { var objectType, name, rawSQL string if err := rows.Scan(&objectType, &name, &rawSQL); err != nil { - rows.Close() - return err + return joinCloseError(err, rows) } key := objectType + ":" + name expected, exists := expectedProjectionSchemaSQL[key] if !exists { - rows.Close() - return fmt.Errorf("unexpected projection schema object %s", key) + return joinCloseError(fmt.Errorf("unexpected projection schema object %s", key), rows) } if canonicalSQLiteSchemaSQL(rawSQL) != expected { - rows.Close() - return fmt.Errorf("projection schema SQL for %s differs from the exact versioned DDL", key) + return joinCloseError(fmt.Errorf("projection schema SQL for %s differs from the exact versioned DDL", key), rows) } seen[key] = struct{}{} } @@ -922,7 +947,7 @@ func hashBoundFile(path string, maximum int64) (string, int64, error) { if err != nil || openedAfter.Size() != openedBefore.Size() || !openedAfter.ModTime().Equal(openedBefore.ModTime()) { return "", 0, errors.New("file metadata changed while hashing") } - pathAfter, err := os.Lstat(path) + pathAfter, err := os.Lstat(path) // #nosec G703 -- operator-selected projection path is inspected for symlinks and regular-file identity before use. if err != nil || pathAfter.Mode()&os.ModeSymlink != 0 || !os.SameFile(openedBefore, pathAfter) { return "", 0, errors.New("file path changed while hashing") } @@ -961,16 +986,14 @@ func openProjectionPublisher(outputPath string) (*projectionPublisher, error) { } opened, err := root.Stat(".") if err != nil || !os.SameFile(before, opened) { - root.Close() - return nil, errors.New("projection parent changed while it was opened") + return nil, joinCloseError(errors.Join(err, errors.New("projection parent changed while it was opened")), root) } publisher := &projectionPublisher{ parentPath: parentPath, root: root, parentInfo: opened, projectionName: projectionName, receiptName: projectionName + projectionReceiptSuffix, } if err := publisher.verifyBinding(); err != nil { - root.Close() - return nil, err + return nil, joinCloseError(err, root) } return publisher, nil } @@ -1116,7 +1139,7 @@ func (publisher *projectionPublisher) stagePrivateFile(sourcePath, target, expec if err := publisher.verifyBinding(); err != nil { return rootedProjectionStage{}, err } - source, err := os.Open(sourcePath) + source, err := os.Open(sourcePath) // #nosec G304 -- path is an operator-selected projection artifact or a child of the private projection workspace. if err != nil { return rootedProjectionStage{}, err } @@ -1135,9 +1158,8 @@ func (publisher *projectionPublisher) stagePrivateFile(sourcePath, target, expec } createdInfo, err := destination.Stat() if err != nil || !createdInfo.Mode().IsRegular() { - destination.Close() - _ = publisher.root.Remove(name) - return rootedProjectionStage{}, errors.New("rooted projection stage identity is unavailable") + closeErr := joinCloseError(errors.New("rooted projection stage identity is unavailable"), destination) + return rootedProjectionStage{}, errors.Join(closeErr, publisher.root.Remove(name)) } stage := rootedProjectionStage{name: name, hash: expectedHash, size: expectedSize, target: target, info: createdInfo} hash := sha256.New() @@ -1180,8 +1202,7 @@ func (publisher *projectionPublisher) targetMatches(name, expectedHash string, e } opened, err := file.Stat() if err != nil || !os.SameFile(info, opened) { - file.Close() - return false, errors.New("immutable projection target changed before read") + return false, joinCloseError(errors.New("immutable projection target changed before read"), file) } digest := sha256.New() size, readErr := io.Copy(digest, io.LimitReader(file, expectedSize+1)) @@ -1230,7 +1251,7 @@ func projectionRandomChild(prefix string) (string, error) { } func writePrivateSyncedFile(pathValue string, value []byte) error { - file, err := os.OpenFile(pathValue, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + file, err := os.OpenFile(pathValue, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) // #nosec G304 -- path is an operator-selected projection artifact or a child of the private projection workspace. if err != nil { return err } @@ -1259,7 +1280,7 @@ func validateDirectoryPathNoSymlinks(path string) error { continue } current = filepath.Join(current, part) - info, err := os.Lstat(current) + info, err := os.Lstat(current) // #nosec G703 -- operator-selected projection path is inspected for symlinks and regular-file identity before use. if err != nil { return err } @@ -1274,7 +1295,7 @@ func validateDirectoryPathNoSymlinks(path string) error { } func requireReadOnlyRegular(path string) error { - info, err := os.Lstat(path) + info, err := os.Lstat(path) // #nosec G703 -- operator-selected projection path is inspected for symlinks and regular-file identity before use. if err != nil { return err } @@ -1288,7 +1309,7 @@ func requireReadOnlyRegular(path string) error { } func syncRegularFile(path string) error { - file, err := os.Open(path) + file, err := os.Open(path) // #nosec G304 -- path is an operator-selected projection artifact or a child of the private projection workspace. if err != nil { return err } @@ -1301,7 +1322,7 @@ func syncRegularFile(path string) error { } func syncDirectory(path string) error { - directory, err := os.Open(path) + directory, err := os.Open(path) // #nosec G304 -- path is an operator-selected projection artifact or a child of the private projection workspace. if err != nil { return err } @@ -1465,7 +1486,7 @@ func (projection *registryProjection) searchComponents(query, ecosystem, kind, l return errorOutcome(stateBackendUnavailable, "count compact component projection: "+err.Error(), nil) } queryArguments := append(append([]any{}, arguments...), maxProjectionSearchRows+1) - rows, err := projection.db.QueryContext(ctx, "SELECT "+projectedComponentColumns+" FROM components WHERE "+clause+" ORDER BY component_id LIMIT ?", queryArguments...) + rows, err := projection.db.QueryContext(ctx, "SELECT "+projectedComponentColumns+" FROM components WHERE "+clause+" ORDER BY component_id LIMIT ?", queryArguments...) // #nosec G202 -- SQL structure is built only from fixed query fragments; all caller values use bound placeholders. if err != nil { return errorOutcome(stateBackendUnavailable, "query compact component projection: "+err.Error(), nil) } @@ -1775,7 +1796,7 @@ func (projection *registryProjection) relationRows(ctx context.Context, column, return nil, false, errors.New("invalid relation direction column") } query := `SELECT from_component_id,to_component_id,relationship,scope,optional,edge_count, -evidence_occurrence_ids_json FROM relations WHERE ` + column + "=?" +evidence_occurrence_ids_json FROM relations WHERE ` + column + "=?" // #nosec G202 -- SQL structure is built only from fixed query fragments; all caller values use bound placeholders. arguments := []any{componentID} if scope != "" { query += " AND scope=? COLLATE NOCASE" @@ -1878,7 +1899,7 @@ func (projection *registryProjection) gapEvidence(ctx context.Context, component start, _ := pageBounds(total, offset, limit) queryArguments := append(append([]any{}, arguments...), limit, start) rows, err := projection.db.QueryContext(ctx, `SELECT gap_id,gap_type,component_id,component_version_id, -source_id,message,evidence_json FROM gaps WHERE `+clause+` ORDER BY gap_id LIMIT ? OFFSET ?`, queryArguments...) +source_id,message,evidence_json FROM gaps WHERE `+clause+` ORDER BY gap_id LIMIT ? OFFSET ?`, queryArguments...) // #nosec G202 -- SQL structure is built only from fixed query fragments; all caller values use bound placeholders. if err != nil { return nil, 0, 0, err } diff --git a/tools/knowledge-mcp/projection_table_guard_test.go b/tools/knowledge-mcp/projection_table_guard_test.go new file mode 100644 index 0000000..eed9a2a --- /dev/null +++ b/tools/knowledge-mcp/projection_table_guard_test.go @@ -0,0 +1,51 @@ +package main + +import ( + "path/filepath" + "strings" + "testing" +) + +func TestProjectionReceiptTableAllowlist(t *testing.T) { + directory := t.TempDir() + manifestPath := filepath.Join(directory, "candidate.json") + writeManifest(t, manifestPath, fixtureManifest(t, false)) + projectionPath := filepath.Join(directory, "candidate.sqlite") + if _, err := compileRegistryProjection(manifestPath, projectionPath, defaultMaxRegistryBytes); err != nil { + t.Fatal(err) + } + projection, err := openRegistryProjection(projectionPath, defaultMaxRegistryBytes) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := projection.close(); err != nil { + t.Errorf("close projection: %v", err) + } + }) + knownCount := projection.receipt.TableCounts["components"] + for _, test := range []struct { + name, table, wantError string + count int + }{ + {"known table", "components", "", knownCount}, + {"wrong count", "components", "count mismatch for components", knownCount + 1}, + {"injection", "x; DROP TABLE components", "unknown projection table", 0}, + {"empty name", "", "unknown projection table", 0}, + {"unknown name", "x", "unknown projection table", 0}, + } { + t.Run(test.name, func(t *testing.T) { + projection.receipt.TableCounts = map[string]int{test.table: test.count} + err := projection.validate() + if test.wantError == "" { + if err != nil { + t.Fatal(err) + } + return + } + if err == nil || !strings.Contains(err.Error(), test.wantError) || !strings.Contains(err.Error(), test.table) { + t.Fatalf("table %q: want %q naming table, got %v", test.table, test.wantError, err) + } + }) + } +} diff --git a/tools/knowledge-mcp/publish.go b/tools/knowledge-mcp/publish.go index 671b97f..0aac891 100644 --- a/tools/knowledge-mcp/publish.go +++ b/tools/knowledge-mcp/publish.go @@ -26,6 +26,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/http" "net/url" "os" @@ -376,7 +377,7 @@ func describePayloadFile(source, bundlePath string) (publishBundleEntry, error) } func hashRegularFile(source string) (string, int64, error) { - file, err := os.Open(source) + file, err := os.Open(source) // #nosec G304 -- path comes from the operator publish configuration or the validated bundle entry and private staging directory. if err != nil { return "", 0, err } @@ -401,62 +402,58 @@ func writePublishBundle(output string, manifest publishBundleManifest, sources m return "", fmt.Errorf("validate bundle output parent: %w", err) } temporary := output + ".partial" - file, err := os.OpenFile(temporary, os.O_WRONLY|os.O_CREATE|os.O_EXCL|os.O_TRUNC, 0o600) + file, err := os.OpenFile(temporary, os.O_WRONLY|os.O_CREATE|os.O_EXCL|os.O_TRUNC, 0o600) // #nosec G304 -- operator-selected output parent was validated above; exclusive creation rejects an existing symlink. if err != nil { return "", fmt.Errorf("create bundle: %w", err) } digest := sha256.New() writer := tar.NewWriter(io.MultiWriter(file, digest)) + if err := writeBundleEntries(writer, manifest, sources); err != nil { + return "", removeFileError(joinCloseError(err, file), temporary) + } + if err := writer.Close(); err != nil { + return "", removeFileError(joinCloseError(err, file), temporary) + } + if err := file.Sync(); err != nil { + return "", removeFileError(joinCloseError(err, file), temporary) + } + if err := file.Close(); err != nil { + return "", removeFileError(err, temporary) + } + if err := os.Chmod(temporary, publishFileMode); err != nil { + return "", removeFileError(err, temporary) + } + if err := os.Rename(temporary, output); err != nil { + return "", removeFileError(err, temporary) + } + return hex.EncodeToString(digest.Sum(nil)), nil +} +func writeBundleEntries(writer *tar.Writer, manifest publishBundleManifest, sources map[string]string) error { encoded, err := json.Marshal(manifest) if err != nil { - file.Close() - os.Remove(temporary) - return "", err + return err } encoded = append(encoded, '\n') - failure := func(cause error) (string, error) { - file.Close() - os.Remove(temporary) - return "", cause - } if err := writeBundleHeader(writer, publishManifestName, int64(len(encoded))); err != nil { - return failure(err) + return err } if _, err := writer.Write(encoded); err != nil { - return failure(err) + return err } for _, entry := range manifest.Entries { source, ok := sources[entry.Path] if !ok { - return failure(fmt.Errorf("bundle entry %q has no source file", entry.Path)) + return fmt.Errorf("bundle entry %q has no source file", entry.Path) } if err := writeBundleHeader(writer, entry.Path, entry.Size); err != nil { - return failure(err) + return err } if err := copyBundlePayload(writer, source, entry); err != nil { - return failure(err) + return err } } - if err := writer.Close(); err != nil { - return failure(err) - } - if err := file.Sync(); err != nil { - return failure(err) - } - if err := file.Close(); err != nil { - os.Remove(temporary) - return "", err - } - if err := os.Chmod(temporary, publishFileMode); err != nil { - os.Remove(temporary) - return "", err - } - if err := os.Rename(temporary, output); err != nil { - os.Remove(temporary) - return "", err - } - return hex.EncodeToString(digest.Sum(nil)), nil + return nil } func writeBundleHeader(writer *tar.Writer, name string, size int64) error { @@ -472,7 +469,7 @@ func writeBundleHeader(writer *tar.Writer, name string, size int64) error { } func copyBundlePayload(writer io.Writer, source string, entry publishBundleEntry) error { - file, err := os.Open(source) + file, err := os.Open(source) // #nosec G304 -- path comes from the operator publish configuration or the validated bundle entry and private staging directory. if err != nil { return err } @@ -500,50 +497,79 @@ func materializePublishBundle(configuration config) (status *publishStatus, err return nil, fmt.Errorf("resolve publish target: %w", err) } staging := filepath.Join(target, publishStagingDirName) - defer os.RemoveAll(staging) - defer func() { - if err != nil { - clearPublishedTrees(target) - } - writePublishStatus(target, status, err) - }() - - if err = os.MkdirAll(target, publishDirMode); err != nil { - return nil, fmt.Errorf("create publish target: %w", err) + defer func() { err = finishPublishMaterialization(target, staging, status, err, os.RemoveAll) }() + if err := preparePublishTarget(target, staging); err != nil { + return nil, err } - clearPublishedTrees(target) - if err = os.RemoveAll(staging); err != nil { + manifest, digest, err := fetchAndExtractPublishBundle(configuration, staging) + if err != nil { + return nil, err + } + payload := filepath.Join(staging, "payload") + snapshot, err := verifyPublishPayload(configuration, payload, manifest) + if err != nil { return nil, err } - if err = os.MkdirAll(staging, publishDirMode); err != nil { + status = &publishStatus{ + SchemaVersion: publishBundleSchemaVersion, + RecordKind: "knowledge_store_publication_status", State: "published", + BundleSHA256: digest, PointerID: snapshot.pointer.PointerID, + CurrentManifestID: snapshot.pointer.CurrentManifestID, PublisherVersion: serverVersion, + } + if err := placePublishPayload(configuration, payload, target, status); err != nil { return nil, err } + return status, nil +} + +func finishPublishMaterialization(target, staging string, status *publishStatus, cause error, cleanup func(string) error) error { + cause = errors.Join(cause, cleanup(staging)) + if cause != nil { + cause = errors.Join(cause, clearPublishedTrees(target)) + } + if err := writePublishStatus(target, status, cause); err != nil { + return errors.Join(cause, err, clearPublishedTrees(target)) + } + return cause +} +func preparePublishTarget(target, staging string) error { + if err := os.MkdirAll(target, publishDirMode); err != nil { + return fmt.Errorf("create publish target: %w", err) + } + if err := clearPublishedTrees(target); err != nil { + return err + } + if err := os.RemoveAll(staging); err != nil { + return err + } + return os.MkdirAll(staging, publishDirMode) +} + +func fetchAndExtractPublishBundle(configuration config, staging string) (*publishBundleManifest, string, error) { bundlePath := filepath.Join(staging, "bundle.tar") - var digest string - if digest, err = fetchPublishBundle(configuration.publishMaterialize, bundlePath); err != nil { - return nil, fmt.Errorf("fetch bundle: %w", err) + digest, err := fetchPublishBundle(configuration.publishMaterialize, bundlePath) + if err != nil { + return nil, "", fmt.Errorf("fetch bundle: %w", err) } if digest != configuration.publishSHA256 { - return nil, fmt.Errorf("bundle SHA-256 %s does not match the operator pin %s", digest, configuration.publishSHA256) + return nil, "", fmt.Errorf("bundle SHA-256 %s does not match the operator pin %s", digest, configuration.publishSHA256) } - - payload := filepath.Join(staging, "payload") - var manifest *publishBundleManifest - if manifest, err = extractPublishBundle(bundlePath, payload); err != nil { - return nil, fmt.Errorf("extract bundle: %w", err) + manifest, err := extractPublishBundle(bundlePath, filepath.Join(staging, "payload")) + if err != nil { + return nil, "", fmt.Errorf("extract bundle: %w", err) } + return manifest, digest, nil +} +func verifyPublishPayload(configuration config, payload string, manifest *publishBundleManifest) (*registrySnapshot, error) { trustDirectory := filepath.Join(payload, publishTrustDirName) - if err = materializeTrustRoot(configuration.publishTrustSource, trustDirectory); err != nil { + if err := materializeTrustRoot(configuration.publishTrustSource, trustDirectory); err != nil { return nil, fmt.Errorf("materialize trust root: %w", err) } - trustPath := filepath.Join(trustDirectory, publishTrustFileName) - - stagedStore := filepath.Join(payload, publishStateDirName) snapshot := newRegistryProvider(config{ - stateDirectory: stagedStore, - allowedSigners: trustPath, + stateDirectory: filepath.Join(payload, publishStateDirName), + allowedSigners: filepath.Join(trustDirectory, publishTrustFileName), maxRegistrySize: configuration.maxRegistrySize, }).snapshot() if snapshot.state != stateOK || snapshot.pointer == nil { @@ -552,30 +578,31 @@ func materializePublishBundle(configuration config) (status *publishStatus, err if snapshot.pointer.PointerID != manifest.PointerID || snapshot.pointer.CurrentManifestID != manifest.CurrentManifestID { return nil, errors.New("staged generation store does not match the bundle manifest binding") } + return snapshot, nil +} - status = &publishStatus{ - SchemaVersion: publishBundleSchemaVersion, - RecordKind: "knowledge_store_publication_status", - State: "published", - BundleSHA256: digest, - PointerID: snapshot.pointer.PointerID, - CurrentManifestID: snapshot.pointer.CurrentManifestID, - PublisherVersion: serverVersion, - } - if err = os.Rename(stagedStore, filepath.Join(target, publishStateDirName)); err != nil { - return nil, fmt.Errorf("place generation store: %w", err) - } - if err = os.Rename(trustDirectory, filepath.Join(target, publishTrustDirName)); err != nil { - return nil, fmt.Errorf("place trust root: %w", err) +func placePublishPayload(configuration config, payload, target string, status *publishStatus) error { + for _, name := range []string{publishStateDirName, publishTrustDirName} { + if err := os.Rename(filepath.Join(payload, name), filepath.Join(target, name)); err != nil { + return fmt.Errorf("place %s: %w", name, err) + } } - stagedRetrieval := filepath.Join(payload, publishRetrievalDirName) if _, statErr := os.Stat(stagedRetrieval); statErr == nil { - status.RetrievalState = placeRetrievalTree(configuration, stagedRetrieval, target) + state, err := placeRetrievalTree(configuration, stagedRetrieval, target) + if err != nil { + return fmt.Errorf("place retrieval payload: %w", err) + } + status.RetrievalState = state + } else if !errors.Is(statErr, os.ErrNotExist) { + return fmt.Errorf("inspect staged retrieval tree: %w", statErr) } - sealDirectoryTree(filepath.Join(target, publishStateDirName)) - sealDirectoryTree(filepath.Join(target, publishTrustDirName)) - return status, nil + for _, name := range []string{publishStateDirName, publishTrustDirName} { + if err := sealDirectoryTree(filepath.Join(target, name)); err != nil { + return err + } + } + return nil } // placeRetrievalTree moves the retrieval tree to its final path and only then @@ -583,31 +610,38 @@ func materializePublishBundle(configuration config) (status *publishStatus, err // in-cluster paths that do not exist under staging. A catalog that fails to // load takes the whole retrieval tree back out, leaving the broker to report a // typed degraded federation rather than serving unpinned lanes. -func placeRetrievalTree(configuration config, stagedRetrieval, target string) string { +func placeRetrievalTree(configuration config, stagedRetrieval, target string) (string, error) { final := filepath.Join(target, publishRetrievalDirName) if err := os.Rename(stagedRetrieval, final); err != nil { - return "unavailable: place retrieval tree: " + err.Error() + return "", fmt.Errorf("place retrieval tree: %w", err) + } + if err := sealDirectoryTree(final); err != nil { + return "", fmt.Errorf("seal retrieval tree: %w", err) } - sealDirectoryTree(final) if configuration.retrievalCatalog == "" || configuration.retrievalCatalogHash == "" { - return "published_unverified: no catalog pin was configured" + return "published_unverified: no catalog pin was configured", nil } provider := newRetrievalProvider(config{ retrievalCatalog: configuration.retrievalCatalog, retrievalCatalogHash: configuration.retrievalCatalogHash, }) if _, err := provider.loadRetrievalCatalog(); err != nil { - os.RemoveAll(final) - return "unavailable: catalog verification failed: " + err.Error() + if cleanupErr := errors.Join(unsealDirectoryTree(final), os.RemoveAll(final)); cleanupErr != nil { + return "", errors.Join(fmt.Errorf("catalog verification failed: %w", err), + fmt.Errorf("remove rejected retrieval tree: %w", cleanupErr)) + } + return "unavailable: catalog verification failed: " + err.Error(), nil } - return "published" + return "published", nil } -func clearPublishedTrees(target string) { +func clearPublishedTrees(target string) error { + var failure error for _, name := range []string{publishStateDirName, publishTrustDirName, publishRetrievalDirName} { - unsealDirectoryTree(filepath.Join(target, name)) - os.RemoveAll(filepath.Join(target, name)) + path := filepath.Join(target, name) + failure = errors.Join(failure, unsealDirectoryTree(path), os.RemoveAll(path)) } + return failure } // fetchPublishBundle accepts a local path or an http(s) URL. The transport is @@ -622,7 +656,7 @@ func fetchPublishBundle(source, destination string) (string, error) { if err != nil { return "", err } - file, err := os.Open(absolute) + file, err := os.Open(absolute) // #nosec G304 -- path comes from the operator publish configuration or the validated bundle entry and private staging directory. if err != nil { return "", err } @@ -644,7 +678,7 @@ func downloadPublishBundle(source, destination string) (string, error) { } func streamToFile(reader io.Reader, destination string) (string, error) { - file, err := os.OpenFile(destination, os.O_WRONLY|os.O_CREATE|os.O_EXCL|os.O_TRUNC, 0o600) + file, err := os.OpenFile(destination, os.O_WRONLY|os.O_CREATE|os.O_EXCL|os.O_TRUNC, 0o600) // #nosec G304 -- path comes from the operator publish configuration or the validated bundle entry and private staging directory. if err != nil { return "", err } @@ -664,7 +698,7 @@ func streamToFile(reader io.Reader, destination string) (string, error) { // under state/ and retrieval/ are accepted; trust/ is refused outright so a // bundle can never supply the trust root that authorises it. func extractPublishBundle(bundlePath, payload string) (*publishBundleManifest, error) { - file, err := os.Open(bundlePath) + file, err := os.Open(bundlePath) // #nosec G304 -- path comes from the operator publish configuration or the validated bundle entry and private staging directory. if err != nil { return nil, err } @@ -774,7 +808,7 @@ func extractBundleEntry(reader io.Reader, payload string, entry publishBundleEnt if err := os.MkdirAll(filepath.Dir(destination), publishDirMode); err != nil { return err } - file, err := os.OpenFile(destination, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + file, err := os.OpenFile(destination, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) // #nosec G304 -- path comes from the operator publish configuration or the validated bundle entry and private staging directory. if err != nil { return err } @@ -825,7 +859,7 @@ func materializeTrustRoot(source, directory string) error { if strings.TrimSpace(source) == "" { return errors.New("no allowed-signers trust source was configured") } - raw, err := os.ReadFile(source) + raw, err := os.ReadFile(source) // #nosec G304 -- path comes from the operator publish configuration or the validated bundle entry and private staging directory. if err != nil { return err } @@ -839,41 +873,68 @@ func materializeTrustRoot(source, directory string) error { return err } destination := filepath.Join(directory, publishTrustFileName) - if err := os.WriteFile(destination, raw, 0o600); err != nil { + if err := os.WriteFile(destination, raw, 0o600); err != nil { // #nosec G703 -- destination is the fixed trust-file child of the private extraction directory. return err } return os.Chmod(destination, publishFileMode) } -func sealDirectoryTree(root string) { +func sealDirectoryTree(root string) error { + return setDirectoryTreeMode(root, publishSealedDirMode) +} + +func unsealDirectoryTree(root string) error { + return setDirectoryTreeMode(root, publishDirMode) +} + +// Directory modes are changed through open descriptors: Root.Chmod on Unix +// can race with a symlink replacement, while File.Chmod keeps the opened inode. +func setDirectoryTreeMode(path string, mode os.FileMode) (err error) { + root, err := openBoundDirectoryRoot(path) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("open directory tree: %w", err) + } + defer func() { err = errors.Join(err, root.Close()) }() directories := make([]string, 0, 8) - _ = filepath.WalkDir(root, func(current string, item os.DirEntry, walkErr error) error { + err = fs.WalkDir(root.FS(), ".", func(name string, item fs.DirEntry, walkErr error) error { if walkErr != nil { - return nil + return walkErr } if item.IsDir() { - directories = append(directories, current) + directories = append(directories, name) } return nil }) + if err != nil { + return err + } for index := len(directories) - 1; index >= 0; index-- { - _ = os.Chmod(directories[index], publishSealedDirMode) + if err := chmodRootedDirectory(root, directories[index], mode); err != nil { + return err + } } + return nil } -func unsealDirectoryTree(root string) { - _ = filepath.WalkDir(root, func(current string, item os.DirEntry, walkErr error) error { - if walkErr != nil { - return nil - } - if item.IsDir() { - _ = os.Chmod(current, publishDirMode) - } - return nil - }) +func chmodRootedDirectory(root *os.Root, name string, mode os.FileMode) error { + file, err := root.Open(name) + if err != nil { + return fmt.Errorf("open rooted directory: %w", err) + } + info, err := file.Stat() + if err == nil && !info.IsDir() { + err = errors.New("rooted mode target is not a directory") + } + if err == nil { + err = file.Chmod(mode) + } + return joinCloseError(err, file) } -func writePublishStatus(target string, status *publishStatus, failure error) { +func writePublishStatus(target string, status *publishStatus, failure error) error { record := status if failure != nil || record == nil { record = &publishStatus{ @@ -888,7 +949,7 @@ func writePublishStatus(target string, status *publishStatus, failure error) { } raw, err := json.Marshal(record) if err != nil { - return + return err } - _ = os.WriteFile(filepath.Join(target, publishStatusFileName), append(raw, '\n'), 0o644) + return os.WriteFile(filepath.Join(target, publishStatusFileName), append(raw, '\n'), 0o600) // #nosec G304 -- fixed status-file child of the operator-configured publication target. } diff --git a/tools/knowledge-mcp/registry.go b/tools/knowledge-mcp/registry.go index 0b8fb07..60e5087 100644 --- a/tools/knowledge-mcp/registry.go +++ b/tools/knowledge-mcp/registry.go @@ -505,7 +505,7 @@ func (provider *registryProvider) loadStore(aliasRaw, trustedPolicy []byte, trus } func fileSize(path string) int64 { - info, err := os.Stat(path) + info, err := os.Stat(path) // #nosec G703 -- operator-selected registry or trust path is inspected for size, regular-file type and identity before use. if err != nil { return 0 } @@ -574,7 +574,7 @@ func readBoundRegular(path string, maximum int64, rejectSymlink bool) ([]byte, e if err != nil || openedAfter.Size() != openedBefore.Size() || !openedAfter.ModTime().Equal(openedBefore.ModTime()) { return nil, errors.New("registry object metadata changed while it was read") } - pathAfter, err := os.Lstat(path) + pathAfter, err := os.Lstat(path) // #nosec G703 -- operator-selected registry or trust path is inspected for size, regular-file type and identity before use. if err != nil || (rejectSymlink && pathAfter.Mode()&os.ModeSymlink != 0) || !os.SameFile(openedBefore, pathAfter) { return nil, errors.New("registry object path changed while it was read") } @@ -582,7 +582,7 @@ func readBoundRegular(path string, maximum int64, rejectSymlink bool) ([]byte, e } func openBoundRegular(path string, maximum int64, rejectSymlink bool) (*os.File, int64, error) { - info, err := os.Lstat(path) + info, err := os.Lstat(path) // #nosec G703 -- operator-selected registry or trust path is inspected for size, regular-file type and identity before use. if err != nil { return nil, 0, err } @@ -595,18 +595,16 @@ func openBoundRegular(path string, maximum int64, rejectSymlink bool) (*os.File, if info.Size() > maximum { return nil, 0, fmt.Errorf("registry object exceeds %d-byte limit", maximum) } - file, err := os.Open(path) + file, err := os.Open(filepath.Clean(path)) // #nosec G703 -- operator-selected registry or trust path is inspected for size, regular-file type and identity before use. if err != nil { return nil, 0, err } opened, err := file.Stat() if err != nil { - file.Close() - return nil, 0, err + return nil, 0, joinCloseError(err, file) } if !os.SameFile(info, opened) || !opened.Mode().IsRegular() { - file.Close() - return nil, 0, errors.New("registry object changed while it was opened") + return nil, 0, joinCloseError(errors.Join(err, errors.New("registry object changed while it was opened")), file) } return file, opened.Size(), nil } @@ -1177,8 +1175,8 @@ func verifyReceiptAuthorization(stateDir string, receipt map[string]any, trusted if hashBytes(policy) != policyHash || trustedPolicyHash != policyHash || !bytes.Equal(policy, trustedPolicy) { return errors.New("persisted allowed-signers policy does not match the external operator trust root") } - trustedInfo, trustedErr := os.Lstat(trustedPolicyPath) - persistedInfo, persistedErr := os.Lstat(policyPath) + trustedInfo, trustedErr := os.Lstat(trustedPolicyPath) // #nosec G703 -- operator-selected registry or trust path is inspected for size, regular-file type and identity before use. + persistedInfo, persistedErr := os.Lstat(policyPath) // #nosec G703 -- operator-selected registry or trust path is inspected for size, regular-file type and identity before use. if trustedErr != nil || persistedErr != nil || os.SameFile(trustedInfo, persistedInfo) { return errors.New("persisted policy and external operator trust root must be independent files") } @@ -1291,24 +1289,19 @@ func immutableExecInput(prefix string, data []byte) (*os.File, error) { } path := file.Name() if err := os.Remove(path); err != nil { - file.Close() - return nil, err + return nil, joinCloseError(err, file) } if err := file.Chmod(0o400); err != nil { - file.Close() - return nil, err + return nil, joinCloseError(err, file) } if _, err := file.Write(data); err != nil { - file.Close() - return nil, err + return nil, joinCloseError(err, file) } if err := file.Sync(); err != nil { - file.Close() - return nil, err + return nil, joinCloseError(err, file) } if _, err := file.Seek(0, io.SeekStart); err != nil { - file.Close() - return nil, err + return nil, joinCloseError(err, file) } return file, nil } @@ -1327,7 +1320,7 @@ func validateSSHKeygen() error { func runSSHKeygen(payload []byte, extraFiles []*os.File, arguments ...string) (string, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - command := exec.CommandContext(ctx, sshKeygenPath, arguments...) + command := exec.CommandContext(ctx, sshKeygenPath, arguments...) // #nosec G204 -- executes fixed /usr/bin/ssh-keygen with separate arguments and descriptor-backed inputs. command.ExtraFiles = extraFiles command.Stdin = bytes.NewReader(payload) var output boundedBuffer diff --git a/tools/knowledge-mcp/retrieval.go b/tools/knowledge-mcp/retrieval.go index b38e341..5239e89 100644 --- a/tools/knowledge-mcp/retrieval.go +++ b/tools/knowledge-mcp/retrieval.go @@ -23,7 +23,6 @@ const ( maxRetrievalOutputBytes = 1 << 20 maxRetrievalDiagnosticBytes = 64 << 10 retrievalCommandTimeout = 15 * time.Second - retrievalExecutableFDPath = "/proc/self/fd/3" ) type retrievalProvider struct { @@ -495,22 +494,20 @@ func (provider *retrievalProvider) openValidatedExecutable() (*os.File, error) { if err := validateDirectoryPathNoSymlinks(filepath.Dir(provider.executable)); err != nil { return nil, err } - info, err := os.Lstat(provider.executable) + info, err := os.Lstat(provider.executable) // #nosec G703 -- operator-pinned executable is absolute, has symlink-free parents, and is checked for immutable inode identity and SHA-256. if err != nil || info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() || info.Mode().Perm()&0o111 == 0 || info.Mode().Perm()&0o222 != 0 { return nil, errors.New("executable is not an immutable executable regular file") } - file, err := os.Open(provider.executable) + file, err := os.Open(provider.executable) // #nosec G703 -- operator-pinned executable is absolute, has symlink-free parents, and is checked for immutable inode identity and SHA-256. if err != nil { return nil, err } opened, err := file.Stat() if err != nil || !os.SameFile(info, opened) { - file.Close() - return nil, errors.New("executable path changed while it was opened") + return nil, joinCloseError(errors.Join(err, errors.New("executable path changed while it was opened")), file) } if err := provider.validateOpenedExecutable(file); err != nil { - file.Close() - return nil, err + return nil, joinCloseError(err, file) } return file, nil } @@ -538,7 +535,7 @@ func (provider *retrievalProvider) validateOpenedExecutable(file *os.File) error if hex.EncodeToString(digest.Sum(nil)) != provider.executableSHA256 { return errors.New("executable SHA-256 does not match the operator pin") } - pathInfo, err := os.Lstat(provider.executable) + pathInfo, err := os.Lstat(provider.executable) // #nosec G703 -- operator-pinned executable is absolute, has symlink-free parents, and is checked for immutable inode identity and SHA-256. if err != nil || pathInfo.Mode()&os.ModeSymlink != 0 || !os.SameFile(after, pathInfo) { return errors.New("configured executable path no longer names the opened inode") } @@ -547,7 +544,8 @@ func (provider *retrievalProvider) validateOpenedExecutable(file *os.File) error } func (provider *retrievalProvider) retrievalCommand(ctx context.Context, executableFile *os.File, arguments []string) *exec.Cmd { - command := exec.CommandContext(ctx, retrievalExecutableFDPath, arguments...) + command := exec.CommandContext(ctx, "/proc/self/fd/3") // #nosec G702 -- fixed descriptor path executes the immutable SHA-256-pinned binary; caller text remains separate argument values. + command.Args = append(command.Args, arguments...) command.Dir = "/" command.ExtraFiles = []*os.File{executableFile} command.Env = []string{"LANG=C", "LC_ALL=C", "TZ=UTC", "PATH=/usr/bin:/bin"} diff --git a/tools/knowledge-mcp/retrieval_federation.go b/tools/knowledge-mcp/retrieval_federation.go index 40a2f72..e9f0950 100644 --- a/tools/knowledge-mcp/retrieval_federation.go +++ b/tools/knowledge-mcp/retrieval_federation.go @@ -205,7 +205,7 @@ func (provider *retrievalProvider) loadRetrievalCatalog() (*retrievalFederationC if err := validateDirectoryPathNoSymlinks(filepath.Dir(provider.catalog)); err != nil { return nil, err } - info, err := os.Lstat(provider.catalog) + info, err := os.Lstat(provider.catalog) // #nosec G703 -- catalog is the operator-selected absolute path; regular-file, symlink, permission and SHA-256 checks follow. if err != nil || info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() || info.Mode().Perm()&0o222 != 0 { return nil, errors.New("catalog is not an immutable regular file") } diff --git a/tools/knowledge-mcp/security_cleanup.go b/tools/knowledge-mcp/security_cleanup.go new file mode 100644 index 0000000..6bf989b --- /dev/null +++ b/tools/knowledge-mcp/security_cleanup.go @@ -0,0 +1,44 @@ +package main + +import ( + "errors" + "fmt" + "io" + "os" +) + +func joinCloseError(cause error, closer io.Closer) error { + if err := closer.Close(); err != nil { + return errors.Join(cause, fmt.Errorf("close resource: %w", err)) + } + return cause +} + +func removeFileError(cause error, path string) error { + if err := os.Remove(path); err != nil { + return errors.Join(cause, fmt.Errorf("remove temporary file: %w", err)) + } + return cause +} + +func openBoundDirectoryRoot(path string) (*os.Root, error) { + before, err := os.Lstat(path) // #nosec G703 -- caller-selected directory is bound to the opened root inode below. + if err != nil { + return nil, err + } + if !before.IsDir() || before.Mode()&os.ModeSymlink != 0 { + return nil, fmt.Errorf("directory root is not a non-symlink directory") + } + root, err := os.OpenRoot(path) + if err != nil { + return nil, err + } + opened, err := root.Stat(".") + if err != nil { + return nil, joinCloseError(err, root) + } + if !os.SameFile(before, opened) { + return nil, joinCloseError(fmt.Errorf("directory root changed while opening"), root) + } + return root, nil +} diff --git a/tools/knowledge-mcp/security_cleanup_test.go b/tools/knowledge-mcp/security_cleanup_test.go new file mode 100644 index 0000000..0ec9f93 --- /dev/null +++ b/tools/knowledge-mcp/security_cleanup_test.go @@ -0,0 +1,282 @@ +package main + +import ( + "encoding/json" + "errors" + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +type failingCloser struct{ failure error } + +func (closer failingCloser) Close() error { return closer.failure } + +func TestJoinCloseError(t *testing.T) { + primary := errors.New("primary") + closing := errors.New("closing") + for _, cause := range []error{nil, primary} { + err := joinCloseError(cause, failingCloser{closing}) + if !errors.Is(err, closing) || (cause != nil && !errors.Is(err, cause)) { + t.Fatalf("joined error %v lost cause %v or close error", err, cause) + } + } + if err := joinCloseError(nil, io.NopCloser(nil)); err != nil { + t.Fatal(err) + } +} + +func TestDirectoryTreeRejectsSymlinkRoot(t *testing.T) { + outside := t.TempDir() + if err := os.Chmod(outside, 0o700); err != nil { + t.Fatal(err) + } + link := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(outside, link); err != nil { + t.Fatal(err) + } + if err := sealDirectoryTree(link); err == nil { + t.Error("accepted symlink tree root") + } + info, err := os.Stat(outside) + if err != nil || info.Mode().Perm() != 0o700 { + t.Fatalf("outside mode changed: %v %v", info, err) + } +} + +func TestDirectoryTreeModesAndBoundaries(t *testing.T) { + root := t.TempDir() + child := filepath.Join(root, "child") + if err := os.Mkdir(child, 0o700); err != nil { + t.Fatal(err) + } + for _, mode := range []os.FileMode{publishSealedDirMode, publishDirMode} { + if err := setDirectoryTreeMode(root, mode); err != nil { + t.Fatal(err) + } + for _, path := range []string{root, child} { + info, err := os.Stat(path) + if err != nil || info.Mode().Perm() != mode { + t.Fatalf("mode %s: %v %v", path, info, err) + } + } + } + if err := setDirectoryTreeMode(filepath.Join(root, "absent"), publishDirMode); err != nil { + t.Fatal(err) + } + file := filepath.Join(root, "file") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := setDirectoryTreeMode(file, publishDirMode); err == nil { + t.Error("accepted regular file as tree") + } +} + +func TestRootedDirectoryModeRejectsEscape(t *testing.T) { + outside := t.TempDir() + if err := os.Chmod(outside, 0o700); err != nil { + t.Fatal(err) + } + directory := t.TempDir() + if err := os.Symlink(outside, filepath.Join(directory, "escape")); err != nil { + t.Fatal(err) + } + root, err := os.OpenRoot(directory) + if err != nil { + t.Fatal(err) + } + defer func() { + if err := root.Close(); err != nil { + t.Error(err) + } + }() + if err := chmodRootedDirectory(root, "escape", 0o777); err == nil { + t.Error("accepted outside symlink") + } + if err := chmodRootedDirectory(root, "../escape", 0o777); err == nil { + t.Error("accepted parent escape") + } + info, err := os.Stat(outside) + if err != nil || info.Mode().Perm() != 0o700 { + t.Fatalf("outside changed: %v %v", info, err) + } +} + +func TestRemoveFileError(t *testing.T) { + primary := errors.New("primary") + path := filepath.Join(t.TempDir(), "temporary") + if err := os.WriteFile(path, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := removeFileError(primary, path); !errors.Is(err, primary) { + t.Fatal(err) + } + err := removeFileError(primary, path) + if !errors.Is(err, primary) || !errors.Is(err, os.ErrNotExist) { + t.Fatalf("lost cleanup cause: %v", err) + } +} + +func TestPublishStatusErrorsAndPermissions(t *testing.T) { + root := t.TempDir() + if err := writePublishStatus(root, nil, errors.New("failure")); err != nil { + t.Fatal(err) + } + info, err := os.Stat(filepath.Join(root, publishStatusFileName)) + if err != nil || info.Mode().Perm() != 0o600 { + t.Fatalf("status mode: %v %v", info, err) + } + if err := writePublishStatus(filepath.Join(root, "missing"), nil, nil); err == nil { + t.Error("lost status write failure") + } +} + +func TestPublishCleanupFailureClearsPublishedTrees(t *testing.T) { + target := t.TempDir() + published := filepath.Join(target, publishStateDirName) + if err := os.Mkdir(published, 0o700); err != nil { + t.Fatal(err) + } + failure := errors.New("staging cleanup failed") + status := &publishStatus{State: "published"} + err := finishPublishMaterialization(target, "staging", status, nil, func(string) error { return failure }) + if !errors.Is(err, failure) { + t.Fatalf("lost staging cleanup failure: %v", err) + } + if _, err := os.Stat(published); !errors.Is(err, os.ErrNotExist) { + t.Errorf("published tree survived failure: %v", err) + } + raw, err := os.ReadFile(filepath.Join(target, publishStatusFileName)) + if err != nil { + t.Fatal(err) + } + var record publishStatus + if err := json.Unmarshal(raw, &record); err != nil { + t.Fatal(err) + } + if record.State != "degraded" || record.Reason != failure.Error() { + t.Errorf("status mismatch: %+v", record) + } +} + +func TestPublishCleanupSuccessKeepsPublishedTrees(t *testing.T) { + target := t.TempDir() + published := filepath.Join(target, publishStateDirName) + if err := os.Mkdir(published, 0o700); err != nil { + t.Fatal(err) + } + status := &publishStatus{State: "published"} + if err := finishPublishMaterialization(target, "staging", status, nil, func(string) error { return nil }); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(published); err != nil { + t.Fatal(err) + } +} + +func placementFixture(t *testing.T) (string, string) { + t.Helper() + payload, target := t.TempDir(), t.TempDir() + for _, name := range []string{publishStateDirName, publishTrustDirName} { + if err := os.Mkdir(filepath.Join(payload, name), 0o700); err != nil { + t.Fatal(err) + } + } + return payload, target +} + +func assertDegradedPublication(t *testing.T, target string) { + t.Helper() + for _, name := range []string{publishStateDirName, publishTrustDirName, publishRetrievalDirName} { + if _, err := os.Lstat(filepath.Join(target, name)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("published %s survived failure: %v", name, err) + } + } + raw, err := os.ReadFile(filepath.Join(target, publishStatusFileName)) + if err != nil { + t.Fatal(err) + } + var record publishStatus + if err := json.Unmarshal(raw, &record); err != nil { + t.Fatal(err) + } + if record.State != "degraded" { + t.Errorf("status mismatch: %+v", record) + } +} + +func TestRetrievalSealFailureDegradesPublication(t *testing.T) { + payload, target := placementFixture(t) + outside := t.TempDir() + if err := os.Chmod(outside, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(payload, publishRetrievalDirName)); err != nil { + t.Fatal(err) + } + status := &publishStatus{State: "published"} + cause := placePublishPayload(config{}, payload, target, status) + err := finishPublishMaterialization(target, "unused", status, cause, func(string) error { return nil }) + if err == nil { + t.Error("retrieval sealing failure was not propagated") + } + assertDegradedPublication(t, target) + info, err := os.Stat(outside) + if err != nil || info.Mode().Perm() != 0o700 { + t.Errorf("outside directory changed: %v %v", info, err) + } +} + +func TestUnpinnedRetrievalPublication(t *testing.T) { + payload, target := placementFixture(t) + if err := os.Mkdir(filepath.Join(payload, publishRetrievalDirName), 0o700); err != nil { + t.Fatal(err) + } + status := &publishStatus{State: "published"} + if err := placePublishPayload(config{}, payload, target, status); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := unsealDirectoryTree(target); err != nil { + t.Error(err) + } + }) + if status.RetrievalState != "published_unverified: no catalog pin was configured" { + t.Fatal(status.RetrievalState) + } +} + +func TestRetrievalPlacementRenameFailure(t *testing.T) { + target := t.TempDir() + state, err := placeRetrievalTree(config{}, filepath.Join(target, "missing"), target) + if err == nil || !errors.Is(err, os.ErrNotExist) { + t.Fatalf("rename failure: %q %v", state, err) + } + if state != "" { + t.Fatalf("failed placement reported state: %q", state) + } +} + +func TestRejectedRetrievalCatalogRemovesTree(t *testing.T) { + payload, target := placementFixture(t) + retrieval := filepath.Join(payload, publishRetrievalDirName) + if err := os.MkdirAll(filepath.Join(retrieval, "nested"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(retrieval, "nested", "fixture"), nil, 0o600); err != nil { + t.Fatal(err) + } + state, err := placeRetrievalTree(config{retrievalCatalog: filepath.Join(target, "missing-catalog"), retrievalCatalogHash: strings.Repeat("0", 64)}, retrieval, target) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(state, "unavailable: catalog verification failed:") { + t.Fatal(state) + } + if _, err := os.Lstat(filepath.Join(target, publishRetrievalDirName)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("rejected retrieval tree survived: %v", err) + } +} diff --git a/tools/knowledge-registry/p1-src03-acquisition-runner/checked_io.go b/tools/knowledge-registry/p1-src03-acquisition-runner/checked_io.go new file mode 100644 index 0000000..b977bb1 --- /dev/null +++ b/tools/knowledge-registry/p1-src03-acquisition-runner/checked_io.go @@ -0,0 +1,30 @@ +package main + +import ( + "errors" + "fmt" + "io" + "math" + "syscall" +) + +func checkedUID(value int) (uint32, error) { + if value < 0 || uint64(value) > math.MaxUint32 { + return 0, fmt.Errorf("effective user id %d is outside uint32 range", value) + } + return uint32(value), nil +} + +func closeWithError(primary error, closer io.Closer) error { + if err := closer.Close(); err != nil { + return errors.Join(primary, fmt.Errorf("close resource: %w", err)) + } + return primary +} + +func closeDescriptorWithError(primary error, descriptor int) error { + if err := syscall.Close(descriptor); err != nil { + return errors.Join(primary, fmt.Errorf("close descriptor: %w", err)) + } + return primary +} diff --git a/tools/knowledge-registry/p1-src03-acquisition-runner/checked_io_test.go b/tools/knowledge-registry/p1-src03-acquisition-runner/checked_io_test.go new file mode 100644 index 0000000..c4eda17 --- /dev/null +++ b/tools/knowledge-registry/p1-src03-acquisition-runner/checked_io_test.go @@ -0,0 +1,77 @@ +package main + +import ( + "errors" + "math" + "strconv" + "testing" +) + +type failingCloser struct { + err error + calls int +} + +func (c *failingCloser) Close() error { c.calls++; return c.err } + +func TestCheckedUID(t *testing.T) { + cases := []struct { + name string + value int + want uint32 + bad bool + }{ + {"negative", -1, 0, true}, {"zero", 0, 0, false}, {"positive", 42, 42, false}, + } + if strconv.IntSize == 64 { + cases = append(cases, + struct { + name string + value int + want uint32 + bad bool + }{"maximum", math.MaxUint32, math.MaxUint32, false}, + struct { + name string + value int + want uint32 + bad bool + }{"overflow", math.MaxUint32 + 1, 0, true}) + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := checkedUID(tc.value) + if (err != nil) != tc.bad || got != tc.want { + t.Fatalf("checkedUID(%d) = %d, %v", tc.value, got, err) + } + }) + } +} + +func TestCloseWithError(t *testing.T) { + primary := errors.New("operation") + cleanup := errors.New("cleanup") + cases := []struct { + name string + primary, cleanup error + }{ + {"success", nil, nil}, {"primary", primary, nil}, {"cleanup", nil, cleanup}, {"both", primary, cleanup}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + closer := &failingCloser{err: tc.cleanup} + got := closeWithError(tc.primary, closer) + if closer.calls != 1 { + t.Fatalf("close calls = %d", closer.calls) + } + if (got == nil) != (tc.primary == nil && tc.cleanup == nil) { + t.Fatalf("error = %v", got) + } + for _, want := range []error{tc.primary, tc.cleanup} { + if want != nil && !errors.Is(got, want) { + t.Fatalf("lost error %v: %v", want, got) + } + } + }) + } +} diff --git a/tools/knowledge-registry/p1-src03-acquisition-runner/http_error_test.go b/tools/knowledge-registry/p1-src03-acquisition-runner/http_error_test.go new file mode 100644 index 0000000..b89e426 --- /dev/null +++ b/tools/knowledge-registry/p1-src03-acquisition-runner/http_error_test.go @@ -0,0 +1,71 @@ +package main + +import ( + "context" + "errors" + "io" + "net/http" + "net/url" + "strings" + "testing" +) + +type failedResponseBody struct{ err error } + +func (b failedResponseBody) Read([]byte) (int, error) { return 0, b.err } +func (b failedResponseBody) Close() error { return nil } + +func TestGitObjectStatusError(t *testing.T) { + failedRead := errors.New("response read failed") + cases := []struct { + name string + status int + body io.ReadCloser + want error + }{ + {"success", http.StatusOK, io.NopCloser(strings.NewReader("{}")), nil}, + {"http failure", http.StatusForbidden, io.NopCloser(strings.NewReader("denied")), nil}, + {"drain failure", http.StatusForbidden, failedResponseBody{failedRead}, failedRead}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := gitObjectStatusError(&http.Response{StatusCode: tc.status, Body: tc.body}) + if (got == nil) != (tc.status == http.StatusOK) { + t.Fatalf("status error = %v", got) + } + if tc.want != nil && !errors.Is(got, tc.want) { + t.Fatalf("lost drain error: %v", got) + } + }) + } +} + +type responseTransport func(*http.Request) (*http.Response, error) + +func (f responseTransport) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +type closeErrorBody struct { + io.Reader + err error +} + +func (b closeErrorBody) Close() error { return b.err } + +func TestGetJSONPreservesResponseCloseError(t *testing.T) { + cleanup := errors.New("response close failed") + for _, closeErr := range []error{nil, cleanup} { + origin, err := url.Parse("https://api.github.com") + if err != nil { + t.Fatal(err) + } + client := &http.Client{Transport: responseTransport(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Header: http.Header{"Content-Type": []string{"application/json"}}, Request: req, Body: closeErrorBody{strings.NewReader("{}"), closeErr}}, nil + })} + r := &runner{client: client, baseURL: origin, allowedRepos: map[string]struct{}{"argoproj/argo-helm": {}}} + err = r.getJSON(context.Background(), "argoproj/argo-helm", "/git/test", &map[string]any{}) + + if !errors.Is(err, closeErr) { + t.Fatalf("response close error = %v, want %v", err, closeErr) + } + } +} diff --git a/tools/knowledge-registry/p1-src03-acquisition-runner/runner.go b/tools/knowledge-registry/p1-src03-acquisition-runner/runner.go index 1070cc9..ce52067 100644 --- a/tools/knowledge-registry/p1-src03-acquisition-runner/runner.go +++ b/tools/knowledge-registry/p1-src03-acquisition-runner/runner.go @@ -3,7 +3,7 @@ package main import ( "bytes" "context" - "crypto/sha1" + "crypto/sha1" // #nosec G505 -- Git blob object identifiers require SHA-1; content integrity is also bound by SHA-256. "crypto/sha256" "encoding/base64" "encoding/hex" @@ -660,7 +660,16 @@ func (r *runner) getBlob(ctx context.Context, repository string, expected contra if blob.SHA != expected.GitBlobSHA1 || blob.Size == nil || *blob.Size != expected.Bytes || blob.Encoding != "base64" { return nil, errors.New("blob response identity, encoding, or size drifted") } - compact := strings.NewReplacer("\n", "", "\r", "").Replace(blob.Content) + raw, err := decodeGitBlob(blob.Content, expected) + if err != nil { + return nil, err + } + r.blobCache[key] = raw + return raw, nil +} + +func decodeGitBlob(content string, expected contractFile) ([]byte, error) { + compact := strings.NewReplacer("\n", "", "\r", "").Replace(content) raw, err := base64.StdEncoding.Strict().DecodeString(compact) if err != nil { return nil, errors.New("blob content is not strict base64") @@ -669,7 +678,7 @@ func (r *runner) getBlob(ctx context.Context, repository string, expected contra return nil, errors.New("decoded blob size drifted") } identity := append([]byte("blob "+strconv.Itoa(len(raw))+"\x00"), raw...) - gitDigest := sha1.Sum(identity) + gitDigest := sha1.Sum(identity) // #nosec G401 -- Reconstruct the protocol-defined Git blob SHA-1 identity; SHA-256 separately binds the artifact bytes. if hex.EncodeToString(gitDigest[:]) != expected.GitBlobSHA1 { return nil, errors.New("decoded blob does not reproduce the exact Git SHA-1") } @@ -677,11 +686,10 @@ func (r *runner) getBlob(ctx context.Context, repository string, expected contra if "sha256:"+hex.EncodeToString(rawDigest[:]) != expected.RawSHA256 { return nil, errors.New("decoded blob raw SHA-256 drifted") } - r.blobCache[key] = raw return raw, nil } -func (r *runner) getJSON(ctx context.Context, repository, suffix string, destination any) error { +func (r *runner) getJSON(ctx context.Context, repository, suffix string, destination any) (err error) { if _, ok := r.allowedRepos[repository]; !ok { return errors.New("repository is outside the exact three-source denominator") } @@ -689,7 +697,7 @@ func (r *runner) getJSON(ctx context.Context, repository, suffix string, destina return errors.New("HTTP request is outside the exact capability") } endpoint := r.baseURL.String() + "/repos/" + repository + suffix - request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) // #nosec G704 -- newRunner pins the production API origin to api.github.com and forbids redirects before sending; alternate origins require explicit test mode. if err != nil { return err } @@ -701,32 +709,17 @@ func (r *runner) getJSON(ctx context.Context, repository, suffix string, destina request.Header.Set("Authorization", "Bearer "+r.token) } r.requests++ - response, err := r.client.Do(request) + response, err := r.client.Do(request) // #nosec G704 -- newRunner pins the production API origin to api.github.com and forbids redirects before sending; alternate origins require explicit test mode. if err != nil { return err } - defer response.Body.Close() - if response.StatusCode != http.StatusOK { - _, _ = io.Copy(io.Discard, io.LimitReader(response.Body, 4096)) - return fmt.Errorf("GitHub REST returned HTTP %d", response.StatusCode) - } - if response.Request == nil || response.Request.URL.String() != endpoint { - return errors.New("Git-object response arrived through a redirected or changed endpoint") - } - if len(response.Header.Values("Link")) != 0 { - return errors.New("Git-object endpoint returned a pagination Link header") - } - encodings := response.Header.Values("Content-Encoding") - if len(encodings) > 1 || (len(encodings) == 1 && encodings[0] != "identity") { - return errors.New("Git-object response used an unexpected content encoding") - } - contentTypes := response.Header.Values("Content-Type") - if len(contentTypes) != 1 { - return errors.New("Git-object response has an ambiguous or missing content type") - } - contentType, _, err := mime.ParseMediaType(contentTypes[0]) - if err != nil || contentType != "application/json" { - return errors.New("Git-object response is not application/json") + defer func() { err = closeWithError(err, response.Body) }() + return r.readGitObjectResponse(response, endpoint, destination) +} + +func (r *runner) readGitObjectResponse(response *http.Response, endpoint string, destination any) error { + if err := validateGitObjectResponse(response, endpoint); err != nil { + return err } remaining := maxResponseBytes - r.responseBytes if remaining <= 0 { @@ -753,6 +746,48 @@ func (r *runner) getJSON(ctx context.Context, repository, suffix string, destina return nil } +func validateGitObjectResponse(response *http.Response, endpoint string) error { + if err := gitObjectStatusError(response); err != nil { + return err + } + + if response.Request == nil || response.Request.URL.String() != endpoint { + return errors.New("Git-object response arrived through a redirected or changed endpoint") + } + if len(response.Header.Values("Link")) != 0 { + return errors.New("Git-object endpoint returned a pagination Link header") + } + encodings := response.Header.Values("Content-Encoding") + if len(encodings) > 1 || (len(encodings) == 1 && encodings[0] != "identity") { + return errors.New("Git-object response used an unexpected content encoding") + } + return validateGitObjectContentType(response) +} + +func validateGitObjectContentType(response *http.Response) error { + contentTypes := response.Header.Values("Content-Type") + if len(contentTypes) != 1 { + return errors.New("Git-object response has an ambiguous or missing content type") + } + contentType, _, err := mime.ParseMediaType(contentTypes[0]) + if err != nil || contentType != "application/json" { + return errors.New("Git-object response is not application/json") + } + + return nil +} + +func gitObjectStatusError(response *http.Response) error { + if response.StatusCode != http.StatusOK { + _, drainErr := io.Copy(io.Discard, io.LimitReader(response.Body, 4096)) + if drainErr != nil { + drainErr = fmt.Errorf("drain Git-object error response: %w", drainErr) + } + return errors.Join(fmt.Errorf("GitHub REST returned HTTP %d", response.StatusCode), drainErr) + } + return nil +} + func exactTreeEntry(entries []gitTreeEntry, name string) (gitTreeEntry, error) { seen := make(map[string]struct{}, len(entries)) var match *gitTreeEntry diff --git a/tools/knowledge-registry/p1-src03-acquisition-runner/store.go b/tools/knowledge-registry/p1-src03-acquisition-runner/store.go index 7223c7d..b559488 100644 --- a/tools/knowledge-registry/p1-src03-acquisition-runner/store.go +++ b/tools/knowledge-registry/p1-src03-acquisition-runner/store.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "slices" "sort" "strings" "syscall" @@ -32,70 +33,89 @@ func newCASStore(root string) (*casStore, error) { if absolute == string(os.PathSeparator) { return nil, errors.New("output root must not be the filesystem root") } - parent, err := openDirectoryPathNoFollow(filepath.Dir(absolute)) - if err != nil { - return nil, fmt.Errorf("output parent: %w", err) - } - defer parent.Close() - leaf := filepath.Base(absolute) - rootFile, err := openDirectoryAt(parent, leaf) - if errors.Is(err, os.ErrNotExist) { - if err := syscall.Mkdirat(int(parent.Fd()), leaf, 0o700); err != nil && !errors.Is(err, syscall.EEXIST) { - return nil, fmt.Errorf("create output root: %w", err) - } - if err := syncOpenedFile(parent); err != nil { - return nil, fmt.Errorf("persist output root: %w", err) - } - rootFile, err = openDirectoryAt(parent, leaf) - } + rootFile, err := openCASRoot(absolute) if err != nil { - return nil, fmt.Errorf("inspect output root: %w", err) + return nil, err } info, err := rootFile.Stat() if err != nil { - rootFile.Close() - return nil, fmt.Errorf("inspect output root: %w", err) + return nil, closeWithError(fmt.Errorf("inspect output root: %w", err), rootFile) } if err := validateOwnedDirectory(info, "output root"); err != nil { - rootFile.Close() - return nil, err + return nil, closeWithError(err, rootFile) } store := &casStore{root: absolute, rootFile: rootFile, rootInfo: info} if err := store.verifyRootPath(); err != nil { - rootFile.Close() - return nil, err + return nil, closeWithError(err, rootFile) } - receipts, _, err := store.listOptionalPublishedDirectory("receipts", 2) - if err != nil { - rootFile.Close() - return nil, fmt.Errorf("preflight receipts: %w", err) + if err := store.initializeDirectories(); err != nil { + return nil, closeWithError(err, rootFile) } - proposals, _, err := store.listOptionalPublishedDirectory("inventory-proposals", 2) + return store, nil +} + +func (s *casStore) initializeDirectories() error { + recoveryState, err := s.hasRecoveryState() if err != nil { - rootFile.Close() - return nil, fmt.Errorf("preflight inventory proposals: %w", err) - } - if len(receipts) == 0 && len(proposals) != 0 { - rootFile.Close() - return nil, errors.New("inventory proposal exists without its predecessor receipt") + return err } - recoveryState := len(receipts) != 0 || len(proposals) != 0 for _, relative := range []string{"cas", "cas/sha256", "receipts", "inventory-proposals"} { if recoveryState { - directory, _, openErr := store.openStoreDirectory(relative, false) + directory, _, openErr := s.openStoreDirectory(relative, false) if openErr == nil { - directory.Close() + if err := closeWithError(nil, directory); err != nil { + return err + } continue } - rootFile.Close() - return nil, fmt.Errorf("recovery state is missing required directory %s: %w", relative, openErr) + return fmt.Errorf("recovery state is missing required directory %s: %w", relative, openErr) } - if _, err := store.ensureDirectory(relative); err != nil { - rootFile.Close() - return nil, err + if _, err := s.ensureDirectory(relative); err != nil { + return err } } - return store, nil + return nil +} + +func (s *casStore) hasRecoveryState() (bool, error) { + receipts, _, err := s.listOptionalPublishedDirectory("receipts", 2) + if err != nil { + return false, fmt.Errorf("preflight receipts: %w", err) + } + proposals, _, err := s.listOptionalPublishedDirectory("inventory-proposals", 2) + if err != nil { + return false, fmt.Errorf("preflight inventory proposals: %w", err) + } + if len(receipts) == 0 && len(proposals) != 0 { + return false, errors.New("inventory proposal exists without its predecessor receipt") + } + return len(receipts) != 0 || len(proposals) != 0, nil +} + +func openCASRoot(absolute string) (file *os.File, err error) { + parent, err := openDirectoryPathNoFollow(filepath.Dir(absolute)) + if err != nil { + return nil, fmt.Errorf("output parent: %w", err) + } + defer func() { + err = closeWithError(err, parent) + if err != nil && file != nil { + err = closeWithError(err, file) + file = nil + } + }() + leaf := filepath.Base(absolute) + file, err = openDirectoryAt(parent, leaf) + if !errors.Is(err, os.ErrNotExist) { + return file, err + } + if err := syscall.Mkdirat(int(parent.Fd()), leaf, 0o700); err != nil && !errors.Is(err, syscall.EEXIST) { + return nil, fmt.Errorf("create output root: %w", err) + } + if err := syncOpenedFile(parent); err != nil { + return nil, fmt.Errorf("persist output root: %w", err) + } + return openDirectoryAt(parent, leaf) } func (s *casStore) listOptionalPublishedDirectory(relative string, maximum int) ([]string, bool, error) { @@ -162,12 +182,12 @@ func (s *casStore) putProposal(proposalID string, raw []byte) (string, error) { return relative, nil } -func (s *casStore) ensureDirectory(relative string) (string, error) { +func (s *casStore) ensureDirectory(relative string) (result string, err error) { directory, _, err := s.openStoreDirectory(relative, true) if err != nil { return "", err } - defer directory.Close() + defer func() { err = closeWithError(err, directory) }() return filepath.Join(s.root, filepath.FromSlash(relative)), nil } @@ -209,7 +229,7 @@ func (s *casStore) verifyRunSet(receiptPath string, receiptRaw []byte, proposalP if err != nil { return err } - if len(receipts) != 1 || receipts[0] != filepath.Base(receiptPath) || len(proposals) != 1 || proposals[0] != filepath.Base(proposalPath) { + if !slices.Equal(receipts, []string{filepath.Base(receiptPath)}) || !slices.Equal(proposals, []string{filepath.Base(proposalPath)}) { return errors.New("published receipt/proposal set is incomplete or ambiguous") } actualReceipt, err := s.readPublished(receiptPath, maxPublishedJSONBytes) @@ -223,7 +243,7 @@ func (s *casStore) verifyRunSet(receiptPath string, receiptRaw []byte, proposalP return nil } -func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) error { +func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) (err error) { if int64(len(raw)) > maximum { return errors.New("published object exceeds its byte bound") } @@ -236,7 +256,7 @@ func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) e if err != nil { return err } - defer parent.Close() + defer func() { err = closeWithError(err, parent) }() leaf := filepath.Base(relative) descriptor, err := syscall.Openat(int(parent.Fd()), leaf, syscall.O_WRONLY|syscall.O_CREAT|syscall.O_EXCL|syscall.O_CLOEXEC|syscall.O_NOFOLLOW, 0o600) if err != nil { @@ -247,39 +267,50 @@ func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) e } file := os.NewFile(uintptr(descriptor), leaf) if file == nil { - syscall.Close(descriptor) - return errors.New("could not own exclusive published object descriptor") + return closeDescriptorWithError(errors.New("could not own exclusive published object descriptor"), descriptor) } + written, err := writePublishedContents(file, raw) + if err != nil { + return closeWithError(err, file) + } + return closeWithError(s.commitPublishedContents(parentRelative, parent, parentInfo, leaf, raw, maximum, written), file) +} + +func (s *casStore) commitPublishedContents(parentRelative string, parent *os.File, parentInfo os.FileInfo, leaf string, raw []byte, maximum int64, written os.FileInfo) error { + if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed before directory commit"); err != nil { + return err + } + if err := syncOpenedFile(parent); err != nil { + return err + } + if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed after directory commit"); err != nil { + return err + } + if err := s.verifyRootPath(); err != nil { + return err + } + return nil +} + +func writePublishedContents(file *os.File, raw []byte) (os.FileInfo, error) { created, err := file.Stat() if err != nil { - return errors.Join(err, file.Close()) + return nil, err } if err := validateOwnedRegular(created, "new published object"); err != nil { - return errors.Join(err, file.Close()) + return nil, err } if err := writeAll(file, raw); err != nil { - return errors.Join(err, file.Close()) + return nil, err } if err := syncOpenedFile(file); err != nil { - return errors.Join(err, file.Close()) + return nil, err } written, err := file.Stat() if err != nil || !os.SameFile(created, written) || written.Size() != int64(len(raw)) { - return errors.Join(errors.New("exclusive published object write is incomplete or unstable"), err, file.Close()) - } - if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed before directory commit"); err != nil { - return errors.Join(err, file.Close()) - } - if err := syncOpenedFile(parent); err != nil { - return errors.Join(err, file.Close()) - } - if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed after directory commit"); err != nil { - return errors.Join(err, file.Close()) - } - if err := s.verifyRootPath(); err != nil { - return errors.Join(err, file.Close()) + return nil, errors.Join(errors.New("exclusive published object write is incomplete or unstable"), err) } - return file.Close() + return written, nil } func (s *casStore) rehabilitateExisting(parentRelative string, parent *os.File, parentInfo os.FileInfo, leaf string, raw []byte, maximum int64) error { @@ -299,24 +330,12 @@ func (s *casStore) rehabilitateExisting(parentRelative string, parent *os.File, return s.verifyRootPath() } -func verifyPublishedInDirectory(parent *os.File, leaf string, raw []byte, maximum int64, required os.FileInfo, synchronize bool, conflict string) (os.FileInfo, error) { - access := syscall.O_RDONLY - if synchronize { - access = syscall.O_RDWR - } - descriptor, err := syscall.Openat(int(parent.Fd()), leaf, access|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) +func verifyPublishedInDirectory(parent *os.File, leaf string, raw []byte, maximum int64, required os.FileInfo, synchronize bool, conflict string) (info os.FileInfo, err error) { + file, err := openPublishedFile(parent, leaf, synchronize) if err != nil { - if errors.Is(err, syscall.ELOOP) { - return nil, errors.New("published object path exists and is not a regular file") - } return nil, err } - file := os.NewFile(uintptr(descriptor), leaf) - if file == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own published object file descriptor") - } - defer file.Close() + defer func() { err = closeWithError(err, file) }() before, err := file.Stat() if err != nil { return nil, err @@ -330,42 +349,73 @@ func verifyPublishedInDirectory(parent *os.File, leaf string, raw []byte, maximu if before.Size() > maximum { return nil, errors.New("published object exceeds its byte bound") } - existing, err := io.ReadAll(io.LimitReader(file, maximum+1)) + existing, after, err := readStablePublished(file, before, maximum) + if err != nil { + return nil, err + } + if !bytes.Equal(existing, raw) { + return nil, errors.New(conflict) + } + if synchronize { + return synchronizePublished(file, after) + } + return after, nil +} + +func openPublishedFile(parent *os.File, leaf string, synchronize bool) (*os.File, error) { + access := syscall.O_RDONLY + if synchronize { + access = syscall.O_RDWR + } + descriptor, err := syscall.Openat(int(parent.Fd()), leaf, access|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) if err != nil { + if errors.Is(err, syscall.ELOOP) { + return nil, errors.New("published object path exists and is not a regular file") + } return nil, err } + file := os.NewFile(uintptr(descriptor), leaf) + if file == nil { + return nil, closeDescriptorWithError(errors.New("could not own published object file descriptor"), descriptor) + } + return file, nil +} + +func readStablePublished(file *os.File, before os.FileInfo, maximum int64) ([]byte, os.FileInfo, error) { + existing, err := io.ReadAll(io.LimitReader(file, maximum+1)) + if err != nil { + return nil, nil, err + } if int64(len(existing)) > maximum { - return nil, errors.New("published object exceeds its byte bound") + return nil, nil, errors.New("published object exceeds its byte bound") } after, err := file.Stat() if err != nil { - return nil, err + return nil, nil, err } if !os.SameFile(before, after) || before.Size() != after.Size() || before.ModTime() != after.ModTime() { - return nil, errors.New("published object changed while it was read") + return nil, nil, errors.New("published object changed while it was read") } - if !bytes.Equal(existing, raw) { - return nil, errors.New(conflict) + return existing, after, nil +} + +func synchronizePublished(file *os.File, after os.FileInfo) (os.FileInfo, error) { + if err := syncOpenedFile(file); err != nil { + return nil, err } - if synchronize { - if err := syncOpenedFile(file); err != nil { - return nil, err - } - afterSync, err := file.Stat() - if err != nil || !os.SameFile(after, afterSync) || after.Size() != afterSync.Size() || after.ModTime() != afterSync.ModTime() { - return nil, errors.Join(errors.New("published object changed while it was synchronized"), err) - } - after = afterSync + afterSync, err := file.Stat() + if err != nil || !os.SameFile(after, afterSync) || after.Size() != afterSync.Size() || after.ModTime() != afterSync.ModTime() { + return nil, errors.Join(errors.New("published object changed while it was synchronized"), err) } - return after, nil + return afterSync, nil } -func (s *casStore) verifyPublishedAtPath(parentRelative string, requiredParent os.FileInfo, leaf string, raw []byte, maximum int64, requiredFile os.FileInfo, synchronize bool, conflict string) error { +func (s *casStore) verifyPublishedAtPath(parentRelative string, requiredParent os.FileInfo, leaf string, raw []byte, maximum int64, requiredFile os.FileInfo, synchronize bool, conflict string) (err error) { parent, parentInfo, err := s.openStoreDirectory(parentRelative, false) if err != nil { return err } - defer parent.Close() + defer func() { err = closeWithError(err, parent) }() if !os.SameFile(requiredParent, parentInfo) { return errors.New("published object parent changed during commit") } @@ -373,7 +423,7 @@ func (s *casStore) verifyPublishedAtPath(parentRelative string, requiredParent o return err } -func (s *casStore) readPublished(relative string, maximum int64) ([]byte, error) { +func (s *casStore) readPublished(relative string, maximum int64) (result []byte, err error) { clean := filepath.Clean(relative) if clean != relative || filepath.IsAbs(relative) || strings.HasPrefix(clean, "..") { return nil, errors.New("published object path is not canonical") @@ -383,18 +433,28 @@ func (s *casStore) readPublished(relative string, maximum int64) ([]byte, error) if err != nil { return nil, err } - defer parent.Close() + defer func() { err = closeWithError(err, parent) }() leaf := filepath.Base(relative) - descriptor, err := syscall.Openat(int(parent.Fd()), leaf, syscall.O_RDONLY|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) + file, err := openPublishedFile(parent, leaf, false) if err != nil { return nil, err } - file := os.NewFile(uintptr(descriptor), leaf) - if file == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own published object descriptor") + defer func() { err = closeWithError(err, file) }() + before, err := inspectPublished(file, maximum) + if err != nil { + return nil, err + } + raw, after, err := readStablePublished(file, before, maximum) + if err != nil { + return nil, err } - defer file.Close() + if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, after, false, "published object changed during path replay"); err != nil { + return nil, err + } + return raw, nil +} + +func inspectPublished(file *os.File, maximum int64) (os.FileInfo, error) { before, err := file.Stat() if err != nil { return nil, err @@ -405,21 +465,10 @@ func (s *casStore) readPublished(relative string, maximum int64) ([]byte, error) if before.Size() > maximum { return nil, errors.New("published object exceeds its byte bound") } - raw, err := io.ReadAll(io.LimitReader(file, maximum+1)) - if err != nil || int64(len(raw)) > maximum { - return nil, errors.Join(errors.New("published object read exceeds its bound"), err) - } - after, err := file.Stat() - if err != nil || !os.SameFile(before, after) || before.Size() != after.Size() || before.ModTime() != after.ModTime() { - return nil, errors.Join(errors.New("published object changed while it was read"), err) - } - if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, after, false, "published object changed during path replay"); err != nil { - return nil, err - } - return raw, nil + return before, nil } -func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]string, error) { +func (s *casStore) listPublishedDirectory(relative string, maximum int) (result []string, err error) { if maximum < 1 { return nil, errors.New("directory entry bound must be positive") } @@ -427,7 +476,7 @@ func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]strin if err != nil { return nil, err } - defer directory.Close() + defer func() { err = closeWithError(err, directory) }() entries, err := directory.ReadDir(maximum + 1) if err != nil && !errors.Is(err, io.EOF) { return nil, err @@ -435,19 +484,17 @@ func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]strin if len(entries) > maximum { return nil, errors.New("published directory exceeds its entry bound") } - names := make([]string, 0, len(entries)) - for _, entry := range entries { - name := entry.Name() - if name == "" || name == "." || name == ".." || strings.ContainsAny(name, "/\x00") { - return nil, errors.New("published directory contains an unsafe entry name") - } - names = append(names, name) + names, err := publishedEntryNames(entries) + if err != nil { + return nil, err } replayed, replayedInfo, err := s.openStoreDirectory(relative, false) if err != nil { return nil, err } - replayed.Close() + if err := closeWithError(nil, replayed); err != nil { + return nil, err + } if !os.SameFile(directoryInfo, replayedInfo) { return nil, errors.New("published directory changed while it was listed") } @@ -455,6 +502,18 @@ func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]strin return names, nil } +func publishedEntryNames(entries []os.DirEntry) ([]string, error) { + names := make([]string, 0, len(entries)) + for _, entry := range entries { + name := entry.Name() + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, "/\x00") { + return nil, errors.New("published directory contains an unsafe entry name") + } + names = append(names, name) + } + return names, nil +} + func writeAll(file *os.File, raw []byte) error { for len(raw) > 0 { written, err := file.Write(raw) @@ -504,7 +563,11 @@ func validateEffectiveOwner(info os.FileInfo, label string) error { if !ok { return fmt.Errorf("%s ownership cannot be verified", label) } - if stat.Uid != uint32(os.Geteuid()) { + uid, err := checkedUID(os.Geteuid()) + if err != nil { + return fmt.Errorf("%s ownership: %w", label, err) + } + if stat.Uid != uid { return fmt.Errorf("%s is not owned by the effective user", label) } return nil @@ -521,22 +584,22 @@ func openDirectoryPathNoFollow(pathname string) (*os.File, error) { } current := os.NewFile(uintptr(descriptor), string(os.PathSeparator)) if current == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own filesystem-root descriptor") + return nil, closeDescriptorWithError(errors.New("could not own filesystem-root descriptor"), descriptor) } trimmed := strings.TrimPrefix(absolute, string(os.PathSeparator)) if trimmed == "" { return current, nil } for _, component := range strings.Split(trimmed, string(os.PathSeparator)) { - if component == "" || component == "." || component == ".." { - current.Close() - return nil, errors.New("directory path is not canonical") + if !canonicalDirectoryComponent(component) { + return nil, closeWithError(errors.New("directory path is not canonical"), current) } next, err := openDirectoryAt(current, component) - current.Close() - if err != nil { - return nil, err + if closeErr := closeWithError(err, current); closeErr != nil { + if next != nil { + closeErr = closeWithError(closeErr, next) + } + return nil, closeErr } current = next } @@ -550,8 +613,7 @@ func openDirectoryAt(parent *os.File, name string) (*os.File, error) { } file := os.NewFile(uintptr(descriptor), name) if file == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own directory descriptor") + return nil, closeDescriptorWithError(errors.New("could not own directory descriptor"), descriptor) } return file, nil } @@ -564,8 +626,7 @@ func duplicateFile(file *os.File, name string) (*os.File, error) { syscall.CloseOnExec(descriptor) duplicate := os.NewFile(uintptr(descriptor), name) if duplicate == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own duplicated directory descriptor") + return nil, closeDescriptorWithError(errors.New("could not own duplicated directory descriptor"), descriptor) } return duplicate, nil } @@ -579,7 +640,7 @@ func validateStoreRelativeDirectory(relative string) ([]string, error) { } components := strings.Split(filepath.ToSlash(relative), "/") for _, component := range components { - if component == "" || component == "." || component == ".." { + if !canonicalDirectoryComponent(component) { return nil, errors.New("unsafe store directory component") } } @@ -596,41 +657,43 @@ func (s *casStore) openDirectoryFromRoot(relative string, create bool) (*os.File return nil, nil, err } for _, component := range components { - next, openErr := openDirectoryAt(current, component) - if errors.Is(openErr, os.ErrNotExist) && create { - if mkdirErr := syscall.Mkdirat(int(current.Fd()), component, 0o700); mkdirErr != nil && !errors.Is(mkdirErr, syscall.EEXIST) { - current.Close() - return nil, nil, mkdirErr - } - if syncErr := syncOpenedFile(current); syncErr != nil { - current.Close() - return nil, nil, syncErr + next, openErr := openOrCreateDirectoryAt(current, component, create) + if closeErr := closeWithError(openErr, current); closeErr != nil { + if next != nil { + closeErr = closeWithError(closeErr, next) } - next, openErr = openDirectoryAt(current, component) - } - current.Close() - if openErr != nil { - return nil, nil, openErr + return nil, nil, closeErr } info, statErr := next.Stat() if statErr != nil { - next.Close() - return nil, nil, statErr + return nil, nil, closeWithError(statErr, next) } if err := validateOwnedDirectory(info, relative); err != nil { - next.Close() - return nil, nil, err + return nil, nil, closeWithError(err, next) } current = next } info, err := current.Stat() if err != nil { - current.Close() - return nil, nil, err + return nil, nil, closeWithError(err, current) } return current, info, nil } +func openOrCreateDirectoryAt(current *os.File, component string, create bool) (*os.File, error) { + next, openErr := openDirectoryAt(current, component) + if errors.Is(openErr, os.ErrNotExist) && create { + if mkdirErr := syscall.Mkdirat(int(current.Fd()), component, 0o700); mkdirErr != nil && !errors.Is(mkdirErr, syscall.EEXIST) { + return nil, mkdirErr + } + if syncErr := syncOpenedFile(current); syncErr != nil { + return nil, syncErr + } + next, openErr = openDirectoryAt(current, component) + } + return next, openErr +} + func (s *casStore) openStoreDirectory(relative string, create bool) (*os.File, os.FileInfo, error) { if err := s.verifyRootPath(); err != nil { return nil, nil, err @@ -641,22 +704,21 @@ func (s *casStore) openStoreDirectory(relative string, create bool) (*os.File, o } replayed, replayedInfo, err := s.openDirectoryFromRoot(relative, false) if err != nil { - held.Close() - return nil, nil, err + return nil, nil, closeWithError(err, held) + } + if err := closeWithError(nil, replayed); err != nil { + return nil, nil, closeWithError(err, held) } - replayed.Close() if !os.SameFile(heldInfo, replayedInfo) { - held.Close() - return nil, nil, errors.New("store directory changed during path replay") + return nil, nil, closeWithError(errors.New("store directory changed during path replay"), held) } if err := s.verifyRootPath(); err != nil { - held.Close() - return nil, nil, err + return nil, nil, closeWithError(err, held) } return held, heldInfo, nil } -func (s *casStore) verifyRootPath() error { +func (s *casStore) verifyRootPath() (err error) { if s.rootFile == nil { return errors.New("store root descriptor is closed") } @@ -664,7 +726,7 @@ func (s *casStore) verifyRootPath() error { if err != nil { return fmt.Errorf("reopen output root: %w", err) } - defer current.Close() + defer func() { err = closeWithError(err, current) }() info, err := current.Stat() if err != nil { return err @@ -696,3 +758,7 @@ func stripPrefixedID(value, prefix string) (string, error) { } return digest, nil } + +func canonicalDirectoryComponent(component string) bool { + return component != "" && component != "." && component != ".." +} diff --git a/tools/knowledge-registry/source-acquisition-plan-enumerator/checked_io.go b/tools/knowledge-registry/source-acquisition-plan-enumerator/checked_io.go new file mode 100644 index 0000000..86a399b --- /dev/null +++ b/tools/knowledge-registry/source-acquisition-plan-enumerator/checked_io.go @@ -0,0 +1,14 @@ +package main + +import ( + "errors" + "fmt" + "io" +) + +func closeWithError(primary error, closer io.Closer) error { + if err := closer.Close(); err != nil { + return errors.Join(primary, fmt.Errorf("close resource: %w", err)) + } + return primary +} diff --git a/tools/knowledge-registry/source-acquisition-plan-enumerator/enumerator.go b/tools/knowledge-registry/source-acquisition-plan-enumerator/enumerator.go index bde85a5..5674e4a 100644 --- a/tools/knowledge-registry/source-acquisition-plan-enumerator/enumerator.go +++ b/tools/knowledge-registry/source-acquisition-plan-enumerator/enumerator.go @@ -19,7 +19,7 @@ package main import ( "context" - "crypto/sha1" + "crypto/sha1" // #nosec G505 -- Git blob object identifiers require SHA-1; content integrity is also bound by SHA-256. "crypto/sha256" "encoding/base64" "encoding/hex" @@ -502,7 +502,11 @@ func (e *enumerator) enumerateBlob(ctx context.Context, repository, blobSHA stri if blob.SHA != blobSHA || blob.Size == nil || *blob.Size != expectedSize || blob.Encoding != "base64" { return "", 0, errors.New("blob response identity, encoding, or size drifted") } - compact := strings.NewReplacer("\n", "", "\r", "").Replace(blob.Content) + return decodeGitBlob(blob.Content, blobSHA, expectedSize) +} + +func decodeGitBlob(content, blobSHA string, expectedSize int64) (string, int64, error) { + compact := strings.NewReplacer("\n", "", "\r", "").Replace(content) raw, err := base64.StdEncoding.Strict().DecodeString(compact) if err != nil { return "", 0, errors.New("blob content is not strict base64") @@ -511,7 +515,7 @@ func (e *enumerator) enumerateBlob(ctx context.Context, repository, blobSHA stri return "", 0, errors.New("decoded blob size drifted from the tree edge") } identity := append([]byte("blob "+strconv.Itoa(len(raw))+"\x00"), raw...) - gitDigest := sha1.Sum(identity) + gitDigest := sha1.Sum(identity) // #nosec G401 -- Reconstruct the protocol-defined Git blob SHA-1 identity; SHA-256 separately binds the artifact bytes. if hex.EncodeToString(gitDigest[:]) != blobSHA { return "", 0, errors.New("decoded blob does not reproduce the declared Git SHA-1") } @@ -519,7 +523,6 @@ func (e *enumerator) enumerateBlob(ctx context.Context, repository, blobSHA stri return "sha256:" + hex.EncodeToString(rawDigest[:]), int64(len(raw)), nil } -// getTree fetches and guards one tree. Reused VERBATIM from P1-SRC-03 runner.go. func (e *enumerator) getTree(ctx context.Context, repository, sha string) (gitTree, error) { key := repository + ":" + sha if cached, ok := e.treeCache[key]; ok { @@ -541,7 +544,7 @@ func (e *enumerator) getTree(ctx context.Context, repository, sha string) (gitTr // getJSON performs one bounded, redirect-forbidden, single-content-type GET. // Reused VERBATIM from P1-SRC-03 runner.go (only the User-Agent string differs). -func (e *enumerator) getJSON(ctx context.Context, repository, suffix string, destination any) error { +func (e *enumerator) getJSON(ctx context.Context, repository, suffix string, destination any) (err error) { if _, ok := e.allowedRepos[repository]; !ok { return errors.New("repository is outside the plan-request denominator") } @@ -549,7 +552,7 @@ func (e *enumerator) getJSON(ctx context.Context, repository, suffix string, des return errors.New("HTTP request is outside the exact capability") } endpoint := e.baseURL.String() + "/repos/" + repository + suffix - request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) // #nosec G704 -- newEnumerator pins the production API origin to api.github.com and forbids redirects before sending; alternate origins require explicit test mode. if err != nil { return err } @@ -561,32 +564,17 @@ func (e *enumerator) getJSON(ctx context.Context, repository, suffix string, des request.Header.Set("Authorization", "Bearer "+e.token) } e.requests++ - response, err := e.client.Do(request) + response, err := e.client.Do(request) // #nosec G704 -- newEnumerator pins the production API origin to api.github.com and forbids redirects before sending; alternate origins require explicit test mode. if err != nil { return err } - defer response.Body.Close() - if response.StatusCode != http.StatusOK { - _, _ = io.Copy(io.Discard, io.LimitReader(response.Body, 4096)) - return fmt.Errorf("GitHub REST returned HTTP %d", response.StatusCode) - } - if response.Request == nil || response.Request.URL.String() != endpoint { - return errors.New("Git-object response arrived through a redirected or changed endpoint") - } - if len(response.Header.Values("Link")) != 0 { - return errors.New("Git-object endpoint returned a pagination Link header") - } - encodings := response.Header.Values("Content-Encoding") - if len(encodings) > 1 || (len(encodings) == 1 && encodings[0] != "identity") { - return errors.New("Git-object response used an unexpected content encoding") - } - contentTypes := response.Header.Values("Content-Type") - if len(contentTypes) != 1 { - return errors.New("Git-object response has an ambiguous or missing content type") - } - contentType, _, err := mime.ParseMediaType(contentTypes[0]) - if err != nil || contentType != "application/json" { - return errors.New("Git-object response is not application/json") + defer func() { err = closeWithError(err, response.Body) }() + return e.readGitObjectResponse(response, endpoint, destination) +} + +func (e *enumerator) readGitObjectResponse(response *http.Response, endpoint string, destination any) error { + if err := validateGitObjectResponse(response, endpoint); err != nil { + return err } remaining := maxResponseBytes - e.responseBytes if remaining <= 0 { @@ -615,6 +603,48 @@ func (e *enumerator) getJSON(ctx context.Context, repository, suffix string, des // exactTreeEntry selects one canonical tree child. Reused VERBATIM from // P1-SRC-03 runner.go. +func validateGitObjectResponse(response *http.Response, endpoint string) error { + if err := gitObjectStatusError(response); err != nil { + return err + } + + if response.Request == nil || response.Request.URL.String() != endpoint { + return errors.New("Git-object response arrived through a redirected or changed endpoint") + } + if len(response.Header.Values("Link")) != 0 { + return errors.New("Git-object endpoint returned a pagination Link header") + } + encodings := response.Header.Values("Content-Encoding") + if len(encodings) > 1 || (len(encodings) == 1 && encodings[0] != "identity") { + return errors.New("Git-object response used an unexpected content encoding") + } + return validateGitObjectContentType(response) +} + +func validateGitObjectContentType(response *http.Response) error { + contentTypes := response.Header.Values("Content-Type") + if len(contentTypes) != 1 { + return errors.New("Git-object response has an ambiguous or missing content type") + } + contentType, _, err := mime.ParseMediaType(contentTypes[0]) + if err != nil || contentType != "application/json" { + return errors.New("Git-object response is not application/json") + } + + return nil +} + +func gitObjectStatusError(response *http.Response) error { + if response.StatusCode != http.StatusOK { + _, drainErr := io.Copy(io.Discard, io.LimitReader(response.Body, 4096)) + if drainErr != nil { + drainErr = fmt.Errorf("drain Git-object error response: %w", drainErr) + } + return errors.Join(fmt.Errorf("GitHub REST returned HTTP %d", response.StatusCode), drainErr) + } + return nil +} + func exactTreeEntry(entries []gitTreeEntry, name string) (gitTreeEntry, error) { seen := make(map[string]struct{}, len(entries)) var match *gitTreeEntry diff --git a/tools/knowledge-registry/source-acquisition-plan-enumerator/http_error_test.go b/tools/knowledge-registry/source-acquisition-plan-enumerator/http_error_test.go new file mode 100644 index 0000000..2942e86 --- /dev/null +++ b/tools/knowledge-registry/source-acquisition-plan-enumerator/http_error_test.go @@ -0,0 +1,71 @@ +package main + +import ( + "context" + "errors" + "io" + "net/http" + "net/url" + "strings" + "testing" +) + +type failedResponseBody struct{ err error } + +func (b failedResponseBody) Read([]byte) (int, error) { return 0, b.err } +func (b failedResponseBody) Close() error { return nil } + +func TestGitObjectStatusError(t *testing.T) { + failedRead := errors.New("response read failed") + cases := []struct { + name string + status int + body io.ReadCloser + want error + }{ + {"success", http.StatusOK, io.NopCloser(strings.NewReader("{}")), nil}, + {"http failure", http.StatusForbidden, io.NopCloser(strings.NewReader("denied")), nil}, + {"drain failure", http.StatusForbidden, failedResponseBody{failedRead}, failedRead}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := gitObjectStatusError(&http.Response{StatusCode: tc.status, Body: tc.body}) + if (got == nil) != (tc.status == http.StatusOK) { + t.Fatalf("status error = %v", got) + } + if tc.want != nil && !errors.Is(got, tc.want) { + t.Fatalf("lost drain error: %v", got) + } + }) + } +} + +type responseTransport func(*http.Request) (*http.Response, error) + +func (f responseTransport) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +type closeErrorBody struct { + io.Reader + err error +} + +func (b closeErrorBody) Close() error { return b.err } + +func TestGetJSONPreservesResponseCloseError(t *testing.T) { + cleanup := errors.New("response close failed") + for _, closeErr := range []error{nil, cleanup} { + origin, err := url.Parse("https://api.github.com") + if err != nil { + t.Fatal(err) + } + client := &http.Client{Transport: responseTransport(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Header: http.Header{"Content-Type": []string{"application/json"}}, Request: req, Body: closeErrorBody{strings.NewReader("{}"), closeErr}}, nil + })} + e := &enumerator{client: client, baseURL: origin, allowedRepos: map[string]struct{}{"argoproj/argo-helm": {}}} + err = e.getJSON(context.Background(), "argoproj/argo-helm", "/git/test", &map[string]any{}) + + if !errors.Is(err, closeErr) { + t.Fatalf("response close error = %v, want %v", err, closeErr) + } + } +} diff --git a/tools/knowledge-registry/source-acquisition-plan-enumerator/main.go b/tools/knowledge-registry/source-acquisition-plan-enumerator/main.go index 33c12cd..467085e 100644 --- a/tools/knowledge-registry/source-acquisition-plan-enumerator/main.go +++ b/tools/knowledge-registry/source-acquisition-plan-enumerator/main.go @@ -96,7 +96,7 @@ func readPlanRequest(path string) (planRequest, error) { if info.IsDir() || info.Size() > maxPlanRequestBytes { return planRequest{}, errors.New("plan request is a directory or exceeds the byte bound") } - raw, err := os.ReadFile(path) + raw, err := os.ReadFile(path) // #nosec G304 -- --spec is an explicit local CLI input path, bounded and strictly parsed below. if err != nil { return planRequest{}, err } @@ -119,13 +119,12 @@ func readPlanRequest(path string) (planRequest, error) { // writeExclusive writes the plan document to a new file only; it never // overwrites an existing path. func writeExclusive(path string, document []byte) error { - file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) // #nosec G304 -- --output is an explicit local CLI destination; O_EXCL prevents overwriting an existing path. if err != nil { return err } if _, err := file.Write(document); err != nil { - _ = file.Close() - return err + return errors.Join(err, file.Close()) } return file.Close() } diff --git a/tools/knowledge-registry/source-acquisition-plan-enumerator/main_security_test.go b/tools/knowledge-registry/source-acquisition-plan-enumerator/main_security_test.go new file mode 100644 index 0000000..fb9b0e4 --- /dev/null +++ b/tools/knowledge-registry/source-acquisition-plan-enumerator/main_security_test.go @@ -0,0 +1,34 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteExclusivePrivateAndNoReplace(t *testing.T) { + path := filepath.Join(t.TempDir(), "plan.json") + if err := writeExclusive(path, []byte("first")); err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm()&0o077 != 0 { + t.Fatalf("plan exposes group/world permissions: %o", info.Mode().Perm()) + } + if err := writeExclusive(path, []byte("second")); err == nil { + t.Fatal("replaced existing output") + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(raw) != "first" { + t.Fatalf("output changed to %q", raw) + } + if err := writeExclusive(filepath.Join(path, "child"), nil); err == nil { + t.Fatal("accepted non-directory parent") + } +} diff --git a/tools/knowledge-registry/source-acquisition-runner-v3/checked_io.go b/tools/knowledge-registry/source-acquisition-runner-v3/checked_io.go new file mode 100644 index 0000000..b977bb1 --- /dev/null +++ b/tools/knowledge-registry/source-acquisition-runner-v3/checked_io.go @@ -0,0 +1,30 @@ +package main + +import ( + "errors" + "fmt" + "io" + "math" + "syscall" +) + +func checkedUID(value int) (uint32, error) { + if value < 0 || uint64(value) > math.MaxUint32 { + return 0, fmt.Errorf("effective user id %d is outside uint32 range", value) + } + return uint32(value), nil +} + +func closeWithError(primary error, closer io.Closer) error { + if err := closer.Close(); err != nil { + return errors.Join(primary, fmt.Errorf("close resource: %w", err)) + } + return primary +} + +func closeDescriptorWithError(primary error, descriptor int) error { + if err := syscall.Close(descriptor); err != nil { + return errors.Join(primary, fmt.Errorf("close descriptor: %w", err)) + } + return primary +} diff --git a/tools/knowledge-registry/source-acquisition-runner-v3/checked_io_test.go b/tools/knowledge-registry/source-acquisition-runner-v3/checked_io_test.go new file mode 100644 index 0000000..c4eda17 --- /dev/null +++ b/tools/knowledge-registry/source-acquisition-runner-v3/checked_io_test.go @@ -0,0 +1,77 @@ +package main + +import ( + "errors" + "math" + "strconv" + "testing" +) + +type failingCloser struct { + err error + calls int +} + +func (c *failingCloser) Close() error { c.calls++; return c.err } + +func TestCheckedUID(t *testing.T) { + cases := []struct { + name string + value int + want uint32 + bad bool + }{ + {"negative", -1, 0, true}, {"zero", 0, 0, false}, {"positive", 42, 42, false}, + } + if strconv.IntSize == 64 { + cases = append(cases, + struct { + name string + value int + want uint32 + bad bool + }{"maximum", math.MaxUint32, math.MaxUint32, false}, + struct { + name string + value int + want uint32 + bad bool + }{"overflow", math.MaxUint32 + 1, 0, true}) + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := checkedUID(tc.value) + if (err != nil) != tc.bad || got != tc.want { + t.Fatalf("checkedUID(%d) = %d, %v", tc.value, got, err) + } + }) + } +} + +func TestCloseWithError(t *testing.T) { + primary := errors.New("operation") + cleanup := errors.New("cleanup") + cases := []struct { + name string + primary, cleanup error + }{ + {"success", nil, nil}, {"primary", primary, nil}, {"cleanup", nil, cleanup}, {"both", primary, cleanup}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + closer := &failingCloser{err: tc.cleanup} + got := closeWithError(tc.primary, closer) + if closer.calls != 1 { + t.Fatalf("close calls = %d", closer.calls) + } + if (got == nil) != (tc.primary == nil && tc.cleanup == nil) { + t.Fatalf("error = %v", got) + } + for _, want := range []error{tc.primary, tc.cleanup} { + if want != nil && !errors.Is(got, want) { + t.Fatalf("lost error %v: %v", want, got) + } + } + }) + } +} diff --git a/tools/knowledge-registry/source-acquisition-runner-v3/contract.go b/tools/knowledge-registry/source-acquisition-runner-v3/contract.go index 8f09ae3..9f40ae4 100644 --- a/tools/knowledge-registry/source-acquisition-runner-v3/contract.go +++ b/tools/knowledge-registry/source-acquisition-runner-v3/contract.go @@ -878,17 +878,16 @@ func parseHexQuad(raw []byte) (uint16, bool) { // readRegularNoFollow opens one bounded regular file without following symlinks // and rejects any file that changes underneath the read. -func readRegularNoFollow(path string, maximum int64) ([]byte, error) { +func readRegularNoFollow(path string, maximum int64) (result []byte, err error) { descriptor, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) if err != nil { return nil, err } file := os.NewFile(uintptr(descriptor), path) if file == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own file descriptor") + return nil, closeDescriptorWithError(errors.New("could not own file descriptor"), descriptor) } - defer file.Close() + defer func() { err = closeWithError(err, file) }() before, err := file.Stat() if err != nil { return nil, err @@ -907,7 +906,7 @@ func readRegularNoFollow(path string, maximum int64) ([]byte, error) { if err != nil { return nil, err } - if !os.SameFile(before, after) || before.Size() != after.Size() || before.ModTime() != after.ModTime() { + if !sameStableFile(before, after) { return nil, errors.New("input changed while it was read") } return raw, nil @@ -921,3 +920,7 @@ func sortedKeys(values map[string]struct{}) []string { sort.Strings(keys) return keys } + +func sameStableFile(before, after os.FileInfo) bool { + return os.SameFile(before, after) && before.Size() == after.Size() && before.ModTime() == after.ModTime() +} diff --git a/tools/knowledge-registry/source-acquisition-runner-v3/http_error_test.go b/tools/knowledge-registry/source-acquisition-runner-v3/http_error_test.go new file mode 100644 index 0000000..b89e426 --- /dev/null +++ b/tools/knowledge-registry/source-acquisition-runner-v3/http_error_test.go @@ -0,0 +1,71 @@ +package main + +import ( + "context" + "errors" + "io" + "net/http" + "net/url" + "strings" + "testing" +) + +type failedResponseBody struct{ err error } + +func (b failedResponseBody) Read([]byte) (int, error) { return 0, b.err } +func (b failedResponseBody) Close() error { return nil } + +func TestGitObjectStatusError(t *testing.T) { + failedRead := errors.New("response read failed") + cases := []struct { + name string + status int + body io.ReadCloser + want error + }{ + {"success", http.StatusOK, io.NopCloser(strings.NewReader("{}")), nil}, + {"http failure", http.StatusForbidden, io.NopCloser(strings.NewReader("denied")), nil}, + {"drain failure", http.StatusForbidden, failedResponseBody{failedRead}, failedRead}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := gitObjectStatusError(&http.Response{StatusCode: tc.status, Body: tc.body}) + if (got == nil) != (tc.status == http.StatusOK) { + t.Fatalf("status error = %v", got) + } + if tc.want != nil && !errors.Is(got, tc.want) { + t.Fatalf("lost drain error: %v", got) + } + }) + } +} + +type responseTransport func(*http.Request) (*http.Response, error) + +func (f responseTransport) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } + +type closeErrorBody struct { + io.Reader + err error +} + +func (b closeErrorBody) Close() error { return b.err } + +func TestGetJSONPreservesResponseCloseError(t *testing.T) { + cleanup := errors.New("response close failed") + for _, closeErr := range []error{nil, cleanup} { + origin, err := url.Parse("https://api.github.com") + if err != nil { + t.Fatal(err) + } + client := &http.Client{Transport: responseTransport(func(req *http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, Header: http.Header{"Content-Type": []string{"application/json"}}, Request: req, Body: closeErrorBody{strings.NewReader("{}"), closeErr}}, nil + })} + r := &runner{client: client, baseURL: origin, allowedRepos: map[string]struct{}{"argoproj/argo-helm": {}}} + err = r.getJSON(context.Background(), "argoproj/argo-helm", "/git/test", &map[string]any{}) + + if !errors.Is(err, closeErr) { + t.Fatalf("response close error = %v, want %v", err, closeErr) + } + } +} diff --git a/tools/knowledge-registry/source-acquisition-runner-v3/plan.go b/tools/knowledge-registry/source-acquisition-runner-v3/plan.go index 3c5a56d..4d82809 100644 --- a/tools/knowledge-registry/source-acquisition-runner-v3/plan.go +++ b/tools/knowledge-registry/source-acquisition-runner-v3/plan.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "fmt" "io" "os" "os/exec" @@ -53,7 +54,7 @@ func isArmoredSSHSIG(signature []byte) bool { // invocation used across the knowledge tools: the document is fed on stdin, the // principal and namespace are pinned, and the allowed_signers policy plus the // detached signature are written to a private temp directory. -func verifySSHSIGNamespace(document, signature, allowedSigners []byte, principal, namespace string) error { +func verifySSHSIGNamespace(document, signature, allowedSigners []byte, principal, namespace string) (err error) { info, err := os.Lstat(sshKeygenPath) if err != nil || !info.Mode().IsRegular() || info.Mode()&0o111 == 0 { return errors.New("fixed ssh-keygen verifier is unavailable") @@ -62,7 +63,11 @@ func verifySSHSIGNamespace(document, signature, allowedSigners []byte, principal if err != nil { return err } - defer os.RemoveAll(directory) + defer func() { + if cleanupErr := os.RemoveAll(directory); cleanupErr != nil { + err = errors.Join(err, fmt.Errorf("remove signature verification directory: %w", cleanupErr)) + } + }() policyPath := filepath.Join(directory, "allowed_signers") signaturePath := filepath.Join(directory, "plan.sig") if err := os.WriteFile(policyPath, allowedSigners, 0o600); err != nil { @@ -74,7 +79,7 @@ func verifySSHSIGNamespace(document, signature, allowedSigners []byte, principal ctx, cancel := context.WithTimeout(context.Background(), sshKeygenVerifyTimeout) defer cancel() command := exec.CommandContext(ctx, sshKeygenPath, "-Y", "verify", "-f", policyPath, - "-I", principal, "-n", namespace, "-s", signaturePath) + "-I", principal, "-n", namespace, "-s", signaturePath) // #nosec G204 -- sshKeygenPath is the fixed /usr/bin/ssh-keygen verifier; arguments are passed directly without a shell. command.Stdin = bytes.NewReader(document) command.Stdout = io.Discard command.Stderr = io.Discard diff --git a/tools/knowledge-registry/source-acquisition-runner-v3/runner.go b/tools/knowledge-registry/source-acquisition-runner-v3/runner.go index 76bb6b1..b3b15eb 100644 --- a/tools/knowledge-registry/source-acquisition-runner-v3/runner.go +++ b/tools/knowledge-registry/source-acquisition-runner-v3/runner.go @@ -3,7 +3,7 @@ package main import ( "bytes" "context" - "crypto/sha1" + "crypto/sha1" // #nosec G505 -- Git blob object identifiers require SHA-1; content integrity is also bound by SHA-256. "crypto/sha256" "encoding/base64" "encoding/hex" @@ -704,7 +704,16 @@ func (r *runner) getBlob(ctx context.Context, repository string, expected contra if blob.SHA != expected.GitBlobSHA1 || blob.Size == nil || *blob.Size != expected.Bytes || blob.Encoding != "base64" { return nil, errors.New("blob response identity, encoding, or size drifted") } - compact := strings.NewReplacer("\n", "", "\r", "").Replace(blob.Content) + raw, err := decodeGitBlob(blob.Content, expected) + if err != nil { + return nil, err + } + r.blobCache[key] = raw + return raw, nil +} + +func decodeGitBlob(content string, expected contractFile) ([]byte, error) { + compact := strings.NewReplacer("\n", "", "\r", "").Replace(content) raw, err := base64.StdEncoding.Strict().DecodeString(compact) if err != nil { return nil, errors.New("blob content is not strict base64") @@ -713,7 +722,7 @@ func (r *runner) getBlob(ctx context.Context, repository string, expected contra return nil, errors.New("decoded blob size drifted") } identity := append([]byte("blob "+strconv.Itoa(len(raw))+"\x00"), raw...) - gitDigest := sha1.Sum(identity) + gitDigest := sha1.Sum(identity) // #nosec G401 -- Reconstruct the protocol-defined Git blob SHA-1 identity; SHA-256 separately binds the artifact bytes. if hex.EncodeToString(gitDigest[:]) != expected.GitBlobSHA1 { return nil, errors.New("decoded blob does not reproduce the exact Git SHA-1") } @@ -721,11 +730,10 @@ func (r *runner) getBlob(ctx context.Context, repository string, expected contra if "sha256:"+hex.EncodeToString(rawDigest[:]) != expected.RawSHA256 { return nil, errors.New("decoded blob raw SHA-256 drifted") } - r.blobCache[key] = raw return raw, nil } -func (r *runner) getJSON(ctx context.Context, repository, suffix string, destination any) error { +func (r *runner) getJSON(ctx context.Context, repository, suffix string, destination any) (err error) { if _, ok := r.allowedRepos[repository]; !ok { return errors.New("repository is outside the exact three-source denominator") } @@ -733,7 +741,7 @@ func (r *runner) getJSON(ctx context.Context, repository, suffix string, destina return errors.New("HTTP request is outside the exact capability") } endpoint := r.baseURL.String() + "/repos/" + repository + suffix - request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) // #nosec G704 -- newRunner pins the production API origin to api.github.com and forbids redirects before sending; alternate origins require explicit test mode. if err != nil { return err } @@ -745,32 +753,17 @@ func (r *runner) getJSON(ctx context.Context, repository, suffix string, destina request.Header.Set("Authorization", "Bearer "+r.token) } r.requests++ - response, err := r.client.Do(request) + response, err := r.client.Do(request) // #nosec G704 -- newRunner pins the production API origin to api.github.com and forbids redirects before sending; alternate origins require explicit test mode. if err != nil { return err } - defer response.Body.Close() - if response.StatusCode != http.StatusOK { - _, _ = io.Copy(io.Discard, io.LimitReader(response.Body, 4096)) - return fmt.Errorf("GitHub REST returned HTTP %d", response.StatusCode) - } - if response.Request == nil || response.Request.URL.String() != endpoint { - return errors.New("Git-object response arrived through a redirected or changed endpoint") - } - if len(response.Header.Values("Link")) != 0 { - return errors.New("Git-object endpoint returned a pagination Link header") - } - encodings := response.Header.Values("Content-Encoding") - if len(encodings) > 1 || (len(encodings) == 1 && encodings[0] != "identity") { - return errors.New("Git-object response used an unexpected content encoding") - } - contentTypes := response.Header.Values("Content-Type") - if len(contentTypes) != 1 { - return errors.New("Git-object response has an ambiguous or missing content type") - } - contentType, _, err := mime.ParseMediaType(contentTypes[0]) - if err != nil || contentType != "application/json" { - return errors.New("Git-object response is not application/json") + defer func() { err = closeWithError(err, response.Body) }() + return r.readGitObjectResponse(response, endpoint, destination) +} + +func (r *runner) readGitObjectResponse(response *http.Response, endpoint string, destination any) error { + if err := validateGitObjectResponse(response, endpoint); err != nil { + return err } remaining := maxResponseBytes - r.responseBytes if remaining <= 0 { @@ -797,6 +790,48 @@ func (r *runner) getJSON(ctx context.Context, repository, suffix string, destina return nil } +func validateGitObjectResponse(response *http.Response, endpoint string) error { + if err := gitObjectStatusError(response); err != nil { + return err + } + + if response.Request == nil || response.Request.URL.String() != endpoint { + return errors.New("Git-object response arrived through a redirected or changed endpoint") + } + if len(response.Header.Values("Link")) != 0 { + return errors.New("Git-object endpoint returned a pagination Link header") + } + encodings := response.Header.Values("Content-Encoding") + if len(encodings) > 1 || (len(encodings) == 1 && encodings[0] != "identity") { + return errors.New("Git-object response used an unexpected content encoding") + } + return validateGitObjectContentType(response) +} + +func validateGitObjectContentType(response *http.Response) error { + contentTypes := response.Header.Values("Content-Type") + if len(contentTypes) != 1 { + return errors.New("Git-object response has an ambiguous or missing content type") + } + contentType, _, err := mime.ParseMediaType(contentTypes[0]) + if err != nil || contentType != "application/json" { + return errors.New("Git-object response is not application/json") + } + + return nil +} + +func gitObjectStatusError(response *http.Response) error { + if response.StatusCode != http.StatusOK { + _, drainErr := io.Copy(io.Discard, io.LimitReader(response.Body, 4096)) + if drainErr != nil { + drainErr = fmt.Errorf("drain Git-object error response: %w", drainErr) + } + return errors.Join(fmt.Errorf("GitHub REST returned HTTP %d", response.StatusCode), drainErr) + } + return nil +} + func exactTreeEntry(entries []gitTreeEntry, name string) (gitTreeEntry, error) { seen := make(map[string]struct{}, len(entries)) var match *gitTreeEntry diff --git a/tools/knowledge-registry/source-acquisition-runner-v3/store.go b/tools/knowledge-registry/source-acquisition-runner-v3/store.go index 7223c7d..b559488 100644 --- a/tools/knowledge-registry/source-acquisition-runner-v3/store.go +++ b/tools/knowledge-registry/source-acquisition-runner-v3/store.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "slices" "sort" "strings" "syscall" @@ -32,70 +33,89 @@ func newCASStore(root string) (*casStore, error) { if absolute == string(os.PathSeparator) { return nil, errors.New("output root must not be the filesystem root") } - parent, err := openDirectoryPathNoFollow(filepath.Dir(absolute)) - if err != nil { - return nil, fmt.Errorf("output parent: %w", err) - } - defer parent.Close() - leaf := filepath.Base(absolute) - rootFile, err := openDirectoryAt(parent, leaf) - if errors.Is(err, os.ErrNotExist) { - if err := syscall.Mkdirat(int(parent.Fd()), leaf, 0o700); err != nil && !errors.Is(err, syscall.EEXIST) { - return nil, fmt.Errorf("create output root: %w", err) - } - if err := syncOpenedFile(parent); err != nil { - return nil, fmt.Errorf("persist output root: %w", err) - } - rootFile, err = openDirectoryAt(parent, leaf) - } + rootFile, err := openCASRoot(absolute) if err != nil { - return nil, fmt.Errorf("inspect output root: %w", err) + return nil, err } info, err := rootFile.Stat() if err != nil { - rootFile.Close() - return nil, fmt.Errorf("inspect output root: %w", err) + return nil, closeWithError(fmt.Errorf("inspect output root: %w", err), rootFile) } if err := validateOwnedDirectory(info, "output root"); err != nil { - rootFile.Close() - return nil, err + return nil, closeWithError(err, rootFile) } store := &casStore{root: absolute, rootFile: rootFile, rootInfo: info} if err := store.verifyRootPath(); err != nil { - rootFile.Close() - return nil, err + return nil, closeWithError(err, rootFile) } - receipts, _, err := store.listOptionalPublishedDirectory("receipts", 2) - if err != nil { - rootFile.Close() - return nil, fmt.Errorf("preflight receipts: %w", err) + if err := store.initializeDirectories(); err != nil { + return nil, closeWithError(err, rootFile) } - proposals, _, err := store.listOptionalPublishedDirectory("inventory-proposals", 2) + return store, nil +} + +func (s *casStore) initializeDirectories() error { + recoveryState, err := s.hasRecoveryState() if err != nil { - rootFile.Close() - return nil, fmt.Errorf("preflight inventory proposals: %w", err) - } - if len(receipts) == 0 && len(proposals) != 0 { - rootFile.Close() - return nil, errors.New("inventory proposal exists without its predecessor receipt") + return err } - recoveryState := len(receipts) != 0 || len(proposals) != 0 for _, relative := range []string{"cas", "cas/sha256", "receipts", "inventory-proposals"} { if recoveryState { - directory, _, openErr := store.openStoreDirectory(relative, false) + directory, _, openErr := s.openStoreDirectory(relative, false) if openErr == nil { - directory.Close() + if err := closeWithError(nil, directory); err != nil { + return err + } continue } - rootFile.Close() - return nil, fmt.Errorf("recovery state is missing required directory %s: %w", relative, openErr) + return fmt.Errorf("recovery state is missing required directory %s: %w", relative, openErr) } - if _, err := store.ensureDirectory(relative); err != nil { - rootFile.Close() - return nil, err + if _, err := s.ensureDirectory(relative); err != nil { + return err } } - return store, nil + return nil +} + +func (s *casStore) hasRecoveryState() (bool, error) { + receipts, _, err := s.listOptionalPublishedDirectory("receipts", 2) + if err != nil { + return false, fmt.Errorf("preflight receipts: %w", err) + } + proposals, _, err := s.listOptionalPublishedDirectory("inventory-proposals", 2) + if err != nil { + return false, fmt.Errorf("preflight inventory proposals: %w", err) + } + if len(receipts) == 0 && len(proposals) != 0 { + return false, errors.New("inventory proposal exists without its predecessor receipt") + } + return len(receipts) != 0 || len(proposals) != 0, nil +} + +func openCASRoot(absolute string) (file *os.File, err error) { + parent, err := openDirectoryPathNoFollow(filepath.Dir(absolute)) + if err != nil { + return nil, fmt.Errorf("output parent: %w", err) + } + defer func() { + err = closeWithError(err, parent) + if err != nil && file != nil { + err = closeWithError(err, file) + file = nil + } + }() + leaf := filepath.Base(absolute) + file, err = openDirectoryAt(parent, leaf) + if !errors.Is(err, os.ErrNotExist) { + return file, err + } + if err := syscall.Mkdirat(int(parent.Fd()), leaf, 0o700); err != nil && !errors.Is(err, syscall.EEXIST) { + return nil, fmt.Errorf("create output root: %w", err) + } + if err := syncOpenedFile(parent); err != nil { + return nil, fmt.Errorf("persist output root: %w", err) + } + return openDirectoryAt(parent, leaf) } func (s *casStore) listOptionalPublishedDirectory(relative string, maximum int) ([]string, bool, error) { @@ -162,12 +182,12 @@ func (s *casStore) putProposal(proposalID string, raw []byte) (string, error) { return relative, nil } -func (s *casStore) ensureDirectory(relative string) (string, error) { +func (s *casStore) ensureDirectory(relative string) (result string, err error) { directory, _, err := s.openStoreDirectory(relative, true) if err != nil { return "", err } - defer directory.Close() + defer func() { err = closeWithError(err, directory) }() return filepath.Join(s.root, filepath.FromSlash(relative)), nil } @@ -209,7 +229,7 @@ func (s *casStore) verifyRunSet(receiptPath string, receiptRaw []byte, proposalP if err != nil { return err } - if len(receipts) != 1 || receipts[0] != filepath.Base(receiptPath) || len(proposals) != 1 || proposals[0] != filepath.Base(proposalPath) { + if !slices.Equal(receipts, []string{filepath.Base(receiptPath)}) || !slices.Equal(proposals, []string{filepath.Base(proposalPath)}) { return errors.New("published receipt/proposal set is incomplete or ambiguous") } actualReceipt, err := s.readPublished(receiptPath, maxPublishedJSONBytes) @@ -223,7 +243,7 @@ func (s *casStore) verifyRunSet(receiptPath string, receiptRaw []byte, proposalP return nil } -func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) error { +func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) (err error) { if int64(len(raw)) > maximum { return errors.New("published object exceeds its byte bound") } @@ -236,7 +256,7 @@ func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) e if err != nil { return err } - defer parent.Close() + defer func() { err = closeWithError(err, parent) }() leaf := filepath.Base(relative) descriptor, err := syscall.Openat(int(parent.Fd()), leaf, syscall.O_WRONLY|syscall.O_CREAT|syscall.O_EXCL|syscall.O_CLOEXEC|syscall.O_NOFOLLOW, 0o600) if err != nil { @@ -247,39 +267,50 @@ func (s *casStore) atomicNoReplace(relative string, raw []byte, maximum int64) e } file := os.NewFile(uintptr(descriptor), leaf) if file == nil { - syscall.Close(descriptor) - return errors.New("could not own exclusive published object descriptor") + return closeDescriptorWithError(errors.New("could not own exclusive published object descriptor"), descriptor) } + written, err := writePublishedContents(file, raw) + if err != nil { + return closeWithError(err, file) + } + return closeWithError(s.commitPublishedContents(parentRelative, parent, parentInfo, leaf, raw, maximum, written), file) +} + +func (s *casStore) commitPublishedContents(parentRelative string, parent *os.File, parentInfo os.FileInfo, leaf string, raw []byte, maximum int64, written os.FileInfo) error { + if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed before directory commit"); err != nil { + return err + } + if err := syncOpenedFile(parent); err != nil { + return err + } + if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed after directory commit"); err != nil { + return err + } + if err := s.verifyRootPath(); err != nil { + return err + } + return nil +} + +func writePublishedContents(file *os.File, raw []byte) (os.FileInfo, error) { created, err := file.Stat() if err != nil { - return errors.Join(err, file.Close()) + return nil, err } if err := validateOwnedRegular(created, "new published object"); err != nil { - return errors.Join(err, file.Close()) + return nil, err } if err := writeAll(file, raw); err != nil { - return errors.Join(err, file.Close()) + return nil, err } if err := syncOpenedFile(file); err != nil { - return errors.Join(err, file.Close()) + return nil, err } written, err := file.Stat() if err != nil || !os.SameFile(created, written) || written.Size() != int64(len(raw)) { - return errors.Join(errors.New("exclusive published object write is incomplete or unstable"), err, file.Close()) - } - if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed before directory commit"); err != nil { - return errors.Join(err, file.Close()) - } - if err := syncOpenedFile(parent); err != nil { - return errors.Join(err, file.Close()) - } - if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, written, false, "exclusive published object changed after directory commit"); err != nil { - return errors.Join(err, file.Close()) - } - if err := s.verifyRootPath(); err != nil { - return errors.Join(err, file.Close()) + return nil, errors.Join(errors.New("exclusive published object write is incomplete or unstable"), err) } - return file.Close() + return written, nil } func (s *casStore) rehabilitateExisting(parentRelative string, parent *os.File, parentInfo os.FileInfo, leaf string, raw []byte, maximum int64) error { @@ -299,24 +330,12 @@ func (s *casStore) rehabilitateExisting(parentRelative string, parent *os.File, return s.verifyRootPath() } -func verifyPublishedInDirectory(parent *os.File, leaf string, raw []byte, maximum int64, required os.FileInfo, synchronize bool, conflict string) (os.FileInfo, error) { - access := syscall.O_RDONLY - if synchronize { - access = syscall.O_RDWR - } - descriptor, err := syscall.Openat(int(parent.Fd()), leaf, access|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) +func verifyPublishedInDirectory(parent *os.File, leaf string, raw []byte, maximum int64, required os.FileInfo, synchronize bool, conflict string) (info os.FileInfo, err error) { + file, err := openPublishedFile(parent, leaf, synchronize) if err != nil { - if errors.Is(err, syscall.ELOOP) { - return nil, errors.New("published object path exists and is not a regular file") - } return nil, err } - file := os.NewFile(uintptr(descriptor), leaf) - if file == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own published object file descriptor") - } - defer file.Close() + defer func() { err = closeWithError(err, file) }() before, err := file.Stat() if err != nil { return nil, err @@ -330,42 +349,73 @@ func verifyPublishedInDirectory(parent *os.File, leaf string, raw []byte, maximu if before.Size() > maximum { return nil, errors.New("published object exceeds its byte bound") } - existing, err := io.ReadAll(io.LimitReader(file, maximum+1)) + existing, after, err := readStablePublished(file, before, maximum) + if err != nil { + return nil, err + } + if !bytes.Equal(existing, raw) { + return nil, errors.New(conflict) + } + if synchronize { + return synchronizePublished(file, after) + } + return after, nil +} + +func openPublishedFile(parent *os.File, leaf string, synchronize bool) (*os.File, error) { + access := syscall.O_RDONLY + if synchronize { + access = syscall.O_RDWR + } + descriptor, err := syscall.Openat(int(parent.Fd()), leaf, access|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) if err != nil { + if errors.Is(err, syscall.ELOOP) { + return nil, errors.New("published object path exists and is not a regular file") + } return nil, err } + file := os.NewFile(uintptr(descriptor), leaf) + if file == nil { + return nil, closeDescriptorWithError(errors.New("could not own published object file descriptor"), descriptor) + } + return file, nil +} + +func readStablePublished(file *os.File, before os.FileInfo, maximum int64) ([]byte, os.FileInfo, error) { + existing, err := io.ReadAll(io.LimitReader(file, maximum+1)) + if err != nil { + return nil, nil, err + } if int64(len(existing)) > maximum { - return nil, errors.New("published object exceeds its byte bound") + return nil, nil, errors.New("published object exceeds its byte bound") } after, err := file.Stat() if err != nil { - return nil, err + return nil, nil, err } if !os.SameFile(before, after) || before.Size() != after.Size() || before.ModTime() != after.ModTime() { - return nil, errors.New("published object changed while it was read") + return nil, nil, errors.New("published object changed while it was read") } - if !bytes.Equal(existing, raw) { - return nil, errors.New(conflict) + return existing, after, nil +} + +func synchronizePublished(file *os.File, after os.FileInfo) (os.FileInfo, error) { + if err := syncOpenedFile(file); err != nil { + return nil, err } - if synchronize { - if err := syncOpenedFile(file); err != nil { - return nil, err - } - afterSync, err := file.Stat() - if err != nil || !os.SameFile(after, afterSync) || after.Size() != afterSync.Size() || after.ModTime() != afterSync.ModTime() { - return nil, errors.Join(errors.New("published object changed while it was synchronized"), err) - } - after = afterSync + afterSync, err := file.Stat() + if err != nil || !os.SameFile(after, afterSync) || after.Size() != afterSync.Size() || after.ModTime() != afterSync.ModTime() { + return nil, errors.Join(errors.New("published object changed while it was synchronized"), err) } - return after, nil + return afterSync, nil } -func (s *casStore) verifyPublishedAtPath(parentRelative string, requiredParent os.FileInfo, leaf string, raw []byte, maximum int64, requiredFile os.FileInfo, synchronize bool, conflict string) error { +func (s *casStore) verifyPublishedAtPath(parentRelative string, requiredParent os.FileInfo, leaf string, raw []byte, maximum int64, requiredFile os.FileInfo, synchronize bool, conflict string) (err error) { parent, parentInfo, err := s.openStoreDirectory(parentRelative, false) if err != nil { return err } - defer parent.Close() + defer func() { err = closeWithError(err, parent) }() if !os.SameFile(requiredParent, parentInfo) { return errors.New("published object parent changed during commit") } @@ -373,7 +423,7 @@ func (s *casStore) verifyPublishedAtPath(parentRelative string, requiredParent o return err } -func (s *casStore) readPublished(relative string, maximum int64) ([]byte, error) { +func (s *casStore) readPublished(relative string, maximum int64) (result []byte, err error) { clean := filepath.Clean(relative) if clean != relative || filepath.IsAbs(relative) || strings.HasPrefix(clean, "..") { return nil, errors.New("published object path is not canonical") @@ -383,18 +433,28 @@ func (s *casStore) readPublished(relative string, maximum int64) ([]byte, error) if err != nil { return nil, err } - defer parent.Close() + defer func() { err = closeWithError(err, parent) }() leaf := filepath.Base(relative) - descriptor, err := syscall.Openat(int(parent.Fd()), leaf, syscall.O_RDONLY|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) + file, err := openPublishedFile(parent, leaf, false) if err != nil { return nil, err } - file := os.NewFile(uintptr(descriptor), leaf) - if file == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own published object descriptor") + defer func() { err = closeWithError(err, file) }() + before, err := inspectPublished(file, maximum) + if err != nil { + return nil, err + } + raw, after, err := readStablePublished(file, before, maximum) + if err != nil { + return nil, err } - defer file.Close() + if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, after, false, "published object changed during path replay"); err != nil { + return nil, err + } + return raw, nil +} + +func inspectPublished(file *os.File, maximum int64) (os.FileInfo, error) { before, err := file.Stat() if err != nil { return nil, err @@ -405,21 +465,10 @@ func (s *casStore) readPublished(relative string, maximum int64) ([]byte, error) if before.Size() > maximum { return nil, errors.New("published object exceeds its byte bound") } - raw, err := io.ReadAll(io.LimitReader(file, maximum+1)) - if err != nil || int64(len(raw)) > maximum { - return nil, errors.Join(errors.New("published object read exceeds its bound"), err) - } - after, err := file.Stat() - if err != nil || !os.SameFile(before, after) || before.Size() != after.Size() || before.ModTime() != after.ModTime() { - return nil, errors.Join(errors.New("published object changed while it was read"), err) - } - if err := s.verifyPublishedAtPath(parentRelative, parentInfo, leaf, raw, maximum, after, false, "published object changed during path replay"); err != nil { - return nil, err - } - return raw, nil + return before, nil } -func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]string, error) { +func (s *casStore) listPublishedDirectory(relative string, maximum int) (result []string, err error) { if maximum < 1 { return nil, errors.New("directory entry bound must be positive") } @@ -427,7 +476,7 @@ func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]strin if err != nil { return nil, err } - defer directory.Close() + defer func() { err = closeWithError(err, directory) }() entries, err := directory.ReadDir(maximum + 1) if err != nil && !errors.Is(err, io.EOF) { return nil, err @@ -435,19 +484,17 @@ func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]strin if len(entries) > maximum { return nil, errors.New("published directory exceeds its entry bound") } - names := make([]string, 0, len(entries)) - for _, entry := range entries { - name := entry.Name() - if name == "" || name == "." || name == ".." || strings.ContainsAny(name, "/\x00") { - return nil, errors.New("published directory contains an unsafe entry name") - } - names = append(names, name) + names, err := publishedEntryNames(entries) + if err != nil { + return nil, err } replayed, replayedInfo, err := s.openStoreDirectory(relative, false) if err != nil { return nil, err } - replayed.Close() + if err := closeWithError(nil, replayed); err != nil { + return nil, err + } if !os.SameFile(directoryInfo, replayedInfo) { return nil, errors.New("published directory changed while it was listed") } @@ -455,6 +502,18 @@ func (s *casStore) listPublishedDirectory(relative string, maximum int) ([]strin return names, nil } +func publishedEntryNames(entries []os.DirEntry) ([]string, error) { + names := make([]string, 0, len(entries)) + for _, entry := range entries { + name := entry.Name() + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, "/\x00") { + return nil, errors.New("published directory contains an unsafe entry name") + } + names = append(names, name) + } + return names, nil +} + func writeAll(file *os.File, raw []byte) error { for len(raw) > 0 { written, err := file.Write(raw) @@ -504,7 +563,11 @@ func validateEffectiveOwner(info os.FileInfo, label string) error { if !ok { return fmt.Errorf("%s ownership cannot be verified", label) } - if stat.Uid != uint32(os.Geteuid()) { + uid, err := checkedUID(os.Geteuid()) + if err != nil { + return fmt.Errorf("%s ownership: %w", label, err) + } + if stat.Uid != uid { return fmt.Errorf("%s is not owned by the effective user", label) } return nil @@ -521,22 +584,22 @@ func openDirectoryPathNoFollow(pathname string) (*os.File, error) { } current := os.NewFile(uintptr(descriptor), string(os.PathSeparator)) if current == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own filesystem-root descriptor") + return nil, closeDescriptorWithError(errors.New("could not own filesystem-root descriptor"), descriptor) } trimmed := strings.TrimPrefix(absolute, string(os.PathSeparator)) if trimmed == "" { return current, nil } for _, component := range strings.Split(trimmed, string(os.PathSeparator)) { - if component == "" || component == "." || component == ".." { - current.Close() - return nil, errors.New("directory path is not canonical") + if !canonicalDirectoryComponent(component) { + return nil, closeWithError(errors.New("directory path is not canonical"), current) } next, err := openDirectoryAt(current, component) - current.Close() - if err != nil { - return nil, err + if closeErr := closeWithError(err, current); closeErr != nil { + if next != nil { + closeErr = closeWithError(closeErr, next) + } + return nil, closeErr } current = next } @@ -550,8 +613,7 @@ func openDirectoryAt(parent *os.File, name string) (*os.File, error) { } file := os.NewFile(uintptr(descriptor), name) if file == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own directory descriptor") + return nil, closeDescriptorWithError(errors.New("could not own directory descriptor"), descriptor) } return file, nil } @@ -564,8 +626,7 @@ func duplicateFile(file *os.File, name string) (*os.File, error) { syscall.CloseOnExec(descriptor) duplicate := os.NewFile(uintptr(descriptor), name) if duplicate == nil { - syscall.Close(descriptor) - return nil, errors.New("could not own duplicated directory descriptor") + return nil, closeDescriptorWithError(errors.New("could not own duplicated directory descriptor"), descriptor) } return duplicate, nil } @@ -579,7 +640,7 @@ func validateStoreRelativeDirectory(relative string) ([]string, error) { } components := strings.Split(filepath.ToSlash(relative), "/") for _, component := range components { - if component == "" || component == "." || component == ".." { + if !canonicalDirectoryComponent(component) { return nil, errors.New("unsafe store directory component") } } @@ -596,41 +657,43 @@ func (s *casStore) openDirectoryFromRoot(relative string, create bool) (*os.File return nil, nil, err } for _, component := range components { - next, openErr := openDirectoryAt(current, component) - if errors.Is(openErr, os.ErrNotExist) && create { - if mkdirErr := syscall.Mkdirat(int(current.Fd()), component, 0o700); mkdirErr != nil && !errors.Is(mkdirErr, syscall.EEXIST) { - current.Close() - return nil, nil, mkdirErr - } - if syncErr := syncOpenedFile(current); syncErr != nil { - current.Close() - return nil, nil, syncErr + next, openErr := openOrCreateDirectoryAt(current, component, create) + if closeErr := closeWithError(openErr, current); closeErr != nil { + if next != nil { + closeErr = closeWithError(closeErr, next) } - next, openErr = openDirectoryAt(current, component) - } - current.Close() - if openErr != nil { - return nil, nil, openErr + return nil, nil, closeErr } info, statErr := next.Stat() if statErr != nil { - next.Close() - return nil, nil, statErr + return nil, nil, closeWithError(statErr, next) } if err := validateOwnedDirectory(info, relative); err != nil { - next.Close() - return nil, nil, err + return nil, nil, closeWithError(err, next) } current = next } info, err := current.Stat() if err != nil { - current.Close() - return nil, nil, err + return nil, nil, closeWithError(err, current) } return current, info, nil } +func openOrCreateDirectoryAt(current *os.File, component string, create bool) (*os.File, error) { + next, openErr := openDirectoryAt(current, component) + if errors.Is(openErr, os.ErrNotExist) && create { + if mkdirErr := syscall.Mkdirat(int(current.Fd()), component, 0o700); mkdirErr != nil && !errors.Is(mkdirErr, syscall.EEXIST) { + return nil, mkdirErr + } + if syncErr := syncOpenedFile(current); syncErr != nil { + return nil, syncErr + } + next, openErr = openDirectoryAt(current, component) + } + return next, openErr +} + func (s *casStore) openStoreDirectory(relative string, create bool) (*os.File, os.FileInfo, error) { if err := s.verifyRootPath(); err != nil { return nil, nil, err @@ -641,22 +704,21 @@ func (s *casStore) openStoreDirectory(relative string, create bool) (*os.File, o } replayed, replayedInfo, err := s.openDirectoryFromRoot(relative, false) if err != nil { - held.Close() - return nil, nil, err + return nil, nil, closeWithError(err, held) + } + if err := closeWithError(nil, replayed); err != nil { + return nil, nil, closeWithError(err, held) } - replayed.Close() if !os.SameFile(heldInfo, replayedInfo) { - held.Close() - return nil, nil, errors.New("store directory changed during path replay") + return nil, nil, closeWithError(errors.New("store directory changed during path replay"), held) } if err := s.verifyRootPath(); err != nil { - held.Close() - return nil, nil, err + return nil, nil, closeWithError(err, held) } return held, heldInfo, nil } -func (s *casStore) verifyRootPath() error { +func (s *casStore) verifyRootPath() (err error) { if s.rootFile == nil { return errors.New("store root descriptor is closed") } @@ -664,7 +726,7 @@ func (s *casStore) verifyRootPath() error { if err != nil { return fmt.Errorf("reopen output root: %w", err) } - defer current.Close() + defer func() { err = closeWithError(err, current) }() info, err := current.Stat() if err != nil { return err @@ -696,3 +758,7 @@ func stripPrefixedID(value, prefix string) (string, error) { } return digest, nil } + +func canonicalDirectoryComponent(component string) bool { + return component != "" && component != "." && component != ".." +} diff --git a/tools/knowledge-retrieval/bm25f.go b/tools/knowledge-retrieval/bm25f.go index e299d95..759b368 100644 --- a/tools/knowledge-retrieval/bm25f.go +++ b/tools/knowledge-retrieval/bm25f.go @@ -14,8 +14,8 @@ const ( legacyLexicalProfile = "deterministic_raw_term_frequency@1.0.0" bm25fLexicalProfile = "deterministic_bm25f_fixed_point@1.0.0" bm25fLexicalProfileV11 = "deterministic_bm25f_fixed_point@1.1.0" - bm25fTokenizerID = "unicode_alnum_camel_acronym_conservative_plural@1.0.0" - bm25fTokenizerIDV11 = "unicode_alnum_camel_acronym_conservative_plural@1.1.0" + bm25fTokenizerID = "unicode_alnum_camel_acronym_conservative_plural@1.0.0" // #nosec G101 -- Public versioned tokenizer algorithm identifier; no authentication authority. + bm25fTokenizerIDV11 = "unicode_alnum_camel_acronym_conservative_plural@1.1.0" // #nosec G101 -- Public versioned tokenizer algorithm identifier; no authentication authority. bm25fScorerID = "bm25f_k1_6_5_b_3_4_heading_2_cap_half_fixed_ppm@1.0.0" bm25fIndexSchema = "2.0.0" bm25fIndexKind = "deterministic_bm25f_lexical_inverted_index" diff --git a/tools/knowledge-retrieval/dense_materialize.go b/tools/knowledge-retrieval/dense_materialize.go index 3288597..9da2e47 100644 --- a/tools/knowledge-retrieval/dense_materialize.go +++ b/tools/knowledge-retrieval/dense_materialize.go @@ -332,9 +332,7 @@ func embedDenseInputs(config DenseMaterializeConfig, inputs []string) ([][]float if err != nil { return nil, fmt.Errorf("embedding request failed: %w", err) } - decoded := embeddingResponse{} - decodeErr := json.NewDecoder(response.Body).Decode(&decoded) - response.Body.Close() + decoded, decodeErr := decodeEmbeddingResponse(response.Body) if response.StatusCode != http.StatusOK { return nil, fmt.Errorf("embedding endpoint returned status %d", response.StatusCode) } diff --git a/tools/knowledge-retrieval/lifecycle_signing.go b/tools/knowledge-retrieval/lifecycle_signing.go index 42e0003..8918cec 100644 --- a/tools/knowledge-retrieval/lifecycle_signing.go +++ b/tools/knowledge-retrieval/lifecycle_signing.go @@ -224,7 +224,7 @@ func infoSysStat(info os.FileInfo) (*syscall.Stat_t, bool) { } func runSSHKeygen(ctx context.Context, directory string, payload []byte, args ...string) ([]byte, error) { - command := exec.CommandContext(ctx, sshKeygenPath, args...) + command := exec.CommandContext(ctx, sshKeygenPath, args...) // #nosec G204 -- Fixed /usr/bin/ssh-keygen binary; arguments bypass a shell. command.Dir = directory command.Env = []string{"PATH=/usr/bin:/bin", "LANG=C", "LC_ALL=C", "HOME=/nonexistent"} command.Stdin = bytes.NewReader(payload) @@ -251,12 +251,10 @@ func readAndAuthorizeAllowedSignersBytes(policy []byte, actor, keyID string) ([] name := temp.Name() defer os.Remove(name) if err := temp.Chmod(0o600); err != nil { - temp.Close() - return nil, err + return nil, closeWithError(err, temp) } if _, err := temp.Write(policy); err != nil { - temp.Close() - return nil, err + return nil, closeWithError(err, temp) } if err := temp.Close(); err != nil { return nil, err @@ -265,17 +263,15 @@ func readAndAuthorizeAllowedSignersBytes(policy []byte, actor, keyID string) ([] } func writePrivateVerificationFile(filePath string, data []byte) error { - file, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + file, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) // #nosec G304 -- Caller uses fixed artifact filenames under a new private verification directory; exclusive create rejects replacements. if err != nil { return errors.New("cannot create SSHSIG verification input") } if _, err := file.Write(data); err != nil { - file.Close() - return errors.New("cannot write SSHSIG verification input") + return closeWithError(errors.New("cannot write SSHSIG verification input"), file) } if err := file.Sync(); err != nil { - file.Close() - return errors.New("cannot sync SSHSIG verification input") + return closeWithError(errors.New("cannot sync SSHSIG verification input"), file) } if err := file.Close(); err != nil { return errors.New("cannot close SSHSIG verification input") diff --git a/tools/knowledge-retrieval/lifecycle_store.go b/tools/knowledge-retrieval/lifecycle_store.go index 9fd4537..79baba7 100644 --- a/tools/knowledge-retrieval/lifecycle_store.go +++ b/tools/knowledge-retrieval/lifecycle_store.go @@ -201,12 +201,10 @@ func writeAtomicPointerAlias(stateRoot string, pointer RetrievalGenerationPointe } defer root.Remove(temporary) if _, err := file.Write(data); err != nil { - file.Close() - return lifecycleFailure("pointer-alias-write-failed", "cannot write generation pointer alias") + return closeWithError(lifecycleFailure("pointer-alias-write-failed", "cannot write generation pointer alias"), file) } if err := file.Sync(); err != nil { - file.Close() - return lifecycleFailure("pointer-alias-write-failed", "cannot sync generation pointer alias") + return closeWithError(lifecycleFailure("pointer-alias-write-failed", "cannot sync generation pointer alias"), file) } if err := file.Close(); err != nil { return lifecycleFailure("pointer-alias-write-failed", "cannot close generation pointer alias") diff --git a/tools/knowledge-retrieval/main.go b/tools/knowledge-retrieval/main.go index 5b9cbd9..c134616 100644 --- a/tools/knowledge-retrieval/main.go +++ b/tools/knowledge-retrieval/main.go @@ -723,7 +723,7 @@ func runEvaluateRetrieval(args []string) { } func mustOutputFileHash(filePath string) string { - data, err := os.ReadFile(filePath) + data, err := os.ReadFile(filePath) // #nosec G304 -- Path comes from this command's generated output manifest/report result under the operator-selected store. if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(2) diff --git a/tools/knowledge-retrieval/security_cleanup.go b/tools/knowledge-retrieval/security_cleanup.go new file mode 100644 index 0000000..581c11c --- /dev/null +++ b/tools/knowledge-retrieval/security_cleanup.go @@ -0,0 +1,21 @@ +package main + +import ( + "encoding/json" + "errors" + "fmt" + "io" +) + +func closeWithError(primary error, closer io.Closer) error { + return errors.Join(primary, closer.Close()) +} + +func decodeEmbeddingResponse(body io.ReadCloser) (embeddingResponse, error) { + var decoded embeddingResponse + decodeErr := json.NewDecoder(body).Decode(&decoded) + if err := closeWithError(decodeErr, body); err != nil { + return decoded, fmt.Errorf("read or close embedding response: %w", err) + } + return decoded, nil +} diff --git a/tools/knowledge-retrieval/security_cleanup_test.go b/tools/knowledge-retrieval/security_cleanup_test.go new file mode 100644 index 0000000..da588ef --- /dev/null +++ b/tools/knowledge-retrieval/security_cleanup_test.go @@ -0,0 +1,79 @@ +package main + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +type failingCloser struct { + err error + calls int +} + +func (closer *failingCloser) Close() error { closer.calls++; return closer.err } +func TestCloseWithErrorPreservesBothFailures(t *testing.T) { + primary := errors.New("operation failed") + cleanup := errors.New("close failed") + for _, closeErr := range []error{nil, cleanup} { + closer := &failingCloser{err: closeErr} + got := closeWithError(primary, closer) + if !errors.Is(got, primary) || (closeErr != nil && !errors.Is(got, closeErr)) || closer.calls != 1 { + t.Fatalf("close result %v calls %d", got, closer.calls) + } + } + if got := closeWithError(nil, &failingCloser{}); got != nil { + t.Fatal(got) + } +} + +func TestOutputDirectoryPrivateAndRejectsSymlink(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "new", "store") + if err := ensureAbsoluteDirectoryTree(target); err != nil { + t.Fatal(err) + } + info, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm()&0o077 != 0 { + t.Errorf("output directory visible outside owner: %o", info.Mode().Perm()) + } + link := filepath.Join(root, "link") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + if err := ensureAbsoluteDirectoryTree(filepath.Join(link, "child")); err == nil { + t.Fatal("accepted symlink output directory") + } +} + +type embeddingBody struct { + *strings.Reader + err error + closed bool +} + +func (body *embeddingBody) Close() error { body.closed = true; return body.err } +func TestDecodeEmbeddingResponseClosesBodyAndReportsErrors(t *testing.T) { + closeErr := errors.New("body close failed") + for _, fixture := range []struct { + payload string + closeErr error + wantErr bool + }{ + {`{"data":[]}`, nil, false}, {`{"data":[]}`, closeErr, true}, {`{`, nil, true}, {``, nil, true}, + } { + body := &embeddingBody{Reader: strings.NewReader(fixture.payload), err: fixture.closeErr} + _, err := decodeEmbeddingResponse(body) + if !body.closed || (err != nil) != fixture.wantErr { + t.Errorf("payload %q err %v closed %v", fixture.payload, err, body.closed) + } + if fixture.closeErr != nil && !errors.Is(err, fixture.closeErr) { + t.Errorf("lost close cause: %v", err) + } + } +} diff --git a/tools/knowledge-retrieval/util.go b/tools/knowledge-retrieval/util.go index 11931d6..a85e1ae 100644 --- a/tools/knowledge-retrieval/util.go +++ b/tools/knowledge-retrieval/util.go @@ -107,14 +107,13 @@ func readStableFile(filePath, label string, maxBytes int64) ([]byte, error) { if err != nil || !before.Mode().IsRegular() || before.Mode()&os.ModeSymlink != 0 || before.Size() > maxBytes { return nil, fmt.Errorf("%s is not a bounded regular file", label) } - handle, err := os.Open(filePath) + handle, err := os.Open(filePath) // #nosec G304 -- Operator-selected input passes component, regular-file, size and identity checks. if err != nil { return nil, fmt.Errorf("%s cannot be opened", label) } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return nil, fmt.Errorf("%s changed before read", label) + return nil, closeWithError(fmt.Errorf("%s changed before read", label), handle) } data, readErr := io.ReadAll(io.LimitReader(handle, maxBytes+1)) closeErr := handle.Close() @@ -238,12 +237,10 @@ func writeImmutableObject(storeRoot, relative string, data []byte) error { } defer root.Remove(tempRelative) if _, err := temp.Write(data); err != nil { - temp.Close() - return err + return closeWithError(err, temp) } if err := temp.Sync(); err != nil { - temp.Close() - return err + return closeWithError(err, temp) } if err := temp.Close(); err != nil { return err @@ -300,12 +297,10 @@ func openSafeOutputRoot(storeRoot string) (*os.Root, fs.FileInfo, error) { } opened, err := root.Stat(".") if err != nil || !os.SameFile(before, opened) { - root.Close() - return nil, nil, errors.New("output store root changed while opening") + return nil, nil, closeWithError(errors.New("output store root changed while opening"), root) } if err := verifyRootStillBound(abs, root, opened); err != nil { - root.Close() - return nil, nil, err + return nil, nil, closeWithError(err, root) } return root, opened, nil } @@ -328,7 +323,7 @@ func ensureAbsoluteDirectoryTree(directory string) error { current = filepath.Join(current, part) info, statErr := os.Lstat(current) if errors.Is(statErr, fs.ErrNotExist) { - if err := os.Mkdir(current, 0o755); err != nil && !errors.Is(err, fs.ErrExist) { + if err := os.Mkdir(current, 0o700); err != nil && !errors.Is(err, fs.ErrExist) { return err } info, statErr = os.Lstat(current) @@ -407,8 +402,7 @@ func readRootStableFile(root *os.Root, relative string, maxBytes int64) ([]byte, } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return nil, errors.New("rooted object changed before read") + return nil, closeWithError(errors.New("rooted object changed before read"), handle) } data, readErr := io.ReadAll(io.LimitReader(handle, maxBytes+1)) closeErr := handle.Close() diff --git a/tools/knowledge-retrieval/verifier.go b/tools/knowledge-retrieval/verifier.go index e032f15..ef39aa0 100644 --- a/tools/knowledge-retrieval/verifier.go +++ b/tools/knowledge-retrieval/verifier.go @@ -82,7 +82,7 @@ func verifyEvidenceBoundary(inputs verifierInputs, verifierPath, verifierHash st // checked above. Re-opening the caller path for exec would leave a path // replacement window between hashing and execution. verifiedExecutable := filepath.Join(verifyStore, "knowledge-evidence") - if err := os.WriteFile(verifiedExecutable, verifierBytes, 0o500); err != nil { + if err := os.WriteFile(verifiedExecutable, verifierBytes, 0o500); err != nil { // #nosec G306 -- Owner execute is required to run the digest-pinned verifier; owner read and no group/world permissions. return []QuarantineIssue{issue("evidence_verifier_workspace_failed", "evidence-verifier", "verified evidence executable could not be materialized in its isolated workspace")} } if copied, err := readStableFile(verifiedExecutable, "verified evidence executable", maxVerifierBytes); err != nil || digestBytes(copied) != verifierHash { @@ -107,7 +107,7 @@ func verifyEvidenceBoundary(inputs verifierInputs, verifierPath, verifierHash st "--max-spans", strconv.Itoa(limits.maxSpans), "--max-output-bytes", strconv.FormatInt(limits.maxTotalBytes, 10), } - command := exec.CommandContext(ctx, verifiedExecutable, args...) + command := exec.CommandContext(ctx, verifiedExecutable, args...) // #nosec G204 -- Executes isolated knowledge-evidence bytes verified against the operator-pinned digest; no shell. command.Dir = verifyStore command.Env = []string{"PATH=/usr/bin:/bin", "LANG=C", "LC_ALL=C", "HOME=/nonexistent"} stdout := &cappedBuffer{limit: 1024 * 1024} diff --git a/tools/knowledge-snapshot/compiler.go b/tools/knowledge-snapshot/compiler.go index cb8362b..0cac721 100644 --- a/tools/knowledge-snapshot/compiler.go +++ b/tools/knowledge-snapshot/compiler.go @@ -649,7 +649,7 @@ func hashInput(sourcePath string) (string, int64, error) { if err != nil || !before.Mode().IsRegular() || before.Mode()&os.ModeSymlink != 0 { return "", 0, errors.New("unsafe source file") } - source, err := os.Open(sourcePath) + source, err := os.Open(sourcePath) // #nosec G304 -- Source is in the operator-selected mirror; regular-file and descriptor identity checks bracket reads. if err != nil { return "", 0, err } @@ -682,7 +682,7 @@ func stageAndHashInputRoot(sourcePath string, store *outputStore, stageRelative if err != nil || !before.Mode().IsRegular() || before.Mode()&os.ModeSymlink != 0 { return "", 0, errors.New("unsafe source file") } - source, err := os.Open(sourcePath) + source, err := os.Open(sourcePath) // #nosec G304 -- Source is in the operator-selected mirror; regular-file and descriptor identity checks bracket reads. if err != nil { return "", 0, err } @@ -737,7 +737,7 @@ func stageAndHashInputRoot(sourcePath string, store *outputStore, stageRelative } func verifyFileDigest(filePath, expectedDigest string, expectedSize int64) error { - file, err := os.Open(filePath) + file, err := os.Open(filePath) // #nosec G304 -- Path identifies a compiler-created CAS object, checked against its declared size and SHA-256 digest. if err != nil { return err } diff --git a/tools/knowledge-snapshot/output_store.go b/tools/knowledge-snapshot/output_store.go index 95fd827..805c85b 100644 --- a/tools/knowledge-snapshot/output_store.go +++ b/tools/knowledge-snapshot/output_store.go @@ -46,13 +46,11 @@ func openOutputStore(storeRoot string) (*outputStore, error) { } opened, err := root.Stat(".") if err != nil || !os.SameFile(before, opened) { - root.Close() - return nil, errors.New("output store root changed while opening") + return nil, closeWithError(errors.New("output store root changed while opening"), root) } store := &outputStore{path: abs, root: root, info: opened} if err := store.verifyBinding(); err != nil { - root.Close() - return nil, err + return nil, closeWithError(err, root) } return store, nil } @@ -92,7 +90,7 @@ func ensureOutputDirectoryTree(directory string) error { current = filepath.Join(current, component) info, statErr := os.Lstat(current) if errors.Is(statErr, fs.ErrNotExist) { - if err := os.Mkdir(current, 0o755); err != nil && !errors.Is(err, fs.ErrExist) { + if err := os.Mkdir(current, 0o700); err != nil && !errors.Is(err, fs.ErrExist) { return err } info, statErr = os.Lstat(current) @@ -198,8 +196,7 @@ func (store *outputStore) readStable(relative string, maxBytes int64) ([]byte, e } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return nil, errors.New("rooted object changed before read") + return nil, closeWithError(errors.New("rooted object changed before read"), handle) } data, readErr := io.ReadAll(io.LimitReader(handle, maxBytes+1)) closeErr := handle.Close() @@ -241,12 +238,10 @@ func (store *outputStore) writeImmutable(relative string, data []byte, mode fs.F } defer store.root.Remove(tempRelative) if _, err := temp.Write(data); err != nil { - temp.Close() - return err + return closeWithError(err, temp) } if err := temp.Sync(); err != nil { - temp.Close() - return err + return closeWithError(err, temp) } if err := temp.Close(); err != nil { return err @@ -371,8 +366,7 @@ func (store *outputStore) verifyDigest(relative, expectedDigest string, expected } opened, err := handle.Stat() if err != nil || !os.SameFile(before, opened) { - handle.Close() - return errors.New("rooted object changed before digest read") + return closeWithError(errors.New("rooted object changed before digest read"), handle) } hash := sha256.New() size, readErr := io.Copy(hash, handle) diff --git a/tools/knowledge-snapshot/review.go b/tools/knowledge-snapshot/review.go index da69862..ac67409 100644 --- a/tools/knowledge-snapshot/review.go +++ b/tools/knowledge-snapshot/review.go @@ -255,7 +255,7 @@ func readBoundedRegular(filePath string, limit int64) ([]byte, error) { if err != nil || !before.Mode().IsRegular() || before.Mode()&os.ModeSymlink != 0 || before.Size() > limit { return nil, errors.New("unsafe or oversized review artifact") } - file, err := os.Open(filePath) + file, err := os.Open(filePath) // #nosec G304 -- Operator-selected review artifact passes component, regular-file, size and identity checks. if err != nil { return nil, err } @@ -394,7 +394,7 @@ func verifySSHSIGNamespace(document, signature, allowedSigners []byte, principal ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() command := exec.CommandContext(ctx, sshKeygenPath, "-Y", "verify", "-f", policyPath, "-I", principal, - "-n", namespace, "-s", signaturePath) + "-n", namespace, "-s", signaturePath) // #nosec G204 -- Fixed /usr/bin/ssh-keygen verifier; private artifact paths and principal bypass a shell. command.Stdin = bytes.NewReader(document) command.Stdout = io.Discard command.Stderr = io.Discard diff --git a/tools/knowledge-snapshot/security_cleanup.go b/tools/knowledge-snapshot/security_cleanup.go new file mode 100644 index 0000000..e7927b3 --- /dev/null +++ b/tools/knowledge-snapshot/security_cleanup.go @@ -0,0 +1,10 @@ +package main + +import ( + "errors" + "io" +) + +func closeWithError(primary error, closer io.Closer) error { + return errors.Join(primary, closer.Close()) +} diff --git a/tools/knowledge-snapshot/security_cleanup_test.go b/tools/knowledge-snapshot/security_cleanup_test.go new file mode 100644 index 0000000..c29f834 --- /dev/null +++ b/tools/knowledge-snapshot/security_cleanup_test.go @@ -0,0 +1,51 @@ +package main + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +type failingCloser struct { + err error + calls int +} + +func (closer *failingCloser) Close() error { closer.calls++; return closer.err } +func TestCloseWithErrorPreservesBothFailures(t *testing.T) { + primary := errors.New("operation failed") + cleanup := errors.New("close failed") + for _, closeErr := range []error{nil, cleanup} { + closer := &failingCloser{err: closeErr} + got := closeWithError(primary, closer) + if !errors.Is(got, primary) || (closeErr != nil && !errors.Is(got, closeErr)) || closer.calls != 1 { + t.Fatalf("close result %v calls %d", got, closer.calls) + } + } + if got := closeWithError(nil, &failingCloser{}); got != nil { + t.Fatal(got) + } +} + +func TestOutputDirectoryPrivateAndRejectsSymlink(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "new", "store") + if err := ensureOutputDirectoryTree(target); err != nil { + t.Fatal(err) + } + info, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm()&0o077 != 0 { + t.Errorf("output directory visible outside owner: %o", info.Mode().Perm()) + } + link := filepath.Join(root, "link") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + if err := ensureOutputDirectoryTree(filepath.Join(link, "child")); err == nil { + t.Fatal("accepted symlink output directory") + } +} diff --git a/tools/knowledge-snapshot/standing_admission.go b/tools/knowledge-snapshot/standing_admission.go index 6d395eb..0e2ee2f 100644 --- a/tools/knowledge-snapshot/standing_admission.go +++ b/tools/knowledge-snapshot/standing_admission.go @@ -604,7 +604,7 @@ func privateSignerPublicDigest(keyPath, storeRoot string) (string, error) { } ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() - command := exec.CommandContext(ctx, "/usr/bin/ssh-keygen", "-y", "-f", keyPath) + command := exec.CommandContext(ctx, "/usr/bin/ssh-keygen", "-y", "-f", keyPath) // #nosec G204 -- Fixed /usr/bin/ssh-keygen extracts the operator-selected signing key; no shell. command.Stdin = nil var public bytes.Buffer command.Stdout = &public @@ -643,7 +643,7 @@ func signStandingReceipt(keyPath string, receipt []byte) ([]byte, error) { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() command := exec.CommandContext(ctx, "/usr/bin/ssh-keygen", "-Y", "sign", "-q", "-f", keyPath, - "-n", reviewSignatureNamespace, receiptPath) + "-n", reviewSignatureNamespace, receiptPath) // #nosec G204 -- Fixed /usr/bin/ssh-keygen signs the private receipt with the operator-selected key; no shell. command.Stdin = nil command.Stdout = io.Discard command.Stderr = io.Discard