From 6dd82c6de1bdec6a76e35dff2d352d28e7f97af8 Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Mon, 26 Feb 2018 14:28:49 -0800 Subject: [PATCH] Validate directory flag. Unlock on preexecute err. Ensure -d flag uses relative dirs and doesn't allow for directory traversal. Fix bug where if there was an error in PreExecute, we wouldn't unlock, leaving a possibly abandoned lock. --- server/events/apply_executor.go | 43 ++++++++++++------ server/events/event_parser.go | 48 ++++++++++++-------- server/events/event_parser_test.go | 68 +++++++++++++++++++++------- server/events/plan_executor.go | 28 ++++++++---- server/events/plan_executor_test.go | 34 ++++++++++++++ server/events/project_pre_execute.go | 28 +++++++++--- 6 files changed, 185 insertions(+), 64 deletions(-) diff --git a/server/events/apply_executor.go b/server/events/apply_executor.go index 47cfa658b..f38033d82 100644 --- a/server/events/apply_executor.go +++ b/server/events/apply_executor.go @@ -46,22 +46,39 @@ func (a *ApplyExecutor) Execute(ctx *CommandContext) CommandResponse { // Plans are stored at project roots by their workspace names. We just // need to find them. var plans []models.Plan - err = filepath.Walk(repoDir, func(path string, info os.FileInfo, err error) error { + // If they didn't specify a directory, we apply all plans we can find for + // this workspace. + if ctx.Command.Dir == "" { + err = filepath.Walk(repoDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + // Check if the plan is for the right workspace, + if !info.IsDir() && info.Name() == ctx.Command.Workspace+".tfplan" { + rel, _ := filepath.Rel(repoDir, filepath.Dir(path)) + plans = append(plans, models.Plan{ + Project: models.NewProject(ctx.BaseRepo.FullName, rel), + LocalPath: path, + }) + } + return nil + }) if err != nil { - return err + return CommandResponse{Error: errors.Wrap(err, "finding plans")} } - // Check if the plan is for the right workspace, - if !info.IsDir() && info.Name() == ctx.Command.Workspace+".tfplan" { - rel, _ := filepath.Rel(repoDir, filepath.Dir(path)) - plans = append(plans, models.Plan{ - Project: models.NewProject(ctx.BaseRepo.FullName, rel), - LocalPath: path, - }) + } else { + // If they did specify a dir, we apply just the plan in that directory + // for this workspace. + path := filepath.Join(repoDir, ctx.Command.Dir, ctx.Command.Workspace+".tfplan") + stat, err := os.Stat(path) + if err != nil || stat.IsDir() { + return CommandResponse{Error: errors.Wrapf(err, "finding plan for dir %q and workspace %q", ctx.Command.Dir, ctx.Command.Workspace)} } - return nil - }) - if err != nil { - return CommandResponse{Error: errors.Wrap(err, "finding plans")} + rel, _ := filepath.Rel(repoDir, filepath.Dir(path)) + plans = append(plans, models.Plan{ + Project: models.NewProject(ctx.BaseRepo.FullName, filepath.Dir(rel)), + LocalPath: path, + }) } if len(plans) == 0 { return CommandResponse{Failure: "No plans found for that workspace."} diff --git a/server/events/event_parser.go b/server/events/event_parser.go index 612bdd725..bf7d76a89 100644 --- a/server/events/event_parser.go +++ b/server/events/event_parser.go @@ -3,6 +3,7 @@ package events import ( "errors" "fmt" + "path/filepath" "strings" "github.com/google/go-github/github" @@ -21,7 +22,10 @@ type Command struct { Workspace string Verbose bool Flags []string - Dir string + // Dir is the path relative to the repo root to run the command in. + // If empty string then it wasn't specified. "." is the root of the repo. + // Dir will never end in "/". + Dir string } type EventParsing interface { @@ -86,19 +90,19 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma var name CommandName // Set up the flag parsing depending on the command. + const defaultWorkspace = "default" if command == "plan" { name = Plan flagSet = pflag.NewFlagSet("plan", pflag.ContinueOnError) - flagSet.StringVarP(&workspace, "workspace", "w", "default", "Switch to this Terraform workspace before planning.") - flagSet.StringVarP(&dir, "dir", "d", ".", "Which directory to run plan in. Defaults to the root of the repo.") + flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, fmt.Sprintf("Switch to this Terraform workspace before planning. Defaults to '%s'", defaultWorkspace)) + flagSet.StringVarP(&dir, "dir", "d", "", "Which directory to run plan in relative to root of repo. Use '.' for root. If not specified, will attempt to run plan for all Terraform projects we think were modified in this changeset.") flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") } else if command == "apply" { name = Apply flagSet = pflag.NewFlagSet("apply", pflag.ContinueOnError) - flagSet.StringVarP(&workspace, "workspace", "w", "default", "Apply the plan for this Terraform workspace.") - flagSet.StringVarP(&dir, "dir", "d", ".", "Run apply in this directory. Defaults to the root of the repo.") + flagSet.StringVarP(&workspace, "workspace", "w", defaultWorkspace, fmt.Sprintf("Apply the plan for this Terraform workspace. Defaults to '%s'", defaultWorkspace)) + flagSet.StringVarP(&dir, "dir", "d", "", "Run apply in this directory relative to root of repo. Use '.' for root. If not specified, will run apply against all plans created for this workspace.") flagSet.BoolVarP(&verbose, "verbose", "", false, "Append Atlantis log to comment.") - } else { return nil, fmt.Errorf("unknown command %q – this is a bug", command) } @@ -116,7 +120,26 @@ func (e *EventParser) DetermineCommand(comment string, vcsHost vcs.Host) (*Comma extraArgs = flagSet.Args()[flagSet.ArgsLenAtDash():] } - // todo: validate args + // If dir is specified, must ensure it's a valid path. + if dir != "" { + validatedDir := filepath.Clean(dir) + // Join with . so the path is relative. This helps us if they use '/', + // and is safe to do if their path is relative since it's a no-op. + validatedDir = filepath.Join(".", validatedDir) + // Need to clean again to resolve relative validatedDirs. + validatedDir = filepath.Clean(validatedDir) + // Detect relative dirs since they're not allowed. + if strings.HasPrefix(validatedDir, "..") { + return nil, fmt.Errorf("relative path %q not allowed", dir) + } + + dir = validatedDir + } + // Because we use the workspace name as a file, need to make sure it's + // not doing something weird like being a relative dir. + if strings.Contains(workspace, "..") { + return nil, errors.New("workspace can't contain '..'") + } c := &Command{Name: name, Verbose: verbose, Workspace: workspace, Dir: dir, Flags: extraArgs} return c, nil @@ -321,14 +344,3 @@ func (e *EventParser) stringInSlice(a string, list []string) bool { } return false } - -// nolint: unparam -func (e *EventParser) removeOccurrences(a string, list []string) []string { - var out []string - for _, b := range list { - if b != a { - out = append(out, b) - } - } - return out -} diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index cad775e3f..c8c5bc8ae 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -36,6 +36,15 @@ func TestDetermineCommandInvalid(t *testing.T) { "atlantis slkjd", "@github-user slkjd", "atlantis plans", + // relative dirs + "atlantis plan -d ..", + "atlantis plan -d ../", + "atlantis plan -d a/../../", + // using .. in workspace + "atlantis plan -w a..", + "atlantis plan -w ../", + "atlantis plan -w ..", + "atlantis plan -w a/../b", // misc "related comment mentioning atlantis", } @@ -94,7 +103,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { { "", "default", - ".", + "", false, "", }, @@ -102,7 +111,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { { "-w workspace", "workspace", - ".", + "", false, "", }, @@ -116,7 +125,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { { "--verbose", "default", - ".", + "", true, "", }, @@ -153,7 +162,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { { "-w workspace -- -d dir --verbose", "workspace", - ".", + "", false, "-d dir --verbose", }, @@ -161,7 +170,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { { "-w -d dir --verbose", "-d", - ".", + "", true, "", }, @@ -169,14 +178,14 @@ func TestDetermineCommand_Parsing(t *testing.T) { { "--", "default", - ".", + "", false, "", }, { "abc --", "default", - ".", + "", false, "", }, @@ -202,6 +211,42 @@ func TestDetermineCommand_Parsing(t *testing.T) { true, "arg one -two --three &&", }, + // Test that the dir string is normalized. + { + "-d /", + "default", + ".", + false, + "", + }, + { + "-d /adir", + "default", + "adir", + false, + "", + }, + { + "-d .", + "default", + ".", + false, + "", + }, + { + "-d ./", + "default", + ".", + false, + "", + }, + { + "-d ./adir", + "default", + "adir", + false, + "", + }, } for _, test := range cases { for _, cmdName := range []string{"plan", "apply"} { @@ -437,15 +482,6 @@ func TestParseGitlabMergeCommentEvent(t *testing.T) { }, user) } -func containsVerbose(list []string) bool { - for _, b := range list { - if b == "--verbose" { - return true - } - } - return false -} - var mergeEventJSON = `{ "object_kind": "merge_request", "user": { diff --git a/server/events/plan_executor.go b/server/events/plan_executor.go index 17583a9eb..b7c949984 100644 --- a/server/events/plan_executor.go +++ b/server/events/plan_executor.go @@ -52,21 +52,29 @@ func (p *PlanExecutor) SetLockURL(f func(id string) (url string)) { // Execute executes terraform plan for the ctx. func (p *PlanExecutor) Execute(ctx *CommandContext) CommandResponse { - // Figure out what projects have been modified so we know where to run plan. - modifiedFiles, err := p.VCSClient.GetModifiedFiles(ctx.BaseRepo, ctx.Pull, ctx.VCSHost) - if err != nil { - return CommandResponse{Error: errors.Wrap(err, "getting modified files")} - } - cloneDir, err := p.Workspace.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, ctx.Command.Workspace) if err != nil { return CommandResponse{Error: err} } - ctx.Log.Info("found %d files modified in this pull request", len(modifiedFiles)) - projects := p.ProjectFinder.DetermineProjects(ctx.Log, modifiedFiles, ctx.BaseRepo.FullName, cloneDir) - if len(projects) == 0 { - return CommandResponse{Failure: "No Terraform files were modified."} + var projects []models.Project + if ctx.Command.Dir == "" { + // If they didn't specify a directory to plan in, figure out what + // projects have been modified so we know where to run plan. + modifiedFiles, err := p.VCSClient.GetModifiedFiles(ctx.BaseRepo, ctx.Pull, ctx.VCSHost) + if err != nil { + return CommandResponse{Error: errors.Wrap(err, "getting modified files")} + } + ctx.Log.Info("found %d files modified in this pull request", len(modifiedFiles)) + projects = p.ProjectFinder.DetermineProjects(ctx.Log, modifiedFiles, ctx.BaseRepo.FullName, cloneDir) + if len(projects) == 0 { + return CommandResponse{Failure: "No Terraform files were modified."} + } + } else { + projects = []models.Project{{ + Path: ctx.Command.Dir, + RepoFullName: ctx.BaseRepo.FullName, + }} } var results []ProjectResult diff --git a/server/events/plan_executor_test.go b/server/events/plan_executor_test.go index ec07b4906..a26440eed 100644 --- a/server/events/plan_executor_test.go +++ b/server/events/plan_executor_test.go @@ -4,6 +4,7 @@ import ( "errors" "testing" + "github.com/mohae/deepcopy" . "github.com/petergtz/pegomock" "github.com/runatlantis/atlantis/server/events" "github.com/runatlantis/atlantis/server/events/locking" @@ -22,6 +23,7 @@ var planCtx = events.CommandContext{ Command: &events.Command{ Name: events.Plan, Workspace: "workspace", + Dir: "", }, Log: logging.NewNoopLogger(), BaseRepo: models.Repo{}, @@ -63,6 +65,38 @@ func TestExecute_CloneErr(t *testing.T) { Equals(t, "err", r.Error.Error()) } +func TestExecute_DirectoryAndWorkspaceSet(t *testing.T) { + t.Log("Test that we run plan in the right directory and workspace if they're set") + p, runner, _ := setupPlanExecutorTest(t) + ctx := deepcopy.Copy(planCtx).(events.CommandContext) + ctx.Log = logging.NewNoopLogger() + ctx.Command.Dir = "dir1/dir2" + ctx.Command.Workspace = "workspace-flag" + + When(p.Workspace.Clone(ctx.Log, ctx.BaseRepo, ctx.HeadRepo, ctx.Pull, "workspace-flag")). + ThenReturn("/tmp/clone-repo", nil) + When(p.ProjectPreExecute.Execute(&ctx, "/tmp/clone-repo", models.Project{RepoFullName: "", Path: "dir1/dir2"})). + ThenReturn(events.PreExecuteResult{ + LockResponse: locking.TryLockResponse{ + LockKey: "key", + }, + }) + r := p.Execute(&ctx) + + runner.VerifyWasCalledOnce().RunCommandWithVersion( + ctx.Log, + "/tmp/clone-repo/dir1/dir2", + []string{"plan", "-refresh", "-no-color", "-out", "/tmp/clone-repo/dir1/dir2/workspace-flag.tfplan", "-var", "atlantis_user=anubhavmishra"}, + nil, + "workspace-flag", + ) + Assert(t, len(r.ProjectResults) == 1, "exp one project result") + result := r.ProjectResults[0] + Assert(t, result.PlanSuccess != nil, "exp plan success to not be nil") + Equals(t, "", result.PlanSuccess.TerraformOutput) + Equals(t, "lockurl-key", result.PlanSuccess.LockURL) +} + func TestExecute_Success(t *testing.T) { t.Log("If there are no errors, the plan should be returned") p, runner, _ := setupPlanExecutorTest(t) diff --git a/server/events/project_pre_execute.go b/server/events/project_pre_execute.go index e831d8a41..3dde2bf92 100644 --- a/server/events/project_pre_execute.go +++ b/server/events/project_pre_execute.go @@ -51,14 +51,28 @@ func (p *DefaultProjectPreExecutor) Execute(ctx *CommandContext, repoDir string, lockAttempt.CurrLock.Pull.Num)}} } ctx.Log.Info("acquired lock with id %q", lockAttempt.LockKey) + config, tfVersion, err := p.executeWithLock(ctx, repoDir, project) + if err != nil { + p.Locker.Unlock(lockAttempt.LockKey) // nolint: errcheck + return PreExecuteResult{ProjectResult: ProjectResult{Error: err}} + } + return PreExecuteResult{ProjectConfig: config, TerraformVersion: tfVersion, LockResponse: lockAttempt} +} + +// executeWithLock executes the pre plan/apply tasks after the lock has been +// acquired. This helper func makes revoking the lock on error easier. +// Returns the project config, terraform version, or an error. +func (p *DefaultProjectPreExecutor) executeWithLock(ctx *CommandContext, repoDir string, project models.Project) (ProjectConfig, *version.Version, error) { + workspace := ctx.Command.Workspace // Check if config file is found, if not we continue the run. var config ProjectConfig absolutePath := filepath.Join(repoDir, project.Path) if p.ConfigReader.Exists(absolutePath) { + var err error config, err = p.ConfigReader.Read(absolutePath) if err != nil { - return PreExecuteResult{ProjectResult: ProjectResult{Error: err}} + return config, nil, err } ctx.Log.Info("parsed atlantis config file in %q", absolutePath) } @@ -74,25 +88,25 @@ func (p *DefaultProjectPreExecutor) Execute(ctx *CommandContext, repoDir string, if len(config.PreInit) > 0 { _, err := p.Run.Execute(ctx.Log, config.PreInit, absolutePath, workspace, terraformVersion, "pre_init") if err != nil { - return PreExecuteResult{ProjectResult: ProjectResult{Error: errors.Wrapf(err, "running %s commands", "pre_init")}} + return config, nil, errors.Wrapf(err, "running %s commands", "pre_init") } } _, err := p.Terraform.Init(ctx.Log, absolutePath, workspace, config.GetExtraArguments("init"), terraformVersion) if err != nil { - return PreExecuteResult{ProjectResult: ProjectResult{Error: err}} + return config, nil, err } } else { ctx.Log.Info("determined that we are running terraform with version < 0.9.0. Running version %s", terraformVersion) if len(config.PreGet) > 0 { _, err := p.Run.Execute(ctx.Log, config.PreGet, absolutePath, workspace, terraformVersion, "pre_get") if err != nil { - return PreExecuteResult{ProjectResult: ProjectResult{Error: errors.Wrapf(err, "running %s commands", "pre_get")}} + return config, nil, errors.Wrapf(err, "running %s commands", "pre_get") } } terraformGetCmd := append([]string{"get", "-no-color"}, config.GetExtraArguments("get")...) _, err := p.Terraform.RunCommandWithVersion(ctx.Log, absolutePath, terraformGetCmd, terraformVersion, workspace) if err != nil { - return PreExecuteResult{ProjectResult: ProjectResult{Error: err}} + return config, nil, err } } @@ -106,8 +120,8 @@ func (p *DefaultProjectPreExecutor) Execute(ctx *CommandContext, repoDir string, if len(commands) > 0 { _, err := p.Run.Execute(ctx.Log, commands, absolutePath, workspace, terraformVersion, stage) if err != nil { - return PreExecuteResult{ProjectResult: ProjectResult{Error: errors.Wrapf(err, "running %s commands", stage)}} + return config, nil, errors.Wrapf(err, "running %s commands", stage) } } - return PreExecuteResult{ProjectConfig: config, TerraformVersion: terraformVersion, LockResponse: lockAttempt} + return config, terraformVersion, nil }