diff --git a/cmd/bbox/agents_import.go b/cmd/bbox/agents_import.go index f4f9fbf..a3b1dc2 100644 --- a/cmd/bbox/agents_import.go +++ b/cmd/bbox/agents_import.go @@ -4,39 +4,67 @@ package main import ( + "context" "errors" "fmt" + "io" + "io/fs" "os" + "strings" + "github.com/google/go-containerregistry/pkg/name" "github.com/spf13/cobra" infraconfig "github.com/stacklok/brood-box/internal/infra/config" + "github.com/stacklok/brood-box/internal/infra/configfile" + infraocimage "github.com/stacklok/brood-box/internal/infra/ocimage" domainconfig "github.com/stacklok/brood-box/pkg/domain/config" ) -// agentsImportCmd is `bbox agents import `: read a standalone agent -// manifest, validate it, and append the agent to the global config (reuse the -// exact path `agents add` takes so behavior, receipts, and safety match). +// agentsImportCmd is `bbox agents import `: read a standalone agent +// manifest — from a local YAML file or a self-describing OCI image — validate +// it, and append the agent to the global config (reuse the exact path `agents +// add` takes so behavior, receipts, and safety match). func agentsImportCmd() *cobra.Command { var ( - cfgPath string - force bool - jsonOut bool + cfgPath string + nameOverride string + force bool + jsonOut bool ) cmd := &cobra.Command{ - Use: "import ", - Short: "Import a custom agent from a manifest file into the global config", - Long: `Reads a standalone agent manifest (a YAML file with a top-level name -plus the same fields as an agents: block), validates it with the same -checks the loader uses, and appends it to the global config + Use: "import ", + Short: "Import a custom agent from a manifest file or OCI image into the global config", + Long: `Reads a standalone agent manifest, validates it with the same checks the +loader uses, and appends it to the global config (~/.config/broodbox/config.yaml, or the path given by --config). Existing comments/formatting in the config are preserved; the added agent block is written as normalized YAML. +A source is treated as an OCI image when it is not an existing local file AND +it looks like an image reference an operator would type. Specifically, the +source must contain an ` + "`@sha256:`" + ` digest, or a ` + "`/`" + ` whose +leading segment looks like a registry host (e.g. ` + "`ghcr.io`" + ` in +ghcr.io/acme/aider-bbox:latest, or ` + "`localhost:5001`" + `). A bare +library image like ` + "`ubuntu:24.04`" + ` (no slash), or a path with a +subdirectory that isn't registry-shaped like ` + "`manifests/aider.yaml`" + `, +is treated as a local file path — use the fully-qualified +` + "`docker.io/library/ubuntu:24.04`" + ` to import a bare library image. For +an image, the manifest is extracted from an embedded +agent.yaml declared by the ` + "`org.stacklok.broodbox.agent`" + ` config label, +or from the well-known path ` + "`/usr/share/broodbox/agent.yaml`" + ` when the +label is absent. The imported image ref is pinned to its resolved digest so the +registered agent is reproducible. If the embedded manifest declares a different +repository, a warning is printed and the imported (digest-pinned) ref overrides +it. + Custom agents are GLOBAL-ONLY: this command never writes to a workspace .broodbox.yaml. Refuses to overwrite a built-in or an existing custom agent unless --force. +--name overrides the manifest's name (useful when the embedded name collides +or is undescriptive). + The manifest format is identical to an agents: entry, with a top-level name field: @@ -52,25 +80,45 @@ name field: - { name: api.openai.com, ports: [443] }`, Example: ` bbox agents import ./broodbox-agent.yaml bbox agents import ./aider.yaml --json - bbox agents import ./aider.yaml --force # overwrite an existing agent`, + bbox agents import ghcr.io/acme/aider-bbox:latest + bbox agents import ghcr.io/acme/aider-bbox:latest --name aider2 --force`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return runAgentsImport(cmd, args[0], cfgPath, force, jsonOut) + return runAgentsImport(cmd, args[0], cfgPath, nameOverride, force, jsonOut, defaultImportFetcher) }, } cmd.Flags().StringVar(&cfgPath, "config", "", "Config file path (default: ~/.config/broodbox/config.yaml)") + cmd.Flags().StringVar(&nameOverride, "name", "", "Override the manifest's agent name") cmd.Flags().BoolVarP(&force, "force", "f", false, "Overwrite a built-in or existing custom agent") cmd.Flags().BoolVar(&jsonOut, "json", false, "Emit a JSON receipt of the mutation instead of human-readable output") return cmd } -func runAgentsImport(cmd *cobra.Command, manifestPath, cfgPath string, force, jsonOut bool) error { - manifest, err := infraconfig.LoadManifest(manifestPath) +// defaultImportFetcher is the Fetcher used by the real CLI. Tests override it +// (via runAgentsImport's fetcher parameter) to inject an in-memory fake so the +// e2e path is exercised without network. +var defaultImportFetcher infraocimage.Fetcher = infraocimage.NewRemoteFetcher() + +// runAgentsImport imports a custom agent from a local manifest file or an OCI +// image. source is classified by isImageRef; when it is an image, fetcher +// pulls the embedded manifest and the digest-pinned image ref. The validate → +// collision-gate → UpsertAgent → receipt tail is shared with the file path. +func runAgentsImport( + cmd *cobra.Command, + source, cfgPath, nameOverride string, + force, jsonOut bool, + fetcher infraocimage.Fetcher, +) error { + manifest, sourceLabel, err := loadImportManifest(cmd.Context(), source, fetcher, cmd.ErrOrStderr()) if err != nil { return err } + + if nameOverride != "" { + manifest.Name = nameOverride + } if manifest.Name == "" { - return fmt.Errorf("manifest %s: top-level %q field is required", manifestPath, "name") + return fmt.Errorf("manifest %s: top-level %q field is required", sourceLabel, "name") } override := manifest.AgentOverride @@ -106,6 +154,108 @@ func runAgentsImport(cmd *cobra.Command, manifestPath, cfgPath string, force, js return emitAddResult(cmd.OutOrStdout(), receipt, jsonOut) } +// loadImportManifest loads an AgentManifest from a local file or an OCI image, +// depending on whether source is an existing file or an image reference. For an +// image, the manifest's image field is overridden with the digest-pinned ref +// (with a warning on mismatch written to warn), and sourceLabel describes the +// source for error messages. +func loadImportManifest( + ctx context.Context, + source string, + fetcher infraocimage.Fetcher, + warn io.Writer, +) (domainconfig.AgentManifest, string, error) { + if isImageRef(source) { + manifestBytes, pinnedRef, err := fetcher.FetchAgentManifest(ctx, source) + if err != nil { + // The Fetcher contract promises a ref-identifying error already — + // don't double-wrap (RemoteFetcher wraps every error path with + // "importing agent from image %q"). + return domainconfig.AgentManifest{}, source, err + } + // Reuse the same size cap + strict decode as the file path so a + // malformed or oversized embedded manifest fails loudly. + var m domainconfig.AgentManifest + if err := configfile.DecodeStrict(manifestBytes, &m); err != nil { + return domainconfig.AgentManifest{}, source, fmt.Errorf("parsing manifest from image %s: %w", source, err) + } + + // The imported ref is authoritative: pin to the resolved digest so the + // registered agent is reproducible. Warn (don't fail) only on a real + // REPOSITORY mismatch — a same-repo tag→digest pin is the normal case and + // should be silent. + if m.Image != "" { + declaredRepo, parseErr := name.ParseReference(m.Image) + if parseErr != nil { + _, _ = fmt.Fprintf(warn, "Warning: image manifest declares a malformed image %q; overriding with imported ref %q\n", m.Image, pinnedRef) + } else if importedRepo, err := name.ParseReference(source); err == nil && + declaredRepo.Context().Name() != importedRepo.Context().Name() { + _, _ = fmt.Fprintf(warn, "Warning: image manifest declares image %q; overriding with imported ref %q\n", m.Image, pinnedRef) + } + } + m.Image = pinnedRef + return m, source, nil + } + + manifest, err := infraconfig.LoadManifest(source) + if err != nil { + return domainconfig.AgentManifest{}, source, err + } + return manifest, source, nil +} + +// isImageRef reports whether source should be treated as an OCI image reference +// rather than a local manifest file. A source is an image when it is NOT an +// existing local file AND it contains an "@sha256:" digest (an explicit digest +// is unambiguous intent) OR contains a "/" whose first segment looks like a +// registry host (has a "." or ":", or is exactly "localhost" — e.g. +// ghcr.io/acme/x:latest, localhost:5001/x:latest), AND it parses as a valid +// image reference. A slash-containing source whose first segment does NOT look +// like a registry host (e.g. "manifests/aider.yaml", "subdir/foo.yaml") is a +// subdirectory file path, not an image reference — this excludes bare +// local-looking names, including ones with subdirectories, so a typo'd or +// relative manifest path does not trigger a network pull. A nonexistent path +// that does not qualify falls through to the file path, which then produces a +// precise "no such file" error from LoadManifest. +func isImageRef(source string) bool { + if _, err := os.Stat(source); err == nil { + // Exists on disk — treat as a file, even if it also looks like a ref. + return false + } else if !errors.Is(err, fs.ErrNotExist) { + // A stat error that isn't "not found" (e.g. permission) is surfaced by + // the file path, which wraps it readably. Don't claim it's an image. + return false + } + if strings.HasPrefix(source, "./") || strings.HasPrefix(source, "../") || strings.HasPrefix(source, "/") { + // A relative or absolute path — even one that contains "/" — is a + // file path, not an image reference. + return false + } + hasDigest := strings.Contains(source, "@sha256:") + if slashIdx := strings.Index(source, "/"); slashIdx < 0 { + if !hasDigest { + // No slash, no digest: a bare name like "aider" or "ubuntu:24.04". + return false + } + } else if !hasDigest && !looksLikeRegistryHost(source[:slashIdx]) { + // Slash present but the leading segment isn't registry-host-shaped and + // there's no digest to disambiguate — treat as a subdirectory path. + return false + } + if _, err := name.ParseReference(source); err != nil { + return false + } + return true +} + +// looksLikeRegistryHost reports whether segment (the text before the first +// "/" in a source string) is shaped like a registry host rather than a +// directory name — i.e. it contains a "." (a domain, e.g. ghcr.io) or a ":" +// (a host:port, e.g. localhost:5001), or is exactly "localhost". +func looksLikeRegistryHost(segment string) bool { + return strings.ContainsAny(segment, ".:") || segment == "localhost" +} + // agentsExportCmd is `bbox agents export `: emit a standalone manifest // for an existing custom agent to stdout. Env VALUES are never emitted — // DefaultEnv is stripped so only env NAMES/patterns leave the host. diff --git a/cmd/bbox/agents_import_test.go b/cmd/bbox/agents_import_test.go index 10bb5f7..ee573eb 100644 --- a/cmd/bbox/agents_import_test.go +++ b/cmd/bbox/agents_import_test.go @@ -5,7 +5,9 @@ package main import ( "bytes" + "context" "encoding/json" + "fmt" "os" "path/filepath" "testing" @@ -421,3 +423,517 @@ func TestLoadManifestOperatorPathAcceptsSymlink(t *testing.T) { require.NoError(t, err) assert.Equal(t, "aider", m.Name) } + +// fakeFetcher is an in-memory ocimage.Fetcher for e2e tests of the image +// import path. It returns canned manifest bytes and a fake digest-pinned ref, +// so the full CLI path is exercised without network. +type fakeFetcher struct { + manifestBytes []byte + pinnedRef string + err error + calledRef string +} + +func (f *fakeFetcher) FetchAgentManifest(_ context.Context, ref string) ([]byte, string, error) { + f.calledRef = ref + if f.err != nil { + // Mirror RemoteFetcher's contract: every error is wrapped with the ref + // so callers never need to add their own ref-identifying wrap. + return nil, "", fmt.Errorf("importing agent from image %q: %w", ref, f.err) + } + return f.manifestBytes, f.pinnedRef, nil +} + +// imageManifestYAML is a manifest as it would be embedded in an image. Its +// image field is intentionally the un-pinned tag; the import path overrides it +// with the digest-pinned ref returned by the fake fetcher. +const imageManifestYAML = `name: aider +image: ghcr.io/acme/aider-bbox:latest +command: ["aider"] +description: ACME agent +env_forward: [OPENAI_API_KEY] +env_required: [OPENAI_API_KEY] +egress_profile: standard +egress_hosts: + standard: + - name: api.openai.com + ports: [443] +mcp: + enabled: true + mode: env + authz: + profile: safe-tools +` + +// fakePinnedRef is a digest-pinned ref shape the fake fetcher returns. It must +// pass imageRefValidator (name.ParseReference), so it is a valid digest ref. +const fakePinnedRef = "ghcr.io/acme/aider-bbox@sha256:0000000000000000000000000000000000000000000000000000000000000000" + +func TestAgentsImportImageEndToEnd(t *testing.T) { + t.Setenv("OPENAI_API_KEY", "present-value") + + fetcher := &fakeFetcher{ + manifestBytes: []byte(imageManifestYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + var out, errBuf bytes.Buffer + cmd := agentsImportCmd() + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath, "--json"}) + require.NoError(t, runAgentsImport(cmd, imageRef, cfgPath, "", false, true, fetcher)) + + // The fetcher was invoked with the operator-typed ref. + assert.Equal(t, imageRef, fetcher.calledRef) + + var receipt agentReceipt + require.NoError(t, json.Unmarshal(out.Bytes(), &receipt)) + assert.Equal(t, "agents import", receipt.Command) + assert.True(t, receipt.OK) + assert.Equal(t, "custom", receipt.Agent.Type) + assert.Equal(t, "aider", receipt.Agent.Name) + // The image field is the digest-pinned ref, not the un-pinned tag. + assert.Equal(t, fakePinnedRef, receipt.Agent.Image) + assert.Equal(t, "standard", receipt.Agent.EgressProfile) + assert.Equal(t, domainconfig.MCPModeEnv, receipt.Agent.MCPMode) + require.NotNil(t, receipt.Write) + assert.True(t, receipt.Write.Created) + + // doctor --json on the imported agent passes (the pinned ref is a valid ref). + var dout bytes.Buffer + dcmd := agentsCmd() + dcmd.SetOut(&dout) + dcmd.SetErr(&dout) + dcmd.SetArgs([]string{"doctor", "aider", "--config", cfgPath, "--json"}) + require.NoError(t, dcmd.Execute()) + + var dr agentReceipt + require.NoError(t, json.Unmarshal(dout.Bytes(), &dr)) + assert.True(t, dr.OK) + + // The written config round-trips through the loader with the pinned image. + loaded, err := infraconfig.NewLoader(cfgPath).Load() + require.NoError(t, err) + custom, ok := loaded.Agents["aider"] + require.True(t, ok) + assert.Equal(t, fakePinnedRef, custom.Image) + assert.Equal(t, []string{"aider"}, custom.Command) + assert.Equal(t, []string{"OPENAI_API_KEY"}, custom.EnvForward) + require.NotNil(t, custom.MCP) + assert.Equal(t, domainconfig.MCPModeEnv, custom.MCP.Mode) +} + +func TestAgentsImportImageWarnsOnDeclaredImageMismatch(t *testing.T) { + t.Parallel() + + // The embedded manifest declares a DIFFERENT repository than the imported + // ref — this is a real mismatch and must warn. (A same-repo tag→digest pin + // is silent, exercised by TestAgentsImportImageSameRepoNoWarning.) + embeddedYAML := `name: aider +image: ghcr.io/acme/other-bbox:latest +command: ["aider"] +egress_profile: permissive +` + fetcher := &fakeFetcher{ + manifestBytes: []byte(embeddedYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + var out, errBuf bytes.Buffer + cmd := agentsImportCmd() + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + require.NoError(t, runAgentsImport(cmd, imageRef, cfgPath, "", false, false, fetcher)) + + // The warning names the declared image and the overriding pinned ref. + warning := errBuf.String() + assert.Contains(t, warning, "Warning: image manifest declares image") + assert.Contains(t, warning, "ghcr.io/acme/other-bbox:latest") + assert.Contains(t, warning, fakePinnedRef) + + // The persisted image is the pinned ref, not the declared tag. + loaded, err := infraconfig.NewLoader(cfgPath).Load() + require.NoError(t, err) + assert.Equal(t, fakePinnedRef, loaded.Agents["aider"].Image) +} + +func TestAgentsImportImageSameRepoNoWarning(t *testing.T) { + t.Parallel() + + // The embedded manifest declares the same repository as the imported ref + // (only the tag differs). Pinning the tag to a digest is the normal case — + // no warning should be emitted. + fetcher := &fakeFetcher{ + manifestBytes: []byte(imageManifestYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + var out, errBuf bytes.Buffer + cmd := agentsImportCmd() + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath, "--json"}) + require.NoError(t, runAgentsImport(cmd, imageRef, cfgPath, "", false, true, fetcher)) + + // No mismatch warning — the declared tag and pinned digest share a repo. + assert.NotContains(t, errBuf.String(), "Warning: image manifest declares image") +} + +func TestAgentsImportImageMalformedDeclaredImageWarns(t *testing.T) { + t.Parallel() + + // The embedded manifest declares a malformed image ref — the import path + // cannot compare repositories, so it warns that the declared image is + // malformed. + embeddedYAML := `name: aider +image: "not a valid ref with spaces" +command: ["aider"] +egress_profile: permissive +` + fetcher := &fakeFetcher{ + manifestBytes: []byte(embeddedYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + var out, errBuf bytes.Buffer + cmd := agentsImportCmd() + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + require.NoError(t, runAgentsImport(cmd, imageRef, cfgPath, "", false, false, fetcher)) + + assert.Contains(t, errBuf.String(), "malformed image") + + loaded, err := infraconfig.NewLoader(cfgPath).Load() + require.NoError(t, err) + assert.Equal(t, fakePinnedRef, loaded.Agents["aider"].Image) +} + +func TestAgentsImportImageNameOverride(t *testing.T) { + t.Parallel() + + fetcher := &fakeFetcher{ + manifestBytes: []byte(imageManifestYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + var out, errBuf bytes.Buffer + cmd := agentsImportCmd() + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath, "--json", "--name", "aider2"}) + require.NoError(t, runAgentsImport(cmd, imageRef, cfgPath, "aider2", false, true, fetcher)) + + var receipt agentReceipt + require.NoError(t, json.Unmarshal(out.Bytes(), &receipt)) + assert.Equal(t, "aider2", receipt.Agent.Name) + + loaded, err := infraconfig.NewLoader(cfgPath).Load() + require.NoError(t, err) + _, ok := loaded.Agents["aider2"] + assert.True(t, ok) + _, originalOk := loaded.Agents["aider"] + assert.False(t, originalOk, "the --name override renamed the agent, not duplicated it") +} + +func TestAgentsImportImageRefusesBuiltinWithoutForce(t *testing.T) { + t.Parallel() + + // The embedded manifest names a built-in; without --force this is rejected + // before any config is written. + fetcher := &fakeFetcher{ + manifestBytes: []byte(`name: claude-code +image: ghcr.io/acme/x:latest +command: ["x"] +egress_profile: permissive +`), + pinnedRef: "ghcr.io/acme/x@sha256:1111111111111111111111111111111111111111111111111111111111111111", + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/x:latest" + + cmd := agentsImportCmd() + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + err := runAgentsImport(cmd, imageRef, cfgPath, "", false, false, fetcher) + require.Error(t, err) + assert.Contains(t, err.Error(), "built-in agent") + _, statErr := os.Stat(cfgPath) + assert.True(t, os.IsNotExist(statErr)) +} + +func TestAgentsImportImageRejectsMalformedManifest(t *testing.T) { + t.Parallel() + + fetcher := &fakeFetcher{ + manifestBytes: []byte("name: : not yaml {"), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + cmd := agentsImportCmd() + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + err := runAgentsImport(cmd, imageRef, cfgPath, "", false, false, fetcher) + require.Error(t, err) + assert.Contains(t, err.Error(), "parsing manifest from image") +} + +func TestAgentsImportImageFetcherErrorPropagation(t *testing.T) { + t.Parallel() + + // The fetcher returns an error; the CLI surfaces it wrapped with the image + // ref and the fetcher's error message. + fetchErr := fmt.Errorf("boom: connection refused") + fetcher := &fakeFetcher{ + err: fetchErr, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + cmd := agentsImportCmd() + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + err := runAgentsImport(cmd, imageRef, cfgPath, "", false, false, fetcher) + require.Error(t, err) + assert.Contains(t, err.Error(), imageRef) + assert.Contains(t, err.Error(), "connection refused") + + // No config written on fetch failure. + _, statErr := os.Stat(cfgPath) + assert.True(t, os.IsNotExist(statErr)) +} + +func TestAgentsImportImageNameOverrideToBuiltin(t *testing.T) { + t.Parallel() + + // --name overriding to a built-in name is rejected even though the + // embedded manifest's name is a custom one. + fetcher := &fakeFetcher{ + manifestBytes: []byte(imageManifestYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + cmd := agentsImportCmd() + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath, "--name", "claude-code"}) + err := runAgentsImport(cmd, imageRef, cfgPath, "claude-code", false, false, fetcher) + require.Error(t, err) + assert.Contains(t, err.Error(), "built-in agent") + + _, statErr := os.Stat(cfgPath) + assert.True(t, os.IsNotExist(statErr)) +} + +func TestAgentsImportImageCollidesWithExistingCustom(t *testing.T) { + t.Parallel() + + fetcher := &fakeFetcher{ + manifestBytes: []byte(imageManifestYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + // First import succeeds. + cmd1 := agentsImportCmd() + cmd1.SetOut(&bytes.Buffer{}) + cmd1.SetErr(&bytes.Buffer{}) + cmd1.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + require.NoError(t, runAgentsImport(cmd1, imageRef, cfgPath, "", false, false, fetcher)) + + // Second import without --force fails with "already exists". + cmd2 := agentsImportCmd() + cmd2.SetOut(&bytes.Buffer{}) + cmd2.SetErr(&bytes.Buffer{}) + cmd2.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + err := runAgentsImport(cmd2, imageRef, cfgPath, "", false, false, fetcher) + require.Error(t, err) + assert.Contains(t, err.Error(), "already exists") + + // Second import with --force succeeds. + cmd3 := agentsImportCmd() + cmd3.SetOut(&bytes.Buffer{}) + cmd3.SetErr(&bytes.Buffer{}) + cmd3.SetArgs([]string{"import", imageRef, "--config", cfgPath}) + require.NoError(t, runAgentsImport(cmd3, imageRef, cfgPath, "", true, false, fetcher)) +} + +func TestAgentsImportImageNoImageField(t *testing.T) { + t.Parallel() + + // An image manifest with no `image` field: import succeeds, no warning is + // printed, and the config holds the digest-pinned ref from the import. + embeddedYAML := `name: aider +command: ["aider"] +egress_profile: permissive +` + fetcher := &fakeFetcher{ + manifestBytes: []byte(embeddedYAML), + pinnedRef: fakePinnedRef, + } + + cfgPath := filepath.Join(t.TempDir(), "config.yaml") + imageRef := "ghcr.io/acme/aider-bbox:latest" + + var out, errBuf bytes.Buffer + cmd := agentsImportCmd() + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + cmd.SetArgs([]string{"import", imageRef, "--config", cfgPath, "--json"}) + require.NoError(t, runAgentsImport(cmd, imageRef, cfgPath, "", false, true, fetcher)) + + // No warning when the declared image is absent. + assert.NotContains(t, errBuf.String(), "Warning:") + + var receipt agentReceipt + require.NoError(t, json.Unmarshal(out.Bytes(), &receipt)) + assert.True(t, receipt.OK) + assert.Equal(t, fakePinnedRef, receipt.Agent.Image) + + // The persisted image is the digest-pinned ref. + loaded, err := infraconfig.NewLoader(cfgPath).Load() + require.NoError(t, err) + assert.Equal(t, fakePinnedRef, loaded.Agents["aider"].Image) +} + +func TestIsImageRef(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + existingFile := filepath.Join(dir, "aider.yaml") + require.NoError(t, os.WriteFile(existingFile, []byte("name: x\n"), 0o600)) + + tests := []struct { + name string + source string + want bool + }{ + { + name: "existing local file is not an image", + source: existingFile, + want: false, + }, + { + name: "fully-qualified image ref is an image", + source: "ghcr.io/acme/aider-bbox:latest", + want: true, + }, + { + name: "image ref by digest is an image", + source: "ghcr.io/acme/aider-bbox@sha256:0000000000000000000000000000000000000000000000000000000000000000", + want: true, + }, + { + name: "nonexistent path that does not parse as a ref falls through to file", + source: filepath.Join(dir, "does-not-exist-and-not-a-ref.yaml"), + want: false, + }, + { + name: "docker hub short ref with no slash is treated as a file", + source: "ubuntu:24.04", + want: false, + }, + { + name: "fully-qualified docker hub library ref is an image", + source: "docker.io/library/ubuntu:24.04", + want: true, + }, + { + name: "misspelled bare manifest path is not an image", + source: "aider.yaml", + want: false, + }, + { + name: "misspelled bare relative manifest path is not an image", + source: "./aider.yaml", + want: false, + }, + { + name: "bare name with no extension is not an image", + source: "aider", + want: false, + }, + { + name: "image ref by digest with slash is an image", + source: "ghcr.io/acme/x@sha256:0000000000000000000000000000000000000000000000000000000000000000", + want: true, + }, + { + name: "parent-dir relative path is not an image", + source: "../aider.yaml", + want: false, + }, + { + name: "absolute path is not an image even if it parses as a ref", + source: "/nonexistent/absolute/path.yaml", + want: false, + }, + { + name: "subdirectory manifest path is not an image", + source: "manifests/aider.yaml", + want: false, + }, + { + name: "another subdirectory manifest path is not an image", + source: "subdir/foo.yaml", + want: false, + }, + { + name: "localhost with port and repo is an image", + source: "localhost:5001/mecatui:latest", + want: true, + }, + { + name: "ghcr.io ref is an image", + source: "ghcr.io/acme/x:latest", + want: true, + }, + { + name: "docker.io fully-qualified ref is an image", + source: "docker.io/library/ubuntu:24.04", + want: true, + }, + // A permission-denied stat error (non-ErrNotExist) is intentionally not + // tested here: it is filesystem- and privilege-dependent (root bypasses + // it, some CI runners run as root, some filesystems don't honor 0o000), + // making a hermetic, non-flaky subtest impractical. The branch is covered + // by inspection of isImageRef: any non-ErrNotExist stat error returns + // false and falls through to the file path. + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, isImageRef(tc.source)) + }) + } +} diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index f3342fb..5fb28bb 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -199,6 +199,37 @@ agents: memory: "1g" ``` +### Importing agents + +`bbox agents import` reads a standalone agent manifest, validates it, and +appends the agent to the global config. It works from two sources, picked +automatically: + +- **A local manifest file** — `bbox agents import ./aider.yaml`. The manifest + is a YAML file with a top-level `name` plus the same fields as an + `agents:` block. +- **A self-describing OCI image** — `bbox agents import ghcr.io/acme/aider-bbox:latest`. + The manifest is read from an `agent.yaml` embedded in the image, located by + the `org.stacklok.broodbox.agent` config label or the well-known path + `/usr/share/broodbox/agent.yaml`. The image ref is pinned to its resolved + digest so the registered agent is reproducible. + +```bash +# From a local file +bbox agents import ./aider.yaml + +# From an image (reads the embedded manifest, pins to digest) +bbox agents import ghcr.io/acme/aider-bbox:latest + +# Override the manifest's name, or overwrite an existing agent +bbox agents import ghcr.io/acme/aider-bbox:latest --name aider2 --force +``` + +Both paths reuse the same validation, collision-gating (`--force` to overwrite +a built-in or existing custom agent), and `--json` receipt as `bbox agents add`. +`bbox agents export ` writes a manifest back out for a custom agent +(stripping env values), so `export` → `import` round-trips cleanly. + ### Config Resolution Order For each setting, the first non-zero value wins: diff --git a/docs/run-image.md b/docs/run-image.md index 9c74e54..62121c1 100644 --- a/docs/run-image.md +++ b/docs/run-image.md @@ -95,6 +95,42 @@ An arbitrary image used with `run-image` must provide: `bbox-init` is **not** in the image — it is embedded by the VM runtime and runs as PID 1. The image only provides the userspace/agent tooling. +## Self-describing images (`agents import`) + +An image that satisfies the minimum contract above can also be **self-describing**: +`bbox agents import IMAGE` reads an embedded `agent.yaml` from the image and +registers the agent in the global config with no local YAML authoring. The +manifest is located by one of: + +- the OCI config label `org.stacklok.broodbox.agent`, whose value is the path + to the manifest file inside the image, or +- the well-known fallback path `/usr/share/broodbox/agent.yaml` when the label + is absent. + +Layers are walked topmost-first, so a manifest in a later layer shadows one in +an earlier layer (matching how a container's filesystem is assembled). The +imported image ref is pinned to its resolved digest, so a moving `:latest` tag +is captured at import time; if the embedded manifest declares a different +`image`, `import` warns and overrides it with the imported ref. + +The embedded manifest uses the same format as a local manifest file (a +top-level `name` plus the `agents:` fields): + +```yaml +name: aider +image: ghcr.io/acme/aider-bbox:latest # overridden with the imported, digest-pinned ref +command: ["aider"] +env_forward: [OPENAI_API_KEY] +egress_profile: standard +egress_hosts: + standard: + - { name: api.openai.com, ports: [443] } +``` + +`run-image` itself does not read an embedded manifest — it stays flag-driven. +The label/well-known-path convention is consumed only by `agents import`. See +the User Guide for `agents import` usage. + ## Flags `run-image` accepts a subset of the root command's flags. Notable differences: diff --git a/internal/infra/ocimage/ocimage.go b/internal/infra/ocimage/ocimage.go new file mode 100644 index 0000000..89f6af8 --- /dev/null +++ b/internal/infra/ocimage/ocimage.go @@ -0,0 +1,267 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package ocimage reads a broodbox agent manifest embedded in an OCI image. +// +// A self-describing agent image carries its agent.yaml either at a path named +// by the org.stacklok.broodbox.agent config label, or at the well-known path +// /usr/share/broodbox/agent.yaml when the label is absent. FetchAgentManifest +// pulls just enough of the image (config + the one layer holding the manifest) +// to read those bytes and resolve the digest the ref pinned to, so +// `bbox agents import IMAGE` can register a runnable agent with no local YAML +// authoring. +package ocimage + +import ( + "archive/tar" + "context" + "errors" + "fmt" + "io" + "strings" + "time" + + "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/name" + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/partial" + "github.com/google/go-containerregistry/pkg/v1/remote" + "github.com/google/go-containerregistry/pkg/v1/remote/transport" + + "github.com/stacklok/brood-box/internal/infra/configfile" +) + +const ( + // LabelAgent is the OCI config label naming the in-image path to the + // broodbox agent manifest. When present, its value overrides the + // well-known path. + LabelAgent = "org.stacklok.broodbox.agent" + + // WellKnownManifestPath is the fallback location of the agent manifest + // inside an image that does not set LabelAgent. + WellKnownManifestPath = "/usr/share/broodbox/agent.yaml" +) + +const ( + // maxImageConfigSize caps the OCI image config blob size (as declared by + // the manifest's config descriptor) that extractFromImage will fetch. The + // config is small JSON (labels, env, entrypoint) — a registry declaring a + // multi-MB+ config is not a legitimate agent image, it's a DoS attempt. + maxImageConfigSize = 4 << 20 // 4 MiB — OCI config is small JSON; reject absurd declarations + + // maxImageLayers caps the number of layers extractFromImage will walk + // searching for the embedded manifest. Generous headroom above real-world + // images (well under Docker's historical 127-layer norm) while still + // bounding a maliciously crafted manifest with thousands of layers. + maxImageLayers = 512 // generous; real images are well under Docker's historical 127-layer norm +) + +// Fetcher reads a broodbox agent manifest from an OCI image. +type Fetcher interface { + // FetchAgentManifest pulls just enough of the image to read the embedded + // broodbox agent manifest. It returns the raw manifest bytes and the + // digest-pinned reference (repo@sha256:...) the source ref resolved to, + // for pinning the agent's image field at import time. + FetchAgentManifest(ctx context.Context, ref string) (manifest []byte, pinnedRef string, err error) +} + +// RemoteFetcher pulls manifests from a remote registry via go-containerregistry. +// The zero value is usable but pulls anonymously; use NewRemoteFetcher to pick up +// host credentials from the default keychain (docker login/podman login). Remote +// options (auth, platform) may be supplied via NewRemoteFetcher, which stores +// them in the unexported remoteOptions field. +type RemoteFetcher struct { + // remoteOptions are passed to remote.Image (auth, transport, platform + // selection). nil/empty uses anonymous access and the host platform. + remoteOptions []remote.Option +} + +// NewRemoteFetcher returns a RemoteFetcher that pulls with the given +// remote.Options appended after the default keychain, so `docker login`/`podman +// login` credentials are picked up automatically. Callers that need fully +// custom auth can pass a remote.WithAuth option, which overrides the keychain +// for that request. +func NewRemoteFetcher(opts ...remote.Option) *RemoteFetcher { + return &RemoteFetcher{remoteOptions: append([]remote.Option{remote.WithAuthFromKeychain(authn.DefaultKeychain)}, opts...)} +} + +// FetchAgentManifest resolves ref, pulls the image config + the layer holding +// the manifest, and returns the manifest bytes plus the digest-pinned ref. The +// pull is bounded by a 60s timeout and uses the configured keychain for auth. +func (f *RemoteFetcher) FetchAgentManifest(ctx context.Context, ref string) ([]byte, string, error) { + ctx, cancel := context.WithTimeout(ctx, 60*time.Second) + defer cancel() + + parsed, err := name.ParseReference(ref) + if err != nil { + return nil, "", fmt.Errorf("parsing image reference %q: %w", ref, err) + } + + opts := append([]remote.Option{}, f.remoteOptions...) + opts = append(opts, remote.WithContext(ctx)) + img, err := remote.Image(parsed, opts...) + if err != nil { + return nil, "", fmt.Errorf("importing agent from image %q: %w", ref, enrichRemoteError(err)) + } + + manifestBytes, err := extractFromImage(img, LabelAgent, WellKnownManifestPath) + if err != nil { + return nil, "", fmt.Errorf("importing agent from image %q: %w", ref, err) + } + + pinned, err := pinnedDigestRef(parsed, img) + if err != nil { + return nil, "", fmt.Errorf("importing agent from image %q: resolving digest: %w", ref, err) + } + return manifestBytes, pinned, nil +} + +// pinnedDigestRef returns the digest-pinned reference (repo@sha256:...) for the +// image the parsed ref resolved to. The repository is taken from the parsed +// ref's Context() (fully-qualified, no tag/digest); the digest comes from the +// image's manifest digest. Repository.Digest returns a name.Digest whose Name() +// is the canonical pinned ref. +func pinnedDigestRef(parsed name.Reference, img v1.Image) (string, error) { + digest, err := img.Digest() + if err != nil { + return "", fmt.Errorf("computing image digest: %w", err) + } + return parsed.Context().Digest(digest.String()).Name(), nil +} + +// extractFromImage reads the broodbox agent manifest from img. The manifest +// path is the value of the LabelAgent config label when set, otherwise +// WellKnownManifestPath (defaultPath). Layers are walked topmost-first; the +// first layer containing the target path wins (later layers shadow earlier +// ones, matching how a container's filesystem is assembled). It is pure-ish — +// it only reads the in-memory/passed v1.Image, so tests can feed a hand-built +// image built with crane.Layer + mutate.Append + mutate.ConfigFile. +func extractFromImage(img v1.Image, label, defaultPath string) ([]byte, error) { + // Check the declared config size before fetching it. Reading the manifest + // itself is bounded by go-containerregistry's own 100MiB manifest limit. + manifest, err := img.Manifest() + if err != nil { + return nil, fmt.Errorf("reading image manifest: %w", err) + } + if manifest.Config.Size > maxImageConfigSize { + return nil, fmt.Errorf("image config blob is too large: %d bytes (limit %d)", manifest.Config.Size, maxImageConfigSize) + } + + target := defaultPath + if cfg, err := img.ConfigFile(); err == nil && cfg != nil { + if p, ok := cfg.Config.Labels[label]; ok && strings.TrimSpace(p) != "" { + target = p + } + } + + layers, err := img.Layers() + if err != nil { + return nil, fmt.Errorf("listing image layers: %w", err) + } + if len(layers) > maxImageLayers { + return nil, fmt.Errorf("image has too many layers: %d (limit %d)", len(layers), maxImageLayers) + } + + // Layers() returns base-first (oldest first). The topmost layer is last, + // and shadows earlier layers for a given path — so walk newest-first and + // return the first hit. + for i := len(layers) - 1; i >= 0; i-- { + data, found, err := readLayerPath(layers[i], target) + if err != nil { + return nil, fmt.Errorf("reading layer: %w", err) + } + if found { + return data, nil + } + } + + return nil, fmt.Errorf( + "image does not contain a broodbox agent manifest (label %q missing and well-known path %q not found in any layer)", + label, target) +} + +// readLayerPath scans a single uncompressed layer's tar for an entry at path +// and returns its bytes. found is false (with nil error) when the layer does +// not contain the path. +func readLayerPath(layer v1.Layer, path string) (data []byte, found bool, err error) { + // Reject foreign-layer URLs (OCI "foreign" blobs) before any I/O. A remote + // layer whose descriptor carries a `urls` field would otherwise cause + // go-containerregistry's Uncompressed()/Compressed() to fetch those + // attacker-controlled URLs on the host, bypassing the sandbox VM egress + // policy (DNS-rebinding to cloud metadata is possible). partial.Descriptor + // returns the original manifest descriptor (with URLs) for remote layers + // and computes one for in-memory layers (no URLs); if it errors or returns + // nil, proceed normally. + if desc, derr := partial.Descriptor(layer); derr == nil && desc != nil && len(desc.URLs) > 0 { + label := path + if desc.Digest.String() != "" { + label = desc.Digest.String() + } + return nil, false, fmt.Errorf("foreign-layer URLs are not permitted for the agent manifest (layer %s declares %d external URL(s))", label, len(desc.URLs)) + } + + rc, err := layer.Uncompressed() + if err != nil { + return nil, false, fmt.Errorf("opening uncompressed layer: %w", err) + } + defer func() { _ = rc.Close() }() + + target := cleanTarPath(path) + tr := tar.NewReader(rc) + for { + hdr, err := tr.Next() + if err != nil { + if errors.Is(err, io.EOF) { + return nil, false, nil + } + return nil, false, fmt.Errorf("reading tar entry: %w", err) + } + if cleanTarPath(hdr.Name) != target { + continue + } + // A directory entry (or other non-regular type) at the target path is + // not the manifest — skip it so a later layer or the not-found error + // surfaces. + if hdr.Typeflag != tar.TypeReg { + continue + } + // Reject an oversized manifest outright rather than silently + // truncating it via LimitReader below. hdr.Size is attacker-declared, + // but combined with the LimitReader this mirrors the file-config + // path's stat+limit pattern (internal/infra/configfile.ReadFile). + if hdr.Size > configfile.MaxSize { + return nil, false, fmt.Errorf("manifest %q in image layer is too large: %d bytes (limit %d)", path, hdr.Size, configfile.MaxSize) + } + buf, err := io.ReadAll(io.LimitReader(tr, configfile.MaxSize)) + if err != nil { + return nil, false, fmt.Errorf("reading %q from layer: %w", path, err) + } + return buf, true, nil + } +} + +// cleanTarPath normalizes a tar entry name or a target manifest path for +// comparison. docker build/podman build commonly emit entries with a leading +// "./" (e.g. "./usr/share/broodbox/agent.yaml"), and the target path may be +// absolute (e.g. "/usr/share/broodbox/agent.yaml"). Stripping a leading "/" and +// a leading "./" from both sides makes the two forms comparable. +func cleanTarPath(s string) string { + s = strings.TrimPrefix(s, "/") + s = strings.TrimPrefix(s, "./") + return s +} + +// enrichRemoteError appends a registry hint when the error is an HTTP 401/403/404 +// from the registry, so the operator gets an actionable message. +func enrichRemoteError(err error) error { + var terr *transport.Error + if errors.As(err, &terr) { + switch terr.StatusCode { + case 401, 403: + return fmt.Errorf("%w (registry returned %d — authenticate, e.g. `docker login`/`podman login` to the registry)", err, terr.StatusCode) + case 404: + return fmt.Errorf("%w (image or tag not found; confirm the reference and that you have access)", err) + } + } + return err +} diff --git a/internal/infra/ocimage/ocimage_test.go b/internal/infra/ocimage/ocimage_test.go new file mode 100644 index 0000000..5771a5d --- /dev/null +++ b/internal/infra/ocimage/ocimage_test.go @@ -0,0 +1,370 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package ocimage + +import ( + "bytes" + "context" + "errors" + "io" + "strings" + "testing" + "time" + + "github.com/google/go-containerregistry/pkg/crane" + "github.com/google/go-containerregistry/pkg/name" + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/empty" + "github.com/google/go-containerregistry/pkg/v1/mutate" + "github.com/google/go-containerregistry/pkg/v1/remote/transport" + "github.com/google/go-containerregistry/pkg/v1/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/brood-box/internal/infra/configfile" +) + +// validManifestYAML is a minimal, schema-valid agent manifest used as the +// embedded file content in the in-memory test images. extractFromImage only +// reads bytes; it does not parse the manifest, so the content is exercised by +// the e2e tests in cmd/bbox, but it must be non-empty and shaped like a real +// manifest. +const validManifestYAML = `name: aider +image: ghcr.io/acme/aider-bbox:latest +command: ["aider"] +egress_profile: permissive +` + +// buildImage builds an in-memory v1.Image with the given file contents across +// the given layers (each map is appended as one layer, in order — first map is +// the base, last is topmost) and the given config labels. No network is used. +func buildImage(t *testing.T, layers []map[string][]byte, labels map[string]string) v1.Image { + t.Helper() + img := empty.Image + for _, files := range layers { + layer, err := crane.Layer(files) + require.NoError(t, err) + img, err = mutate.AppendLayers(img, layer) + require.NoError(t, err) + } + if labels != nil { + cf, err := img.ConfigFile() + require.NoError(t, err) + cfg := cf.DeepCopy() + if cfg.Config.Labels == nil { + cfg.Config.Labels = map[string]string{} + } + for k, v := range labels { + cfg.Config.Labels[k] = v + } + img, err = mutate.ConfigFile(img, cfg) + require.NoError(t, err) + } + return img +} + +func TestExtractFromImage(t *testing.T) { + t.Parallel() + + customPath := "/etc/aider/agent.yaml" + topmost := "name: topmost\nimage: ghcr.io/acme/topmost:latest\ncommand: [\"t\"]\negress_profile: permissive\n" + bad := "name: : not yaml {" + + tests := []struct { + name string + layers []map[string][]byte + labels map[string]string + wantData string + wantErr bool + errContains []string + }{ + { + name: "LabelPointsToPath", + layers: []map[string][]byte{ + {customPath: []byte(validManifestYAML)}, + }, + labels: map[string]string{LabelAgent: customPath}, + wantData: validManifestYAML, + }, + { + name: "LabelAbsentFallsBackToWellKnown", + layers: []map[string][]byte{ + {WellKnownManifestPath: []byte(validManifestYAML)}, + }, + wantData: validManifestYAML, + }, + { + name: "NeitherLabelNorWellKnown", + layers: []map[string][]byte{ + {"/usr/bin/aider": []byte("binary")}, + }, + wantErr: true, + errContains: []string{"does not contain a broodbox agent manifest", LabelAgent, WellKnownManifestPath}, + }, + { + name: "ManifestInNonTopmostLayer", + layers: []map[string][]byte{ + {WellKnownManifestPath: []byte(validManifestYAML)}, + {"/usr/bin/aider": []byte("binary")}, + }, + wantData: validManifestYAML, + }, + { + name: "TopmostLayerShadowsBase", + layers: []map[string][]byte{ + {WellKnownManifestPath: []byte(validManifestYAML)}, + {WellKnownManifestPath: []byte(topmost)}, + }, + wantData: topmost, + }, + { + name: "MalformedManifestBytesSurface", + layers: []map[string][]byte{ + {WellKnownManifestPath: []byte(bad)}, + }, + wantData: bad, + }, + { + name: "EmptyLabelValueIgnored", + layers: []map[string][]byte{ + {WellKnownManifestPath: []byte(validManifestYAML)}, + }, + labels: map[string]string{LabelAgent: " "}, + wantData: validManifestYAML, + }, + { + name: "LabelPointsToMissingFile", + layers: []map[string][]byte{ + {WellKnownManifestPath: []byte(validManifestYAML)}, + }, + labels: map[string]string{LabelAgent: customPath}, + wantErr: true, + errContains: []string{customPath}, + }, + { + // docker build/podman build emit tar entries with a leading "./". + name: "LeadingDotSlashTarEntryMatchesAbsolutePath", + layers: []map[string][]byte{ + {"./usr/share/broodbox/agent.yaml": []byte(validManifestYAML)}, + }, + wantData: validManifestYAML, + }, + { + // Relative tar entry (no leading "/" or "./") must still match an + // absolute target path. + name: "RelativeTarEntryMatchesAbsolutePath", + layers: []map[string][]byte{ + {"usr/share/broodbox/agent.yaml": []byte(validManifestYAML)}, + }, + wantData: validManifestYAML, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + img := buildImage(t, tc.layers, tc.labels) + + data, err := extractFromImage(img, LabelAgent, WellKnownManifestPath) + if tc.wantErr { + require.Error(t, err) + for _, s := range tc.errContains { + assert.Contains(t, err.Error(), s) + } + return + } + require.NoError(t, err) + assert.Equal(t, tc.wantData, string(data)) + }) + } +} + +func TestExtractFromImageRejectsOversizedConfig(t *testing.T) { + t.Parallel() + + // Inflate the real config JSON (via a large label value) past the cap — + // this is a genuinely oversized config blob, not a forged descriptor. + bloat := strings.Repeat("a", maxImageConfigSize+1024) + img := buildImage(t, []map[string][]byte{ + {WellKnownManifestPath: []byte(validManifestYAML)}, + }, map[string]string{"bloat": bloat}) + + _, err := extractFromImage(img, LabelAgent, WellKnownManifestPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "config blob is too large") +} + +func TestExtractFromImageRejectsTooManyLayers(t *testing.T) { + t.Parallel() + + layers := make([]map[string][]byte, maxImageLayers+1) + for i := range layers { + layers[i] = map[string][]byte{} + } + img := buildImage(t, layers, nil) + + _, err := extractFromImage(img, LabelAgent, WellKnownManifestPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "too many layers") +} + +func TestExtractFromImageRejectsOversizedManifest(t *testing.T) { + t.Parallel() + + // A real (not forged) manifest file bigger than configfile.MaxSize must + // error, not be silently truncated by the LimitReader. + oversized := bytes.Repeat([]byte("a"), int(configfile.MaxSize)+1024) + img := buildImage(t, []map[string][]byte{ + {WellKnownManifestPath: oversized}, + }, nil) + + _, err := extractFromImage(img, LabelAgent, WellKnownManifestPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "too large") +} + +func TestPinnedDigestRef_Normalizes(t *testing.T) { + t.Parallel() + + // Build an image and resolve its digest-pinned ref from a tag ref. We + // cannot use a real registry, but we can use a fully-qualified repo name + // (ghcr.io/acme/aider-bbox) and assert the pinned ref parses as a Digest + // and round-trips the repository. + img := buildImage(t, []map[string][]byte{ + {WellKnownManifestPath: []byte(validManifestYAML)}, + }, nil) + + parsed, err := name.ParseReference("ghcr.io/acme/aider-bbox:latest") + require.NoError(t, err) + pinned, err := pinnedDigestRef(parsed, img) + require.NoError(t, err) + + assert.True(t, strings.HasPrefix(pinned, "ghcr.io/acme/aider-bbox@sha256:")) + assert.NotContains(t, pinned, ":latest") + + // name.NewDigest accepts the result (validates algorithm + hex). + dig, err := name.NewDigest(pinned) + require.NoError(t, err) + assert.Equal(t, "ghcr.io/acme/aider-bbox", dig.Repository.Name()) +} + +func TestEnrichRemoteError(t *testing.T) { + t.Parallel() + + base := errors.New("some failure") + tests := []struct { + name string + err error + wantSubstr string + }{ + { + name: "401 surfaces authenticate hint", + err: &transport.Error{StatusCode: 401}, + wantSubstr: "authenticate", + }, + { + name: "403 surfaces authenticate hint", + err: &transport.Error{StatusCode: 403}, + wantSubstr: "authenticate", + }, + { + name: "404 surfaces not-found hint", + err: &transport.Error{StatusCode: 404}, + wantSubstr: "not found", + }, + { + name: "non-transport error is unchanged", + err: base, + wantSubstr: "some failure", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := enrichRemoteError(tc.err) + require.Error(t, got) + assert.Contains(t, got.Error(), tc.wantSubstr) + }) + } +} + +// foreignLayer is a v1.Layer whose Descriptor carries an attacker-controlled +// `urls` list (OCI "foreign" blob). Uncompressed panics, proving the foreign-URL +// rejection happens before any I/O. +type foreignLayer struct { + desc v1.Descriptor +} + +func (f *foreignLayer) Digest() (v1.Hash, error) { return f.desc.Digest, nil } +func (f *foreignLayer) DiffID() (v1.Hash, error) { return f.desc.Digest, nil } +func (f *foreignLayer) Compressed() (io.ReadCloser, error) { + panic("Compressed must not be called for foreign layers") +} +func (f *foreignLayer) Uncompressed() (io.ReadCloser, error) { + panic("Uncompressed must not be called for foreign layers") +} +func (f *foreignLayer) Size() (int64, error) { return f.desc.Size, nil } +func (f *foreignLayer) MediaType() (types.MediaType, error) { return f.desc.MediaType, nil } +func (f *foreignLayer) Descriptor() (*v1.Descriptor, error) { return &f.desc, nil } + +func TestExtractFromImageRejectsForeignLayerURLs(t *testing.T) { + t.Parallel() + + // An attacker-controlled foreign layer whose Descriptor carries URLs. The + // rejection must fire before Uncompressed is ever called. + digest, err := v1.NewHash("sha256:" + strings.Repeat("00", 32)) + require.NoError(t, err) + layer := &foreignLayer{ + desc: v1.Descriptor{ + Digest: digest, + URLs: []string{"https://attacker.example/x"}, + MediaType: types.OCILayer, + }, + } + img, err := mutate.Append(empty.Image, mutate.Addendum{Layer: layer}) + require.NoError(t, err) + + _, err = extractFromImage(img, LabelAgent, WellKnownManifestPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "foreign-layer URLs are not permitted") + assert.Contains(t, err.Error(), "1 external URL(s)") +} + +func TestNewRemoteFetcherIncludesDefaultKeychain(t *testing.T) { + t.Parallel() + + // Weak but useful: NewRemoteFetcher prepends the default-keychain option so + // `docker login`/`podman login` credentials are picked up. Asserting at + // least one option is present catches an accidental removal of the + // keychain wiring. remote.Option is an opaque interface, so introspecting + // which option carries the keychain is not feasible without a live + // registry. + f := NewRemoteFetcher() + require.NotEmpty(t, f.remoteOptions, "NewRemoteFetcher must register at least the default keychain option") +} + +func TestFetchAgentManifestRespectsCancelledContext(t *testing.T) { + // Not parallel: relies on context timing. Use a ref that parses but points + // nowhere (localhost:1 will not accept, but go-containerregistry checks the + // context before dialing). A pre-cancelled context must yield an error + // quickly without a real network round-trip. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + f := NewRemoteFetcher() + done := make(chan error, 1) + go func() { + _, _, err := f.FetchAgentManifest(ctx, "localhost:1/test:latest") + done <- err + }() + + select { + case err := <-done: + require.Error(t, err) + // Generous deadline: context cancellation should short-circuit well before. + case <-time.After(5 * time.Second): + t.Fatal("FetchAgentManifest did not return within 5s of a cancelled context") + } +}