Check HTTP Status code in webhook authorizer/authenticator.

plugin/pkg/auth/authenticator/token/webhook/webhook.go

@@ -18,6 +18,7 @@ limitations under the License.
 package webhook
 
 import (
+	"fmt"
 	"time"
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
@@ -64,11 +65,15 @@ func (w *WebhookTokenAuthenticator) AuthenticateToken(token string) (user.Info,
 		if err := result.Error(); err != nil {
 			return nil, false, err
 		}
+		var statusCode int
+		if result.StatusCode(&statusCode); statusCode < 200 || statusCode >= 300 {
+			return nil, false, fmt.Errorf("Error contacting webhook: %d", statusCode)
+		}
 		spec := r.Spec
 		if err := result.Into(r); err != nil {
 			return nil, false, err
 		}
-		go w.responseCache.Add(spec, r.Status, w.ttl)
+		w.responseCache.Add(spec, r.Status, w.ttl)
 	}
 	if !r.Status.Authenticated {
 		return nil, false, nil

plugin/pkg/auth/authenticator/token/webhook/webhook_test.go

@@ -27,6 +27,7 @@ import (
 	"os"
 	"reflect"
 	"testing"
+	"time"
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apis/authentication.k8s.io/v1beta1"
@@ -39,6 +40,7 @@ type Service interface {
 	// Review looks at the TokenReviewSpec and provides an authentication
 	// response in the TokenReviewStatus.
 	Review(*v1beta1.TokenReview)
+	HTTPStatusCode() int
 }
 
 // NewTestServer wraps a Service as an httptest.Server.
@@ -68,6 +70,10 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error
 			http.Error(w, fmt.Sprintf("failed to decode body: %v", err), http.StatusBadRequest)
 			return
 		}
+		if s.HTTPStatusCode() < 200 || s.HTTPStatusCode() >= 300 {
+			http.Error(w, "HTTP Error", s.HTTPStatusCode())
+			return
+		}
 		s.Review(&review)
 		type userInfo struct {
 			Username string   `json:"username"`
@@ -104,7 +110,8 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error
 
 // A service that can be set to say yes or no to authentication requests.
 type mockService struct {
-	allow bool
+	allow      bool
+	statusCode int
 }
 
 func (m *mockService) Review(r *v1beta1.TokenReview) {
@@ -113,12 +120,13 @@ func (m *mockService) Review(r *v1beta1.TokenReview) {
 		r.Status.User.Username = "realHooman@email.com"
 	}
 }
-func (m *mockService) Allow() { m.allow = true }
+func (m *mockService) Allow()              { m.allow = true }
-func (m *mockService) Deny()  { m.allow = false }
+func (m *mockService) Deny()               { m.allow = false }
+func (m *mockService) HTTPStatusCode() int { return m.statusCode }
 
 // newTokenAuthenticator creates a temporary kubeconfig file from the provided
 // arguments and attempts to load a new WebhookTokenAuthenticator from it.
-func newTokenAuthenticator(serverURL string, clientCert, clientKey, ca []byte) (*WebhookTokenAuthenticator, error) {
+func newTokenAuthenticator(serverURL string, clientCert, clientKey, ca []byte, cacheTime time.Duration) (*WebhookTokenAuthenticator, error) {
 	tempfile, err := ioutil.TempFile("", "")
 	if err != nil {
 		return nil, err
@@ -140,7 +148,7 @@ func newTokenAuthenticator(serverURL string, clientCert, clientKey, ca []byte) (
 	if err := json.NewEncoder(tempfile).Encode(config); err != nil {
 		return nil, err
 	}
-	return New(p, 0)
+	return New(p, cacheTime)
 }
 
 func TestTLSConfig(t *testing.T) {
@@ -187,6 +195,7 @@ func TestTLSConfig(t *testing.T) {
 		// Use a closure so defer statements trigger between loop iterations.
 		func() {
 			service := new(mockService)
+			service.statusCode = 200
 
 			server, err := NewTestServer(service, tt.serverCert, tt.serverKey, tt.serverCA)
 			if err != nil {
@@ -195,7 +204,7 @@ func TestTLSConfig(t *testing.T) {
 			}
 			defer server.Close()
 
-			wh, err := newTokenAuthenticator(server.URL, tt.clientCert, tt.clientKey, tt.clientCA)
+			wh, err := newTokenAuthenticator(server.URL, tt.clientCert, tt.clientKey, tt.clientCA, 0)
 			if err != nil {
 				t.Errorf("%s: failed to create client: %v", tt.test, err)
 				return
@@ -239,6 +248,8 @@ func (rec *recorderService) Review(r *v1beta1.TokenReview) {
 	r.Status = rec.response
 }
 
+func (rec *recorderService) HTTPStatusCode() int { return 200 }
+
 func TestWebhookTokenAuthenticator(t *testing.T) {
 	serv := &recorderService{}
 
@@ -248,7 +259,7 @@ func TestWebhookTokenAuthenticator(t *testing.T) {
 	}
 	defer s.Close()
 
-	wh, err := newTokenAuthenticator(s.URL, clientCert, clientKey, caCert)
+	wh, err := newTokenAuthenticator(s.URL, clientCert, clientKey, caCert, 0)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -350,3 +361,52 @@ func (a *authenticationUserInfo) GetExtra() map[string][]string { return a.Extra
 // Ensure v1beta1.UserInfo contains the fields necessary to implement the
 // user.Info interface.
 var _ user.Info = (*authenticationUserInfo)(nil)
+
+// TestWebhookCache verifies that error responses from the server are not
+// cached, but successful responses are.
+func TestWebhookCache(t *testing.T) {
+	serv := new(mockService)
+	s, err := NewTestServer(serv, serverCert, serverKey, caCert)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+
+	// Create an authenticator that caches successful responses "forever" (100 days).
+	wh, err := newTokenAuthenticator(s.URL, clientCert, clientKey, caCert, 2400*time.Hour)
+	if err != nil {
+		t.Fatal(err)
+	}
+	token := "t0k3n"
+	serv.allow = true
+	serv.statusCode = 500
+	if _, _, err := wh.AuthenticateToken(token); err == nil {
+		t.Errorf("Webhook returned HTTP 500, but authorizer reported success.")
+	}
+	serv.statusCode = 404
+	if _, _, err := wh.AuthenticateToken(token); err == nil {
+		t.Errorf("Webhook returned HTTP 404, but authorizer reported success.")
+	}
+	serv.statusCode = 200
+	if _, _, err := wh.AuthenticateToken(token); err != nil {
+		t.Errorf("Webhook returned HTTP 200, but authorizer reported unauthorized.")
+	}
+	serv.statusCode = 500
+	if _, _, err := wh.AuthenticateToken(token); err != nil {
+		t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.")
+	}
+	// For a different request, webhook should be called again.
+	token = "an0th3r_t0k3n"
+	serv.statusCode = 500
+	if _, _, err := wh.AuthenticateToken(token); err == nil {
+		t.Errorf("Webhook returned HTTP 500, but authorizer reported success.")
+	}
+	serv.statusCode = 200
+	if _, _, err := wh.AuthenticateToken(token); err != nil {
+		t.Errorf("Webhook returned HTTP 200, but authorizer reported unauthorized.")
+	}
+	serv.statusCode = 500
+	if _, _, err := wh.AuthenticateToken(token); err != nil {
+		t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.")
+	}
+}

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

@@ -20,6 +20,7 @@ package webhook
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"time"
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
@@ -151,16 +152,18 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (err error) {
 		if err := result.Error(); err != nil {
 			return err
 		}
+		var statusCode int
+		if result.StatusCode(&statusCode); statusCode < 200 || statusCode >= 300 {
+			return fmt.Errorf("Error contacting webhook: %d", statusCode)
+		}
 		if err := result.Into(r); err != nil {
 			return err
 		}
-		go func() {
+		if r.Status.Allowed {
-			if r.Status.Allowed {
+			w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
-				w.responseCache.Add(string(key), r.Status, w.authorizedTTL)
+		} else {
-			} else {
+			w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
-				w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
+		}
-			}
-		}()
 	}
 	if r.Status.Allowed {
 		return nil

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

@@ -29,6 +29,7 @@ import (
 	"reflect"
 	"testing"
 	"text/template"
+	"time"
 
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apis/authorization/v1beta1"
@@ -197,6 +198,7 @@ current-context: default
 // Service mocks a remote service.
 type Service interface {
 	Review(*v1beta1.SubjectAccessReview)
+	HTTPStatusCode() int
 }
 
 // NewTestServer wraps a Service as an httptest.Server.
@@ -226,6 +228,10 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error
 			http.Error(w, fmt.Sprintf("failed to decode body: %v", err), http.StatusBadRequest)
 			return
 		}
+		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"`
@@ -250,18 +256,20 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error
 
 // A service that can be set to allow all or deny all authorization requests.
 type mockService struct {
-	allow bool
+	allow      bool
+	statusCode int
 }
 
 func (m *mockService) Review(r *v1beta1.SubjectAccessReview) {
 	r.Status.Allowed = m.allow
 }
-func (m *mockService) Allow() { m.allow = true }
+func (m *mockService) Allow()              { m.allow = true }
-func (m *mockService) Deny()  { m.allow = false }
+func (m *mockService) Deny()               { m.allow = false }
+func (m *mockService) HTTPStatusCode() int { return m.statusCode }
 
 // newAuthorizer creates a temporary kubeconfig file from the provided arguments and attempts to load
 // a new WebhookAuthorizer from it.
-func newAuthorizer(callbackURL string, clientCert, clientKey, ca []byte) (*WebhookAuthorizer, error) {
+func newAuthorizer(callbackURL string, clientCert, clientKey, ca []byte, cacheTime time.Duration) (*WebhookAuthorizer, error) {
 	tempfile, err := ioutil.TempFile("", "")
 	if err != nil {
 		return nil, err
@@ -283,7 +291,7 @@ func newAuthorizer(callbackURL string, clientCert, clientKey, ca []byte) (*Webho
 	if err := json.NewEncoder(tempfile).Encode(config); err != nil {
 		return nil, err
 	}
-	return New(p, 0, 0)
+	return New(p, cacheTime, cacheTime)
 }
 
 func TestTLSConfig(t *testing.T) {
@@ -330,6 +338,7 @@ func TestTLSConfig(t *testing.T) {
 		// Use a closure so defer statements trigger between loop iterations.
 		func() {
 			service := new(mockService)
+			service.statusCode = 200
 
 			server, err := NewTestServer(service, tt.serverCert, tt.serverKey, tt.serverCA)
 			if err != nil {
@@ -338,7 +347,7 @@ func TestTLSConfig(t *testing.T) {
 			}
 			defer server.Close()
 
-			wh, err := newAuthorizer(server.URL, tt.clientCert, tt.clientKey, tt.clientCA)
+			wh, err := newAuthorizer(server.URL, tt.clientCert, tt.clientKey, tt.clientCA, 0)
 			if err != nil {
 				t.Errorf("%s: failed to create client: %v", tt.test, err)
 				return
@@ -384,6 +393,8 @@ func (rec *recorderService) Last() (v1beta1.SubjectAccessReview, error) {
 	return rec.last, rec.err
 }
 
+func (rec *recorderService) HTTPStatusCode() int { return 200 }
+
 func TestWebhook(t *testing.T) {
 	serv := new(recorderService)
 	s, err := NewTestServer(serv, serverCert, serverKey, caCert)
@@ -392,7 +403,7 @@ func TestWebhook(t *testing.T) {
 	}
 	defer s.Close()
 
-	wh, err := newAuthorizer(s.URL, clientCert, clientKey, caCert)
+	wh, err := newAuthorizer(s.URL, clientCert, clientKey, caCert, 0)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -477,3 +488,53 @@ func TestWebhook(t *testing.T) {
 		}
 	}
 }
+
+// TestWebhookCache verifies that error responses from the server are not
+// cached, but successful responses are.
+func TestWebhookCache(t *testing.T) {
+	serv := new(mockService)
+	s, err := NewTestServer(serv, serverCert, serverKey, caCert)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+
+	// Create an authorizer that caches successful responses "forever" (100 days).
+	wh, err := newAuthorizer(s.URL, clientCert, clientKey, caCert, 2400*time.Hour)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	attr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}}
+	serv.allow = true
+	serv.statusCode = 500
+	if err := wh.Authorize(attr); err == nil {
+		t.Errorf("Webhook returned HTTP 500, but authorizer reported success.")
+	}
+	serv.statusCode = 404
+	if err := wh.Authorize(attr); err == nil {
+		t.Errorf("Webhook returned HTTP 404, but authorizer reported success.")
+	}
+	serv.statusCode = 200
+	if err := wh.Authorize(attr); err != nil {
+		t.Errorf("Webhook returned HTTP 200, but authorizer reported unauthorized.")
+	}
+	serv.statusCode = 500
+	if err := wh.Authorize(attr); err != nil {
+		t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.")
+	}
+	// For a different request, webhook should be called again.
+	attr = authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "bob"}}
+	serv.statusCode = 500
+	if err := wh.Authorize(attr); err == nil {
+		t.Errorf("Webhook returned HTTP 500, but authorizer reported success.")
+	}
+	serv.statusCode = 200
+	if err := wh.Authorize(attr); err != nil {
+		t.Errorf("Webhook returned HTTP 200, but authorizer reported unauthorized.")
+	}
+	serv.statusCode = 500
+	if err := wh.Authorize(attr); err != nil {
+		t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.")
+	}
+}