diff --git a/plugin/pkg/auth/authorizer/node/node_authorizer.go b/plugin/pkg/auth/authorizer/node/node_authorizer.go index 30836e2cb4e..0239ababd3e 100644 --- a/plugin/pkg/auth/authorizer/node/node_authorizer.go +++ b/plugin/pkg/auth/authorizer/node/node_authorizer.go @@ -91,9 +91,9 @@ func (r *NodeAuthorizer) Authorize(attrs authorizer.Attributes) (authorizer.Deci requestResource := schema.GroupResource{Group: attrs.GetAPIGroup(), Resource: attrs.GetResource()} switch requestResource { case secretResource: - return r.authorizeGet(nodeName, secretVertexType, attrs) + return r.authorizeReadNamespacedObject(nodeName, secretVertexType, attrs) case configMapResource: - return r.authorizeGet(nodeName, configMapVertexType, attrs) + return r.authorizeReadNamespacedObject(nodeName, configMapVertexType, attrs) case pvcResource: if r.features.Enabled(features.ExpandPersistentVolumes) { if attrs.GetSubresource() == "status" { @@ -154,6 +154,24 @@ func (r *NodeAuthorizer) authorizeGet(nodeName string, startingType vertexType, return r.authorize(nodeName, startingType, attrs) } +// authorizeReadNamespacedObject authorizes "get", "list" and "watch" requests to single objects of a +// specified types if they are related to the specified node. +func (r *NodeAuthorizer) authorizeReadNamespacedObject(nodeName string, startingType vertexType, attrs authorizer.Attributes) (authorizer.Decision, string, error) { + if attrs.GetVerb() != "get" && attrs.GetVerb() != "list" && attrs.GetVerb() != "watch" { + glog.V(2).Infof("NODE DENY: %s %#v", nodeName, attrs) + return authorizer.DecisionNoOpinion, "can only read resources of this type", nil + } + if len(attrs.GetSubresource()) > 0 { + glog.V(2).Infof("NODE DENY: %s %#v", nodeName, attrs) + return authorizer.DecisionNoOpinion, "cannot read subresource", nil + } + if len(attrs.GetNamespace()) == 0 { + glog.V(2).Infof("NODE DENY: %s %#v", nodeName, attrs) + return authorizer.DecisionNoOpinion, "can only read namespaced object of this type", nil + } + return r.authorize(nodeName, startingType, attrs) +} + func (r *NodeAuthorizer) authorize(nodeName string, startingType vertexType, attrs authorizer.Attributes) (authorizer.Decision, string, error) { if len(attrs.GetName()) == 0 { glog.V(2).Infof("NODE DENY: %s %#v", nodeName, attrs) diff --git a/plugin/pkg/auth/authorizer/node/node_authorizer_test.go b/plugin/pkg/auth/authorizer/node/node_authorizer_test.go index ecc7bc3fe3d..956f8d29708 100644 --- a/plugin/pkg/auth/authorizer/node/node_authorizer_test.go +++ b/plugin/pkg/auth/authorizer/node/node_authorizer_test.go @@ -104,6 +104,31 @@ func TestAuthorizer(t *testing.T) { attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "secrets", Name: "secret0-pod0-node0", Namespace: "ns0"}, expect: authorizer.DecisionAllow, }, + { + name: "list allowed secret via pod", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "list", Resource: "secrets", Name: "secret0-pod0-node0", Namespace: "ns0"}, + expect: authorizer.DecisionAllow, + }, + { + name: "watch allowed secret via pod", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "watch", Resource: "secrets", Name: "secret0-pod0-node0", Namespace: "ns0"}, + expect: authorizer.DecisionAllow, + }, + { + name: "disallowed list many secrets", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "list", Resource: "secrets", Name: "", Namespace: "ns0"}, + expect: authorizer.DecisionNoOpinion, + }, + { + name: "disallowed watch many secrets", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "watch", Resource: "secrets", Name: "", Namespace: "ns0"}, + expect: authorizer.DecisionNoOpinion, + }, + { + name: "disallowed list secrets from all namespaces with name", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "list", Resource: "secrets", Name: "secret0-pod0-node0", Namespace: ""}, + expect: authorizer.DecisionNoOpinion, + }, { name: "allowed shared secret via pod", attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "secrets", Name: "secret0-shared", Namespace: "ns0"}, diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go index 8e5e4d76367..767285938d5 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go @@ -208,19 +208,25 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch if hasName { // metadata.name is the canonical internal name. - // SelectionPredicate will notice that this is - // a request for a single object and optimize the - // storage query accordingly. + // SelectionPredicate will notice that this is a request for + // a single object and optimize the storage query accordingly. nameSelector := fields.OneTermEqualSelector("metadata.name", name) + + // Note that fieldSelector setting explicitly the "metadata.name" + // will result in reaching this branch (as the value of that field + // is propagated to requestInfo as the name parameter. + // That said, the allowed field selectors in this branch are: + // nil, fields.Everything and field selector matching metadata.name + // for our name. if opts.FieldSelector != nil && !opts.FieldSelector.Empty() { - // It doesn't make sense to ask for both a name - // and a field selector, since just the name is - // sufficient to narrow down the request to a - // single object. - scope.err(errors.NewBadRequest("both a name and a field selector provided; please provide one or the other."), w, req) - return + selectedName, ok := opts.FieldSelector.RequiresExactMatch("metadata.name") + if !ok || name != selectedName { + scope.err(errors.NewBadRequest("fieldSelector metadata.name doesn't match requested name"), w, req) + return + } + } else { + opts.FieldSelector = nameSelector } - opts.FieldSelector = nameSelector } if opts.Watch || forceWatch { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/request/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/request/BUILD index d6caba2de08..2ed30ff7a47 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/request/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/request/BUILD @@ -30,6 +30,9 @@ go_library( ], importpath = "k8s.io/apiserver/pkg/endpoints/request", deps = [ + "//vendor/github.com/golang/glog:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/validation/path:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/apis/meta/internalversion:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go b/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go index 057d1956584..1520bb3c9e5 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go @@ -22,8 +22,12 @@ import ( "net/http" "strings" + "k8s.io/apimachinery/pkg/api/validation/path" + metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" + + "github.com/golang/glog" ) // LongRunningRequestCheck is a predicate which is true for long-running http requests. @@ -203,19 +207,35 @@ func (r *RequestInfoFactory) NewRequestInfo(req *http.Request) (*RequestInfo, er // if there's no name on the request and we thought it was a get before, then the actual verb is a list or a watch if len(requestInfo.Name) == 0 && requestInfo.Verb == "get" { - // Assumes v1.ListOptions - // Any query value that is not 0 or false is considered true - // see apimachinery/pkg/runtime/conversion.go Convert_Slice_string_To_bool - if values := req.URL.Query()["watch"]; len(values) > 0 { - switch strings.ToLower(values[0]) { - case "false", "0": - requestInfo.Verb = "list" - default: - requestInfo.Verb = "watch" + opts := metainternalversion.ListOptions{} + if err := metainternalversion.ParameterCodec.DecodeParameters(req.URL.Query(), metav1.SchemeGroupVersion, &opts); err != nil { + // An error in parsing request will result in default to "list" and not setting "name" field. + glog.Errorf("Couldn't parse request %#v: %v", req.URL.Query(), err) + // Reset opts to not rely on partial results from parsing. + // However, if watch is set, let's report it. + opts = metainternalversion.ListOptions{} + if values := req.URL.Query()["watch"]; len(values) > 0 { + switch strings.ToLower(values[0]) { + case "false", "0": + default: + opts.Watch = true + } } + } + + if opts.Watch { + requestInfo.Verb = "watch" } else { requestInfo.Verb = "list" } + + if opts.FieldSelector != nil { + if name, ok := opts.FieldSelector.RequiresExactMatch("metadata.name"); ok { + if len(path.IsValidPathSegmentName(name)) == 0 { + requestInfo.Name = name + } + } + } } // if there's no name on the request and we thought it was a delete before, then the actual verb is deletecollection if len(requestInfo.Name) == 0 && requestInfo.Verb == "delete" { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo_test.go index cf127da85d1..a5c521e5b4d 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo_test.go @@ -17,8 +17,10 @@ limitations under the License. package request import ( + "fmt" "net/http" "reflect" + "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -187,3 +189,77 @@ func newTestRequestInfoResolver() *RequestInfoFactory { GrouplessAPIPrefixes: sets.NewString("api"), } } + +func TestFieldSelectorParsing(t *testing.T) { + tests := []struct { + name string + url string + expectedName string + expectedErr error + expectedVerb string + }{ + { + name: "no selector", + url: "/apis/group/version/resource", + expectedVerb: "list", + }, + { + name: "metadata.name selector", + url: "/apis/group/version/resource?fieldSelector=metadata.name=name1", + expectedName: "name1", + expectedVerb: "list", + }, + { + name: "metadata.name selector with watch", + url: "/apis/group/version/resource?watch=true&fieldSelector=metadata.name=name1", + expectedName: "name1", + expectedVerb: "watch", + }, + { + name: "random selector", + url: "/apis/group/version/resource?fieldSelector=foo=bar", + expectedName: "", + expectedVerb: "list", + }, + { + name: "invalid selector with metadata.name", + url: "/apis/group/version/resource?fieldSelector=metadata.name=name1,foo", + expectedName: "", + expectedErr: fmt.Errorf("invalid selector"), + expectedVerb: "list", + }, + { + name: "invalid selector with metadata.name with watch", + url: "/apis/group/version/resource?fieldSelector=metadata.name=name1,foo&watch=true", + expectedName: "", + expectedErr: fmt.Errorf("invalid selector"), + expectedVerb: "watch", + }, + { + name: "invalid selector with metadata.name with watch false", + url: "/apis/group/version/resource?fieldSelector=metadata.name=name1,foo&watch=false", + expectedName: "", + expectedErr: fmt.Errorf("invalid selector"), + expectedVerb: "list", + }, + } + + resolver := newTestRequestInfoResolver() + + for _, tc := range tests { + req, _ := http.NewRequest("GET", tc.url, nil) + + apiRequestInfo, err := resolver.NewRequestInfo(req) + if err != nil { + if tc.expectedErr == nil || !strings.Contains(err.Error(), tc.expectedErr.Error()) { + t.Errorf("%s: Unexpected error %v", tc.name, err) + } + } + if e, a := tc.expectedName, apiRequestInfo.Name; e != a { + t.Errorf("%s: expected %v, actual %v", tc.name, e, a) + } + if e, a := tc.expectedVerb, apiRequestInfo.Verb; e != a { + t.Errorf("%s: expected verb %v, actual %v", tc.name, e, a) + } + } +} diff --git a/test/integration/apiserver/apiserver_test.go b/test/integration/apiserver/apiserver_test.go index d7eb01b7aa7..38e604faf84 100644 --- a/test/integration/apiserver/apiserver_test.go +++ b/test/integration/apiserver/apiserver_test.go @@ -238,3 +238,86 @@ func TestAPIListChunking(t *testing.T) { t.Errorf("unexpected items: %#v", list) } } + +func makeSecret(name string) *v1.Secret { + return &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Data: map[string][]byte{ + "key": []byte("value"), + }, + } +} + +func TestNameInFieldSelector(t *testing.T) { + s, clientSet, closeFn := setup(t) + defer closeFn() + + numNamespaces := 3 + namespaces := make([]*v1.Namespace, 0, numNamespaces) + for i := 0; i < 3; i++ { + ns := framework.CreateTestingNamespace(fmt.Sprintf("ns%d", i), s, t) + defer framework.DeleteTestingNamespace(ns, s, t) + namespaces = append(namespaces, ns) + + _, err := clientSet.CoreV1().Secrets(ns.Name).Create(makeSecret("foo")) + if err != nil { + t.Errorf("Couldn't create secret: %v", err) + } + _, err = clientSet.CoreV1().Secrets(ns.Name).Create(makeSecret("bar")) + if err != nil { + t.Errorf("Couldn't create secret: %v", err) + } + } + + testcases := []struct { + namespace string + selector string + expectedSecrets int + }{ + { + namespace: "", + selector: "metadata.name=foo", + expectedSecrets: numNamespaces, + }, + { + namespace: "", + selector: "metadata.name=foo,metadata.name=bar", + expectedSecrets: 0, + }, + { + namespace: "", + selector: "metadata.name=foo,metadata.namespace=ns1", + expectedSecrets: 1, + }, + { + namespace: "ns1", + selector: "metadata.name=foo,metadata.namespace=ns1", + expectedSecrets: 1, + }, + { + namespace: "ns1", + selector: "metadata.name=foo,metadata.namespace=ns2", + expectedSecrets: 0, + }, + { + namespace: "ns1", + selector: "metadata.name=foo,metadata.namespace=", + expectedSecrets: 0, + }, + } + + for _, tc := range testcases { + opts := metav1.ListOptions{ + FieldSelector: tc.selector, + } + secrets, err := clientSet.CoreV1().Secrets(tc.namespace).List(opts) + if err != nil { + t.Errorf("%s: Unexpected error: %v", tc.selector, err) + } + if len(secrets.Items) != tc.expectedSecrets { + t.Errorf("%s: Unexpected number of secrets: %d, expected: %d", tc.selector, len(secrets.Items), tc.expectedSecrets) + } + } +}