Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check HTTP Status code in webhook authorizer/authenticator. #27889

Merged
merged 1 commit into from
Jun 23, 2016
Merged

Check HTTP Status code in webhook authorizer/authenticator. #27889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion plugin/pkg/auth/authenticator/token/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ limitations under the License.
package webhook

import (
"fmt"
"time"

"k8s.io/kubernetes/pkg/api/unversioned"
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you want to cache 400 responses for a shorter period, or am I misremembering?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to cache error responses from the webhook server at all. We do want to cache our successful "No" responses.

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
Expand Down
74 changes: 67 additions & 7 deletions plugin/pkg/auth/authenticator/token/webhook/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"os"
"reflect"
"testing"
"time"

"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/apis/authentication.k8s.io/v1beta1"
Expand All @@ -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.
Expand Down Expand Up @@ -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"`
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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{}

Expand All @@ -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)
}
Expand Down Expand Up @@ -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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about the cache.

}
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.")
}
}
17 changes: 10 additions & 7 deletions plugin/pkg/auth/authorizer/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package webhook
import (
"encoding/json"
"errors"
"fmt"
"time"

"k8s.io/kubernetes/pkg/api/unversioned"
Expand Down Expand Up @@ -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
Expand Down
75 changes: 68 additions & 7 deletions plugin/pkg/auth/authorizer/webhook/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"reflect"
"testing"
"text/template"
"time"

"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/apis/authorization/v1beta1"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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"`
Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
Expand Down Expand Up @@ -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.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make sure the cache is correct after these Authorize calls?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, but it'd pretty much be copy-pasting from the internals of the actual authorize call. The subsequent calls are intended to make sure the cache is correct (i.e. not holding onto the 500).

}
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.")
}
}