Allow webhook authorizer to use SubjectAccessReviewInterface

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
+	responseCache       *cache.LRUExpireCache
-	authorizedTTL   time.Duration
+	authorizedTTL       time.Duration
-	unauthorizedTTL 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"`
+			Allowed         bool   `json:"allowed"`
-			Reason  string `json:"reason"`
+			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 {
-	statusCode         int
+	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)
+
-			}
+		if test.expectedCalls != serv.called {
-		} else if !test.expectedAuthorized && authorized {
+			t.Errorf("%d: expected %d calls, got %d", i, test.expectedCalls, serv.called)
-			t.Errorf("Webhook returned HTTP %d, but authorizer reported success.", test.statusCode)
 		}
 	}
 }
@@ -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)
 }