GitHub team allowlist command #1206

jamengual · 2020-10-01T00:15:53Z

Add the ability to specify a allowlist of GitHub teams and Atlantis commands that those teams can execute.

The idea behind this is that an Atlantis operator can pass a parameter to the Atlantis server like so:

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

and by doing so add more granular controls on which teams and command those teams can run.

this has been discussed before in this issue : #308 that have 35 👍

Over the years these types of features have been gained a lot of interest for security reasons.

From what I gather there is a big desire for these features to make it to Atlantis and I'm willing to make the necessary changes to make this happen.

I have been using this code for years and I have no issues at all, and other Atlantis users have used this code in their forks.

The current workarounds leave blank comments or require shell scripts or bash parsing solutions.

I have been authorized by CloudPosse to use their code for this PR.

Thanks.

Merged in #1694

…e names

Multienv and ghteams

…tlantis into github_team_whitelist_command

jamengual · 2020-10-26T16:04:07Z

@lkysow let me know your thoughts, thanks

netguino · 2020-11-06T21:30:37Z

I have followed Atlantis for quite some time now, and I recall a conversation, perhaps a PR request, on an Issue, where it was explained that at its core, Atlantis was simply the "executioner" and shouldn't really be evaluating who can do what. I believe it was the same idea when debating about restricting the UI.

While I could potentially agree with this, I wanted to share an end user's perspective.

One of the things that has made Atlantis great for us, has been that it is a pretty self-contained thing. It also isn't a requirement to go through the atlantis route as one can always execute terraform itself if wanted. As such, the vanilla product offers everything we need. Almost. And this almost is what draws the line between a great fit for us, and an exceptional one.

The ability to restrict per github team is one of the missing features.We already restrict what type of conditions need to be in place before executing, so I don't think it's too far of a stretch to mentally assing a "allowed team" structure to the objects. Would it maybe fit better if this was on a per repo setting instead of a global server one?

nitrocode · 2020-11-06T22:38:21Z

@netguino I completely agree. This PR has gotten a lot of upvotes. What do you think @lkysow ? Can it be merged?

jamengual · 2020-11-06T22:48:31Z

The ability to restrict per github team is one of the missing features.We already restrict what type of conditions need to be in place before executing, so I don't think it's too far of a stretch to mentally assing a "allowed team" structure to the objects. Would it maybe fit better if this was on a per repo setting instead of a global server one?

I do not see why we could not create a subsequent PR ( after this is merged) to accommodate this at the per repo level, that is a great idea and it will be easy enough since we already check per user-level access at the repo level so we know the repo that the command is being executed for.

nishkrishnan

Looks good. couple comments and questions.

Also, I code cov is dropping and I don't see any tests?

nishkrishnan · 2020-12-07T01:09:57Z

server/events/team_allowlist_checker.go

+}
+
+// NewTeamAllowlistChecker constructs a new checker
+func NewTeamAllowlistChecker(whitelist string) (*TeamAllowlistChecker, error) {


change to allowlist

nishkrishnan · 2020-12-07T01:14:53Z

server/events/vcs/github_client.go

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


probably remove this comment

nishkrishnan · 2020-12-07T01:20:29Z

server/events_controller.go

+
+// checkUserPermissions checks if the user has permissions to execute the command
+func (e *EventsController) checkUserPermissions(repo models.Repo, user models.User, cmd *events.CommentCommand) (bool, error) {
+	if cmd.Name == models.ApplyCommand || cmd.Name == models.PlanCommand {


Do we really need this check? Seems a bit redundant

if we add more command like unlock, it could be parsed by this block which it will not work but it could be removed I think since at that point we know is a valid command

I think by the time we get here the command has been parsed and it is valid. Also this might introduce bugs if there is a new command and this place was not updated. I believe this feature should work with any valid Atlantis command, right?

jamengual · 2020-12-07T03:31:42Z

Looks good. couple comments and questions.

Also, I code cov is dropping and I don't see any tests?

I did not write any test since I did not know how, I was hoping to get a bit of guidance on how to proceed with that since I'm still learning the base code.

nishkrishnan · 2021-01-28T20:53:02Z

Looks good. couple comments and questions.
Also, I code cov is dropping and I don't see any tests?

I did not write any test since I did not know how, I was hoping to get a bit of guidance on how to proceed with that since I'm still learning the base code.

There are a bunch of examples in the code base and golang in general has good docs on how to write tests/testing best practices

jamengual · 2021-01-28T20:55:47Z

Looks good. couple comments and questions.
Also, I code cov is dropping and I don't see any tests?

I did not write any test since I did not know how, I was hoping to get a bit of guidance on how to proceed with that since I'm still learning the base code.

There are a bunch of examples in the code base and golang in general has good docs on how to write tests/testing best practices

yes, I have been looking at those hopefully soon I will start working on them.

majormoses · 2021-02-25T13:29:18Z

cmd/server.go

@@ -99,6 +100,7 @@ const (
 	DefaultBitbucketBaseURL = bitbucketcloud.BaseURL
 	DefaultDataDir          = "~/.atlantis"
 	DefaultGHHostname       = "github.com"
+	DefaultGHTeamAllowlist  = "*:*"


I would rather this to something specific (even if its all commands currently supported), and/or force intentional configuration. My rationale is that this makes it safe to implement more dangerous commands that are intentionally not exposed currently such as import and rm. I'd personally prefer a breaking change over backwards compatibility here as whats currently being proposed is users are by default admins. This flies in the face of the https://en.wikipedia.org/wiki/Principle_of_least_privilege. Make admins escalate their privs.

msarvar

Good job, I left couple comments.

msarvar · 2021-03-05T04:22:18Z

cmd/server.go

@@ -167,6 +169,18 @@ var stringFlags = map[string]stringFlag{
 		description:  "Hostname of your Github Enterprise installation. If using github.com, no need to set.",
 		defaultValue: DefaultGHHostname,
 	},
+	GHTeamAllowlistFlag: {


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.

msarvar · 2021-03-05T04:31:33Z

server/events_controller.go

@@ -435,6 +439,20 @@ func (e *EventsController) handleCommentEvent(w http.ResponseWriter, baseRepo mo
 		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)


I think moving this into CommandRunnner might work better. If there is ever another way to run atlantis outside of GitHub PRs it will allow developers not to add another check.

msarvar · 2021-03-05T04:33:07Z

server/events_controller.go

+
+// checkUserPermissions checks if the user has permissions to execute the command
+func (e *EventsController) checkUserPermissions(repo models.Repo, user models.User, cmd *events.CommentCommand) (bool, error) {
+	if cmd.Name == models.ApplyCommand || cmd.Name == models.PlanCommand {


I think by the time we get here the command has been parsed and it is valid. Also this might introduce bugs if there is a new command and this place was not updated. I believe this feature should work with any valid Atlantis command, right?

troby · 2021-06-01T23:29:35Z

Tests have been added to cover new functions in team_allowlist_checker.go.

gaston-te · 2021-08-04T16:43:31Z

@jamengual Is there an ETA for this to be released? This is a feature we're looking forward to

jamengual · 2021-08-04T16:58:05Z

This PR has been taken other by a ex-team mate and they opened a new PR to cleanup history and such. There is few changes requested that I hope they will have time to do them soon.

…

On Wed, Aug 4, 2021 at 9:43 AM Gaston Bezzi ***@***.***> wrote: @jamengual <https://github.com/jamengual> Is there an ETA for this to be released? This is a feature we're looking forward to — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1206 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERCUCUNCTKB54C7GXGLT3FU37ANCNFSM4R7XJ6QA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

paulerickson · 2021-08-04T22:22:16Z

Is there an ETA for this to be released? This is a feature we're looking forward to

@gaston-te, if you want to build it yourself, #1694 is a more up-to-date revision of this.
I am still getting around to the remaining changes, which are mainly refactoring rather than functional.

majormoses · 2021-08-28T18:14:22Z

I have followed Atlantis for quite some time now, and I recall a conversation, perhaps a PR request, on an Issue, where it was explained that at its core, Atlantis was simply the "executioner" and shouldn't really be evaluating who can do what. I believe it was the same idea when debating about restricting the UI.

While I could potentially agree with this, I wanted to share an end user's perspective.

One of the things that has made Atlantis great for us, has been that it is a pretty self-contained thing. It also isn't a requirement to go through the atlantis route as one can always execute terraform itself if wanted. As such, the vanilla product offers everything we need. Almost. And this almost is what draws the line between a great fit for us, and an exceptional one.

The ability to restrict per github team is one of the missing features.We already restrict what type of conditions need to be in place before executing, so I don't think it's too far of a stretch to mentally assing a "allowed team" structure to the objects. Would it maybe fit better if this was on a per repo setting instead of a global server one?

Hey I wanted to chime in specifically about atlantis about being the executioner as I was one of those people that said it. Back then there were larger issues for atlantis to solve around its core. I absolutely see the need as a lot of the protections with CI systems are restricting access to the code that can trigger a run, since its essentially remote code execution as a service.

I do disagree with the default of give people apply access. If we are building security features they should be secure by default. We should follow the principle of least privilege and would prefer it to be released as a major version bump and allow people to switch when they can get the required config in place for the migration.

jamengual · 2021-12-22T19:11:57Z

Merged in #1694

PePe (Jose) Amengual and others added 30 commits June 2, 2020 13:49

Adding cloudposse team whitelist option and custom atlaintis yaml fil…

a8c6563

…e names

Mergin master to 0.13.0

48fbde5

Merge pull request #1 from sonatype/multienv_ghteams

09dd404

Multienv and ghteams

Upgrading to 0.15

b19dce9

Adding Nacy tests

cde82a7

Adding Header.txt

3460d7e

Adding license exclusions

8f69a3f

Adding license exclusions

5405939

Adding license exclusions

30444a2

Fixing Nancy test

497ab03

Fixing Github API changes and delating deprecated RepoWhitelistChecker

540a4be

Adding nancy pipeline

5cc11cc

Adding nancy pipeline

9f61d37

Adding Nexus IQ pipeline

2f2c558

Adding Nexus IQ pipeline

5d88d32

Adding Nexus IQ pipeline and junit tests

59969e3

Running tests

7f7d340

Running tests

4505fe9

Running tests

90bb699

Running tests

adeb9f0

Running tests

b90a069

Running tests

315e02a

Trying Running tests

66c0c01

Trying Running tests

7501299

Trying to fix tests

8666a4f

Trying to fix tests

5b4a4bb

Trying to fix tests

9ae0e06

Trying to fix tests

6e8e1c6

Trying to fix tests

4cb5d52

Trying to fix tests

b64c200

jamengual added 4 commits October 5, 2020 16:51

Fixing test for gosimple

230a8a9

Renaming to allowlist

d3d67d1

Renaming to allowlist

70e2d8a

Merge branch 'github_team_whitelist_command' of github.com:sonatype/a…

beabc12

…tlantis into github_team_whitelist_command

jamengual mentioned this pull request Nov 7, 2020

[Feature Request] Support configurable list of who can atlantis apply #1231

Closed

nishkrishnan reviewed Dec 7, 2020

View reviewed changes

cleanup

0809e73

jamengual mentioned this pull request Jan 8, 2021

Enforce only certain people can plan/apply for gitlab, bitbucket, etc #103

Open

nishkrishnan added the needs tests Change requires tests label Jan 28, 2021

jamengual mentioned this pull request Feb 23, 2021

Restrict Plan or Apply to Github Teams or Github Users #308

Closed

majormoses reviewed Feb 25, 2021

View reviewed changes

msarvar reviewed Mar 5, 2021

View reviewed changes

initial testing for team_allowlist_checker

8dcd8b0

jamengual requested a review from a team as a code owner June 1, 2021 22:17

paulerickson mentioned this pull request Jul 7, 2021

feat: add GitHub team allowlist configuration option #1694

Merged

dimisjim mentioned this pull request Aug 23, 2021

Support atlantis import subcommand #217

Closed

jamengual closed this Dec 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub team allowlist command #1206

GitHub team allowlist command #1206

jamengual commented Oct 1, 2020 •

edited

Loading

jamengual commented Oct 26, 2020

netguino commented Nov 6, 2020 •

edited

Loading

nitrocode commented Nov 6, 2020

jamengual commented Nov 6, 2020 •

edited

Loading

nishkrishnan left a comment

nishkrishnan Dec 7, 2020

nishkrishnan Dec 7, 2020

nishkrishnan Dec 7, 2020

jamengual Dec 7, 2020

msarvar Mar 5, 2021

jamengual commented Dec 7, 2020

nishkrishnan commented Jan 28, 2021

jamengual commented Jan 28, 2021

majormoses Feb 25, 2021 •

edited

Loading

msarvar left a comment

msarvar Mar 5, 2021

msarvar Mar 5, 2021

msarvar Mar 5, 2021

troby commented Jun 1, 2021

gaston-te commented Aug 4, 2021

jamengual commented Aug 4, 2021 via email

paulerickson commented Aug 4, 2021

majormoses commented Aug 28, 2021 •

edited

Loading

jamengual commented Dec 22, 2021

GitHub team allowlist command #1206

GitHub team allowlist command #1206

Conversation

jamengual commented Oct 1, 2020 • edited Loading

jamengual commented Oct 26, 2020

netguino commented Nov 6, 2020 • edited Loading

nitrocode commented Nov 6, 2020

jamengual commented Nov 6, 2020 • edited Loading

nishkrishnan left a 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

jamengual commented Dec 7, 2020

nishkrishnan commented Jan 28, 2021

jamengual commented Jan 28, 2021

majormoses Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

msarvar left a 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

troby commented Jun 1, 2021

gaston-te commented Aug 4, 2021

jamengual commented Aug 4, 2021 via email

paulerickson commented Aug 4, 2021

majormoses commented Aug 28, 2021 • edited Loading

jamengual commented Dec 22, 2021

jamengual commented Oct 1, 2020 •

edited

Loading

netguino commented Nov 6, 2020 •

edited

Loading

jamengual commented Nov 6, 2020 •

edited

Loading

majormoses Feb 25, 2021 •

edited

Loading

majormoses commented Aug 28, 2021 •

edited

Loading