Repository: kopia/kopia
Pull request #737 (2020-12-22 01:38:01)

actions: Added --enable-actions flag
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`

julio-lopez: @miquella FYI

Patch:
diff --git a/cli/command_repository_connect.go b/cli/command_repository_connect.go
index d2ab28f1..64460c55 100644
--- a/cli/command_repository_connect.go
+++ b/cli/command_repository_connect.go
@@ -24,6 +24,7 @@
 	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 81531254..7e804318 100644
--- a/cli/command_snapshot_create.go
+++ b/cli/command_snapshot_create.go
@@ -32,6 +32,8 @@
 	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 9acb793b..5ab44a5c 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 be31284b..b167d705 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 5271c392..9f4fc006 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)
 	}
 


Test patch:
diff --git a/tests/end_to_end_test/snapshot_actions_test.go b/tests/end_to_end_test/snapshot_actions_test.go
index c0479713..37f3278b 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)