Skip to content

Conversation

@evanchaoli
Copy link
Contributor

Signed-off-by: Chao Li chaol@vmware.com

@evanchaoli evanchaoli changed the title Add RFC#41: OPA integration proposal. RFC: OPA integration proposal. Nov 20, 2019
Signed-off-by: Chao Li <chaol@vmware.com>
Signed-off-by: Chao Li <chaol@vmware.com>
Copy link
Member

@jamieklassen jamieklassen left a comment

Choose a reason for hiding this comment

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

For me as a native English speaker, when I hear "white list" I imagine unfettered access to a privilege, whereas in this document "white list" means certain API requests will always be policy-checked (and potentially rejected). Similarly "black list" usually means I am forbidden or banned from a privilege, whereas in this document an API action being black listed actually means it gets to "skip the line" and never get policy checked.

I like the phrase "filter HTTP methods", so maybe analogously we could say "filter action" and then "filter action skip" for the actions that never get checked?

Signed-off-by: Chao Li <chaol@vmware.com>
@evanchaoli
Copy link
Contributor Author

@pivotal-jamie-klassen Thanks for the suggesting regarding to naming. I have updated the doc based on your suggestion.


## Policy check points

* All API calls, for example `set-pipeline`, will also go through policy checks.
Copy link
Member

Choose a reason for hiding this comment

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

What happens when an API request is rejected by the policy manager? Does the API return a certain response (status code or body)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Http code 403 (forbidden) should be returned.

## Policy check points

* All API calls, for example `set-pipeline`, will also go through policy checks.
* Because Concourse supports task file, and task file is only fetched at runtime.
Copy link
Member

Choose a reason for hiding this comment

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

What happens when a task step is rejected by the policy manager? Does the step error with a message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for late update. I have changed RunTask to UsingImage in the implementation. Just updated the RFC. If UsingImage doesn't pass policy check, it should return an error PolicyCheckNotPass from function Run of step check/get/put/task.

## Support multiple policy managers

Like how multiple credential managers are supported, Concourse should allow other
policy managers than OPA.
Copy link
Member

Choose a reason for hiding this comment

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

This sounds reasonable, but I think the RFC should show how this would work or leave it as an open question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a description.


## OPA configuration

* CONCOURSE_OPA_URL - URL of OPA service including path
Copy link
Member

Choose a reason for hiding this comment

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

Do you see this as something that should only be configured cluster-wide? Or would it make sense to you to have this configured on a per-team basis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For our use cases, we don't need per-team OPA. If a team/pipeline wants to use their own OPA, an OPA resource type can be used.

Signed-off-by: Chao Li <chaol@vmware.com>

* All API calls, for example `set-pipeline`, will also go through policy checks. If
the check doesn't pass, API should return HTTP code 403 (forbidden).
* `UsingImage` action will be sent to OPA before Concourse launches a container in
Copy link

@aoldershaw aoldershaw May 30, 2020

Choose a reason for hiding this comment

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

I can also imagine wanting to prevent the use of privileged: true for tasks (perhaps even on a per-team basis), which also feels like it should be handled as some PolicyCheck (and probably OPA?).

This makes me wonder: are there any other things besides images (and privileged) that operators would be interested in checking against OPA? The answer may just be no, but for arguments sake, what if Concourse didn't try to be opinionated about what actions to check against, and instead supported 4 actions beyond the API actions: Check, Get, Put, and Task. The data field for each action would be populated with the entire step definition.

Granted, this would mean that if all you cared about was the image used, you'd need 4 identical policy check definitions - but, by it's nature, this also affords operators more flexibility in what kind of checks they perform. This may be overkill depending on the use cases in practice, though. However, it does feel more similar to the approach taken for API endpoints (being unopinionated and sending the entire body of the API call).

…edaction.

Signed-off-by: Chao Li <chaol@vmware.com>
@marco-m-pix4d
Copy link

Hello @evanchaoli,
what is the status of this RFC wrt the merged code in concourse/concourse#5034?
should this RFC be merged as done or are there missing parts?

@anderseknert
Copy link

anderseknert commented Feb 11, 2021

Came across this by random and it looks nice! Planning to add this to the OPA ecosystem page. One thing I couldn't find on the Concourse site was docs around how to integrate with OPA. I am not too familiar with the project so might just be I'm not looking in the right places. Could anyone help me out? :)

@marco-m-pix4d
Copy link

@anderseknert the only docs I am aware of are in the PR description, see concourse/concourse#5034 (comment)

@marco-m-pix4d
Copy link

@evanchaoli ping regarding #41 (comment)

@anderseknert
Copy link

Thanks @marco-m-pix4d 👍 Hoping that gets added to the docs then so that I can point to this as a good example of OPA in CI/CD integrations.

@clarafu clarafu removed their assignment May 16, 2022
@taylorsilva
Copy link
Member

Merging as the basic implementation is there and is functional for users today.

@taylorsilva taylorsilva merged commit 184d23e into concourse:master Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.