Auth: Add sub claim check to JWT Auth pre-checks (#61417)

* Auth: Add sub claim check to JWT Auth pre-checks

* Add #nosec annotation to the test tokens

pkg/middleware/middleware_jwt_auth_test.go

@@ -55,7 +55,8 @@ func TestMiddlewareJWTAuth(t *testing.T) {
 		cfg.JWTAuthAllowAssignGrafanaAdmin = true
 	}
 
-	token := "some-token"
+	// #nosec G101 -- This is dummy/test token
+	token := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ2bGFkaW1pckBleGFtcGxlLmNvbSIsImlhdCI6MTUxNjIzOTAyMiwiZm9vLXVzZXJuYW1lIjoidmxhZGltaXIiLCJuYW1lIjoiVmxhZGltaXIgRXhhbXBsZSIsImZvby1lbWFpbCI6InZsYWRpbWlyQGV4YW1wbGUuY29tIn0.MeNU1pCzRHGdQuu5ppeftxT31_2Le2kM1wd1GK2jExs"
 
 	middlewareScenario(t, "Valid token with valid login claim", func(t *testing.T, sc *scenarioContext) {
 		myUsername := "vladimir"
@@ -85,7 +86,7 @@ func TestMiddlewareJWTAuth(t *testing.T) {
 		myUsername := "vladimir"
 		// We can ignore gosec G101 since this does not contain any credentials.
 		// nolint:gosec
-		myToken := "some.jwt.token"
+		myToken := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ2bGFkaW1pckBleGFtcGxlLmNvbSIsImlhdCI6MTUxNjIzOTAyMiwiZm9vLXVzZXJuYW1lIjoidmxhZGltaXIiLCJuYW1lIjoiVmxhZGltaXIgRXhhbXBsZSIsImZvby1lbWFpbCI6InZsYWRpbWlyQGV4YW1wbGUuY29tIn0.MeNU1pCzRHGdQuu5ppeftxT31_2Le2kM1wd1GK2jExs"
 		var verifiedToken string
 		sc.jwtAuthService.VerifyProvider = func(ctx context.Context, token string) (models.JWTClaims, error) {
 			verifiedToken = myToken

pkg/middleware/middleware_test.go

@@ -234,6 +234,50 @@ func TestMiddlewareContext(t *testing.T) {
 		assert.Equal(t, org.RoleEditor, sc.context.OrgRole)
 	}, configureJWTAuthHeader)
 
+	middlewareScenario(t, "Valid Basic Auth header with JWT enabled and empty 'sub' claim", func(t *testing.T, sc *scenarioContext) {
+		const password = "MyPass"
+		const orgID int64 = 2
+		const userID int64 = 12
+		// #nosec G101 -- This is dummy/test token
+		const emptySubToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJzdWIiOiIiLCJpYXQiOjE1MTYyMzkwMjJ9.tnwtOHK58d47dO4DHW4b9MzeToxa1kGiko5Oo887Rqc"
+
+		sc.userService.ExpectedSignedInUser = &user.SignedInUser{OrgID: orgID, UserID: userID}
+		authHeader := util.GetBasicAuthHeader("myuser", password)
+		sc.fakeReq("GET", "/").withAuthorizationHeader(authHeader).withJWTAuthHeader(emptySubToken).exec()
+
+		require.Equal(t, 200, sc.resp.Code)
+
+		assert.True(t, sc.context.IsSignedIn)
+		assert.Equal(t, orgID, sc.context.OrgID)
+		assert.Equal(t, userID, sc.context.UserID)
+	}, func(cfg *setting.Cfg) {
+		cfg.JWTAuthEnabled = true
+		cfg.JWTAuthHeaderName = "X-JWT-Token"
+		cfg.BasicAuthEnabled = true
+	})
+
+	middlewareScenario(t, "Valid Basic Auth header with JWT enabled and missing 'sub' claim", func(t *testing.T, sc *scenarioContext) {
+		const password = "MyPass"
+		const orgID int64 = 2
+		const userID int64 = 12
+		// #nosec G101 -- This is dummy/test token
+		const missingSubToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpYXQiOjE1MTYyMzkwMjJ9.8nYFUX869Y1mnDDDU4yL11aANgVRuifoxrE8BHZY1iE"
+
+		sc.userService.ExpectedSignedInUser = &user.SignedInUser{OrgID: orgID, UserID: userID}
+		authHeader := util.GetBasicAuthHeader("myuser", password)
+		sc.fakeReq("GET", "/").withAuthorizationHeader(authHeader).withJWTAuthHeader(missingSubToken).exec()
+
+		require.Equal(t, 200, sc.resp.Code)
+
+		assert.True(t, sc.context.IsSignedIn)
+		assert.Equal(t, orgID, sc.context.OrgID)
+		assert.Equal(t, userID, sc.context.UserID)
+	}, func(cfg *setting.Cfg) {
+		cfg.JWTAuthEnabled = true
+		cfg.JWTAuthHeaderName = "X-JWT-Token"
+		cfg.BasicAuthEnabled = true
+	})
+
 	middlewareScenario(t, "Valid API key, but does not match DB hash", func(t *testing.T, sc *scenarioContext) {
 		const keyhash = "Something_not_matching"
 		sc.apiKeyService.ExpectedAPIKey = &apikey.APIKey{OrgId: 12, Role: org.RoleEditor, Key: keyhash}
@@ -696,7 +740,7 @@ func TestMiddlewareContext(t *testing.T) {
 		})
 
 		middlewareScenario(t, "Request body should not be read in default context handler, but query should be altered - jwt", func(t *testing.T, sc *scenarioContext) {
-			sc.fakeReq("POST", "/?targetOrgId=123&auth_token=token")
+			sc.fakeReq("POST", "/?targetOrgId=123&auth_token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NSIsImlhdCI6MTUxNjIzOTAyMn0.1E9qmtctlHAeJzNLPgGFfxdA8WfbEl_vwYO91ffQGxs")
 			body := "key=value"
 			sc.req.Body = io.NopCloser(strings.NewReader(body))
 

pkg/services/auth/jwt/auth.go

@@ -6,11 +6,12 @@ import (
 	"errors"
 	"strings"
 
+	"gopkg.in/square/go-jose.v2/jwt"
+
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/remotecache"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
-	"gopkg.in/square/go-jose.v2/jwt"
 )
 
 const ServiceName = "AuthService"
@@ -102,3 +103,19 @@ func (s *AuthService) Verify(ctx context.Context, strToken string) (models.JWTCl
 
 	return claims, nil
 }
+
+// HasSubClaim checks if the provided JWT token contains a non-empty "sub" claim.
+// Returns true if it contains, otherwise returns false.
+func HasSubClaim(jwtToken string) bool {
+	parsed, err := jwt.ParseSigned(sanitizeJWT(jwtToken))
+	if err != nil {
+		return false
+	}
+
+	var claims jwt.Claims
+	if err := parsed.UnsafeClaimsWithoutVerification(&claims); err != nil {
+		return false
+	}
+
+	return claims.Subject != ""
+}

pkg/services/authn/clients/jwt.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/auth"
+	authJWT "github.com/grafana/grafana/pkg/services/auth/jwt"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/login"
 	"github.com/grafana/grafana/pkg/services/org"
@@ -143,21 +144,14 @@ func (s *JWT) Test(ctx context.Context, r *authn.Request) bool {
 		return false
 	}
 
-	// The header is Authorization and the token does not look like a JWT,
+	// If the "sub" claim is missing or empty then pass the control to the next handler
-	// this is likely an API key. Pass it on.
+	if !authJWT.HasSubClaim(jwtToken) {
-	if s.cfg.JWTAuthHeaderName == "Authorization" && !looksLikeJWT(jwtToken) {
 		return false
 	}
 
 	return true
 }
 
-func looksLikeJWT(token string) bool {
-	// A JWT must have 3 parts separated by `.`.
-	parts := strings.Split(token, ".")
-	return len(parts) == 3
-}
-
 const roleGrafanaAdmin = "GrafanaAdmin"
 
 func (s *JWT) extractRoleAndAdmin(claims map[string]interface{}) (org.RoleType, bool) {

pkg/services/authn/clients/jwt_test.go

@@ -7,12 +7,13 @@ import (
 	"net/url"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/models/roletype"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/setting"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func stringPtr(s string) *string {
@@ -87,8 +88,13 @@ func TestAuthenticateJWT(t *testing.T) {
 func TestJWTTest(t *testing.T) {
 	jwtService := &models.FakeJWTService{}
 	jwtHeaderName := "X-Forwarded-User"
-	validFormatToken := "sample.token.valid"
+	// #nosec G101 -- This is dummy/test token
+	validFormatToken := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.XbPfbIHMI6arZ3Y922BhjWgQzWXcXNrz0ogtVhfEd2o"
 	invalidFormatToken := "sampletokeninvalid"
+	// #nosec G101 -- This is dummy/test token
+	missingSubToken := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpYXQiOjE1MTYyMzkwMjJ9.8nYFUX869Y1mnDDDU4yL11aANgVRuifoxrE8BHZY1iE"
+	// #nosec G101 -- This is dummy/test token
+	emptySubToken := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJzdWIiOiIiLCJpYXQiOjE1MTYyMzkwMjJ9.tnwtOHK58d47dO4DHW4b9MzeToxa1kGiko5Oo887Rqc"
 
 	type testCase struct {
 		desc          string
@@ -144,6 +150,20 @@ func TestJWTTest(t *testing.T) {
 			token:         validFormatToken,
 			want:          false,
 		},
+		{
+			desc:          "token without a sub claim",
+			reqHeaderName: "Authorization",
+			cfgHeaderName: "Authorization",
+			token:         missingSubToken,
+			want:          false,
+		},
+		{
+			desc:          "token with an empty sub claim",
+			reqHeaderName: "Authorization",
+			cfgHeaderName: "Authorization",
+			token:         emptySubToken,
+			want:          false,
+		},
 	}
 
 	for _, tc := range testCases {

pkg/services/contexthandler/auth_jwt.go

@@ -6,13 +6,15 @@ import (
 	"net/http"
 	"strings"
 
+	"github.com/jmespath/go-jmespath"
+
 	"github.com/grafana/grafana/pkg/login"
 	"github.com/grafana/grafana/pkg/models"
+	authJWT "github.com/grafana/grafana/pkg/services/auth/jwt"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/org"
 	"github.com/grafana/grafana/pkg/services/user"
-	"github.com/jmespath/go-jmespath"
 )
 
 const (
@@ -59,9 +61,8 @@ func (h *ContextHandler) initContextWithJWT(ctx *models.ReqContext, orgId int64)
 	// Strip the 'Bearer' prefix if it exists.
 	jwtToken = strings.TrimPrefix(jwtToken, "Bearer ")
 
-	// The header is Authorization and the token does not look like a JWT,
+	// If the "sub" claim is missing or empty then pass the control to the next handler
-	// this is likely an API key. Pass it on.
+	if !authJWT.HasSubClaim(jwtToken) {
-	if h.Cfg.JWTAuthHeaderName == "Authorization" && !looksLikeJWT(jwtToken) {
 		return false
 	}
 
@@ -222,9 +223,3 @@ func searchClaimsForStringAttr(attributePath string, claims map[string]interface
 
 	return "", nil
 }
-
-func looksLikeJWT(token string) bool {
-	// A JWT must have 3 parts separated by `.`.
-	parts := strings.Split(token, ".")
-	return len(parts) == 3
-}