diff --git a/src/frontend/apiserver/controllers/auth.go b/src/frontend/apiserver/controllers/auth.go index 0a7e7d2e..d039a6d5 100644 --- a/src/frontend/apiserver/controllers/auth.go +++ b/src/frontend/apiserver/controllers/auth.go @@ -16,6 +16,7 @@ import ( "github.com/gin-gonic/gin" "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" "go.uber.org/zap" ) @@ -48,6 +49,8 @@ func generateJWT(username string, IP string) (string, error) { Username: username, IP: IP, RegisteredClaims: jwt.RegisteredClaims{ + // unique token id: logout revokes by it (bug-1946) + ID: uuid.NewString(), ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Duration(utils.AuthenticationYamlConfig.Token.TokenExpireTime) * time.Second)), IssuedAt: jwt.NewNumericDate(time.Now()), NotBefore: jwt.NewNumericDate(time.Now()), @@ -99,11 +102,13 @@ func storeToken(token string, username string) error { return nil } -// removeToken removes the JWT file for a user. +// removeToken removes the JWT file for a user. A missing file is not an +// error: the file only mirrors the last token issued, and revocation does +// not depend on it. func removeToken(username string) error { tokenDir := filepath.Join(utils.Global_TopDir, "tokens", username) tokenFile := filepath.Join(tokenDir, "token") - if err := os.Remove(tokenFile); err != nil { + if err := os.Remove(tokenFile); err != nil && !os.IsNotExist(err) { return err } // Remove user directory if empty @@ -175,15 +180,22 @@ func (a AuthController) Logout(ctx *gin.Context) { token := ctx.GetHeader("Authorization") // get username from token - var err error - username, err := GetUsernameFromToken(strings.TrimPrefix(token, "Bearer ")) + claims, err := ParseTokenClaims(token) if err != nil { logger.Error(common.ERROR_GET_USERNAME_MSG, zap.Error(err)) ReturnError(ctx, common.ERROR_CODE, common.ERROR_GET_USERNAME_MSG+": "+err.Error(), nil) return } + username := claims.Username logger.Debug("username", zap.String("username", username)) + // Revoke the token until its expiry: validation rejects it from now on. + expiresAt := time.Now().Add(time.Duration(utils.AuthenticationYamlConfig.Token.TokenExpireTime) * time.Second) + if claims.ExpiresAt != nil { + expiresAt = claims.ExpiresAt.Time + } + RevokeToken(token, claims.ID, expiresAt) + // Remove token file from server-managed directory if err := removeToken(username); err != nil { logger.Error(common.ERROR_REMOVE_TOKEN_MSG, zap.Error(err)) @@ -198,19 +210,33 @@ func (a AuthController) Logout(ctx *gin.Context) { ReturnSuccess(ctx, common.SUCCESS_CODE, common.SUCCESS_MSG, response) } -func GetUsernameFromToken(tokenString string) (string, error) { - token, err := jwt.Parse(strings.TrimPrefix(tokenString, "Bearer "), JWTKeyFunc, JWTValidMethods()) +// ParseTokenClaims parses and verifies a token ("Bearer " prefix optional) +// and rejects a token revoked by logout. +func ParseTokenClaims(tokenString string) (*models.JWTClaims, error) { + tokenString = strings.TrimPrefix(tokenString, "Bearer ") + claims := &models.JWTClaims{} + token, err := jwt.ParseWithClaims(tokenString, claims, JWTKeyFunc, JWTValidMethods()) + if err != nil { + return nil, err + } + if !token.Valid { + return nil, fmt.Errorf("invalid token") + } + if IsTokenRevoked(tokenString, claims.ID) { + return nil, fmt.Errorf("token has been revoked") + } + return claims, nil +} +func GetUsernameFromToken(tokenString string) (string, error) { + claims, err := ParseTokenClaims(tokenString) if err != nil { return "", err } - - if claims, ok := token.Claims.(jwt.MapClaims); ok && token.Valid { - if username, ok := claims["username"].(string); ok { - return username, nil - } + if claims.Username == "" { + return "", fmt.Errorf("invalid token") } - return "", fmt.Errorf("invalid token") + return claims.Username, nil } // getAccessQueues returns the list of queues the user can access for the given permission. diff --git a/src/frontend/apiserver/controllers/integration_test.go b/src/frontend/apiserver/controllers/integration_test.go index a57b989d..1d965315 100644 --- a/src/frontend/apiserver/controllers/integration_test.go +++ b/src/frontend/apiserver/controllers/integration_test.go @@ -591,9 +591,10 @@ users: } } -// --- 6.7 P1: Post-logout token reuse (design gap documentation) --- -// JWTs are stateless — no JTI blacklist. A valid token remains usable until expiry. -func TestIntegration_DesignGap_PostLogoutTokenReuse(t *testing.T) { +// --- 6.7 P1: Post-logout token reuse is rejected (bug-1946) --- +// Logout revokes the token until its expiry (in-memory, per process). This +// token has no jti, so it is revoked by its hash. +func TestIntegration_PostLogoutTokenRejected(t *testing.T) { resetIntegrationState() initTestAuthConfig() @@ -602,14 +603,17 @@ func TestIntegration_DesignGap_PostLogoutTokenReuse(t *testing.T) { ctx.JSON(http.StatusOK, gin.H{"status": "ok"}) }) - // Create a valid long-lived token - token := signTestJWT("user1", "basic_user", time.Now().Add(time.Hour)) + expiresAt := time.Now().Add(time.Hour) + token := signTestJWT("user1", "basic_user", expiresAt) + // No RBAC roles are loaded here, so the token passes authentication and + // fails authorization (403); only the 401 matters. + if w := doRequest(router, "GET", "/xce/v1/hosts", token); w.Code == http.StatusUnauthorized { + t.Fatalf("before logout: status = 401, the token must authenticate") + } - // "Logout" is a no-op in stateless JWT — the token is still valid - w := doRequest(router, "GET", "/xce/v1/hosts", token) - if w.Code == http.StatusUnauthorized { - t.Error("post-logout token reuse: got 401, but JWTs are stateless and no blacklist exists — " + - "if this fails, a JTI blacklist has been implemented and this test should be updated") + RevokeToken(token, "", expiresAt) + if w := doRequest(router, "GET", "/xce/v1/hosts", token); w.Code != http.StatusUnauthorized { + t.Errorf("after logout: status = %d, want 401", w.Code) } } diff --git a/src/frontend/apiserver/controllers/token_revocation.go b/src/frontend/apiserver/controllers/token_revocation.go new file mode 100644 index 00000000..15bcb809 --- /dev/null +++ b/src/frontend/apiserver/controllers/token_revocation.go @@ -0,0 +1,73 @@ +package controllers + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "sync" + "time" +) + +// tokenRevocations is the in-memory set of logged-out tokens (bug-1946). +// Each entry is kept until the token's own expiry, after which the token is +// rejected anyway; expired entries are purged on every revocation, so the set +// holds at most the tokens logged out within one token lifetime. +// +// Scope: per process. The set is not persisted and not shared, so a revoked +// token is accepted again by another apiserver sharing XCE_JWT_SECRET, or by +// this one after a restart with the same XCE_JWT_SECRET. +type tokenRevocations struct { + mu sync.Mutex + expires map[string]time.Time // revocation key -> token expiry +} + +var revokedTokens = &tokenRevocations{expires: make(map[string]time.Time)} + +// tokenRevocationKey identifies a token: its jti when present (every token +// generateJWT issues has one), otherwise a SHA-256 of the raw token string. +func tokenRevocationKey(tokenString, jti string) string { + if jti != "" { + return "jti:" + jti + } + sum := sha256.Sum256([]byte(strings.TrimPrefix(tokenString, "Bearer "))) + return "sha256:" + hex.EncodeToString(sum[:]) +} + +// revoke records key as revoked until expiresAt and purges expired entries. +func (r *tokenRevocations) revoke(key string, expiresAt time.Time, now time.Time) { + r.mu.Lock() + defer r.mu.Unlock() + for k, exp := range r.expires { + if !exp.After(now) { + delete(r.expires, k) + } + } + if expiresAt.After(now) { + r.expires[key] = expiresAt + } +} + +// isRevoked reports whether key was revoked and has not expired yet. +func (r *tokenRevocations) isRevoked(key string, now time.Time) bool { + r.mu.Lock() + defer r.mu.Unlock() + exp, ok := r.expires[key] + return ok && exp.After(now) +} + +// size returns the number of entries kept (for tests). +func (r *tokenRevocations) size() int { + r.mu.Lock() + defer r.mu.Unlock() + return len(r.expires) +} + +// RevokeToken revokes a token until its expiry. +func RevokeToken(tokenString, jti string, expiresAt time.Time) { + revokedTokens.revoke(tokenRevocationKey(tokenString, jti), expiresAt, time.Now()) +} + +// IsTokenRevoked reports whether a token was revoked by logout. +func IsTokenRevoked(tokenString, jti string) bool { + return revokedTokens.isRevoked(tokenRevocationKey(tokenString, jti), time.Now()) +} diff --git a/src/frontend/apiserver/controllers/token_revocation_test.go b/src/frontend/apiserver/controllers/token_revocation_test.go new file mode 100644 index 00000000..014452a9 --- /dev/null +++ b/src/frontend/apiserver/controllers/token_revocation_test.go @@ -0,0 +1,45 @@ +package controllers + +import ( + "testing" + "time" +) + +// bug-1946: revocation entries live until the token's expiry and are purged +// afterwards, so memory stays bounded. +func TestTokenRevocations_ExpiryAndPurge(t *testing.T) { + r := &tokenRevocations{expires: make(map[string]time.Time)} + now := time.Unix(1_000_000, 0) + + r.revoke("a", now.Add(time.Hour), now) + r.revoke("b", now.Add(2*time.Hour), now) + r.revoke("already-expired", now.Add(-time.Second), now) + if r.size() != 2 { + t.Fatalf("size = %d, want 2 (an already expired token is not stored)", r.size()) + } + if !r.isRevoked("a", now) || !r.isRevoked("b", now) { + t.Fatal("revoked tokens must be reported as revoked") + } + if r.isRevoked("c", now) { + t.Error("a token never revoked must not be reported as revoked") + } + + later := now.Add(90 * time.Minute) + if r.isRevoked("a", later) { + t.Error("an entry past its token expiry must not count") + } + r.revoke("c", later.Add(time.Hour), later) + if r.size() != 2 { + t.Errorf("size = %d, want 2: the expired entry a must be purged", r.size()) + } +} + +func TestTokenRevocationKey(t *testing.T) { + if got := tokenRevocationKey("tok", "id-1"); got != "jti:id-1" { + t.Errorf("with jti: %q", got) + } + a, b := tokenRevocationKey("Bearer tok", ""), tokenRevocationKey("tok", "") + if a != b || a == "" || a == "jti:" { + t.Errorf("without jti the key must be the token hash, Bearer prefix ignored: %q %q", a, b) + } +} diff --git a/src/frontend/apiserver/middleware/validatehandler.go b/src/frontend/apiserver/middleware/validatehandler.go index 4b61c316..76b0739c 100644 --- a/src/frontend/apiserver/middleware/validatehandler.go +++ b/src/frontend/apiserver/middleware/validatehandler.go @@ -3,7 +3,6 @@ package middleware import ( "apiserver/common" "apiserver/controllers" - "apiserver/models" logger "apiserver/utils" "bytes" "encoding/json" @@ -13,7 +12,6 @@ import ( "strings" "github.com/gin-gonic/gin" - "github.com/golang-jwt/jwt/v4" "go.uber.org/zap" ) @@ -98,20 +96,14 @@ func validateHeader() gin.HandlerFunc { } } +// isValidToken verifies the token (signature, HS256 only, expiry, not +// revoked by logout) and its IP binding. func isValidToken(tokenString string, IP string) (bool, error) { - token, err := jwt.ParseWithClaims(tokenString, &models.JWTClaims{}, - controllers.JWTKeyFunc, controllers.JWTValidMethods()) - + claims, err := controllers.ParseTokenClaims(tokenString) if err != nil { return false, err } - - if claims, ok := token.Claims.(*models.JWTClaims); ok && token.Valid { - if claims.IP == IP { - return true, nil - } - } - return false, nil + return claims.IP == IP, nil } func validateRequestBody(ctx *gin.Context) (bool, error) { diff --git a/src/frontend/apiserver/middleware/validatehandler_test.go b/src/frontend/apiserver/middleware/validatehandler_test.go index d528d825..eebcd91d 100644 --- a/src/frontend/apiserver/middleware/validatehandler_test.go +++ b/src/frontend/apiserver/middleware/validatehandler_test.go @@ -1,11 +1,18 @@ package middleware import ( + "apiserver/backend/auth" + "apiserver/controllers" "apiserver/models" "apiserver/utils" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" "testing" "time" + "github.com/gin-gonic/gin" "github.com/golang-jwt/jwt/v4" "go.uber.org/zap" ) @@ -45,6 +52,81 @@ func signTestToken(t *testing.T, method jwt.SigningMethod, username, ip string) return signed } +// newAuthFlowRouter wires logon, logout and one protected route behind +// ValidateHandler, with a config_file provider holding alice/s3cret. +func newAuthFlowRouter(t *testing.T) *gin.Engine { + t.Helper() + setTestAuthConfig(t) + prevTop, prevProvider := utils.Global_TopDir, auth.GlobalProvider + t.Cleanup(func() { utils.Global_TopDir, auth.GlobalProvider = prevTop, prevProvider }) + utils.Global_TopDir = t.TempDir() + provider, err := auth.NewConfigFileProvider([]byte( + "provider: config_file\nconfig_file:\n users:\n - username: alice\n password: s3cret\n")) + if err != nil { + t.Fatalf("NewConfigFileProvider: %v", err) + } + auth.GlobalProvider = provider + + gin.SetMode(gin.TestMode) + r := gin.New() + r.Use(ValidateHandler()) + authHandler := &controllers.AuthController{} + r.POST("/xce/v1/auth/logon", authHandler.Logon) + r.POST("/xce/v1/auth/logout", authHandler.Logout) + r.GET("/xce/v1/hosts", func(ctx *gin.Context) { + controllers.ReturnSuccess(ctx, http.StatusOK, "success", nil) + }) + return r +} + +func serve(r *gin.Engine, method, path, token, body string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req.RemoteAddr = "10.0.0.1:4000" + if body != "" { + req.Header.Set("Content-Type", "application/json") + } + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w +} + +func logon(t *testing.T, r *gin.Engine) string { + t.Helper() + w := serve(r, http.MethodPost, "/xce/v1/auth/logon", "", `{"username":"alice","password":"s3cret"}`) + var resp struct { + Data models.LogonResponse `json:"data"` + } + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil || resp.Data.Token == "" { + t.Fatalf("logon failed: status %d body %s", w.Code, w.Body.String()) + } + return resp.Data.Token +} + +// bug-1946: after logout the token is rejected; a new logon (even in the +// same second) gets a working token. +func TestLogout_RevokesToken(t *testing.T) { + r := newAuthFlowRouter(t) + + token := logon(t, r) + if w := serve(r, http.MethodGet, "/xce/v1/hosts", token, ""); w.Code != http.StatusOK { + t.Fatalf("before logout: status = %d, want 200 (%s)", w.Code, w.Body.String()) + } + if w := serve(r, http.MethodPost, "/xce/v1/auth/logout", token, ""); w.Code != http.StatusOK { + t.Fatalf("logout: status = %d, want 200 (%s)", w.Code, w.Body.String()) + } + if w := serve(r, http.MethodGet, "/xce/v1/hosts", token, ""); w.Code != http.StatusUnauthorized { + t.Errorf("after logout: status = %d, want 401 (%s)", w.Code, w.Body.String()) + } + + fresh := logon(t, r) + if w := serve(r, http.MethodGet, "/xce/v1/hosts", fresh, ""); w.Code != http.StatusOK { + t.Errorf("token from a new logon: status = %d, want 200 (%s)", w.Code, w.Body.String()) + } +} + // bug-1950: only the HMAC algorithm used for signing (HS256) is accepted. func TestIsValidToken_PinsSigningAlgorithm(t *testing.T) { setTestAuthConfig(t)