Merge pull request #877 from haoming29/refactor-token-verify-v2

Replace ad-hoc token verification logic by token.Verify

director/origin_api.go

@@ -31,7 +31,6 @@ import (
 	"time"
 
 	"github.com/jellydator/ttlcache/v3"
-	"github.com/lestrrat-go/jwx/v2/jwa"
 	"github.com/lestrrat-go/jwx/v2/jwk"
 	"github.com/lestrrat-go/jwx/v2/jwt"
 	"github.com/pkg/errors"
@@ -41,7 +40,6 @@ import (
 	"github.com/pelicanplatform/pelican/config"
 	"github.com/pelicanplatform/pelican/param"
 	"github.com/pelicanplatform/pelican/server_utils"
-	"github.com/pelicanplatform/pelican/token"
 	"github.com/pelicanplatform/pelican/token_scopes"
 )
 
@@ -190,44 +188,3 @@ func VerifyAdvertiseToken(ctx context.Context, token, namespace string) (bool, e
 	}
 	return false, nil
 }
-
-// Verify that a token received is a valid token from director
-func VerifyDirectorTestReportToken(strToken string) (bool, error) {
-	directorURL := param.Federation_DirectorUrl.GetString()
-	parsedToken, err := jwt.Parse([]byte(strToken), jwt.WithVerify(false))
-	if err != nil {
-		return false, err
-	}
-
-	if directorURL != parsedToken.Issuer() {
-		return false, errors.Errorf("Token issuer is not a director")
-	}
-
-	key, err := token.LoadDirectorPublicKey()
-	if err != nil {
-		return false, err
-	}
-
-	tok, err := jwt.Parse([]byte(strToken), jwt.WithKey(jwa.ES256, key), jwt.WithValidate(true))
-	if err != nil {
-		return false, err
-	}
-
-	scope_any, present := tok.Get("scope")
-	if !present {
-		return false, errors.New("No scope is present; required to advertise to director")
-	}
-	scope, ok := scope_any.(string)
-	if !ok {
-		return false, errors.New("scope claim in token is not string-valued")
-	}
-
-	scopes := strings.Split(scope, " ")
-
-	for _, scope := range scopes {
-		if scope == token_scopes.Pelican_DirectorTestReport.String() {
-			return true, nil
-		}
-	}
-	return false, nil
-}

director/redirect.go

@@ -697,14 +697,14 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType common.S
 func discoverOriginCache(ctx *gin.Context) {
 	authOption := token.AuthOption{
 		Sources: []token.TokenSource{token.Header},
-		Issuers: []token.TokenIssuer{token.Issuer},
+		Issuers: []token.TokenIssuer{token.LocalIssuer},
 		Scopes:  []token_scopes.TokenScope{token_scopes.Pelican_DirectorServiceDiscovery},
 	}
 
-	ok := token.CheckAnyAuth(ctx, authOption)
+	status, ok, err := token.Verify(ctx, authOption)
 	if !ok {
-		log.Warningf("Invalid token for accessing director's sevice discovery")
-		ctx.JSON(401, gin.H{"error": "Invalid token for accessing director's sevice discovery"})
+		log.Warningf("Cannot verify token for accessing director's service discovery: %v", err)
+		ctx.JSON(status, gin.H{"error": err.Error()})
 		return
 	}
 

director/redirect_test.go

@@ -776,10 +776,10 @@ func TestDiscoverOriginCache(t *testing.T) {
 		w := httptest.NewRecorder()
 		r.ServeHTTP(w, req)
 
-		assert.Equal(t, 401, w.Code)
-		assert.Equal(t, `{"error":"Invalid token for accessing director's sevice discovery"}`, w.Body.String())
+		assert.Equal(t, 403, w.Code)
+		assert.Equal(t, `{"error":"Authentication is required but no token is present."}`, w.Body.String())
 	})
-	t.Run("token-present-with-wrong-issuer-should-give-401", func(t *testing.T) {
+	t.Run("token-present-with-wrong-issuer-should-give-403", func(t *testing.T) {
 		req, err := http.NewRequest(http.MethodGet, "/test", nil)
 		if err != nil {
 			t.Fatalf("Could not make a GET request: %v", err)
@@ -790,8 +790,8 @@ func TestDiscoverOriginCache(t *testing.T) {
 		w := httptest.NewRecorder()
 		r.ServeHTTP(w, req)
 
-		assert.Equal(t, 401, w.Code)
-		assert.Equal(t, `{"error":"Invalid token for accessing director's sevice discovery"}`, w.Body.String())
+		assert.Equal(t, 403, w.Code)
+		assert.Equal(t, `{"error":"Cannot verify token: Cannot verify token with server issuer:  Token issuer https://wrong-issuer.org does not match the local issuer on the current server. Expecting https://fake-director.org:8888\n"}`, w.Body.String())
 	})
 	t.Run("token-present-valid-should-give-200-and-empty-array", func(t *testing.T) {
 		req, err := http.NewRequest(http.MethodGet, "/test", nil)

go.mod

@@ -19,7 +19,6 @@ require (
 	github.com/hashicorp/go-version v1.6.0
 	github.com/jellydator/ttlcache/v3 v3.1.0
 	github.com/jsipprell/keyctl v1.0.4-0.20211208153515-36ca02672b6c
-	github.com/lestrrat-go/httprc v1.0.4
 	github.com/lestrrat-go/jwx/v2 v2.0.16
 	github.com/minio/minio-go/v7 v7.0.65
 	github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f
@@ -65,6 +64,7 @@ require (
 	github.com/jinzhu/inflection v1.0.0 // indirect
 	github.com/jinzhu/now v1.1.5 // indirect
 	github.com/joncrlsn/dque v0.0.0-20211108142734-c2ef48c5192a // indirect
+	github.com/lestrrat-go/httprc v1.0.4 // indirect
 	github.com/mfridman/interpolate v0.0.2 // indirect
 	github.com/redis/go-redis/v9 v9.0.2 // indirect
 	github.com/sagikazarmark/locafero v0.4.0 // indirect

origin_ui/origin_api.go

@@ -21,14 +21,15 @@ package origin_ui
 import (
 	"context"
 	"fmt"
-	"strings"
+	"net/http"
 	"sync"
 	"time"
 
 	"github.com/gin-gonic/gin"
 	"github.com/pelicanplatform/pelican/common"
-	"github.com/pelicanplatform/pelican/director"
 	"github.com/pelicanplatform/pelican/metrics"
+	"github.com/pelicanplatform/pelican/token"
+	"github.com/pelicanplatform/pelican/token_scopes"
 	"github.com/pkg/errors"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/sync/errgroup"
@@ -62,42 +63,6 @@ func getNotifyChannel() chan bool {
 	return notifyChannel
 }
 
-// Check the Bearer token from requests sent from the director to ensure
-// it's has correct authorization
-func directorRequestAuthHandler(ctx *gin.Context) {
-	authHeader := ctx.Request.Header.Get("Authorization")
-
-	// Check if the Authorization header was provided
-	if authHeader == "" {
-		// Use AbortWithStatusJSON to stop invoking the next chain
-		ctx.AbortWithStatusJSON(401, gin.H{"error": "Authorization header is missing"})
-		return
-	}
-
-	// Check if the Authorization type is Bearer
-	if !strings.HasPrefix(authHeader, "Bearer ") {
-		ctx.AbortWithStatusJSON(401, gin.H{"error": "Authorization header is not Bearer type"})
-		return
-	}
-
-	// Extract the token from the Authorization header
-	token := strings.TrimPrefix(authHeader, "Bearer ")
-	valid, err := director.VerifyDirectorTestReportToken(token)
-
-	if err != nil {
-		log.Warningln(fmt.Sprintf("Error when verifying Bearer token: %s", err))
-		ctx.AbortWithStatusJSON(401, gin.H{"error": fmt.Sprintf("Error when verifying Bearer token: %s", err)})
-		return
-	}
-
-	if !valid {
-		log.Warningln("Can't validate Bearer token")
-		ctx.AbortWithStatusJSON(401, gin.H{"error": "Can't validate Bearer token"})
-		return
-	}
-	ctx.Next()
-}
-
 // Reset the timer safely
 func LaunchPeriodicDirectorTimeout(ctx context.Context, egrp *errgroup.Group) {
 	directorTimeoutTicker := time.NewTicker(directorTimeoutDuration)
@@ -121,28 +86,36 @@ func LaunchPeriodicDirectorTimeout(ctx context.Context, egrp *errgroup.Group) {
 	})
 }
 
-// Director will periodically upload/download files to/from all connected
-// origins and test the health status of origins. It will send a request
-// reporting such status to this endpoint, and we will update origin internal
-// health status metric to reflect the director connection status.
+// The director periodically uploads/downloads files to/from all online
+// origins for testing. It sends a request reporting the status of the test result to this endpoint,
+// and we will update origin internal health status metric by what director returns.
 func directorTestResponse(ctx *gin.Context) {
+	status, ok, err := token.Verify(ctx, token.AuthOption{
+		Sources: []token.TokenSource{token.Header},
+		Issuers: []token.TokenIssuer{token.FederationIssuer},
+		Scopes:  []token_scopes.TokenScope{token_scopes.Pelican_DirectorTestReport},
+	})
+	if !ok {
+		ctx.JSON(status, gin.H{"error": err.Error()})
+	}
+
 	dt := common.DirectorTestResult{}
 	if err := ctx.ShouldBind(&dt); err != nil {
-		log.Errorf("Invalid director test response")
-		ctx.JSON(400, gin.H{"error": "Invalid director test response"})
+		log.Errorf("Invalid director test response: %v", err)
+		ctx.JSON(http.StatusBadRequest, gin.H{"error": "Invalid director test response: " + err.Error()})
 		return
 	}
 	// We will let the timer go timeout if director didn't send a valid json request
 	notifyNewDirectorResponse(ctx)
 	if dt.Status == "ok" {
 		metrics.SetComponentHealthStatus(metrics.OriginCache_Director, metrics.StatusOK, fmt.Sprintf("Director timestamp: %v", dt.Timestamp))
-		ctx.JSON(200, gin.H{"msg": "Success"})
+		ctx.JSON(http.StatusOK, gin.H{"msg": "Success"})
 	} else if dt.Status == "error" {
 		metrics.SetComponentHealthStatus(metrics.OriginCache_Director, metrics.StatusCritical, dt.Message)
-		ctx.JSON(200, gin.H{"msg": "Success"})
+		ctx.JSON(http.StatusOK, gin.H{"msg": "Success"})
 	} else {
 		log.Errorf("Invalid director test response, status: %s", dt.Status)
-		ctx.JSON(400, gin.H{"error": fmt.Sprintf("Invalid director test response status: %s", dt.Status)})
+		ctx.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("Invalid director test response status: %s", dt.Status)})
 	}
 }
 
@@ -157,7 +130,7 @@ func ConfigureOriginAPI(router *gin.Engine, ctx context.Context, egrp *errgroup.
 	LaunchPeriodicDirectorTimeout(ctx, egrp)
 
 	group := router.Group("/api/v1.0/origin-api")
-	group.POST("/directorTest", directorRequestAuthHandler, directorTestResponse)
+	group.POST("/directorTest", directorTestResponse)
 
 	return nil
 }

registry/registry.go

@@ -631,29 +631,8 @@ func deleteNamespaceHandler(ctx *gin.Context) {
 		return
 	}
 
-	/*
-	* The signature is verified, now we need to make sure this token actually gives us
-	* permission to delete the namespace from the db. Let's check the subject and the scope.
-	* NOTE: The validate function also handles checking `iat` and `exp` to make sure the token
-	*       remains valid.
-	 */
-	scopeValidator := jwt.ValidatorFunc(func(_ context.Context, tok jwt.Token) jwt.ValidationError {
-		scope_any, present := tok.Get("scope")
-		if !present {
-			return jwt.NewValidationError(errors.New("No scope is present; required for authorization"))
-		}
-		scope, ok := scope_any.(string)
-		if !ok {
-			return jwt.NewValidationError(errors.New("scope claim in token is not string-valued"))
-		}
+	scopeValidator := token_scopes.CreateScopeValidator([]token_scopes.TokenScope{token_scopes.Pelican_NamespaceDelete}, true)
 
-		for _, scope := range strings.Split(scope, " ") {
-			if scope == token_scopes.Pelican_NamespaceDelete.String() {
-				return nil
-			}
-		}
-		return jwt.NewValidationError(errors.New("Token does not contain namespace deletion authorization"))
-	})
 	if err = jwt.Validate(parsed, jwt.WithValidator(scopeValidator)); err != nil {
 		ctx.JSON(http.StatusInternalServerError, gin.H{"error": "server could not validate the provided deletion token"})
 		log.Errorf("Failed to validate the token: %v", err)