From b1f23dc54d16feeb2e7987c8cd831086719ef9ff Mon Sep 17 00:00:00 2001 From: "PePe (Jose) Amengual" Date: Fri, 2 Jul 2021 17:49:39 -0500 Subject: [PATCH] Add GitHub team allowlist configuration option Co-authored-by: PePe (Jose) Amengual Co-authored-by: Troy Neeriemer Co-authored-by: Ted Roby Co-authored-by: Paul Erickson --- cmd/server.go | 17 +++++ .../controllers/events/events_controller.go | 39 ++++++++++ server/events/team_allowlist_checker.go | 72 +++++++++++++++++++ server/events/team_allowlist_checker_test.go | 35 +++++++++ server/events/vcs/azuredevops_client.go | 5 ++ server/events/vcs/bitbucketcloud/client.go | 5 ++ server/events/vcs/bitbucketserver/client.go | 5 ++ server/events/vcs/client.go | 1 + server/events/vcs/github_client.go | 27 +++++++ server/events/vcs/gitlab_client.go | 5 ++ .../events/vcs/mocks/matchers/models_user.go | 20 ++++++ server/events/vcs/mocks/mock_client.go | 50 +++++++++++++ .../events/vcs/not_configured_vcs_client.go | 3 + server/events/vcs/proxy.go | 4 ++ server/server.go | 5 ++ server/user_config.go | 1 + 16 files changed, 294 insertions(+) create mode 100644 server/events/team_allowlist_checker.go create mode 100644 server/events/team_allowlist_checker_test.go create mode 100644 server/events/vcs/mocks/matchers/models_user.go diff --git a/cmd/server.go b/cmd/server.go index 059142d22f..c0ea4fe0ad 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -62,6 +62,7 @@ const ( EnablePolicyChecksFlag = "enable-policy-checks" EnableRegExpCmdFlag = "enable-regexp-cmd" GHHostnameFlag = "gh-hostname" + GHTeamAllowlistFlag = "gh-team-allowlist" GHTokenFlag = "gh-token" GHUserFlag = "gh-user" GHAppIDFlag = "gh-app-id" @@ -109,6 +110,7 @@ const ( DefaultBitbucketBaseURL = bitbucketcloud.BaseURL DefaultDataDir = "~/.atlantis" DefaultGHHostname = "github.com" + DefaultGHTeamAllowlist = "*:*" DefaultGitlabHostname = "gitlab.com" DefaultLogLevel = "info" DefaultParallelPoolSize = 15 @@ -185,6 +187,18 @@ var stringFlags = map[string]stringFlag{ description: "Hostname of your Github Enterprise installation. If using github.com, no need to set.", defaultValue: DefaultGHHostname, }, + GHTeamAllowlistFlag: { + description: "Comma separated list of key-value pairs representing the GitHub teams and the operations that " + + "the members of a particular team are allowed to perform. " + + "The format is {team}:{command},{team}:{command}. " + + "Valid values for 'command' are 'plan', 'apply' and '*', e.g. 'dev:plan,ops:apply,devops:*'" + + "This example gives the users from the 'dev' GitHub team the permissions to execute the 'plan' command, " + + "the 'ops' team the permissions to execute the 'apply' command, " + + "and allows the 'devops' team to perform any operation. If this argument is not provided, the default value (*:*) " + + "will be used and the default behavior will be to not check permissions " + + "and to allow users from any team to perform any operation.", + defaultValue: DefaultGHTeamAllowlist, + }, GHUserFlag: { description: "GitHub username of API user.", defaultValue: "", @@ -612,6 +626,9 @@ func (s *ServerCmd) setDefaults(c *server.UserConfig) { if c.VCSStatusName == "" { c.VCSStatusName = DefaultVCSStatusName } + if c.GithubTeamAllowlist == "" { + c.GithubTeamAllowlist = DefaultGHTeamAllowlist + } if c.TFEHostname == "" { c.TFEHostname = DefaultTFEHostname } diff --git a/server/controllers/events/events_controller.go b/server/controllers/events/events_controller.go index 7164d10da7..28dcbecda4 100644 --- a/server/controllers/events/events_controller.go +++ b/server/controllers/events/events_controller.go @@ -62,6 +62,7 @@ type VCSEventsController struct { // request validation is done. GitlabWebhookSecret []byte RepoAllowlistChecker *events.RepoAllowlistChecker + TeamAllowlistChecker *events.TeamAllowlistChecker // SilenceAllowlistErrors controls whether we write an error comment on // pull requests from non-allowlisted repos. SilenceAllowlistErrors bool @@ -436,6 +437,20 @@ func (e *VCSEventsController) handleCommentEvent(w http.ResponseWriter, baseRepo return } + // Check if the user who commented has the permissions to execute the 'plan' or 'apply' commands + if !e.TestingMode { + ok, err := e.checkUserPermissions(baseRepo, user, parseResult.Command) + if err != nil { + e.Logger.Err("unable to comment on pull request: %s", err) + return + } + if !ok { + e.commentUserDoesNotHavePermissions(baseRepo, pullNum, user, parseResult.Command) + e.respond(w, logging.Warn, http.StatusForbidden, "User @%s does not have permissions to execute '%s' command", user.Username, parseResult.Command.Name.String()) + return + } + } + e.Logger.Debug("executing command") fmt.Fprintln(w, "Processing...") if !e.TestingMode { @@ -553,3 +568,27 @@ func (e *VCSEventsController) commentNotAllowlisted(baseRepo models.Repo, pullNu e.Logger.Err("unable to comment on pull request: %s", err) } } + +// commentUserDoesNotHavePermissions comments on the pull request that the user +// is not allowed to execute the command. +func (e *VCSEventsController) commentUserDoesNotHavePermissions(baseRepo models.Repo, pullNum int, user models.User, cmd *events.CommentCommand) { + errMsg := fmt.Sprintf("```\nError: User @%s does not have permissions to execute '%s' command.\n```", user.Username, cmd.Name) + if err := e.VCSClient.CreateComment(baseRepo, pullNum, errMsg, ""); err != nil { + e.Logger.Err("unable to comment on pull request: %s", err) + } +} + +// checkUserPermissions checks if the user has permissions to execute the command +func (e *VCSEventsController) checkUserPermissions(repo models.Repo, user models.User, cmd *events.CommentCommand) (bool, error) { + if cmd.Name == models.ApplyCommand || cmd.Name == models.PlanCommand { + teams, err := e.VCSClient.GetTeamNamesForUser(repo, user) + if err != nil { + return false, err + } + ok := e.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(teams, cmd.Name.String()) + if !ok { + return false, nil + } + } + return true, nil +} diff --git a/server/events/team_allowlist_checker.go b/server/events/team_allowlist_checker.go new file mode 100644 index 0000000000..8995365edc --- /dev/null +++ b/server/events/team_allowlist_checker.go @@ -0,0 +1,72 @@ +package events + +import ( + "strings" +) + +// Wildcard matches all teams and all commands +const wildcard = "*" + +// mapOfStrings is an alias for map[string]string +type mapOfStrings map[string]string + +// TeamAllowlistChecker implements checking the teams and the operations that the members +// of a particular team are allowed to perform +type TeamAllowlistChecker struct { + rules []mapOfStrings +} + +// NewTeamAllowlistChecker constructs a new checker +func NewTeamAllowlistChecker(allowlist string) (*TeamAllowlistChecker, error) { + var rules []mapOfStrings + pairs := strings.Split(allowlist, ",") + if pairs[0] != "" { + for _, pair := range pairs { + values := strings.Split(pair, ":") + team := strings.TrimSpace(values[0]) + command := strings.TrimSpace(values[1]) + m := mapOfStrings{team: command} + rules = append(rules, m) + } + } + return &TeamAllowlistChecker{ + rules: rules, + }, nil +} + +// IsCommandAllowedForTeam returns true if the team is allowed to execute the command +// and false otherwise. +func (checker *TeamAllowlistChecker) IsCommandAllowedForTeam(team string, command string) bool { + t := strings.TrimSpace(team) + c := strings.TrimSpace(command) + for _, rule := range checker.rules { + for key, value := range rule { + if (key == wildcard || strings.EqualFold(key, t)) && (value == wildcard || strings.EqualFold(value, c)) { + return true + } + } + } + return false +} + +// IsCommandAllowedForAnyTeam returns true if any of the teams is allowed to execute the command +// and false otherwise. +func (checker *TeamAllowlistChecker) IsCommandAllowedForAnyTeam(teams []string, command string) bool { + c := strings.TrimSpace(command) + if len(teams) == 0 { + for _, rule := range checker.rules { + for key, value := range rule { + if (key == wildcard) && (value == wildcard || strings.EqualFold(value, c)) { + return true + } + } + } + } else { + for _, t := range teams { + if checker.IsCommandAllowedForTeam(t, command) { + return true + } + } + } + return false +} diff --git a/server/events/team_allowlist_checker_test.go b/server/events/team_allowlist_checker_test.go new file mode 100644 index 0000000000..ec8039ded5 --- /dev/null +++ b/server/events/team_allowlist_checker_test.go @@ -0,0 +1,35 @@ +package events_test + +import ( + "testing" + + "github.com/runatlantis/atlantis/server/events" + . "github.com/runatlantis/atlantis/testing" +) + +func TestNewTeamAllowListChecker(t *testing.T) { + allowlist := `bob:plan, dave:apply` + _, err := events.NewTeamAllowlistChecker(allowlist) + Ok(t, err) +} + +func TestIsCommandAllowedForTeam(t *testing.T) { + allowlist := `bob:plan, dave:apply, connie:plan, connie:apply` + checker, err := events.NewTeamAllowlistChecker(allowlist) + Ok(t, err) + Equals(t, true, checker.IsCommandAllowedForTeam("connie", "plan")) + Equals(t, true, checker.IsCommandAllowedForTeam("connie", "apply")) + Equals(t, true, checker.IsCommandAllowedForTeam("dave", "apply")) + Equals(t, true, checker.IsCommandAllowedForTeam("bob", "plan")) + Equals(t, false, checker.IsCommandAllowedForTeam("bob", "apply")) +} + +func TestIsCommandAllowedForAnyTeam(t *testing.T) { + allowlist := `alpha:plan,beta:release` + teams := []string{`alpha`, `beta`} + checker, err := events.NewTeamAllowlistChecker(allowlist) + Ok(t, err) + Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `plan`)) + Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `release`)) + Equals(t, false, checker.IsCommandAllowedForAnyTeam(teams, `noop`)) +} diff --git a/server/events/vcs/azuredevops_client.go b/server/events/vcs/azuredevops_client.go index 2989544ab7..ebd7bce8a8 100644 --- a/server/events/vcs/azuredevops_client.go +++ b/server/events/vcs/azuredevops_client.go @@ -377,6 +377,11 @@ func SplitAzureDevopsRepoFullName(repoFullName string) (owner string, project st return repoFullName[:lastSlashIdx], "", repoFullName[lastSlashIdx+1:] } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (g *AzureDevopsClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + func (g *AzureDevopsClient) SupportsSingleFileDownload(repo models.Repo) bool { return false } diff --git a/server/events/vcs/bitbucketcloud/client.go b/server/events/vcs/bitbucketcloud/client.go index 9ba1ca41ee..d55f3a70b3 100644 --- a/server/events/vcs/bitbucketcloud/client.go +++ b/server/events/vcs/bitbucketcloud/client.go @@ -244,6 +244,11 @@ func (b *Client) makeRequest(method string, path string, reqBody io.Reader) ([]b return respBody, nil } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (b *Client) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + func (b *Client) SupportsSingleFileDownload(models.Repo) bool { return false } diff --git a/server/events/vcs/bitbucketserver/client.go b/server/events/vcs/bitbucketserver/client.go index 27af85354d..6252bc4af6 100644 --- a/server/events/vcs/bitbucketserver/client.go +++ b/server/events/vcs/bitbucketserver/client.go @@ -312,6 +312,11 @@ func (b *Client) makeRequest(method string, path string, reqBody io.Reader) ([]b return respBody, nil } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (b *Client) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + func (b *Client) SupportsSingleFileDownload(repo models.Repo) bool { return false } diff --git a/server/events/vcs/client.go b/server/events/vcs/client.go index 0f49cc89de..059616bd37 100644 --- a/server/events/vcs/client.go +++ b/server/events/vcs/client.go @@ -38,6 +38,7 @@ type Client interface { UpdateStatus(repo models.Repo, pull models.PullRequest, state models.CommitStatus, src string, description string, url string) error MergePull(pull models.PullRequest, pullOptions models.PullRequestOptions) error MarkdownPullLink(pull models.PullRequest) (string, error) + GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) // DownloadRepoConfigFile return `atlantis.yaml` content from VCS (which support fetch a single file from repository) // The first return value indicate that repo contain atlantis.yaml or not diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 6079fe408c..9a01da1a6e 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -379,6 +379,33 @@ func (g *GithubClient) MarkdownPullLink(pull models.PullRequest) (string, error) return fmt.Sprintf("#%d", pull.Num), nil } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +// https://developer.github.com/v3/teams/members/#get-team-membership +func (g *GithubClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + var teamNames []string + opts := &github.ListOptions{} + org := repo.Owner + for { + teams, resp, err := g.client.Teams.ListTeams(g.ctx, org, opts) + if err != nil { + return nil, err + } + for _, t := range teams { + membership, _, err := g.client.Teams.GetTeamMembershipBySlug(g.ctx, org, *t.Slug, user.Username) + if err == nil && membership != nil { + if *membership.State == "active" && (*membership.Role == "member" || *membership.Role == "maintainer") { + teamNames = append(teamNames, t.GetName()) + } + } + } + if resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return teamNames, nil +} + // ExchangeCode returns a newly created app's info func (g *GithubClient) ExchangeCode(code string) (*GithubAppTemporarySecrets, error) { ctx := context.Background() diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index dffab011a5..9bd83d5653 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -274,6 +274,11 @@ func MustConstraint(constraint string) version.Constraints { return c } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (g *GitlabClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + // DownloadRepoConfigFile return `atlantis.yaml` content from VCS (which support fetch a single file from repository) // The first return value indicate that repo contain atlantis.yaml or not // if BaseRepo had one repo config file, its content will placed on the second return value diff --git a/server/events/vcs/mocks/matchers/models_user.go b/server/events/vcs/mocks/matchers/models_user.go new file mode 100644 index 0000000000..aa45448ddd --- /dev/null +++ b/server/events/vcs/mocks/matchers/models_user.go @@ -0,0 +1,20 @@ +// Code generated by pegomock. DO NOT EDIT. +package matchers + +import ( + "reflect" + "github.com/petergtz/pegomock" + models "github.com/runatlantis/atlantis/server/events/models" +) + +func AnyModelsUser() models.User { + pegomock.RegisterMatcher(pegomock.NewAnyMatcher(reflect.TypeOf((*(models.User))(nil)).Elem())) + var nullValue models.User + return nullValue +} + +func EqModelsUser(value models.User) models.User { + pegomock.RegisterMatcher(&pegomock.EqMatcher{Value: value}) + var nullValue models.User + return nullValue +} diff --git a/server/events/vcs/mocks/mock_client.go b/server/events/vcs/mocks/mock_client.go index a39dd61e45..39cc88fa48 100644 --- a/server/events/vcs/mocks/mock_client.go +++ b/server/events/vcs/mocks/mock_client.go @@ -161,6 +161,25 @@ func (mock *MockClient) MarkdownPullLink(pull models.PullRequest) (string, error return ret0, ret1 } +func (mock *MockClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + if mock == nil { + panic("mock must not be nil. Use myMock := NewMockClient().") + } + params := []pegomock.Param{repo, user} + result := pegomock.GetGenericMockFrom(mock).Invoke("GetTeamNamesForUser", params, []reflect.Type{reflect.TypeOf((*[]string)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + var ret0 []string + var ret1 error + if len(result) != 0 { + if result[0] != nil { + ret0 = result[0].([]string) + } + if result[1] != nil { + ret1 = result[1].(error) + } + } + return ret0, ret1 +} + func (mock *MockClient) DownloadRepoConfigFile(pull models.PullRequest) (bool, []byte, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockClient().") @@ -508,6 +527,37 @@ func (c *MockClient_MarkdownPullLink_OngoingVerification) GetAllCapturedArgument return } +func (verifier *VerifierMockClient) GetTeamNamesForUser(repo models.Repo, user models.User) *MockClient_GetTeamNamesForUser_OngoingVerification { + params := []pegomock.Param{repo, user} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "GetTeamNamesForUser", params, verifier.timeout) + return &MockClient_GetTeamNamesForUser_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +} + +type MockClient_GetTeamNamesForUser_OngoingVerification struct { + mock *MockClient + methodInvocations []pegomock.MethodInvocation +} + +func (c *MockClient_GetTeamNamesForUser_OngoingVerification) GetCapturedArguments() (models.Repo, models.User) { + repo, user := c.GetAllCapturedArguments() + return repo[len(repo)-1], user[len(user)-1] +} + +func (c *MockClient_GetTeamNamesForUser_OngoingVerification) GetAllCapturedArguments() (_param0 []models.Repo, _param1 []models.User) { + params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) + if len(params) > 0 { + _param0 = make([]models.Repo, len(c.methodInvocations)) + for u, param := range params[0] { + _param0[u] = param.(models.Repo) + } + _param1 = make([]models.User, len(c.methodInvocations)) + for u, param := range params[1] { + _param1[u] = param.(models.User) + } + } + return +} + func (verifier *VerifierMockClient) DownloadRepoConfigFile(pull models.PullRequest) *MockClient_DownloadRepoConfigFile_OngoingVerification { params := []pegomock.Param{pull} methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "DownloadRepoConfigFile", params, verifier.timeout) diff --git a/server/events/vcs/not_configured_vcs_client.go b/server/events/vcs/not_configured_vcs_client.go index 3f8556765f..c94d321232 100644 --- a/server/events/vcs/not_configured_vcs_client.go +++ b/server/events/vcs/not_configured_vcs_client.go @@ -53,6 +53,9 @@ func (a *NotConfiguredVCSClient) MarkdownPullLink(pull models.PullRequest) (stri func (a *NotConfiguredVCSClient) err() error { return fmt.Errorf("atlantis was not configured to support repos from %s", a.Host.String()) } +func (a *NotConfiguredVCSClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, a.err() +} func (a *NotConfiguredVCSClient) SupportsSingleFileDownload(repo models.Repo) bool { return false diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index c14563a467..98ccc308a3 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -84,6 +84,10 @@ func (d *ClientProxy) MarkdownPullLink(pull models.PullRequest) (string, error) return d.clients[pull.BaseRepo.VCSHost.Type].MarkdownPullLink(pull) } +func (d *ClientProxy) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return d.clients[repo.VCSHost.Type].GetTeamNamesForUser(repo, user) +} + func (d *ClientProxy) DownloadRepoConfigFile(pull models.PullRequest) (bool, []byte, error) { return d.clients[pull.BaseRepo.VCSHost.Type].DownloadRepoConfigFile(pull) } diff --git a/server/server.go b/server/server.go index 9353d92306..1018295ecf 100644 --- a/server/server.go +++ b/server/server.go @@ -581,6 +581,10 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { if err != nil { return nil, err } + githubTeamAllowlistChecker, err := events.NewTeamAllowlistChecker(userConfig.GithubTeamAllowlist) + if err != nil { + return nil, err + } locksController := &controllers.LocksController{ AtlantisVersion: config.AtlantisVersion, AtlantisURL: parsedURL, @@ -606,6 +610,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { GitlabRequestParserValidator: &events_controllers.DefaultGitlabRequestParserValidator{}, GitlabWebhookSecret: []byte(userConfig.GitlabWebhookSecret), RepoAllowlistChecker: repoAllowlist, + TeamAllowlistChecker: githubTeamAllowlistChecker, SilenceAllowlistErrors: userConfig.SilenceAllowlistErrors, SupportedVCSHosts: supportedVCSHosts, VCSClient: vcsClient, diff --git a/server/user_config.go b/server/user_config.go index 5354203fe7..445024171b 100644 --- a/server/user_config.go +++ b/server/user_config.go @@ -38,6 +38,7 @@ type UserConfig struct { GithubAppID int64 `mapstructure:"gh-app-id"` GithubAppKey string `mapstructure:"gh-app-key-file"` GithubAppSlug string `mapstructure:"gh-app-slug"` + GithubTeamAllowlist string `mapstructure:"gh-team-allowlist"` GitlabHostname string `mapstructure:"gitlab-hostname"` GitlabToken string `mapstructure:"gitlab-token"` GitlabUser string `mapstructure:"gitlab-user"`