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

Fix kubectl for namespaced users #13667

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@mkulke
Contributor

mkulke commented Sep 8, 2015

This is an attempt at fixing #13097. Currently users locked in a namespace by the ABAC authorization module cannot use kubectl at all, since kubectl does a version negotiation using /api which is not covered by the ABAC ontologies. So I ended up creating a MetaResource type, representing "/api" and "/healthz" currently. When a user has set "kubectl: true" as a property in the authorization policy file, calls to "/api" by that user will be allowed.

@googlebot

This comment has been minimized.

googlebot commented Sep 8, 2015

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Sep 8, 2015

@k8s-bot

This comment has been minimized.

k8s-bot commented Sep 8, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Sep 8, 2015

Labelling this PR as size/L

@mkulke

This comment has been minimized.

Contributor

mkulke commented Sep 8, 2015

Signed the CLA, the company name is "thinkstep AG".

@thockin thockin assigned erictune and unassigned thockin Sep 8, 2015

@ncdc

This comment has been minimized.

Member

ncdc commented Sep 8, 2015

@@ -85,6 +85,7 @@ Each line is a "policy object". A policy object is a map with the following pro
operations.
- `resource`, type string; a resource from an URL, such as `pods`.
- `namespace`, type string; a namespace string.
- `kubectl`, type boolean; when true, the user is allowed to access the /api endpoint (required for kubectl).

This comment has been minimized.

@liggitt

liggitt Sep 8, 2015

Member

We shouldn't start adding special cases to these attributes. I would be ok with a nonResourceURL or nonResourcePath attribute, so policy could address non-API things.

This comment has been minimized.

@liggitt

liggitt Sep 8, 2015

Member

Also, we don't want policy being oriented to the "user agent", we want it oriented to the operation being done

@@ -378,6 +378,16 @@ func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attrib
// in empty (does not understand defaulting rules.)
attribs.Namespace = apiRequestInfo.Namespace
// If the raw path is just a single item, the request is to a special resource.
if len(apiRequestInfo.Raw) == 1 {

This comment has been minimized.

@liggitt

liggitt Sep 8, 2015

Member

I'd rather see something like this:

if len(attributes.Resource) == 0 && len(attribs.Namespace) == 0 {
  attributes.nonResourcePath = req.URL.Path
}
// If the raw path is just a single item, the request is to a special resource.
if len(apiRequestInfo.Raw) == 1 {
switch apiRequestInfo.Raw[0] {
case "api":

This comment has been minimized.

@liggitt

liggitt Sep 8, 2015

Member

I don't want to special case non-resource urls here... what about /swaggerapi, /, etc? Ideally, policy would be able to address all of those things consistently

ReadOnly bool
Namespace string
Resource string
MetaResource MetaResource

This comment has been minimized.

@liggitt

liggitt Sep 8, 2015

Member

@deads2k thoughts on preferred terminology for a field in authorizer.Attributes to convey a non-resource path?

This comment has been minimized.

@deads2k

deads2k Sep 8, 2015

Contributor

In OpenShift, we settled on NonResourcePath (the path for the request) and Verb. This allowed us to express things like, "you can do GETs on /healthz and /healthz/*".

Enumerating every available endpoint is a losing battle once we you want to subdivide things. For example, what if we decided to subdivide health to support individual component health. It also doesn't really buy much control since anyone wanting to write such a policy it has to know exactly they want anyway.

This comment has been minimized.

@smarterclayton

smarterclayton Sep 9, 2015

Contributor

Do we have a time period to upstream this?

type MetaResource int
const (
ApiVersions MetaResource = 1 << iota

This comment has been minimized.

@deads2k

deads2k Sep 8, 2015

Contributor

I would recommend against enumerating constants for policy like this. It makes you sensitive to any refactoring order change and precludes reasonable extension by any downstream component. String constants will avoid those problems.

@mkulke

This comment has been minimized.

Contributor

mkulke commented Sep 8, 2015

Makes sense. If we want to handle this in a more generic fashion, this could be simplified further:

{"user":"alice", "readonly": true, "customEndpoint": "/api"}
{"user":"chuck", "customEndpoint": "/healthz"}

URL.Path is carried along in the authorizer attribs and the first thing which is evaluated, customEndpoint != "" would override any Namespace & Resource setting.

I'll fix documentation when you guys think this approach works.

@liggitt

This comment has been minimized.

Member

liggitt commented Sep 9, 2015

I'd rather see a single rule allowing "/api" for all users, e.g.:

{"readonly": true, "nonResourcePath": "/api"}
@liggitt

This comment has been minimized.

Member

liggitt commented Sep 9, 2015

also, I think the attributes passed to an authorizer should either contain resource info (namespace, resource, resource name, etc) or a request path (for non-resource requests), not both. Otherwise, we'll end up with authorizers that make path-based authorization decisions on API resources, and break when a new version of the API changes the path structure used to address the same API resource

@@ -378,6 +378,9 @@ func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attrib
// in empty (does not understand defaulting rules.)
attribs.Namespace = apiRequestInfo.Namespace
// We store the full path as well for custom endpoints
attribs.Path = req.URL.Path

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

I think this should only be set when Namespace/Resource are empty

Readonly bool `json:"readonly,omitempty"`
Resource string `json:"resource,omitempty"`
Namespace string `json:"namespace,omitempty"`
CustomEndpoint string `json:"customEndpoint,omitempty"`

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

just call it path or nonResourcePath?

@kargakis

This comment has been minimized.

Member

kargakis commented Sep 9, 2015

@kubernetes/kubectl

@mkulke

This comment has been minimized.

Contributor

mkulke commented Sep 9, 2015

also, I think the attributes passed to an authorizer should either contain resource info (namespace, resource, resource name, etc) or a request path (for non-resource requests), not both. Otherwise, we'll end up with authorizers that make path-based authorization decisions on API resources, and break when a new version of the API changes the path structure used to address the same API resource

ok, when we explicitly exclude the paths which fall into a namespace/resource pattern, it should be called nonResourcePath to highlight that you can't just put custom urls to resources in the policy file.

@@ -67,6 +67,8 @@ A request has 5 attributes that can be considered for authorization:
resource is the empty string.

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

change to "...the resource is the empty string, and the non resource path is populated instead"

@@ -108,12 +111,17 @@ If at least one line matches the request attributes, then the request is authori
To permit any user to do something, write a policy with the user property unset.
To permit an action Policy with an unset namespace applies regardless of namespace.
### Kubectl
Kubectl uses the `/api` endpoint of api-server to negotiate client/server versions. When users, who are restricted by namespaces, should be able to use the kubectl client, `/api` has to exempted from authorization checks via a `"nonResourcePath":"/api"` entry in the policy file (see example below).

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

has to be exempted

Readonly bool `json:"readonly,omitempty"`
Resource string `json:"resource,omitempty"`
Namespace string `json:"namespace,omitempty"`
NonResourcePath string `json:"nonResourcePath,omitempty"`

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

gofmt whitespace

if p.subjectMatches(a) {
switch {
case p.NonResourcePath != "":

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

subjectMatches and readonly checks still apply, move the nonResourcePath check to be a peer to the resource checking

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

subjectMatches returns true for all users if p.user and p.group are both empty

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

not sure what the behavior should be if you set both p.NonResourcePath and p.Namespace/p.Resource... that seems like an error... I think want a single thing allowed per policy rule

ReadOnly bool
Namespace string
Resource string
NonResourcePath string

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

gofmt whitespace

@@ -41,6 +41,9 @@ type Attributes interface {
// The kind of object, if a request is for a REST object.
GetResource() string
// In case a special endpoint like /api, /healthz has been called.
GetNonResourcePath() string

This comment has been minimized.

@liggitt

liggitt Sep 9, 2015

Member

document that either this or GetNamespace()/GetResource() return results, never both

@mkulke mkulke force-pushed the mkulke:fix_kubectl_for_namespaced_users branch from 9a249b5 to 5b6b9dd Sep 9, 2015

@mkulke

This comment has been minimized.

Contributor

mkulke commented Sep 9, 2015

I addressed the remarks and squashed a bunch of commits together, and pushed stuff again using my corporate email address (which I used in the CLA doc).

however that does not seem to impress the the red CLA badge, is there anything else I can do on my side?

@mkulke mkulke force-pushed the mkulke:fix_kubectl_for_namespaced_users branch from 8b2c42a to a43f260 Oct 7, 2015

@googlebot

This comment has been minimized.

googlebot commented Oct 7, 2015

CLAs look good, thanks!

@mikedanese mikedanese assigned liggitt and unassigned deads2k Oct 7, 2015

@bgrant0607 bgrant0607 modified the milestone: v1.1-candidate Oct 9, 2015

@@ -363,18 +376,24 @@ func (r *requestAttributeGetter) GetAttribs(req *http.Request) authorizer.Attrib
}
apiRequestInfo, _ := r.apiRequestInfoResolver.GetAPIRequestInfo(req)

This comment has been minimized.

@liggitt

liggitt Oct 22, 2015

Member

in master, GetAPIRequestInfo changed to GetRequestInfo, and now includes these attributes:

    // IsResourceRequest indicates whether or not the request is for an API resource or subresource
    IsResourceRequest bool
    // Path is the URL path of the request
    Path string
    // Verb is the kube verb associated with the request for API requests, not the http verb.  This includes things like list and watch.
    // for non-resource requests, this is the lowercase http verb
    Verb string

This comment has been minimized.

@liggitt

liggitt Oct 22, 2015

Member

I'd recommend copying those three attributes verbatim into authorizer.Attributes

// we can extract the resource. Otherwise, not.
attribs.Resource = apiRequestInfo.Resource
// Check whether meaningful api information can be resolved for the current path
if isAPIResourceRequest(r.apiRequestInfoResolver.APIPrefixes, req) {

This comment has been minimized.

@liggitt

liggitt Oct 22, 2015

Member

this can just become requestInfo.IsResourceRequest

@@ -201,6 +202,73 @@ func TestTimeout(t *testing.T) {
}
}
func TestIsAPIResourceRequest(t *testing.T) {

This comment has been minimized.

@liggitt

liggitt Oct 22, 2015

Member

can drop this, covered by tests for GetRequestInfo

@@ -100,13 +101,20 @@ func NewFromFile(path string) (policyList, error) {
func (p policy) matches(a authorizer.Attributes) bool {
if p.subjectMatches(a) {
if p.Readonly == false || (p.Readonly == a.IsReadOnly()) {
if p.Resource == "" || (p.Resource == a.GetResource()) {
switch {
case p.NonResourcePath != "":

This comment has been minimized.

@liggitt

liggitt Oct 22, 2015

Member

I'd like to preserve the information from GetRequestInfo in the attributes. If we did, this structure could look something like this:

// Ensure resource rules only match resource requests, and non-resource rules only match non-resource requests
isResourceRule := p.NonResourcePath == ""
if isResourceRule != a.IsResourceRequest() {
  return false
}

if isResourceRule {
  // Require namespace to match, if present in the policy rule
  if p.Namespace != "" && p.Namespace != a.GetNamespace() {
    return false
  }
  // Require resource to match, if present in the policy rule
  if p.Resource != "" && p.Resource != a.GetResource() {
    return false
  }
  return true
}

// Exact non-resource matches
if p.NonResourcePath == a.GetPath() {
  return true
}
// Subpath matches for `.../*` non-resource rules
if strings.HasSuffix(p.NonResourcePath, "*") && strings.HasPrefix(a.GetPath(), string.TrimRight(p.NonResourcePath, "*")) {
  return true
}
return false
@liggitt

This comment has been minimized.

Member

liggitt commented Oct 23, 2015

rebased and added tests and support for wildcard non-resource path suffixes in #16148

@yuvipanda

This comment has been minimized.

Contributor

yuvipanda commented Oct 25, 2015

This no longer cleanly applies to 1.1.0-beta. So I guess this won't make 1.1?

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Oct 28, 2015

Removed from 1.1.

@yuvipanda

This comment has been minimized.

Contributor

yuvipanda commented Nov 11, 2015

I did a super naive rebase on https://github.com/yuvipanda/kubernetes/tree/fix_kubectl_for_namespaced_users

Tagging @erictune and @pmorie too. I'm carrying an older version of this patch on my current install

@mkulke

This comment has been minimized.

Contributor

mkulke commented Nov 11, 2015

@yuvipanda: thx, i really like this to land in upstream. we're relying on namespace'd users and have to run patched versions in our clusters currently as well.

@googlebot

This comment has been minimized.

googlebot commented Nov 11, 2015

CLAs look good, thanks!

@yuvipanda

This comment has been minimized.

Contributor

yuvipanda commented Nov 11, 2015

@mkulke

This comment has been minimized.

Contributor

mkulke commented Nov 11, 2015

@yuvipanda: sorry I forgot, @liggitt opened a PR with the necessary rebase work some weeks ago (see comments above) #16148

@googlebot

This comment has been minimized.

googlebot commented Nov 11, 2015

CLAs look good, thanks!

@liggitt

This comment has been minimized.

Member

liggitt commented Nov 11, 2015

#16148 Is close, I am closing out our 1.1 release and will hopefully get back to it shortly

@yuvipanda

This comment has been minimized.

Contributor

yuvipanda commented Nov 11, 2015

Awesome. In that case can you close this PR?

On Wed, Nov 11, 2015 at 2:14 PM, Jordan Liggitt notifications@github.com
wrote:

#16148 #16148 Is close, I
am closing out our 1.1 release and will hopefully get back to it shortly


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

Yuvi Panda T
http://yuvi.in/blog

@mkulke

This comment has been minimized.

Contributor

mkulke commented Nov 11, 2015

@yuvipanda: yup.

[closed: duplicate, work continues in #16148]

@mkulke mkulke closed this Nov 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment