From eecd9d13c956e88ba0095bddd06169b68f4829fe Mon Sep 17 00:00:00 2001 From: Jarek Kowalski Date: Mon, 21 Dec 2020 18:05:25 -0800 Subject: [PATCH] actions: Added --enable-actions flag (#737) This can be specified at `repo create` or `repo connect` to enable actions. By default actions are disabled to avoid security risks associated with executing code. Alternatively during `snapshot create` one can specify `--force-enable-actions` or `--force-disable-actions` --- cli/command_repository_connect.go | 11 ++-- cli/command_snapshot_create.go | 10 +++ repo/local_config.go | 2 + snapshot/snapshotfs/upload.go | 12 ++-- snapshot/snapshotfs/upload_actions.go | 15 +++-- .../end_to_end_test/snapshot_actions_test.go | 61 ++++++++++++++++++- 6 files changed, 95 insertions(+), 16 deletions(-) diff --git a/cli/command_repository_connect.go b/cli/command_repository_connect.go index d2ab28f13..64460c55b 100644 --- a/cli/command_repository_connect.go +++ b/cli/command_repository_connect.go @@ -24,6 +24,7 @@ var ( connectCheckForUpdates bool connectReadonly bool connectDescription string + connectEnableActions bool ) func setupConnectOptions(cmd *kingpin.CmdClause) { @@ -39,6 +40,7 @@ func setupConnectOptions(cmd *kingpin.CmdClause) { cmd.Flag("check-for-updates", "Periodically check for Kopia updates on GitHub").Default("true").Envar(checkForUpdatesEnvar).BoolVar(&connectCheckForUpdates) cmd.Flag("readonly", "Make repository read-only to avoid accidental changes").BoolVar(&connectReadonly) cmd.Flag("description", "Human-readable description of the repository").StringVar(&connectDescription) + cmd.Flag("enable-actions", "Allow snapshot actions").BoolVar(&connectEnableActions) } func connectOptions() *repo.ConnectOptions { @@ -51,10 +53,11 @@ func connectOptions() *repo.ConnectOptions { MaxListCacheDurationSec: int(connectMaxListCacheDuration.Seconds()), }, ClientOptions: repo.ClientOptions{ - Hostname: connectHostname, - Username: connectUsername, - ReadOnly: connectReadonly, - Description: connectDescription, + Hostname: connectHostname, + Username: connectUsername, + ReadOnly: connectReadonly, + Description: connectDescription, + EnableActions: connectEnableActions, }, } } diff --git a/cli/command_snapshot_create.go b/cli/command_snapshot_create.go index 81531254e..7e8043188 100644 --- a/cli/command_snapshot_create.go +++ b/cli/command_snapshot_create.go @@ -32,6 +32,8 @@ var ( snapshotCreateParallelUploads = snapshotCreateCommand.Flag("parallel", "Upload N files in parallel").PlaceHolder("N").Default("0").Int() snapshotCreateStartTime = snapshotCreateCommand.Flag("start-time", "Override snapshot start timestamp.").String() snapshotCreateEndTime = snapshotCreateCommand.Flag("end-time", "Override snapshot end timestamp.").String() + snapshotCreateForceEnableActions = snapshotCreateCommand.Flag("force-enable-actions", "Enable snapshot actions even if globally disabled on this client").Hidden().Bool() + snapshotCreateForceDisableActions = snapshotCreateCommand.Flag("force-disable-actions", "Disable snapshot actions even if globally enabled on this client").Hidden().Bool() ) func runSnapshotCommand(ctx context.Context, rep repo.Repository) error { @@ -115,6 +117,14 @@ func setupUploader(rep repo.Repository) *snapshotfs.Uploader { u := snapshotfs.NewUploader(rep) u.MaxUploadBytes = *snapshotCreateCheckpointUploadLimitMB << 20 //nolint:gomnd + if *snapshotCreateForceEnableActions { + u.EnableActions = true + } + + if *snapshotCreateForceDisableActions { + u.EnableActions = false + } + if interval := *snapshotCreateCheckpointInterval; interval != 0 { u.CheckpointInterval = interval } diff --git a/repo/local_config.go b/repo/local_config.go index 9acb793bb..5ab44a5cb 100644 --- a/repo/local_config.go +++ b/repo/local_config.go @@ -20,6 +20,8 @@ type ClientOptions struct { // Description is human-readable description of the repository to use in the UI. Description string `json:"description,omitempty"` + + EnableActions bool `json:"enableActions"` } // ApplyDefaults returns a copy of ClientOptions with defaults filled out. diff --git a/snapshot/snapshotfs/upload.go b/snapshot/snapshotfs/upload.go index be31284b8..b167d7058 100644 --- a/snapshot/snapshotfs/upload.go +++ b/snapshot/snapshotfs/upload.go @@ -64,6 +64,9 @@ type Uploader struct { // Number of files to hash and upload in parallel. ParallelUploads int + // Enable snapshot actions + EnableActions bool + // How frequently to create checkpoint snapshot entries. CheckpointInterval time.Duration @@ -399,7 +402,7 @@ func (u *Uploader) uploadDirWithCheckpointing(ctx context.Context, rootDir fs.Di localDirPathOrEmpty := rootDir.LocalFilesystemPath() - overrideDir, err := executeBeforeFolderAction(ctx, "before-snapshot-root", policyTree.EffectivePolicy().Actions.BeforeSnapshotRoot, localDirPathOrEmpty, &hc) + overrideDir, err := u.executeBeforeFolderAction(ctx, "before-snapshot-root", policyTree.EffectivePolicy().Actions.BeforeSnapshotRoot, localDirPathOrEmpty, &hc) if err != nil { return nil, dirReadError{errors.Wrap(err, "error executing before-snapshot-root action")} } @@ -408,7 +411,7 @@ func (u *Uploader) uploadDirWithCheckpointing(ctx context.Context, rootDir fs.Di rootDir = overrideDir } - defer executeAfterFolderAction(ctx, "after-snapshot-root", policyTree.EffectivePolicy().Actions.AfterSnapshotRoot, localDirPathOrEmpty, &hc) + defer u.executeAfterFolderAction(ctx, "after-snapshot-root", policyTree.EffectivePolicy().Actions.AfterSnapshotRoot, localDirPathOrEmpty, &hc) return uploadDirInternal(ctx, u, rootDir, policyTree, previousDirs, localDirPathOrEmpty, ".", &dmb, &cp) } @@ -849,12 +852,12 @@ func uploadDirInternal( var hc actionContext defer cleanupActionContext(ctx, &hc) - overrideDir, herr := executeBeforeFolderAction(ctx, "before-folder", definedActions.BeforeFolder, localDirPathOrEmpty, &hc) + overrideDir, herr := u.executeBeforeFolderAction(ctx, "before-folder", definedActions.BeforeFolder, localDirPathOrEmpty, &hc) if herr != nil { return nil, dirReadError{errors.Wrap(herr, "error executing before-folder action")} } - defer executeAfterFolderAction(ctx, "after-folder", definedActions.AfterFolder, localDirPathOrEmpty, &hc) + defer u.executeAfterFolderAction(ctx, "after-folder", definedActions.AfterFolder, localDirPathOrEmpty, &hc) if overrideDir != nil { directory = overrideDir @@ -963,6 +966,7 @@ func NewUploader(r repo.Repository) *Uploader { Progress: &NullUploadProgress{}, IgnoreReadErrors: false, ParallelUploads: 1, + EnableActions: r.ClientOptions().EnableActions, CheckpointInterval: DefaultCheckpointInterval, getTicker: time.Tick, uploadBufPool: sync.Pool{ diff --git a/snapshot/snapshotfs/upload_actions.go b/snapshot/snapshotfs/upload_actions.go index 5271c3926..9f4fc006b 100644 --- a/snapshot/snapshotfs/upload_actions.go +++ b/snapshot/snapshotfs/upload_actions.go @@ -43,7 +43,7 @@ func (hc *actionContext) envars() []string { } } -func (hc *actionContext) ensureInitialized(dirPathOrEmpty string) error { +func (hc *actionContext) ensureInitialized(ctx context.Context, actionType, dirPathOrEmpty string, uploaderEnabled bool) error { if dirPathOrEmpty == "" { return nil } @@ -53,6 +53,11 @@ func (hc *actionContext) ensureInitialized(dirPathOrEmpty string) error { return nil } + if !uploaderEnabled { + log(ctx).Noticef("Not executing %v action on %v because it's been disabled for this client.", actionType, dirPathOrEmpty) + return nil + } + var randBytes [8]byte if _, err := rand.Read(randBytes[:]); err != nil { @@ -184,12 +189,12 @@ func parseCaptures(v []byte, captures map[string]string) error { return s.Err() } -func executeBeforeFolderAction(ctx context.Context, actionType string, h *policy.ActionCommand, dirPathOrEmpty string, hc *actionContext) (fs.Directory, error) { +func (u *Uploader) executeBeforeFolderAction(ctx context.Context, actionType string, h *policy.ActionCommand, dirPathOrEmpty string, hc *actionContext) (fs.Directory, error) { if h == nil { return nil, nil } - if err := hc.ensureInitialized(dirPathOrEmpty); err != nil { + if err := hc.ensureInitialized(ctx, actionType, dirPathOrEmpty, u.EnableActions); err != nil { return nil, errors.Wrap(err, "error initializing action context") } @@ -215,12 +220,12 @@ func executeBeforeFolderAction(ctx context.Context, actionType string, h *policy return nil, nil } -func executeAfterFolderAction(ctx context.Context, actionType string, h *policy.ActionCommand, dirPathOrEmpty string, hc *actionContext) { +func (u *Uploader) executeAfterFolderAction(ctx context.Context, actionType string, h *policy.ActionCommand, dirPathOrEmpty string, hc *actionContext) { if h == nil { return } - if err := hc.ensureInitialized(dirPathOrEmpty); err != nil { + if err := hc.ensureInitialized(ctx, actionType, dirPathOrEmpty, u.EnableActions); err != nil { log(ctx).Warningf("error initializing action context: %v", err) } diff --git a/tests/end_to_end_test/snapshot_actions_test.go b/tests/end_to_end_test/snapshot_actions_test.go index c04797130..37f3278b1 100644 --- a/tests/end_to_end_test/snapshot_actions_test.go +++ b/tests/end_to_end_test/snapshot_actions_test.go @@ -25,7 +25,7 @@ func TestSnapshotActionsBeforeSnapshotRoot(t *testing.T) { defer e.RunAndExpectSuccess(t, "repo", "disconnect") - e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir, "--override-hostname=foo", "--override-username=foo") + e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir, "--override-hostname=foo", "--override-username=foo", "--enable-actions") e.RunAndExpectSuccess(t, "snapshot", "create", sharedTestDataDir2) envFile1 := filepath.Join(e.LogsDir, "env1.txt") @@ -157,7 +157,7 @@ func TestSnapshotActionsBeforeAfterFolder(t *testing.T) { e := testenv.NewCLITest(t) - e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir) + e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir, "--enable-actions") defer e.RunAndExpectSuccess(t, "repo", "disconnect") // create directory structure @@ -225,7 +225,7 @@ func TestSnapshotActionsEmbeddedScript(t *testing.T) { e := testenv.NewCLITest(t) - e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir) + e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir, "--enable-actions") defer e.RunAndExpectSuccess(t, "repo", "disconnect") var ( @@ -265,6 +265,61 @@ func TestSnapshotActionsEmbeddedScript(t *testing.T) { e.RunAndExpectFailure(t, "snapshot", "create", sharedTestDataDir1) } +func TestSnapshotActionsEnable(t *testing.T) { + t.Parallel() + + th := os.Getenv("TESTING_ACTION_EXE") + if th == "" { + t.Skip("TESTING_ACTION_EXE verifyNoError be set") + } + + cases := []struct { + desc string + connectFlags []string + snapshotFlags []string + wantRun bool + }{ + {desc: "defaults", connectFlags: nil, snapshotFlags: nil, wantRun: false}, + {desc: "override-connect-disable", connectFlags: []string{"--enable-actions"}, snapshotFlags: nil, wantRun: true}, + {desc: "override-connect-disable", connectFlags: []string{"--no-enable-actions"}, snapshotFlags: nil, wantRun: false}, + {desc: "override-snapshot-enable", connectFlags: nil, snapshotFlags: []string{"--force-enable-actions"}, wantRun: true}, + {desc: "override-snapshot-disable", connectFlags: nil, snapshotFlags: []string{"--force-disable-actions"}, wantRun: false}, + {desc: "snapshot-takes-precedence-enable", connectFlags: []string{"--no-enable-actions"}, snapshotFlags: []string{"--force-enable-actions"}, wantRun: true}, + {desc: "snapshot-takes-precedence-disable", connectFlags: []string{"--enable-actions"}, snapshotFlags: []string{"--force-disable-actions"}, wantRun: false}, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + e := testenv.NewCLITest(t) + + defer e.RunAndExpectSuccess(t, "repo", "disconnect") + + e.RunAndExpectSuccess(t, append([]string{"repo", "create", "filesystem", "--path", e.RepoDir}, tc.connectFlags...)...) + + envFile := filepath.Join(e.LogsDir, "env1.txt") + + // set an action before-snapshot-root that fails and which saves the environment to a file. + e.RunAndExpectSuccess(t, + "policy", "set", + sharedTestDataDir1, + "--before-snapshot-root-action", + th+" --save-env="+envFile) + + e.RunAndExpectSuccess(t, append([]string{"snapshot", "create", sharedTestDataDir1}, tc.snapshotFlags...)...) + + _, err := os.Stat(envFile) + didRun := err == nil + if didRun != tc.wantRun { + t.Errorf("unexpected behavior. did run: %v want run: %v", didRun, tc.wantRun) + } + }) + } +} + func tmpfileWithContents(t *testing.T, contents string) string { f, err := ioutil.TempFile("", "kopia-test") verifyNoError(t, err)