feat: add GitHub team allowlist configuration option #1694

paulerickson · 2021-07-07T19:56:33Z

Summary

Configure permissions by GitHub team, with a list of teams and commands they are permitted to use.

Usage

atlantis server --gh-team-allowlist "myteam:plan, secteam:apply"

Background

This change is intended to replace #1206.
The code is the same, so all credit/blame goes to @jamengual & co-authors—I have only squashed those 75 revisions down to one, rebased on master, and resolved a few minor conflicts & incompatibilities.

msarvar · 2021-07-13T03:23:07Z

@paulerickson hey, thanks for updating the PR. Original PR had quite a bit of comments seems like code stayed largely the same, would you be able to address the feedback from there? Comments are largely around moving the functionality into command runner and possibly moving the flag into server config.

majormoses · 2021-09-22T04:05:27Z

I'd also like to ask that we consider the security implications of default allowing all teams to perform all actions. Maybe we should require people to set this and refuse any commands from users unless they are authorized? Regarding the moving of team allowlists around I think it should be controlled serverside in a config. I am wondering however if maybe it should even be configurable on a per repository basis. For example I might have a repository used to bootstrap new aws accounts into my organization and because of the required privileges this comes with we want it locked down to as few people as possible while others can be utilized by larger groups of people. Currently our threat model shows that the most critical points to cover currently due to lack of controls (which this helps build) is the repository itself and the verification of the payloads from Github webhook.

paulerickson · 2021-10-14T17:22:11Z

So sorry for the lack of updates! I fixed up a couple of things, but got stuck with server config, which was not so obvious to me. I have rebased again and am taking another crack at it now.

paulerickson · 2021-10-14T18:00:37Z

@msarvar can you clarify whether this comment was already addressed or else what change is being suggested?

Have you considered moving the flag to server config? It might be easier to manage that way. You will also get unmarshaling through server config. It should also remove the need for string parsing that you have to do https://github.com/runatlantis/atlantis/pull/1206/files#diff-6d78b261a5a26bea8298bc9de21e6460e101e2cbc14f0ad282e2415006d0b760R23.

The new flag is already defined in server.go, and the string parsing mirrors that of the similar flag --repo-allowlist; I'm not sure where else parsing should be done.

paulerickson · 2021-11-11T21:27:37Z

I have moved the permissions check from event handler to command runner, which I think someone requested: b8fce3a

msarvar · 2021-11-11T23:12:40Z

server/events/team_allowlist_checker.go

+	t := strings.TrimSpace(team)
+	c := strings.TrimSpace(command)


I don't think this necessary, is there a reason for this trimming?

Not that I can see. Removed in fbcd74f

msarvar · 2021-11-11T23:13:29Z

server/events/team_allowlist_checker.go

+// 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)


msarvar · 2021-11-11T23:19:17Z

server/events/team_allowlist_checker_test.go

+	Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `plan`))
+	Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `release`))
+	Equals(t, false, checker.IsCommandAllowedForAnyTeam(teams, `noop`))
+}


Let's add some tests for wildcard as well.

Added in 23c5cd9

msarvar · 2021-11-12T22:47:05Z

server/events/vcs/github_client.go

+		}
+		for _, t := range teams {
+			membership, _, err := g.client.Teams.GetTeamMembershipBySlug(g.ctx, org, *t.Slug, user.Username)
+			if err == nil && membership != nil {


We should return if there is an error or at least log the error.

I logged it here: ac049ea

I think continuing with no groups is a simple way to handle it unless we can retry or do some better handling in a higher layer

msarvar · 2021-11-12T22:52:29Z

server/events/vcs/github_client.go

+	for {
+		teams, resp, err := g.client.Teams.ListTeams(g.ctx, org, opts)
+		if err != nil {
+			return nil, err


Let's wrap the error here. Something like `errors.Wrap(err, "retrieving github teams")

addressed in ac049ea

msarvar · 2021-11-17T18:12:38Z

@paulerickson Hey, pr looks good. Left couple comments, let me know what you think

Co-authored-by: PePe (Jose) Amengual <[email protected]> Co-authored-by: Troy Neeriemer <[email protected]> Co-authored-by: Ted Roby <[email protected]> Co-authored-by: Paul Erickson <[email protected]>

paulerickson · 2021-12-17T22:34:24Z

@paulerickson Hey, pr looks good. Left couple comments, let me know what you think

Thanks, @msarvar. I think I've addressed everything, and rebased from master

msarvar

Looks good, thanks for contributing!

osterman · 2021-12-22T20:37:11Z

Relates to #308

…-test-job * 'master' of github.com:runatlantis/atlantis: (79 commits) release: 0.18.0 (#1965) feat: streaming terraform logs in real-time (#1937) build(deps): bump github.com/hashicorp/go-getter from 1.5.9 to 1.5.10 (#1961) docs: update website links (#1964) docs: clarify example for `--azuredevops-token` flag (#1712) docs: typo in heading level (#1960) fix: fallback to default TF version in apply step (#1931) feat: add GitHub team allowlist configuration option (#1694) Add in Dockerfile support for last Terraform 1.0.x version in AVAILABLE_TERRAFORM_VERSIONS (#1957) build(deps): bump github.com/spf13/viper from 1.10.0 to 1.10.1 (#1956) deps: terraform 1.1.2 (#1952) release: 0.17.6 (#1947) docker: make multi-platform atlantis image (#1943) docker(testing): fix arch ref for `linux/arm/v7` docker(testing): updating image build process docs: fix policy check documentation examples (#1945) build: make multi-platform image for testing-env atlantis-base: fix context atalntis-base: update platforms and path trigger atlantis-base: update platforms ...

albertorm95 · 2022-01-04T11:23:38Z

server/events/vcs/github_client.go

+			return nil, errors.Wrap(err, "retrieving GitHub teams")
+		}
+		for _, t := range teams {
+			membership, _, err := g.client.Teams.GetTeamMembershipBySlug(g.ctx, org, *t.Slug, user.Username)


What about the rate limiting? Wound't it be better to use the GraphQL so we could just pull the orgs teams of a user?

https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting

--- edit:
https://stackoverflow.com/questions/30466741/github-api-list-a-users-teams-within-an-organization/47538467#47538467 👀

👍 I haven't worked with the GraphQL API, but it does seem like some calls could be saved that way

@paulerickson Thanks for the answer I could try to switch this feature to use GraphQL :)

marcportabellaclotet-mt · 2022-10-07T09:58:21Z

It would be great if it had project based allowance.
In our case, we are using monorepo approach to store terraform configs and it would be interesting if we could allow gh teams to specific projects.

Somehing like :

--gh-team-allowlist "{ghteam}:{project}:{command}"

Thanks for your work!

nitrocode · 2022-11-25T17:19:09Z

@marcportabellaclotet-mt please open a new issue for that. Sounds like a good idea.

marcportabellaclotet-mt · 2022-11-26T11:11:35Z

@nitrocode done

* Add GitHub team allowlist configuration option Co-authored-by: PePe (Jose) Amengual <[email protected]> Co-authored-by: Troy Neeriemer <[email protected]> Co-authored-by: Ted Roby <[email protected]> Co-authored-by: Paul Erickson <[email protected]> * Check team allowlist in command runner, rather than event controller * Remove unneeded trimming * Test wildcard groups and commands * Improve error logging Co-authored-by: PePe (Jose) Amengual <[email protected]> Co-authored-by: Troy Neeriemer <[email protected]> Co-authored-by: Ted Roby <[email protected]>

paulerickson requested a review from a team as a code owner July 7, 2021 19:56

paulerickson force-pushed the github_team_allowlist_config branch from c02d16c to b1f23dc Compare July 7, 2021 20:04

chenrui333 changed the title ~~Add GitHub team allowlist configuration option~~ feat: add GitHub team allowlist configuration option Jul 7, 2021

paulerickson mentioned this pull request Aug 4, 2021

GitHub team allowlist command #1206

Closed

dimisjim mentioned this pull request Aug 23, 2021

Support atlantis import subcommand #217

Closed

paulerickson force-pushed the github_team_allowlist_config branch from b1f23dc to b88afd0 Compare October 14, 2021 17:11

paulerickson force-pushed the github_team_allowlist_config branch from b88afd0 to a82a789 Compare October 14, 2021 17:52

jamengual approved these changes Nov 10, 2021

View reviewed changes

msarvar reviewed Nov 17, 2021

View reviewed changes

paulerickson force-pushed the github_team_allowlist_config branch from b8fce3a to fdfa88f Compare December 15, 2021 18:26

PePe (Jose) Amengual and others added 5 commits December 17, 2021 16:04

Add GitHub team allowlist configuration option

dbbf507

Co-authored-by: PePe (Jose) Amengual <[email protected]> Co-authored-by: Troy Neeriemer <[email protected]> Co-authored-by: Ted Roby <[email protected]> Co-authored-by: Paul Erickson <[email protected]>

Check team allowlist in command runner, rather than event controller

f22d951

Remove unneeded trimming

fbcd74f

Test wildcard groups and commands

23c5cd9

Improve error logging

ac049ea

paulerickson force-pushed the github_team_allowlist_config branch from fdfa88f to ac049ea Compare December 17, 2021 22:27

msarvar approved these changes Dec 22, 2021

View reviewed changes

msarvar merged commit bc36846 into runatlantis:master Dec 22, 2021

hhsnow mentioned this pull request Dec 30, 2021

v0.18.0 github events fail with teams api permisison error #1967

Closed

albertorm95 reviewed Jan 4, 2022

View reviewed changes

paulerickson deleted the github_team_allowlist_config branch January 4, 2022 17:57

bentmann mentioned this pull request Jan 14, 2022

Support official atlantis version cloudposse-archives/terraform-aws-ecs-atlantis#42

Open

nitrocode mentioned this pull request Nov 25, 2022

Allow --gh-team-allowlist to work with team names and slugs #2719

Merged

3 tasks

DazWorrall mentioned this pull request Jan 9, 2023

feat: Support team owners for conftest policies #2953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add GitHub team allowlist configuration option #1694

feat: add GitHub team allowlist configuration option #1694

paulerickson commented Jul 7, 2021 •

edited

Loading

msarvar commented Jul 13, 2021

majormoses commented Sep 22, 2021

paulerickson commented Oct 14, 2021

paulerickson commented Oct 14, 2021

paulerickson commented Nov 11, 2021

msarvar Nov 11, 2021

paulerickson Dec 17, 2021

msarvar Nov 11, 2021

paulerickson Dec 17, 2021

msarvar Nov 11, 2021

paulerickson Dec 17, 2021

msarvar Nov 12, 2021

paulerickson Dec 17, 2021

msarvar Nov 12, 2021

paulerickson Dec 17, 2021

msarvar commented Nov 17, 2021

paulerickson commented Dec 17, 2021

msarvar left a comment

osterman commented Dec 22, 2021

albertorm95 Jan 4, 2022 •

edited

Loading

paulerickson Jan 4, 2022

albertorm95 Jan 5, 2022

marcportabellaclotet-mt commented Oct 7, 2022

nitrocode commented Nov 25, 2022

marcportabellaclotet-mt commented Nov 26, 2022

feat: add GitHub team allowlist configuration option #1694

feat: add GitHub team allowlist configuration option #1694

Conversation

paulerickson commented Jul 7, 2021 • edited Loading

Summary

Usage

Background

msarvar commented Jul 13, 2021

majormoses commented Sep 22, 2021

paulerickson commented Oct 14, 2021

paulerickson commented Oct 14, 2021

paulerickson commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msarvar commented Nov 17, 2021

paulerickson commented Dec 17, 2021

msarvar left a comment

Choose a reason for hiding this comment

osterman commented Dec 22, 2021

albertorm95 Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcportabellaclotet-mt commented Oct 7, 2022

nitrocode commented Nov 25, 2022

marcportabellaclotet-mt commented Nov 26, 2022

paulerickson commented Jul 7, 2021 •

edited

Loading

albertorm95 Jan 4, 2022 •

edited

Loading