From ae67a4e2095f59fdc1be10ff4f69f36e2a6f2db0 Mon Sep 17 00:00:00 2001 From: CJ Cullen Date: Wed, 22 Jun 2016 11:06:56 -0700 Subject: [PATCH] Check HTTP Status code in webhook authorizer/authenticator. --- .../authenticator/token/webhook/webhook.go | 7 +- .../token/webhook/webhook_test.go | 74 ++++++++++++++++-- plugin/pkg/auth/authorizer/webhook/webhook.go | 17 +++-- .../auth/authorizer/webhook/webhook_test.go | 75 +++++++++++++++++-- 4 files changed, 151 insertions(+), 22 deletions(-) diff --git a/plugin/pkg/auth/authenticator/token/webhook/webhook.go b/plugin/pkg/auth/authenticator/token/webhook/webhook.go index ad298a39aae..43e76d00d87 100644 --- a/plugin/pkg/auth/authenticator/token/webhook/webhook.go +++ b/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 diff --git a/plugin/pkg/auth/authenticator/token/webhook/webhook_test.go b/plugin/pkg/auth/authenticator/token/webhook/webhook_test.go index 9dfcb8d0346..cbf63a24603 100644 --- a/plugin/pkg/auth/authenticator/token/webhook/webhook_test.go +++ b/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) Deny() { m.allow = false } +func (m *mockService) Allow() { m.allow = true } +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.") + } +} diff --git a/plugin/pkg/auth/authorizer/webhook/webhook.go b/plugin/pkg/auth/authorizer/webhook/webhook.go index eeaf2af1d37..956789864f5 100644 --- a/plugin/pkg/auth/authorizer/webhook/webhook.go +++ b/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 { - w.responseCache.Add(string(key), r.Status, w.authorizedTTL) - } else { - w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL) - } - }() + if r.Status.Allowed { + w.responseCache.Add(string(key), r.Status, w.authorizedTTL) + } else { + w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL) + } } if r.Status.Allowed { return nil diff --git a/plugin/pkg/auth/authorizer/webhook/webhook_test.go b/plugin/pkg/auth/authorizer/webhook/webhook_test.go index deb332960f4..2f95751c02d 100644 --- a/plugin/pkg/auth/authorizer/webhook/webhook_test.go +++ b/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) Deny() { m.allow = false } +func (m *mockService) Allow() { m.allow = true } +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.") + } +}