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

Add rules to the config repo #7605

Closed
kritika-singh3 opened this issue Jan 14, 2020 · 34 comments
Closed

Add rules to the config repo #7605

kritika-singh3 opened this issue Jan 14, 2020 · 34 comments

Comments

@kritika-singh3
Copy link
Contributor

kritika-singh3 commented Jan 14, 2020

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?

More questions based on comments

  • Will there be a need to modify RBAC for config-repo?

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
@kritika-singh3 kritika-singh3 added this to the NextUp milestone Jan 14, 2020
@arvindsv
Copy link
Member

cc: @tomzo FYI (for context: #3636 (comment))

@arvindsv
Copy link
Member

To your questions:

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

  2. 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.

  3. Yes, I think preflight should be as close to the real thing as possible.

@kritika-singh3
Copy link
Contributor Author

kritika-singh3 commented Jan 14, 2020

  1. 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
Copy link
Member

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
Copy link
Contributor Author

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

@tomzo
Copy link
Member

tomzo commented Jan 14, 2020

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
Copy link
Contributor Author

@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
Copy link
Member

Can refer imply create?

@arvindsv
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

tomzo commented Jan 14, 2020

Can refer imply create?

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

@kritika-singh3
Copy link
Contributor Author

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
Copy link
Member

arvindsv commented Jan 14, 2020

Let's consider a couple of scenarios:


  1. 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>

  1. 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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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

@arvindsv
Copy link
Member

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
Copy link
Contributor

@adityasood Is my understanding correct?

@kritika-singh3 yes that is what I meant.

@maheshp
Copy link
Contributor

maheshp commented Jan 14, 2020

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

kaushik-navi commented Jan 15, 2020

@maheshp @kritika-singh3 good points

Pros and Cons of both options:

  1. 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

  2. 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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

maheshp commented Jan 17, 2020

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
Copy link
Member

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
Copy link
Contributor

maheshp commented Jan 17, 2020

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
Copy link
Member

Ok, I can live with that.

@kritika-singh3
Copy link
Contributor Author

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

@maheshp
Copy link
Contributor

maheshp commented Mar 16, 2020

Closing this issue since Rajiesh has tested the PR's

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
20.2.0
  
QA Done
Development

No branches or pull requests

6 participants