From 72951efdafe8e039cdee7014624796cd35bce225 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Tue, 16 Jul 2019 14:49:04 -0700 Subject: [PATCH] Cleanup kubelet authz tests & make explicit --- pkg/kubelet/server/BUILD | 1 - pkg/kubelet/server/auth_test.go | 103 ++++++++++++++++- pkg/kubelet/server/server_test.go | 181 +++++++++++------------------- 3 files changed, 165 insertions(+), 120 deletions(-) diff --git a/pkg/kubelet/server/BUILD b/pkg/kubelet/server/BUILD index fdc84081e69..c036a121611 100644 --- a/pkg/kubelet/server/BUILD +++ b/pkg/kubelet/server/BUILD @@ -82,7 +82,6 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/httpstream:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apiserver/pkg/authentication/authenticator:go_default_library", "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library", "//staging/src/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library", diff --git a/pkg/kubelet/server/auth_test.go b/pkg/kubelet/server/auth_test.go index 844be3852f7..d598bc3892b 100644 --- a/pkg/kubelet/server/auth_test.go +++ b/pkg/kubelet/server/auth_test.go @@ -16,7 +16,15 @@ limitations under the License. package server -import "testing" +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" +) func TestIsSubPath(t *testing.T) { testcases := map[string]struct { @@ -51,3 +59,96 @@ func TestIsSubPath(t *testing.T) { } } } + +func TestGetRequestAttributes(t *testing.T) { + for _, test := range AuthzTestCases() { + t.Run(test.Method+":"+test.Path, func(t *testing.T) { + getter := NewNodeAuthorizerAttributesGetter(authzTestNodeName) + + req, err := http.NewRequest(test.Method, "https://localhost:1234"+test.Path, nil) + require.NoError(t, err) + attrs := getter.GetRequestAttributes(AuthzTestUser(), req) + + test.AssertAttributes(t, attrs) + }) + } +} + +const ( + authzTestNodeName = "test" + authzTestUserName = "phibby" +) + +type AuthzTestCase struct { + Method, Path string + + ExpectedVerb, ExpectedSubresource string +} + +func (a *AuthzTestCase) AssertAttributes(t *testing.T, attrs authorizer.Attributes) { + expectedAttributes := authorizer.AttributesRecord{ + User: AuthzTestUser(), + APIGroup: "", + APIVersion: "v1", + Verb: a.ExpectedVerb, + Resource: "nodes", + Name: authzTestNodeName, + Subresource: a.ExpectedSubresource, + ResourceRequest: true, + Path: a.Path, + } + + assert.Equal(t, expectedAttributes, attrs) +} + +func AuthzTestUser() user.Info { + return &user.DefaultInfo{Name: authzTestUserName} +} + +func AuthzTestCases() []AuthzTestCase { + // Path -> ExpectedSubresource + testPaths := map[string]string{ + "/attach/{podNamespace}/{podID}/{containerName}": "proxy", + "/attach/{podNamespace}/{podID}/{uid}/{containerName}": "proxy", + "/configz": "proxy", + "/containerLogs/{podNamespace}/{podID}/{containerName}": "proxy", + "/cri/": "proxy", + "/cri/foo": "proxy", + "/debug/flags/v": "proxy", + "/debug/pprof/{subpath:*}": "proxy", + "/exec/{podNamespace}/{podID}/{containerName}": "proxy", + "/exec/{podNamespace}/{podID}/{uid}/{containerName}": "proxy", + "/healthz": "proxy", + "/healthz/log": "proxy", + "/healthz/ping": "proxy", + "/healthz/syncloop": "proxy", + "/logs/": "log", + "/logs/{logpath:*}": "log", + "/metrics": "metrics", + "/metrics/cadvisor": "metrics", + "/metrics/probes": "metrics", + "/metrics/resource/v1alpha1": "metrics", + "/pods/": "proxy", + "/portForward/{podNamespace}/{podID}": "proxy", + "/portForward/{podNamespace}/{podID}/{uid}": "proxy", + "/run/{podNamespace}/{podID}/{containerName}": "proxy", + "/run/{podNamespace}/{podID}/{uid}/{containerName}": "proxy", + "/runningpods/": "proxy", + "/spec/": "spec", + "/stats/": "stats", + "/stats/container": "stats", + "/stats/summary": "stats", + "/stats/{namespace}/{podName}/{uid}/{containerName}": "stats", + "/stats/{podName}/{containerName}": "stats", + } + testCases := []AuthzTestCase{} + for path, subresource := range testPaths { + testCases = append(testCases, + AuthzTestCase{"POST", path, "create", subresource}, + AuthzTestCase{"GET", path, "get", subresource}, + AuthzTestCase{"PUT", path, "update", subresource}, + AuthzTestCase{"PATCH", path, "patch", subresource}, + AuthzTestCase{"DELETE", path, "delete", subresource}) + } + return testCases +} diff --git a/pkg/kubelet/server/server_test.go b/pkg/kubelet/server/server_test.go index 4761d21afb7..ff1052e2f44 100644 --- a/pkg/kubelet/server/server_test.go +++ b/pkg/kubelet/server/server_test.go @@ -43,7 +43,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/httpstream" "k8s.io/apimachinery/pkg/util/httpstream/spdy" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/authorization/authorizer" @@ -682,144 +681,90 @@ type authTestCase struct { Path string } -func TestAuthFilters(t *testing.T) { +// Ensure all registered handlers & services have an associated testcase. +func TestAuthzCoverage(t *testing.T) { fw := newServerTest() defer fw.testHTTPServer.Close() - testcases := []authTestCase{} - - // This is a sanity check that the Handle->HandleWithFilter() delegation is working - // Ideally, these would move to registered web services and this list would get shorter - expectedPaths := []string{"/healthz", "/metrics", "/metrics/cadvisor"} - paths := sets.NewString(fw.serverUnderTest.restfulCont.RegisteredHandlePaths()...) - for _, expectedPath := range expectedPaths { - if !paths.Has(expectedPath) { - t.Errorf("Expected registered handle path %s was missing", expectedPath) - } - } + // method:path -> has coverage + expectedCases := map[string]bool{} // Test all the non-web-service handlers for _, path := range fw.serverUnderTest.restfulCont.RegisteredHandlePaths() { - testcases = append(testcases, authTestCase{"GET", path}) - testcases = append(testcases, authTestCase{"POST", path}) - // Test subpaths for directory handlers - if strings.HasSuffix(path, "/") { - testcases = append(testcases, authTestCase{"GET", path + "foo"}) - testcases = append(testcases, authTestCase{"POST", path + "foo"}) - } + expectedCases["GET:"+path] = false + expectedCases["POST:"+path] = false } // Test all the generated web-service paths for _, ws := range fw.serverUnderTest.restfulCont.RegisteredWebServices() { for _, r := range ws.Routes() { - testcases = append(testcases, authTestCase{r.Method, r.Path}) + expectedCases[r.Method+":"+r.Path] = false } } - methodToAPIVerb := map[string]string{"GET": "get", "POST": "create", "PUT": "update"} - pathToSubresource := func(path string) string { - switch { - // Cases for subpaths we expect specific subresources for - case isSubpath(path, statsPath): - return "stats" - case isSubpath(path, specPath): - return "spec" - case isSubpath(path, logsPath): - return "log" - case isSubpath(path, metricsPath): - return "metrics" - - // Cases for subpaths we expect to map to the "proxy" subresource - case isSubpath(path, "/attach"), - isSubpath(path, "/configz"), - isSubpath(path, "/containerLogs"), - isSubpath(path, "/debug"), - isSubpath(path, "/exec"), - isSubpath(path, "/healthz"), - isSubpath(path, "/pods"), - isSubpath(path, "/portForward"), - isSubpath(path, "/run"), - isSubpath(path, "/runningpods"), - isSubpath(path, "/cri"): - return "proxy" - - default: - panic(fmt.Errorf(`unexpected kubelet API path %s. -The kubelet API has likely registered a handler for a new path. -If the new path has a use case for partitioned authorization when requested from the kubelet API, -add a specific subresource for it in auth.go#GetRequestAttributes() and in TestAuthFilters(). -Otherwise, add it to the expected list of paths that map to the "proxy" subresource in TestAuthFilters()`, path)) + // This is a sanity check that the Handle->HandleWithFilter() delegation is working + // Ideally, these would move to registered web services and this list would get shorter + expectedPaths := []string{"/healthz", "/metrics", "/metrics/cadvisor"} + for _, expectedPath := range expectedPaths { + if _, expected := expectedCases["GET:"+expectedPath]; !expected { + t.Errorf("Expected registered handle path %s was missing", expectedPath) } } - attributesGetter := NewNodeAuthorizerAttributesGetter(types.NodeName("test")) - for _, tc := range testcases { - var ( - expectedUser = &user.DefaultInfo{Name: "test"} - expectedAttributes = authorizer.AttributesRecord{ - User: expectedUser, - APIGroup: "", - APIVersion: "v1", - Verb: methodToAPIVerb[tc.Method], - Resource: "nodes", - Name: "test", - Subresource: pathToSubresource(tc.Path), - ResourceRequest: true, - Path: tc.Path, + for _, tc := range AuthzTestCases() { + expectedCases[tc.Method+":"+tc.Path] = true + } + + for tc, found := range expectedCases { + if !found { + t.Errorf("Missing authz test case for %s", tc) + } + } +} + +func TestAuthFilters(t *testing.T) { + fw := newServerTest() + defer fw.testHTTPServer.Close() + + attributesGetter := NewNodeAuthorizerAttributesGetter(authzTestNodeName) + + for _, tc := range AuthzTestCases() { + t.Run(tc.Method+":"+tc.Path, func(t *testing.T) { + var ( + expectedUser = AuthzTestUser() + + calledAuthenticate = false + calledAuthorize = false + calledAttributes = false + ) + + fw.fakeAuth.authenticateFunc = func(req *http.Request) (*authenticator.Response, bool, error) { + calledAuthenticate = true + return &authenticator.Response{User: expectedUser}, true, nil + } + fw.fakeAuth.attributesFunc = func(u user.Info, req *http.Request) authorizer.Attributes { + calledAttributes = true + require.Equal(t, expectedUser, u) + return attributesGetter.GetRequestAttributes(u, req) + } + fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (decision authorizer.Decision, reason string, err error) { + calledAuthorize = true + tc.AssertAttributes(t, a) + return authorizer.DecisionNoOpinion, "", nil } - calledAuthenticate = false - calledAuthorize = false - calledAttributes = false - ) + req, err := http.NewRequest(tc.Method, fw.testHTTPServer.URL+tc.Path, nil) + require.NoError(t, err) - fw.fakeAuth.authenticateFunc = func(req *http.Request) (*authenticator.Response, bool, error) { - calledAuthenticate = true - return &authenticator.Response{User: expectedUser}, true, nil - } - fw.fakeAuth.attributesFunc = func(u user.Info, req *http.Request) authorizer.Attributes { - calledAttributes = true - if u != expectedUser { - t.Fatalf("%s: expected user %v, got %v", tc.Path, expectedUser, u) - } - return attributesGetter.GetRequestAttributes(u, req) - } - fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (decision authorizer.Decision, reason string, err error) { - calledAuthorize = true - if a != expectedAttributes { - t.Fatalf("%s: expected attributes\n\t%#v\ngot\n\t%#v", tc.Path, expectedAttributes, a) - } - return authorizer.DecisionNoOpinion, "", nil - } + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() - req, err := http.NewRequest(tc.Method, fw.testHTTPServer.URL+tc.Path, nil) - if err != nil { - t.Errorf("%s: unexpected error: %v", tc.Path, err) - continue - } - resp, err := http.DefaultClient.Do(req) - if err != nil { - t.Errorf("%s: unexpected error: %v", tc.Path, err) - continue - } - defer resp.Body.Close() - if resp.StatusCode != http.StatusForbidden { - t.Errorf("%s: unexpected status code %d", tc.Path, resp.StatusCode) - continue - } - - if !calledAuthenticate { - t.Errorf("%s: Authenticate was not called", tc.Path) - continue - } - if !calledAttributes { - t.Errorf("%s: Attributes were not called", tc.Path) - continue - } - if !calledAuthorize { - t.Errorf("%s: Authorize was not called", tc.Path) - continue - } + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + assert.True(t, calledAuthenticate, "Authenticate was not called") + assert.True(t, calledAttributes, "Attributes were not called") + assert.True(t, calledAuthorize, "Authorize was not called") + }) } }