Proposal: enhance pluggable policy

[deads2k]

This describes modifications to the current authorization APIs to make policy more powerful and easier to extend. This document aligns with OpenShift authorization.

@erictune ptal

@liggitt @smarterclayton @derekwaynecarr fyi

[deads2k]

/cc @preillyme @uruddarraju

[k8s-bot]

GCE e2e build/test passed for commit bb27420cb573818474f1898c77c800808cd9b3a3.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[smarterclayton]

Maybe split this into a second interface which is optionally provided.

[deads2k]

Maybe split this into a second interface which is optionally provided.

GetAllowedSubjects is a pretty fundamental operation. Without the ability to know who can perform an operation, proper auditing of authorization policy is an impossibility. Why would we ever want to allow someone to create an authorizer that couldn't be audited?

[smarterclayton]

Because you don't need audit? Unless every call site needs both methods
(and they don't) I would organize these so that there are two fundamental
interfaces, and if at the highest level you require both, require both.

On Thu, Aug 6, 2015 at 9:30 AM, David Eads notifications@github.com wrote:

In docs/design/enhance-pluggable-policy.md
#12209 (comment)
:

+// OLD
+type Authorizer interface {

• Authorize(a Attributes) error
  +}
  +`

+`
+// NEW
+type Authorizer interface {

• // Authorize takes a Context (for namespace, user, and traceability) and Attributes to make a policy determination.
• // reason is an optional return value that can describe why a policy decision was made.
• Authorize(ctx api.Context, a Attributes) (allowed bool, reason string, evaluationError error)

• // GetAllowedSubjects takes a Context (for namespace and traceability) and Attributes to determine which users and
• // groups are allowed to perform the described action in the namespace. This API enables the ResourceBasedReview requests below
• GetAllowedSubjects(ctx api.Context, a Attributes) (users util.StringSet, groups util.StringSet, evaluationError error)

Maybe split this into a second interface which is optionally provided.

GetAllowedSubjects is a pretty fundamental operation. Without the ability
to know who can perform an operation, proper auditing of authorization
policy is an impossibility. Why would we ever want to allow someone to
create an authorizer that couldn't be audited?

—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/12209/files#r36413537
.

Clayton Coleman | Lead Engineer, OpenShift

[deads2k]

Because you don't need audit? Unless every call site needs both methods
(and they don't) I would organize these so that there are two fundamental
interfaces, and if at the highest level you require both, require both.

So would we make ResourceAccessReview optional? It's essential for doing things like "which namespaces/projects can I see".

[smarterclayton]

I don't think ResourceAccessReview is required for all deployments or
authorizers. It is definitely useful, but not all consumers may choose to
invert the model (or even support inverting the model).

On Fri, Aug 7, 2015 at 11:38 AM, David Eads notifications@github.com
wrote:

In docs/design/enhance-pluggable-policy.md
#12209 (comment)
:

+// OLD
+type Authorizer interface {

• Authorize(a Attributes) error
  +}
  +`

+`
+// NEW
+type Authorizer interface {

• // Authorize takes a Context (for namespace, user, and traceability) and Attributes to make a policy determination.
• // reason is an optional return value that can describe why a policy decision was made.
• Authorize(ctx api.Context, a Attributes) (allowed bool, reason string, evaluationError error)

• // GetAllowedSubjects takes a Context (for namespace and traceability) and Attributes to determine which users and
• // groups are allowed to perform the described action in the namespace. This API enables the ResourceBasedReview requests below
• GetAllowedSubjects(ctx api.Context, a Attributes) (users util.StringSet, groups util.StringSet, evaluationError error)

Because you don't need audit? Unless every call site needs both methods
(and they don't) I would organize these so that there are two fundamental
interfaces, and if at the highest level you require both, require both.

So would we make ResourceAccessReview optional? It's essential for doing
things like "which namespaces/projects can I see".

—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/12209/files#r36531053
.

Clayton Coleman | Lead Engineer, OpenShift

[pmorie]

Nit: I think it's good to use backticks around code constructs

[pmorie]

extends the existing API

[deads2k]

First round comments done. Items not addressed:
1.

Maybe split this into a second interface which is optionally provided.

GetAllowedSubjects is a pretty fundamental operation. Without the ability to know who can perform an operation, proper auditing of authorization policy is an impossibility. Why would we ever want to allow someone to create an authorizer that couldn't be audited?

We may want to make this a SubjectAccessResponse or unify the struct between request and response - action being one nested struct and response being the other (or just use status).

We considered spec/status before, but it was rejected (#3730 (comment)). I like it, but I'd like to hear from @erictune before adding it back in.

[k8s-bot]

GCE e2e build/test passed for commit 2ac69abb207fe6317fa61844e111f515bab832d4.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[smarterclayton]

On Thu, Aug 6, 2015 at 9:39 AM, David Eads notifications@github.com wrote:

First round comments done. Items not addressed:

Maybe split this into a second interface which is optionally provided.

GetAllowedSubjects is a pretty fundamental operation. Without the ability
to know who can perform an operation, proper auditing of authorization
policy is an impossibility. Why would we ever want to allow someone to
create an authorizer that couldn't be audited?

We may want to make this a SubjectAccessResponse or unify the struct
between request and response - action being one nested struct and response
being the other (or just use status).

We considered spec/status before, but it was rejected (#3730 (comment)
#3730 (comment)).
I like it, but I'd like to hear from @erictune
https://github.com/erictune before adding it back in.

It's worth discussing whether the world has changed since then - it's
easier to reason about one object (a POST that materializes the status of
the incoming request) than two in client code, and when I get a response I
like to also see what I requested.

[k8s-github-robot]

Labelling this PR as size/L

[erictune]

I'm on vacation this week (:house: :mouse:) and will take a look next week.

[deads2k]

Have we had any serious disagreement to this proposal so far? I haven't

I haven't seen disagreement. AccessReview and AccessReviewResponse versus spec/status is still an open question, but its not a significant implementation change either way.

[smarterclayton]

Hrm... spec/status definitely fits with our other models.

[liggitt]

yes... though when you "create" a review, you aren't actually persisting anything, so it feels little silly to echo back your request

[deads2k]

I've updated the available attributes to remove content. This forces us to have a PersonalSubjectAccessReview. I have updated the Access Review sections to reflect the groups and the different options since a single Kind cannot be used for multiple Resources and a single Kind cannot be both namespaced and non-namespaced.

[erictune]

I think this is an API. It takes an API type, we want to register it in the API server, it should live in an API group, and we want to protect it with authorization as an API. Making it an API also makes it very easy and consistent to deal with.

Just thinking out loud here:

• We have DeleteOptions which is defined as an API type, and lives in an API group, but which is passed via query parameters rather than as a body. We could use query parameters from a API type on a non-resource Path.
• nonResourcePaths can be authorized as well, as in Add non-resource-request support to ABAC, fix kubectl for namespaced users #16148.

[k8s-bot]

GCE e2e test build/test passed for commit b5a4f1fb49f8be8f69436c71964b7acb30dab08c.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-bot]

GCE e2e test build/test passed for commit 932e3e2c55aeec2b49274b3d570707b206d30e24.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-bot]

GCE e2e test build/test passed for commit c51393b8f2cb8c713b0e15d0b33ecdf8d1b9bfd4.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[deads2k]

Just thinking out loud here:

We have DeleteOptions which is defined as an API type, and lives in an API group, but which is passed via query parameters rather than as a body. We could use query parameters from a API type on a non-resource Path.
nonResourcePaths can be authorized as well, as in #16148.

I think that we'll end up happier with API resources. Using resources makes concepts like namespace level protection for the LocalSAR very easy to reason about and inspect in all the authorizers I've seen so far (abac file, keystone, o. When writing rules for an authorizer, the structure provided by the API resources and divisions provided by namespaces are very easy for users to understand. Attempting to write similar non-resource URLs rules would be more difficult.

[smarterclayton]

Query parameters are usually something you use to change a query. Having a
formal body makes it easier to generate client side structures, but we can
always support a variant in the future that is a HEAD or GET. Either way,
we what a versioned response.

On Nov 25, 2015, at 9:57 AM, David Eads notifications@github.com wrote:

Just thinking out loud here:

We have DeleteOptions which is defined as an API type, and lives in an API
group, but which is passed via query parameters rather than as a body. We
could use query parameters from a API type on a non-resource Path.
nonResourcePaths can be authorized as well, as in #16148
#16148.

I think that we'll end up happier with API resources. Using resources makes
concepts like namespace level protection for the LocalSAR very easy to
reason about and inspect in all the authorizers I've seen so far (abac
file, keystone, o. When writing rules for an authorizer, the structure
provided by the API resources and divisions provided by namespaces are very
easy for users to understand. Attempting to write similar non-resource URLs
rules would be more difficult.

—
Reply to this email directly or view it on GitHub
#12209 (comment)
.

[erictune]

I talked to @bgrant0607. He doesn't have a problem in principle with the subjectAccessReview style of endpoint but would like some time to think about the conventions for such a resource.

Can you proceed with your implementation of /apis/authorization.kubernetes.io/{version}/ns/{namespace}/subjectAccessReview with version v1beta1 but be open to changing the naming or path structure before going to v1?

[deads2k]

Can you proceed with your implementation of /apis/authorization.kubernetes.io/{version}/ns/{namespace}/subjectAccessReview with version v1beta1 but be open to changing the naming or path structure before going to v1?

Yes. As soon as API Groups seem stable enough to build on top of (see #16173 and #17216 for ongoing work), I'll add it.

[erictune]

LGTM

[wojtek-t]

@deads2k

FAIL: changes needed but not made due to --verify
/home/travis/gopath/src/github.com/kubernetes/kubernetes/docs/ is out of date. Please run hack/update-generated-docs.sh

Can you please fix it?

[k8s-github-robot]

PR changed after LGTM, removing LGTM.

[k8s-github-robot]

Automatic merge from submit-queue

[k8s-bot]

GCE e2e build/test failed for commit 17d3db1.

• Build Log
• Test Artifacts
• Internal Jenkins Results