diff --git a/.github/wiki/Plugin-Security.md b/.github/wiki/Plugin-Security.md index 7ebb5e6ea..080614b21 100644 --- a/.github/wiki/Plugin-Security.md +++ b/.github/wiki/Plugin-Security.md @@ -2,6 +2,15 @@ How the CLI decides a plugin download can be trusted, and how it decides a licence response and a cached licence can be trusted. Release signatures come first; licence verification starts at [What ping_api signs](#what-ping_api-signs). +## Machine invocation + +Machine-invoked mounted plugin commands that restore secret arguments or flags +are refused with E421 before the plugin binary starts. The CLI consumes +`NSELF_INVOKE_SECRETS`, but the plugin protocol cannot pass those secrets onward +without exposing them on the plugin process arguments. Calls without a secret +hand-off continue to run. Free-form commands with no declared secret arguments +or flags do not participate in the hand-off. + ## Plugin release signatures Every plugin release is signed by the pipeline that builds it (free plugins: the `release-signing` environment of `nself-org/plugins`; licensed plugins: the owner release machine). The registry and catalog entry carries `release_signature: {key_id, alg: "ed25519", sig}`. The CLI checks it before it extracts a single file. diff --git a/cmd/commands/plugin_mount.go b/cmd/commands/plugin_mount.go index c1112364b..e45bcbcbb 100644 --- a/cmd/commands/plugin_mount.go +++ b/cmd/commands/plugin_mount.go @@ -33,6 +33,7 @@ import ( "strings" "github.com/nself-org/cli/internal/compat" + "github.com/nself-org/cli/internal/errs" "github.com/nself-org/cli/internal/plugin" "github.com/nself-org/cli/internal/plugin/mount" "github.com/spf13/cobra" @@ -226,6 +227,9 @@ func installedNode(s mount.Spec) *cobra.Command { func proxyRunE(node *cobra.Command, s mount.Spec, segs []string) { own := append([]string{}, segs...) node.RunE = func(cmd *cobra.Command, args []string) error { + if restoredInvokeSecrets { + return errs.New("E421", "mounted plugins cannot receive machine-invoked secret arguments").WithWhy("the plugin protocol cannot keep restored secrets off the plugin process arguments") + } argv := append(append([]string{}, own...), stripRootPersistentFlags(args)...) return plugin.ProxyBinaryAt(s.BinaryPath, resolvePluginDir(), s.Slug, argv) } diff --git a/cmd/commands/plugin_mount_secrets_test.go b/cmd/commands/plugin_mount_secrets_test.go new file mode 100644 index 000000000..de6114dca --- /dev/null +++ b/cmd/commands/plugin_mount_secrets_test.go @@ -0,0 +1,78 @@ +package commands + +// Purpose: keep restored machine-invocation secrets off mounted plugin argv. +// Inputs: synthetic secret bundles and a mounted binary that exits with 19. +// Outputs: E421 before execution for secret hand-offs, plugin exit otherwise. +// Constraints: shell fixture requires Unix; no real secrets or external services. + +import ( + "errors" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/nself-org/cli/internal/invoke" + "github.com/nself-org/cli/internal/plugin" + "github.com/nself-org/cli/internal/plugin/mount" + "github.com/spf13/cobra" +) + +func mountedSecretProxy(t *testing.T) *cobra.Command { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("shell fixture") + } + savedArgs, savedSecrets := os.Args, restoredInvokeSecrets + t.Cleanup(func() { os.Args, restoredInvokeSecrets = savedArgs, savedSecrets }) + home := t.TempDir() + t.Setenv("HOME", home) + dir := filepath.Join(home, ".nself", "plugins") + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatal(err) + } + bin := filepath.Join(dir, "nself-demo") + if err := os.WriteFile(bin, []byte("#!/bin/sh\nexit 19\n"), 0755); err != nil { + t.Fatal(err) + } + node := &cobra.Command{Use: "demo"} + proxyRunE(node, mount.Spec{Slug: "demo", BinaryPath: bin}, nil) + return node +} + +func TestMountProxyRestoredSecrets(t *testing.T) { + node := mountedSecretProxy(t) + const placeholder = "nself-invoke-secret:abcd1234abcd1234:0" + for _, arg := range []string{placeholder, "--token=" + placeholder} { + t.Run(arg, func(t *testing.T) { + os.Args = []string{"nself", "demo", arg} + t.Setenv(invoke.InvokedByEnv, "mcp") + t.Setenv(invoke.SecretsEnv, `{"nonce":"abcd1234abcd1234","values":["FAKE-mounted-secret"]}`) + if err := restoreSecretArgv(); err != nil { + t.Fatal(err) + } + if !strings.Contains(os.Args[2], "FAKE-mounted-secret") { + t.Fatal("secret was not restored") + } + err := node.RunE(node, os.Args[2:]) + if err == nil || !strings.Contains(err.Error(), "E421") || strings.Contains(err.Error(), "FAKE-mounted-secret") { + t.Fatalf("expected safe E421 before executing plugin, got %v", err) + } + }) + } + // A following invocation must clear the previous hand-off state. Both + // non-secret machine calls and direct calls must still execute the plugin. + for _, invokedBy := range []string{"mcp", ""} { + t.Setenv(invoke.InvokedByEnv, invokedBy) + t.Setenv(invoke.SecretsEnv, "") + os.Args = []string{"nself", "demo", "ordinary"} + if err := restoreSecretArgv(); err != nil { + t.Fatal(err) + } + var exit *plugin.ExitCodeError + if err := node.RunE(node, os.Args[2:]); !errors.As(err, &exit) || exit.Code != 19 { + t.Fatalf("non-secret call did not execute plugin: %v", err) + } + } +} diff --git a/cmd/commands/secrets_core_logmask.go b/cmd/commands/secrets_core_logmask.go index 776dcb1a7..be6b7054f 100644 --- a/cmd/commands/secrets_core_logmask.go +++ b/cmd/commands/secrets_core_logmask.go @@ -25,15 +25,21 @@ import ( "github.com/spf13/pflag" ) +// restoredInvokeSecrets records the hand-off after its environment is consumed. +// Mounted binaries cannot safely receive these values on their own argv. +var restoredInvokeSecrets bool + // restoreSecretArgv is the first step of Execute: a machine invoker hands the // secret values over the environment (D-0468); put them back into os.Args and // tell the command log to mask them (D-0473). func restoreSecretArgv() error { + restoredInvokeSecrets = false restored, values, err := invoke.RestoreSecretArgv(os.Args) if err != nil { return err } os.Args = restored + restoredInvokeSecrets = len(values) > 0 cmdlog.MaskValues(values...) return nil } diff --git a/cmd/commands/secrets_core_logsite_test.go b/cmd/commands/secrets_core_logsite_test.go index 9f856362c..4f1cee00d 100644 --- a/cmd/commands/secrets_core_logsite_test.go +++ b/cmd/commands/secrets_core_logsite_test.go @@ -58,7 +58,8 @@ func TestRootLogsNoSecretPositional(t *testing.T) { // restored value is on os.Args; a log line built from it must still hide it. func TestRestoreSecretArgvMasksRestoredValues(t *testing.T) { saved := os.Args - t.Cleanup(func() { os.Args = saved }) + savedSecrets := restoredInvokeSecrets + t.Cleanup(func() { os.Args, restoredInvokeSecrets = saved, savedSecrets }) ph := "nself-invoke-secret:abcd1234abcd1234:0" os.Args = []string{"nself", "secrets", "set", "FAKEKEY", ph} t.Setenv(invoke.InvokedByEnv, "mcp") diff --git a/internal/invoke/secretenv.go b/internal/invoke/secretenv.go index c97d99610..074818a35 100644 --- a/internal/invoke/secretenv.go +++ b/internal/invoke/secretenv.go @@ -14,12 +14,11 @@ package invoke // grandchild inherits it, and ignores it unless InvokedByEnv is set. Any // mismatch (bad JSON, bad index, a value unused or used twice) is E421. // MaxSecretEnvBytes caps the bundle per OS (secretenv_limit_*.go). -// Known boundary: a mounted plugin binary that the nself child proxies to -// (internal/plugin/router.go) receives the restored secret values on its own -// argv, where ps shows them. The plugin protocol has no hand-off for the bundle -// yet; the gap is tracked in hq debt. A free-form plugin command (no declared -// args or flags) is also not hidden: nothing marks its words secret, so -// Request.Argv goes on the child argv as given (argv.go). +// Mounted plugin commands refuse a hand-off containing restored secrets with +// E421 before executing the binary: the plugin protocol cannot carry the bundle +// without putting those values on the plugin argv. A free-form plugin command +// (no declared args or flags) is also not hidden: nothing marks its words +// secret, so Request.Argv goes on the child argv as given (argv.go). import ( "crypto/rand"