Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: RBAC #6

Merged
merged 17 commits into from Jan 18, 2019
Merged

RFC: RBAC #6

merged 17 commits into from Jan 18, 2019

Conversation

jwntrs
Copy link
Contributor

@jwntrs jwntrs commented Jul 18, 2018

Proposal

Status: Draft

This doesn't seem like that big of a change, and I think most proposed changes are fairly straight forward. I'm not a huge fan of my proposed solution for fly set-team, but with our static flags (i.e. --github-user) being as they are, I can't really see how else this would work.

Or you can view the original issue

@xtreme-sameer-vohra
Copy link

xtreme-sameer-vohra commented Jul 18, 2018

Copying from #2389

As an alternative to require 2 commands to set some viewers & members, could we also consider alternatives that might allow configuring a team with a single command.
This would remove the point mentioned above

The main drawback here is that we would have to merge auth configuration because viewers and members would be configured in two separate steps. The current behaviour overwrites the entire auth config each time, so in keeping with the current behaviour I guess we would overwrite the entire config for each role separately.

For example set-team could allow you to specify a config file with the following content

{
	"viewer": {
		"groups": [],
		"users": []
	},
	"member": {
		"groups": ["github:myorg:myteam"],
		"users": ["github:pivotal-jwinters"]
	}
}

@xtreme-sameer-vohra
Copy link

For the team viewer role, should they be able to access the team resource itself including the auth config ?
That might be an operation that we might restrict to team members only.

Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a lot of sense to me! Left some suggestions inline.

fly -t mytarget set-team -n myteam --role member --github-user pivotal-jwinters --github-team myorg:myteam
```

The main drawback here is that we would have to merge `auth` configuration because `viewers` and `members` would be configured in two separate steps. The current behaviour overwrites the entire auth config each time, so in keeping with the current behaviour I guess we would overwrite the entire config for each role separately.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.


`team_viewer` or `viewer` - Granted during `fly set-team`. Has readonly access to all the team's resources.

`public` - Not a role. This applies to pipelines. Public pipelines will behave as they did before.

This comment was marked as spam.

@xtreme-sameer-vohra
Copy link

Would we consider seeded teams ( created via the manifest ) static ?
If we allow users to also update a particular team that was configured in the manifest by using fly set-team or fly set-teams then it will be hard to infer the desired behaviour.

@vito
Copy link
Member

vito commented Jul 20, 2018

@xtreme-sameer-vohra yep, i think so: https://github.com/concourse/concourse/issues/1193#issuecomment-393195930

@jama22
Copy link
Member

jama22 commented Jul 27, 2018

This is the permission matrix I've written up to capture both fly and web view actions for each potential member. It's not comprehensive but should cover most use cases.

https://docs.google.com/spreadsheets/d/1np3hyJy3mVRfB2gcgKykz3QTQg5qEj28QgK523SEmao/edit#gid=0

@jchesterpivotal
Copy link

Dumb question: will this overlap with or cause confusion vs Kubernetes RBAC?

```yaml
teams:
main:
admin: true

This comment was marked as spam.


# Proposal

## What roles do we need?

This comment was marked as spam.

@Rukenshia
Copy link

Hi, just read through the RFC and have a couple of questions hoping it is the correct place to ask them:

Is this RFC for "default" roles with predefined permissions? I think this feature should be very configurable for admins as there are probably lots of organisations out there with different compliance requirements that won't be covered by the roles mentioned in the RFC. We have quite a few roles involved in our release process (ugh...) so it would be awesome if I could create my own role, attach permissions to it and then assign that to users.

I am thinking basically having a permission for each kind of action in concourse (i.e. all CLI commands) that can be assigned to roles.

If that is the case, I definitely would add a role that can view pipelines and manually trigger jobs in them, but not update the pipeline configuration. We need this for our "Release Approvers" that are basically just the people pushing buttons to mark their approval for the release.

If this is not the correct place, please tell me where I should add it. Thanks 😊

@william-tran
Copy link

Here here, I'm hoping we get the ability to define our own roles, this would save us a lot of work in writing our own proxy with auth etc.

@jwntrs
Copy link
Contributor Author

jwntrs commented Sep 19, 2018

@Rukenshia @william-tran our first pass at RBAC is going to have static roles, but we have started thinking about what dynamic roles might look like.

The thought (similar to what @Rukenshia mentioned) is that down the road you'll be able to map specific actions to whatever you want, but our initial action/role mapping will probably look something like the following:

	atc.DestroyTeam:                   "admin",
	atc.RenameTeam:                    "admin",
	atc.SetTeam:                       "admin",
	atc.AbortBuild:                    "member",
	atc.CheckResource:                 "member",
	atc.CheckResourceType:             "member",
	atc.CheckResourceWebHook:          "member",
	atc.ClearTaskCache:                "member",
	atc.CreateBuild:                   "member",
	atc.CreateJobBuild:                "member",
	atc.CreatePipelineBuild:           "member",
	atc.DeletePipeline:                "member",
	atc.DeleteWorker:                  "member",
	atc.DisableResourceVersion:        "member",
	atc.EnableResourceVersion:         "member",
	atc.ExposePipeline:                "member",
	atc.HeartbeatWorker:               "member",
	atc.HidePipeline:                  "member",
	atc.HijackContainer:               "member",
	atc.LandWorker:                    "member",
	atc.OrderPipelines:                "member",
	atc.PauseJob:                      "member",
	atc.PausePipeline:                 "member",
	atc.PauseResource:                 "member",
	atc.PruneWorker:                   "member",
	atc.ReadOutputFromBuildPlan:       "member",
	atc.RegisterWorker:                "member",
	atc.RenamePipeline:                "member",
	atc.ReportWorkerContainers:        "member",
	atc.ReportWorkerVolumes:           "member",
	atc.RetireWorker:                  "member",
	atc.SaveConfig:                    "member",
	atc.SendInputToBuildPlan:          "member",
	atc.SetLogLevel:                   "member",
	atc.UnpauseJob:                    "member",
	atc.UnpausePipeline:               "member",
	atc.UnpauseResource:               "member",
	atc.BuildEvents:                   "viewer",
	atc.BuildResources:                "viewer",
	atc.DownloadCLI:                   "viewer",
	atc.GetBuild:                      "viewer",
	atc.GetBuildPlan:                  "viewer",
	atc.GetBuildPreparation:           "viewer",
	atc.GetConfig:                     "viewer",
	atc.GetContainer:                  "viewer",
	atc.GetInfo:                       "viewer",
	atc.GetInfoCreds:                  "viewer",
	atc.GetJob:                        "viewer",
	atc.GetJobBuild:                   "viewer",
	atc.GetLogLevel:                   "viewer",
	atc.GetPipeline:                   "viewer",
	atc.GetResource:                   "viewer",
	atc.GetResourceCausality:          "viewer",
	atc.GetResourceVersion:            "viewer",
	atc.GetVersionsDB:                 "viewer",
	atc.JobBadge:                      "viewer",
	atc.ListAllJobs:                   "viewer",
	atc.ListAllPipelines:              "viewer",
	atc.ListAllResources:              "viewer",
	atc.ListBuilds:                    "viewer",
	atc.ListBuildsWithVersionAsInput:  "viewer",
	atc.ListBuildsWithVersionAsOutput: "viewer",
	atc.ListContainers:                "viewer",
	atc.ListDestroyingContainers:      "viewer",
	atc.ListDestroyingVolumes:         "viewer",
	atc.ListJobBuilds:                 "viewer",
	atc.ListJobInputs:                 "viewer",
	atc.ListJobs:                      "viewer",
	atc.ListPipelineBuilds:            "viewer",
	atc.ListPipelines:                 "viewer",
	atc.ListResourceTypes:             "viewer",
	atc.ListResourceVersions:          "viewer",
	atc.ListResources:                 "viewer",
	atc.ListTeamBuilds:                "viewer",
	atc.ListTeams:                     "viewer",
	atc.ListVolumes:                   "viewer",
	atc.ListWorkers:                   "viewer",
	atc.MainJobBadge:                  "viewer",
	atc.PipelineBadge:                 "viewer",


or this:

### ✅ Option 2 - roles map

This comment was marked as spam.

@elgohr
Copy link

elgohr commented Dec 20, 2018

I'm missing technical users in the discussion. We often do pipe of pipes. In this way there would be the need for a "technical member". I don't know whether this would make a difference to the normal member.
I'm thinking of something like Github Deploy Keys.

Joshua Winters added 9 commits January 18, 2019 11:02
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Joshua Winters and others added 8 commits January 18, 2019 11:03
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Joshua Winters <jwinters@pivotal.io>
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
* be clearer about what's current behavior vs. proposed behavior
* document migration scheme
* don't mention admin stuff as it's not strictly related (it's not
  represented as a role)

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@vito
Copy link
Member

vito commented Jan 18, 2019

Hi all, we think this is good to go at this point and I'm going to merge it in (as our first merged RFC!)

This is jumping the gun on the normal RFC process a bit because this RFC predates the solidification of the process itself. Normally we would go through a more extended review before accepting it, however the implementation for this has already been done in master for months, so there's not much point.

There are a couple outstanding questions left but I think those can come up as later issues to improve on top of the RBAC work established by this RFC and initial implementation. Thanks to everyone that took part in this!

@vito vito merged commit 6a663f2 into concourse:master Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants