Merge pull request #63469 from wojtek-t/allow_list_and_watch_secrets

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Allow for listing & watching individual secrets from nodes

This PR:
- propagates value of `metadata.name` field from fieldSelector to `name` field in RequestInfo (for list and watch requests)
- authorizes list/watch for requests for single secrets/configmaps coming from nodes

As an example:
```
/api/v1/secrets/namespaces/ns?fieldSelector=metadata.name=foo =>
  requestInfo.Name = "foo",
  requestInfo.Verb = "list"
/api/v1/secrets/namespaces/ns?fieldSelector=metadata.name=foo&watch=true =>
  requestInfo.Name = "foo",
  requestInfo.Verb = "list"
```

```release-note
list/watch API requests with a fieldSelector that specifies `metadata.name` can now be authorized as requests for an individual named resource
```

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)

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"},

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

@@ -126,7 +126,7 @@ func NodeRules() []rbac.PolicyRule {
 		// Needed for imagepullsecrets, rbd/ceph and secret volumes, and secrets in envs
 		// Needed for configmap volume and envs
 		// Use the Node authorization mode to limit a node to get secrets/configmaps referenced by pods bound to itself.
-		rbac.NewRule("get").Groups(legacyGroup).Resources("secrets", "configmaps").RuleOrDie(),
+		rbac.NewRule("get", "list", "watch").Groups(legacyGroup).Resources("secrets", "configmaps").RuleOrDie(),
 		// Needed for persistent volumes
 		// Use the Node authorization mode to limit a node to get pv/pvc objects referenced by pods bound to itself.
 		rbac.NewRule("get").Groups(legacyGroup).Resources("persistentvolumeclaims", "persistentvolumes").RuleOrDie(),

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml

@@ -1112,6 +1112,8 @@ items:
     - secrets
     verbs:
     - get
+    - list
+    - watch
   - apiGroups:
     - ""
     resources:

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 {

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",

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" {

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)
+		}
+	}
+}

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)
+		}
+	}
+}