From 0902c55c8b61a30a4ae3ba8d208d6ad8ddcad0ff Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <jliggitt@redhat.com>
Date: Wed, 4 Jan 2017 01:29:30 -0500
Subject: [PATCH] Ensure invalid token returns 401 error

---
 .../authenticator/bearertoken/bearertoken.go  | 18 +++++++++++++++-
 .../bearertoken/bearertoken_test.go           | 21 ++++++++++++++++++-
 test/e2e/kubectl.go                           |  3 +--
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/pkg/auth/authenticator/bearertoken/bearertoken.go b/pkg/auth/authenticator/bearertoken/bearertoken.go
index 35f523d6820..6385493b578 100644
--- a/pkg/auth/authenticator/bearertoken/bearertoken.go
+++ b/pkg/auth/authenticator/bearertoken/bearertoken.go
@@ -17,6 +17,7 @@ limitations under the License.
 package bearertoken
 
 import (
+	"errors"
 	"net/http"
 	"strings"
 
@@ -32,6 +33,8 @@ func New(auth authenticator.Token) *Authenticator {
 	return &Authenticator{auth}
 }
 
+var invalidToken = errors.New("invalid bearer token")
+
 func (a *Authenticator) AuthenticateRequest(req *http.Request) (user.Info, bool, error) {
 	auth := strings.TrimSpace(req.Header.Get("Authorization"))
 	if auth == "" {
@@ -43,5 +46,18 @@ func (a *Authenticator) AuthenticateRequest(req *http.Request) (user.Info, bool,
 	}
 
 	token := parts[1]
-	return a.auth.AuthenticateToken(token)
+
+	// Empty bearer tokens aren't valid
+	if len(token) == 0 {
+		return nil, false, nil
+	}
+
+	user, ok, err := a.auth.AuthenticateToken(token)
+
+	// If the token authenticator didn't error, provide a default error
+	if !ok && err == nil {
+		err = invalidToken
+	}
+
+	return user, ok, err
 }
diff --git a/pkg/auth/authenticator/bearertoken/bearertoken_test.go b/pkg/auth/authenticator/bearertoken/bearertoken_test.go
index 0a90df2611a..60a0943df26 100644
--- a/pkg/auth/authenticator/bearertoken/bearertoken_test.go
+++ b/pkg/auth/authenticator/bearertoken/bearertoken_test.go
@@ -47,9 +47,28 @@ func TestAuthenticateRequestTokenInvalid(t *testing.T) {
 	user, ok, err := auth.AuthenticateRequest(&http.Request{
 		Header: http.Header{"Authorization": []string{"Bearer token"}},
 	})
-	if ok || user != nil || err != nil {
+	if ok || user != nil {
 		t.Errorf("expected not authenticated user")
 	}
+	if err != invalidToken {
+		t.Errorf("expected invalidToken error, got %v", err)
+	}
+}
+
+func TestAuthenticateRequestTokenInvalidCustomError(t *testing.T) {
+	customError := errors.New("custom")
+	auth := New(authenticator.TokenFunc(func(token string) (user.Info, bool, error) {
+		return nil, false, customError
+	}))
+	user, ok, err := auth.AuthenticateRequest(&http.Request{
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	})
+	if ok || user != nil {
+		t.Errorf("expected not authenticated user")
+	}
+	if err != customError {
+		t.Errorf("expected custom error, got %v", err)
+	}
 }
 
 func TestAuthenticateRequestTokenError(t *testing.T) {
diff --git a/test/e2e/kubectl.go b/test/e2e/kubectl.go
index b6c68589262..e4bcb429742 100644
--- a/test/e2e/kubectl.go
+++ b/test/e2e/kubectl.go
@@ -599,8 +599,7 @@ var _ = framework.KubeDescribe("Kubectl client", func() {
 			Expect(err).To(ContainSubstring("Using in-cluster namespace"))
 			Expect(err).To(ContainSubstring("Using in-cluster configuration"))
 			Expect(err).To(ContainSubstring("Authorization: Bearer invalid"))
-			// TODO(kubernetes/kubernetes#39267): We should only see a 401 from an invalid bearer token.
-			Expect(err).To(Or(ContainSubstring("Response Status: 403 Forbidden"), ContainSubstring("Response Status: 401 Unauthorized")))
+			Expect(err).To(ContainSubstring("Response Status: 401 Unauthorized"))
 
 			By("trying to use kubectl with invalid server")
 			_, err = framework.RunHostCmd(ns, simplePodName, "/kubectl get pods --server=invalid --v=6 2>&1")