Merge pull request #34071 from liggitt/authz-webhook

Automatic merge from submit-queue

Allow webhook authorizer to use SubjectAccessReviewInterface

Refactors the authorization webhook to be able to be fed a kubeconfig file or a SubjectAccessReviewsInterface 

Added tests to exercise retry logic, and ensure the correct serialized version is sent to the remote webhook (I also made sure the new tests passed on the current webhook impl in master)

c.f. https://github.com/kubernetes/kubernetes/pull/32547
c.f. https://github.com/kubernetes/kubernetes/pull/32518

plugin/pkg/auth/authorizer/webhook/webhook.go

@@ -19,15 +19,15 @@ package webhook
 
 import (
 	"encoding/json"
-	"fmt"
 	"time"
 
 	"github.com/golang/glog"
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
+	"k8s.io/kubernetes/pkg/apis/authorization"
 	"k8s.io/kubernetes/pkg/apis/authorization/v1beta1"
 	"k8s.io/kubernetes/pkg/auth/authorizer"
-	"k8s.io/kubernetes/pkg/client/restclient"
+	authorizationclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/unversioned"
 	"k8s.io/kubernetes/pkg/util/cache"
 	"k8s.io/kubernetes/plugin/pkg/webhook"
 
@@ -44,10 +44,16 @@ const retryBackoff = 500 * time.Millisecond
 var _ authorizer.Authorizer = (*WebhookAuthorizer)(nil)
 
 type WebhookAuthorizer struct {
-	*webhook.GenericWebhook
+	subjectAccessReview authorizationclient.SubjectAccessReviewInterface
 	responseCache       *cache.LRUExpireCache
 	authorizedTTL       time.Duration
 	unauthorizedTTL     time.Duration
+	initialBackoff      time.Duration
+}
+
+// NewFromInterface creates a WebhookAuthorizer using the given subjectAccessReview client
+func NewFromInterface(subjectAccessReview authorizationclient.SubjectAccessReviewInterface, authorizedTTL, unauthorizedTTL time.Duration) (*WebhookAuthorizer, error) {
+	return newWithBackoff(subjectAccessReview, authorizedTTL, unauthorizedTTL, retryBackoff)
 }
 
 // New creates a new WebhookAuthorizer from the provided kubeconfig file.
@@ -71,16 +77,22 @@ type WebhookAuthorizer struct {
 // For additional HTTP configuration, refer to the kubeconfig documentation
 // http://kubernetes.io/v1.1/docs/user-guide/kubeconfig-file.html.
 func New(kubeConfigFile string, authorizedTTL, unauthorizedTTL time.Duration) (*WebhookAuthorizer, error) {
-	return newWithBackoff(kubeConfigFile, authorizedTTL, unauthorizedTTL, retryBackoff)
+	subjectAccessReview, err := subjectAccessReviewInterfaceFromKubeconfig(kubeConfigFile)
-}
-
-// newWithBackoff allows tests to skip the sleep.
-func newWithBackoff(kubeConfigFile string, authorizedTTL, unauthorizedTTL, initialBackoff time.Duration) (*WebhookAuthorizer, error) {
-	gw, err := webhook.NewGenericWebhook(kubeConfigFile, groupVersions, initialBackoff)
 	if err != nil {
 		return nil, err
 	}
-	return &WebhookAuthorizer{gw, cache.NewLRUExpireCache(1024), authorizedTTL, unauthorizedTTL}, nil
+	return newWithBackoff(subjectAccessReview, authorizedTTL, unauthorizedTTL, retryBackoff)
+}
+
+// newWithBackoff allows tests to skip the sleep.
+func newWithBackoff(subjectAccessReview authorizationclient.SubjectAccessReviewInterface, authorizedTTL, unauthorizedTTL, initialBackoff time.Duration) (*WebhookAuthorizer, error) {
+	return &WebhookAuthorizer{
+		subjectAccessReview: subjectAccessReview,
+		responseCache:       cache.NewLRUExpireCache(1024),
+		authorizedTTL:       authorizedTTL,
+		unauthorizedTTL:     unauthorizedTTL,
+		initialBackoff:      initialBackoff,
+	}, nil
 }
 
 // Authorize makes a REST request to the remote service describing the attempted action as a JSON
@@ -128,9 +140,9 @@ func newWithBackoff(kubeConfigFile string, authorizedTTL, unauthorizedTTL, initi
 //     }
 //
 func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bool, reason string, err error) {
-	r := &v1beta1.SubjectAccessReview{}
+	r := &authorization.SubjectAccessReview{}
 	if user := attr.GetUser(); user != nil {
-		r.Spec = v1beta1.SubjectAccessReviewSpec{
+		r.Spec = authorization.SubjectAccessReviewSpec{
 			User:   user.GetName(),
 			Groups: user.GetGroups(),
 			Extra:  convertToSARExtra(user.GetExtra()),
@@ -138,7 +150,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo
 	}
 
 	if attr.IsResourceRequest() {
-		r.Spec.ResourceAttributes = &v1beta1.ResourceAttributes{
+		r.Spec.ResourceAttributes = &authorization.ResourceAttributes{
 			Namespace:   attr.GetNamespace(),
 			Verb:        attr.GetVerb(),
 			Group:       attr.GetAPIGroup(),
@@ -148,7 +160,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo
 			Name:        attr.GetName(),
 		}
 	} else {
-		r.Spec.NonResourceAttributes = &v1beta1.NonResourceAttributes{
+		r.Spec.NonResourceAttributes = &authorization.NonResourceAttributes{
 			Path: attr.GetPath(),
 			Verb: attr.GetVerb(),
 		}
@@ -158,26 +170,22 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo
 		return false, "", err
 	}
 	if entry, ok := w.responseCache.Get(string(key)); ok {
-		r.Status = entry.(v1beta1.SubjectAccessReviewStatus)
+		r.Status = entry.(authorization.SubjectAccessReviewStatus)
 	} else {
-		result := w.WithExponentialBackoff(func() restclient.Result {
+		var (
-			return w.RestClient.Post().Body(r).Do()
+			result *authorization.SubjectAccessReview
+			err    error
+		)
+		webhook.WithExponentialBackoff(w.initialBackoff, func() error {
+			result, err = w.subjectAccessReview.Create(r)
+			return err
 		})
-		if err := result.Error(); err != nil {
+		if err != nil {
 			// An error here indicates bad configuration or an outage. Log for debugging.
 			glog.Errorf("Failed to make webhook authorizer request: %v", err)
 			return false, "", err
 		}
-		var statusCode int
+		r.Status = result.Status
-		result.StatusCode(&statusCode)
-		switch {
-		case statusCode < 200,
-			statusCode >= 300:
-			return false, "", fmt.Errorf("Error contacting webhook: %d", statusCode)
-		}
-		if err := result.Into(r); err != nil {
-			return false, "", err
-		}
 		if r.Status.Allowed {
 			w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
 		} else {
@@ -187,14 +195,35 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo
 	return r.Status.Allowed, r.Status.Reason, nil
 }
 
-func convertToSARExtra(extra map[string][]string) map[string]v1beta1.ExtraValue {
+func convertToSARExtra(extra map[string][]string) map[string]authorization.ExtraValue {
 	if extra == nil {
 		return nil
 	}
-	ret := map[string]v1beta1.ExtraValue{}
+	ret := map[string]authorization.ExtraValue{}
 	for k, v := range extra {
-		ret[k] = v1beta1.ExtraValue(v)
+		ret[k] = authorization.ExtraValue(v)
 	}
 
 	return ret
 }
+
+// subjectAccessReviewInterfaceFromKubeconfig builds a client from the specified kubeconfig file,
+// and returns a SubjectAccessReviewInterface that uses that client. Note that the client submits SubjectAccessReview
+// requests to the exact path specified in the kubeconfig file, so arbitrary non-API servers can be targeted.
+func subjectAccessReviewInterfaceFromKubeconfig(kubeConfigFile string) (authorizationclient.SubjectAccessReviewInterface, error) {
+	gw, err := webhook.NewGenericWebhook(kubeConfigFile, groupVersions, 0)
+	if err != nil {
+		return nil, err
+	}
+	return &subjectAccessReviewClient{gw}, nil
+}
+
+type subjectAccessReviewClient struct {
+	w *webhook.GenericWebhook
+}
+
+func (t *subjectAccessReviewClient) Create(subjectAccessReview *authorization.SubjectAccessReview) (*authorization.SubjectAccessReview, error) {
+	result := &authorization.SubjectAccessReview{}
+	err := t.w.RestClient.Post().Body(subjectAccessReview).Do().Into(result)
+	return result, err
+}

plugin/pkg/auth/authorizer/webhook/webhook_test.go

@@ -24,6 +24,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -183,7 +184,11 @@ current-context: default
 				return fmt.Errorf("failed to execute test template: %v", err)
 			}
 			// Create a new authorizer
-			_, err = newWithBackoff(p, 0, 0, 0)
+			sarClient, err := subjectAccessReviewInterfaceFromKubeconfig(p)
+			if err != nil {
+				return fmt.Errorf("error building sar client: %v", err)
+			}
+			_, err = newWithBackoff(sarClient, 0, 0, 0)
 			return err
 		}()
 		if err != nil && !tt.wantErr {
@@ -203,6 +208,7 @@ type Service interface {
 
 // NewTestServer wraps a Service as an httptest.Server.
 func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error) {
+	const webhookPath = "/testserver"
 	var tlsConfig *tls.Config
 	if cert != nil {
 		cert, err := tls.X509KeyPair(cert, key)
@@ -223,26 +229,44 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error
 	}
 
 	serveHTTP := func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "POST" {
+			http.Error(w, fmt.Sprintf("unexpected method: %v", r.Method), http.StatusMethodNotAllowed)
+			return
+		}
+		if r.URL.Path != webhookPath {
+			http.Error(w, fmt.Sprintf("unexpected path: %v", r.URL.Path), http.StatusNotFound)
+			return
+		}
+
 		var review v1beta1.SubjectAccessReview
-		if err := json.NewDecoder(r.Body).Decode(&review); err != nil {
+		bodyData, _ := ioutil.ReadAll(r.Body)
+		if err := json.Unmarshal(bodyData, &review); err != nil {
 			http.Error(w, fmt.Sprintf("failed to decode body: %v", err), http.StatusBadRequest)
 			return
 		}
+
+		// ensure we received the serialized review as expected
+		if review.APIVersion != "authorization.k8s.io/v1beta1" {
+			http.Error(w, fmt.Sprintf("wrong api version: %s", string(bodyData)), http.StatusBadRequest)
+			return
+		}
+		// once we have a successful request, always call the review to record that we were called
+		s.Review(&review)
 		if s.HTTPStatusCode() < 200 || s.HTTPStatusCode() >= 300 {
 			http.Error(w, "HTTP Error", s.HTTPStatusCode())
 			return
 		}
-		s.Review(&review)
 		type status struct {
 			Allowed         bool   `json:"allowed"`
 			Reason          string `json:"reason"`
+			EvaluationError string `json:"evaluationError"`
 		}
 		resp := struct {
 			APIVersion string `json:"apiVersion"`
 			Status     status `json:"status"`
 		}{
 			APIVersion: v1beta1.SchemeGroupVersion.String(),
-			Status:     status{review.Status.Allowed, review.Status.Reason},
+			Status:     status{review.Status.Allowed, review.Status.Reason, review.Status.EvaluationError},
 		}
 		w.Header().Set("Content-Type", "application/json")
 		json.NewEncoder(w).Encode(resp)
@@ -251,6 +275,12 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error
 	server := httptest.NewUnstartedServer(http.HandlerFunc(serveHTTP))
 	server.TLS = tlsConfig
 	server.StartTLS()
+
+	// Adjust the path to point to our custom path
+	serverURL, _ := url.Parse(server.URL)
+	serverURL.Path = webhookPath
+	server.URL = serverURL.String()
+
 	return server, nil
 }
 
@@ -258,9 +288,11 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error
 type mockService struct {
 	allow      bool
 	statusCode int
+	called     int
 }
 
 func (m *mockService) Review(r *v1beta1.SubjectAccessReview) {
+	m.called++
 	r.Status.Allowed = m.allow
 }
 func (m *mockService) Allow()              { m.allow = true }
@@ -291,7 +323,11 @@ func newAuthorizer(callbackURL string, clientCert, clientKey, ca []byte, cacheTi
 	if err := json.NewEncoder(tempfile).Encode(config); err != nil {
 		return nil, err
 	}
-	return newWithBackoff(p, cacheTime, cacheTime, 0)
+	sarClient, err := subjectAccessReviewInterfaceFromKubeconfig(p)
+	if err != nil {
+		return nil, fmt.Errorf("error building sar client: %v", err)
+	}
+	return newWithBackoff(sarClient, cacheTime, cacheTime, 0)
 }
 
 func TestTLSConfig(t *testing.T) {
@@ -506,29 +542,36 @@ func TestWebhook(t *testing.T) {
 }
 
 type webhookCacheTestCase struct {
+	attr authorizer.AttributesRecord
+
+	allow      bool
 	statusCode int
+
 	expectedErr        bool
 	expectedAuthorized bool
-	expectedCached     bool
+	expectedCalls      int
 }
 
-func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorizer, attr authorizer.AttributesRecord, tests []webhookCacheTestCase) {
+func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorizer, tests []webhookCacheTestCase) {
-	for _, test := range tests {
+	for i, test := range tests {
+		serv.called = 0
+		serv.allow = test.allow
 		serv.statusCode = test.statusCode
-		authorized, _, err := wh.Authorize(attr)
+		authorized, _, err := wh.Authorize(test.attr)
 		if test.expectedErr && err == nil {
-			t.Errorf("Expected error")
+			t.Errorf("%d: Expected error", i)
+			continue
 		} else if !test.expectedErr && err != nil {
-			t.Fatal(err)
+			t.Errorf("%d: unexpected error: %v", i, err)
+			continue
 		}
-		if test.expectedAuthorized && !authorized {
+
-			if test.expectedCached {
+		if test.expectedAuthorized != authorized {
-				t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.")
+			t.Errorf("%d: expected authorized=%v, got %v", i, test.expectedAuthorized, authorized)
-			} else {
-				t.Errorf("Webhook returned HTTP %d, but authorizer reported unauthorized.", test.statusCode)
 		}
-		} else if !test.expectedAuthorized && authorized {
+
-			t.Errorf("Webhook returned HTTP %d, but authorizer reported success.", test.statusCode)
+		if test.expectedCalls != serv.called {
+			t.Errorf("%d: expected %d calls, got %d", i, test.expectedCalls, serv.called)
 		}
 	}
 }
@@ -549,27 +592,29 @@ func TestWebhookCache(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	aliceAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}}
+	bobAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "bob"}}
+
 	tests := []webhookCacheTestCase{
-		{statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCached: false},
+		// server error and 429's retry
-		{statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCached: false},
+		{attr: aliceAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
-		{statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCached: false},
+		{attr: aliceAttr, allow: false, statusCode: 429, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
-		{statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCached: false},
+		// regular errors return errors but do not retry
-		{statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCached: false},
+		{attr: aliceAttr, allow: false, statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
-		{statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCached: true},
+		{attr: aliceAttr, allow: false, statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
+		{attr: aliceAttr, allow: false, statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCalls: 1},
+		// successful responses are cached
+		{attr: aliceAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
+		// later requests within the cache window don't hit the backend
+		{attr: aliceAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
+
+		// a request with different attributes doesn't hit the cache
+		{attr: bobAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5},
+		// successful response for other attributes is cached
+		{attr: bobAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1},
+		// later requests within the cache window don't hit the backend
+		{attr: bobAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0},
 	}
 
-	attr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}}
+	testWebhookCacheCases(t, serv, wh, tests)
-	serv.allow = true
-
-	testWebhookCacheCases(t, serv, wh, attr, tests)
-
-	// For a different request, webhook should be called again.
-	tests = []webhookCacheTestCase{
-		{statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCached: false},
-		{statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCached: false},
-		{statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCached: true},
-	}
-	attr = authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "bob"}}
-
-	testWebhookCacheCases(t, serv, wh, attr, tests)
 }