Merge pull request #39408 from liggitt/invalid-token-401

Automatic merge from submit-queue

Ensure invalid token returns 401 error, not 403

fixes #39267

If a user attempts to use a bearer token, and the token is rejected, the authenticator should return an error. This distinguishes requests that did not provide a bearer token (and are unauthenticated without error) from ones that attempted to, and failed.

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
 }

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) {

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