Add rules to the config repo

kritika-singh3 commented 4 years ago

Issue Type

Feature enhancement

Summary

Epic Issue: #5175 (search for Better authorization), #3636

Proposal: Add Rules to the config repo to allow/deny creation/access to environments, pipeline groups and pipelines.

The rules defined for that config repo will govern what all entities can be added/referred to the GoCD. Proposed actions are:

Action	Entity Type	Description
create	environment/pipeline_group/pipeline	allow/deny ability to create a new environment/pipeline group/pipeline with the specified pattern
refer	environment	allow/deny pipelines(and agents?) to be added to the specified environment
refer	pipeline_group	allow/deny pipelines to be added to the specified pipeline group
refer	pipeline	allow/deny the specified pipeline to be used as an upstream dependency

Note: Deny rule will take precedence.

Example of the config:

<config-repo pluginId="json.config.plugin" id="json">
    <git url="/tmp/config-repo" />
    <rules>
        <allow action="refer" type="environment">env_*</allow>
        <allow action="create" type="pipeline_group">grp_*</allow>
      </rules>
</config-repo>

Need to decide:

Should the agents addition to environment be restricted as well?
Will the default rule to be deny all? The existing GoCD installations using config-repos may break if so. (Maybe we can do a config migration to get around this?)
Should we consider the rules while checking the definitions (via repoId) on preflight API?

Conclusion

Possible actions

Action	Entity Type	Description
refer	environment	allow/deny pipelines and agents to be added to the specified environment
refer	pipeline_group	allow/deny pipelines to be added to the specified pipeline group
refer	pipeline	allow/deny the specified pipeline to be used as an upstream dependency

Additional Info

deny rule will take precedence.
Should the agents addition to environment be restricted as well?: Yes
Will the default rule to be deny all? The existing GoCD installations using config-repos may break if so. (Maybe we can do a config migration to get around this?): Yes, the default rule will be deny. Will consider a config migration to add a default allow all entity.
Should we consider the rules while checking the definitions (via repoId) on preflight API?: Yes
Will there be a need to modify RBAC for config-repo?: No, since it is an option that has to be explicitly given, no need to make any change in RBAC

arvindsv commented 4 years ago

cc: @tomzo FYI (for context: https://github.com/gocd/gocd/issues/3636#issuecomment-313439556)

arvindsv commented 4 years ago

To your questions:

Yes, I think agent association should be restricted too (using the same action + type combination).
I think the default rule should be deny, with a config migration making every existing one allow *. I'm conflicted about this and can see reasons to make this default to allow.
Yes, I think preflight should be as close to the real thing as possible.

kritika-singh3 commented 4 years ago

Yes, I think agent association should be restricted too (using the same action + type combination).

If we go down this path, the combination refer + agent will make sense, but create + agent won't. Will that we ok?

arvindsv commented 4 years ago

I meant:

<allow action="refer" type="environment">env_*</allow>

... should apply to agents too, right? I don't think type="agent" is necessary. Agents can only refer to an environment. You can't really create them (I guess you can provide a UUID of a non-existing agent).

kritika-singh3 commented 4 years ago

@arvindsv Yes, we can only specify agents UUID. So the refer + env will be applied to both pipelines and agents.

tomzo commented 4 years ago

I think the default rule should be deny, with a config migration making every existing one allow *. I'm conflicted about this and can see reasons to make this default to allow

I agree on that part. The other option is allowing explicitly every resource that was created there. Which would costly to implement because it would require to parse config repos first and then migrate the xml.

I not sure if the create pipeline_group rule is meaningful. A pipeline group cannot be created alone via the config-repos, it always has to be created by referencing it in a pipeline.

Consider what's the difference for user in these cases:

case 1

allow create mygroup
allow reference mygroup

With config repo:

pipelines:
  mypipe:
    group: mygroup

case 2

Then what's the benefit being able to configure this?

deny create mygroup
allow reference mygroup

With config repo:

pipelines:
  mypipe:
    group: mygroup

kritika-singh3 commented 4 years ago

@tomzo Correct, the pipeline group will be created via reference only. But if the said group does not already exist in GoCD, we explicitly use to create one.

Hence, the deny create pipeline_group will disallow creation of such new pipeline groups and deny refer pipeline_group will disallow adding new pipelines in an existing pipeline group.

So, in the case 2 above, the allow refer grp rule will come into effect only if the said grp is already present, otherwise, we'll check for create rule.

The same goes for environment as well.

arvindsv commented 4 years ago

Can refer imply create?

arvindsv commented 4 years ago

I mean: Does create really give us any more control? If there is an allow refer to environment env1, does it matter if env1 exists or would be created by this config-repo?

kritika-singh3 commented 4 years ago

If a team/project follows certain pattern that their environments/pipeline groups/pipelines must adhere to, then maybe yes, a create rule might give the users a bit more control.

adityasood commented 4 years ago

What happens when there are two pipelines in the same group pg_1. What is the minimum permission required to create and then refer a pipeline-group?

tomzo commented 4 years ago

Can refer imply create?

That was my point. I think, it's more intuitive for user.

kritika-singh3 commented 4 years ago

Multiple config-repos can add pipelines to the same group. So taking in the points suggested above, maybe we can remove create rule. So the updated actions would look like:

Action	Entity Type	Description
refer	environment	allow/deny pipelines and agents to be added to the specified environment
refer	pipeline_group	allow/deny pipelines to be added to the specified pipeline group
refer	pipeline	allow/deny the specified pipeline to be used as an upstream dependency

So, with this table in mind, the min. permission for scenario raised by @adityasood would be allow refer pipeline_group. But now there is no control on the name of newly added env/grp/pipeline. Will that be ok?

arvindsv commented 4 years ago

Let's consider a couple of scenarios:

An org wants to separate teams within the same GoCD instance, so that they have full control over their entities, but don't / can't overwrite each others' entities. So:
- Want config-repo with name team1 to only be able to control pipeline groups named team1_pg_1 and team1_pg_2. Doesn't matter if the groups don't exist (in config XML).
- Want config-repo with name team1 to only be able to refer to and control environments named team1_env_1 and team1_env_2. Doesn't matter if the environments don't exist (in config XML).
- Want config-repo with name team1 to be able to refer to (and fetch artifacts from) a pipeline named common.

This is possible using:

<allow action="refer" type="environment">team1_env_*</allow>
<allow action="refer" type="pipeline_group">team1_pg_*</allow>
<allow action="refer" type="pipeline">common</allow>

Same as (1) but the org also wants to force pipeline names defined in the repo to follow a format such as team1_pipeline_1, team1_our_other_pipeline, etc. (I'm not saying this is a good idea, but thinking about it).

This isn't possible currently since the refer for pipeline only works for pipeline dependency references.

What other scenarios can we think of, that might be useful?

arvindsv commented 4 years ago

So, with this table in mind, the min. permission for scenario raised by @adityasood would be allow refer pipeline_group.

What does that mean? I'm not sure I understood @adityasood's question.

But now there is no control on the name of newly added env/grp/pipeline. Will that be ok?

Doesn't the refer provide control over the name / pattern?

kritika-singh3 commented 4 years ago

But now there is no control on the name of newly added env/grp/pipeline. Will that be ok?

Doesn't the refer provide control over the name / pattern?

What I meant was allow refer pipeline can only provide control over one thing - which pipeline can be referred as an upstream dependency. The same rule can't be used to control the name of the newly created pipeline.

kritika-singh3 commented 4 years ago

So, with this table in mind, the min. permission for scenario raised by @adityasood would be allow refer pipeline_group.

What does that mean? I'm not sure I understood @adityasood's question.

What I understood was that there are two pipelines which are in same group coming from config-repos. Now these two can either come from the same config-repo or different. If a single repo is there, a create rule would be sufficient. But if two repos are there, since, we don't know in which order the repos would be parsed, the create rule would be applied for one and refer for another. Hence, for both the repos, both the rules needed to be present.

@adityasood Is my understanding correct?

arvindsv commented 4 years ago

The same rule can't be used to control the name of the newly created pipeline.

Ah, yes. Maybe we're missing a: allow define pipeline. I think it's the same as (2) in my comment above, right?

arvindsv commented 4 years ago

I like to think of it as define rather than create, because it doesn't really get created every time it the repo is parsed. It's only about whether we allow that config-repo to define that pipeline or not.

kritika-singh3 commented 4 years ago

Agreed. define makes sense over create. But then question is allow define environment and allow define pipeline_group will not make sense. Right?

arvindsv commented 4 years ago

Right. Unlike pipeline, those two can be defined in either place (config XML or config repo). And they'll be merged together. We can just say: refer implies define for them.

adityasood commented 4 years ago

@adityasood Is my understanding correct?

@kritika-singh3 yes that is what I meant.

maheshp commented 4 years ago

I am just wondering if the rules should be defined on a config repo or on the entities the config repo refers to (environment, pipeline_group or pipeline).

With granular authorization, we can have a non System Admin with permission to create a config_repo. Now, this user can create a config repo and define rules which grants access to refer to any environment or pipeline_group. Am I thinking right?

arvindsv commented 4 years ago

Yes, you're thinking right (and what you say will be possible), but trying to prevent that will mean that the permissions will be much harder to understand. We might have to knowingly allow this to happen.

Also, it's about referring. You cannot access (or administer) a pipeline that is already defined. However, I know that it will lead to escaping the permissions provideed to a non-system-admin.

kritika-singh3 commented 4 years ago

IMO, rules defined on the config-repos will give us a centralised control over what all can be modified by the entity in question. Adding rules on env and pipeline group, will be two separate places of edit/errors. Also, if in future we expand the scope of config-repo, we would need to add the rules on them as well.

For RBAC, I was thinking of introducing a separate action, which when granted, will give permission to edit rules. But this would require that the rules declaration/modification should be via a different endpoint rather than using existing config-repo creation/modification endpoints.

kaushik-navi commented 4 years ago

@maheshp @kritika-singh3 good points

Pros and Cons of both options:

Keeping rules at entities like PipelineGroups: + It goes with user/role permissions that are already defined at the entities - It's hard to define rules like config-repo-1/elastic-agent-profile-1 should only create repos in pipeline-group-1. Because we have to edit all other pipeline groups to deny config-repo-1 access
Keeping rules at config repo / elastic agent profiles + centralised control over what all can be modified by the entity - It's hard to define rules like pipeline-group-1 can only be modified by config-repo-1/elastic-agent-profile-1. Because non-system-admins can create entities that by pass this rule

Is there an option to restrict non-system-admins from creating config repos and elastic agent profiles? Then we can go with option2 IMO adding config repos and elastic agents can be a system admin task, doesn't change as often as pipelines in the config repos

@arvindsv wdyt?

arvindsv commented 4 years ago

Is there an option to restrict non-system-admins from creating config repos and elastic agent profiles? Then we can go with option2

Yes. It is restricted to system admins only. However, with recent improvements to granular auth (see "Improvements to Role Based Access Control" in https://www.gocd.org/releases/#19-12-0), it is possible to delegate access to administer config repos to non-system-admins as well. So, it is an option.

My opinion is that: Since it is an option to provide access to non-system-admins, we can go with option 2 in your comment above and keep it at the config repo.

kritika-singh3 commented 4 years ago

Summarizing:

rules will be added to the config repo to allow/deny creation/access to environments, pipeline groups and pipelines.

The rules defined for that config repo will govern what all entities can be added/referred to the GoCD. Proposed actions are:

Action	Entity Type	Description
refer	environment	allow/deny pipelines and agents to be added to the specified environment (same for define)
refer	pipeline_group	allow/deny pipelines to be added to the specified pipeline group (same for define)
define	pipeline	allow/deny ability to create a pipeline with the specified pattern
refer	pipeline	allow/deny the specified pipeline to be used as an upstream dependency

Note: deny rule will take precedence.

Example of the config:

<config-repo pluginId="json.config.plugin" id="json">
    <git url="/tmp/config-repo" />
    <rules>
        <allow action="refer" type="environment">env_*</allow>
        <allow action="define" type="pipeline">teamA_*</allow>
        <allow action="refer" type="pipeline">common_*</allow>
      </rules>
</config-repo>

Should the agents addition to environment be restricted as well?: Yes
Will the default rule to be deny all? The existing GoCD installations using config-repos may break if so. (Maybe we can do a config migration to get around this?): Yes, the default rule will be deny. Will consider a config migration to add a default allow all entity.
Should we consider the rules while checking the definitions (via repoId) on preflight API?: Yes
Will there be a need to modify RBAC for config-repo?: No, since it is an option that has to be explicitly given, I don't think we need to make any change in RBAC

Does this need any change?

maheshp commented 4 years ago

Same as (1) but the org also wants to force pipeline names defined in the repo to follow a format such as team1_pipeline_1, team1_our_other_pipeline, etc. (I'm not saying this is a good idea, but thinking about it).

@kritika-singh3 @arvindsv I believe we are adding a new action ~deny~ define to support the above use case. What will happen if a System Admin creates a config_repo without a rule to define pipelines, do we allow any pipelines to be defined or disallow it since the default rule is to deny. I think this can be confusing to users, and most of the use cases we have seen so far has been to namespace a pipeline_group or an environment. I would probably not add support for define.

arvindsv commented 4 years ago

a new action deny

You mean define, right?

It's true that it can be confusing if it's not there. But then, without it, we can't support scenario (2) (in that comment) and allow namespacing within a pipeline group. Is that ok?

maheshp commented 4 years ago

Yeah I meant define.

I think it is ok for now to not allow namespacing within a pipeline group. Based on specific use cases we can add it later, probably that might require a config migration to support default behaviour.

arvindsv commented 4 years ago

Ok, I can live with that.

kritika-singh3 commented 4 years ago

Great!!! I have updated the issue description with the conclusion.

maheshp commented 4 years ago

Closing this issue since Rajiesh has tested the PR's

gocd / gocd