From 6cc95e11aec1d2744496565acbf6abd4f6435714 Mon Sep 17 00:00:00 2001 From: andres-portainer <91705312+andres-portainer@users.noreply.github.com> Date: Fri, 30 Aug 2024 20:24:14 -0300 Subject: [PATCH] fix(bouncer): add support for JWT revocation BE-11179 (#12165) --- api/http/handler/auth/logout.go | 2 + api/http/security/bouncer.go | 57 +++++++++++++++++--- api/http/security/bouncer_test.go | 59 +++++++++++++++++++++ api/internal/testhelpers/request_bouncer.go | 2 + api/jwt/jwt.go | 11 ++-- api/jwt/jwt_test.go | 55 +++++++++++++++++++ api/portainer.go | 2 +- 7 files changed, 176 insertions(+), 12 deletions(-) diff --git a/api/http/handler/auth/logout.go b/api/http/handler/auth/logout.go index f551cbcf1..f7ef9d6a0 100644 --- a/api/http/handler/auth/logout.go +++ b/api/http/handler/auth/logout.go @@ -28,5 +28,7 @@ func (handler *Handler) logout(w http.ResponseWriter, r *http.Request) *httperro security.RemoveAuthCookie(w) + handler.bouncer.RevokeJWT(tokenData.Token) + return response.Empty(w) } diff --git a/api/http/security/bouncer.go b/api/http/security/bouncer.go index 56edd7f20..3d684a667 100644 --- a/api/http/security/bouncer.go +++ b/api/http/security/bouncer.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "strings" + "sync" "time" portainer "github.com/portainer/portainer/api" @@ -16,6 +17,9 @@ import ( "github.com/pkg/errors" ) +const apiKeyHeader = "X-API-KEY" +const jwtTokenHeader = "Authorization" + type ( BouncerService interface { PublicAccess(http.Handler) http.Handler @@ -30,6 +34,7 @@ type ( TrustedEdgeEnvironmentAccess(dataservices.DataStoreTx, *portainer.Endpoint) error CookieAuthLookup(*http.Request) (*portainer.TokenData, error) JWTAuthLookup(*http.Request) (*portainer.TokenData, error) + RevokeJWT(string) } // RequestBouncer represents an entity that manages API request accesses @@ -37,6 +42,7 @@ type ( dataStore dataservices.DataStore jwtService portainer.JWTService apiKeyService apikey.APIKeyService + revokedJWT sync.Map } // RestrictedRequestContext is a data structure containing information @@ -52,16 +58,22 @@ type ( tokenLookup func(*http.Request) (*portainer.TokenData, error) ) -const apiKeyHeader = "X-API-KEY" -const jwtTokenHeader = "Authorization" +var ( + ErrInvalidKey = errors.New("Invalid API key") + ErrRevokedJWT = errors.New("the JWT has been revoked") +) // NewRequestBouncer initializes a new RequestBouncer func NewRequestBouncer(dataStore dataservices.DataStore, jwtService portainer.JWTService, apiKeyService apikey.APIKeyService) *RequestBouncer { - return &RequestBouncer{ + b := &RequestBouncer{ dataStore: dataStore, jwtService: jwtService, apiKeyService: apiKeyService, } + + go b.cleanUpExpiredJWT() + + return b } // PublicAccess defines a security check for public API endpoints. @@ -317,11 +329,15 @@ func (bouncer *RequestBouncer) CookieAuthLookup(r *http.Request) (*portainer.Tok return nil, nil } - tokenData, err := bouncer.jwtService.ParseAndVerifyToken(token) + tokenData, jti, _, err := bouncer.jwtService.ParseAndVerifyToken(token) if err != nil { return nil, err } + if _, ok := bouncer.revokedJWT.Load(jti); ok { + return nil, ErrRevokedJWT + } + return tokenData, nil } @@ -333,15 +349,44 @@ func (bouncer *RequestBouncer) JWTAuthLookup(r *http.Request) (*portainer.TokenD return nil, nil } - tokenData, err := bouncer.jwtService.ParseAndVerifyToken(token) + tokenData, jti, _, err := bouncer.jwtService.ParseAndVerifyToken(token) if err != nil { return nil, err } + if _, ok := bouncer.revokedJWT.Load(jti); ok { + return nil, ErrRevokedJWT + } + return tokenData, nil } -var ErrInvalidKey = errors.New("Invalid API key") +func (bouncer *RequestBouncer) RevokeJWT(token string) { + _, jti, exp, err := bouncer.jwtService.ParseAndVerifyToken(token) + if err != nil { + return + } + + bouncer.revokedJWT.Store(jti, exp) +} + +func (bouncer *RequestBouncer) cleanUpExpiredJWTPass() { + bouncer.revokedJWT.Range(func(key, value any) bool { + if time.Now().After(value.(time.Time)) { + bouncer.revokedJWT.Delete(key) + } + + return true + }) +} + +func (bouncer *RequestBouncer) cleanUpExpiredJWT() { + ticker := time.NewTicker(time.Hour) + + for range ticker.C { + bouncer.cleanUpExpiredJWTPass() + } +} // apiKeyLookup looks up an verifies an api-key by: // - computing the digest of the raw api-key diff --git a/api/http/security/bouncer_test.go b/api/http/security/bouncer_test.go index b202d63c9..74c455403 100644 --- a/api/http/security/bouncer_test.go +++ b/api/http/security/bouncer_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" portainer "github.com/portainer/portainer/api" "github.com/portainer/portainer/api/apikey" @@ -14,6 +15,7 @@ import ( "github.com/portainer/portainer/api/jwt" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // testHandler200 is a simple handler which returns HTTP status 200 OK @@ -459,3 +461,60 @@ func Test_ShouldSkipCSRFCheck(t *testing.T) { }) } } + +func TestJWTRevocation(t *testing.T) { + _, store := datastore.MustNewTestStore(t, true, true) + + jwtService, err := jwt.NewService("1h", store) + require.NoError(t, err) + + err = store.User().Create(&portainer.User{ID: 1}) + require.NoError(t, err) + + jwtService.SetUserSessionDuration(time.Second) + + token, _, err := jwtService.GenerateToken(&portainer.TokenData{ID: 1}) + require.NoError(t, err) + + apiKeyService := apikey.NewAPIKeyService(nil, nil) + + bouncer := NewRequestBouncer(store, jwtService, apiKeyService) + + r, err := http.NewRequest(http.MethodGet, "url", nil) + require.NoError(t, err) + + r.Header.Add(jwtTokenHeader, "Bearer "+token) + + r.AddCookie(&http.Cookie{Name: portainer.AuthCookieKey, Value: token}) + + _, err = bouncer.JWTAuthLookup(r) + require.NoError(t, err) + + _, err = bouncer.CookieAuthLookup(r) + require.NoError(t, err) + + bouncer.RevokeJWT(token) + + revokeLen := func() (l int) { + bouncer.revokedJWT.Range(func(key, value any) bool { + l++ + + return true + }) + + return l + } + require.Equal(t, 1, revokeLen()) + + _, err = bouncer.JWTAuthLookup(r) + require.Error(t, err) + + _, err = bouncer.CookieAuthLookup(r) + require.Error(t, err) + + time.Sleep(time.Second) + + bouncer.cleanUpExpiredJWTPass() + + require.Equal(t, 0, revokeLen()) +} diff --git a/api/internal/testhelpers/request_bouncer.go b/api/internal/testhelpers/request_bouncer.go index 692f23eb4..b89154549 100644 --- a/api/internal/testhelpers/request_bouncer.go +++ b/api/internal/testhelpers/request_bouncer.go @@ -58,6 +58,8 @@ func (testRequestBouncer) JWTAuthLookup(r *http.Request) (*portainer.TokenData, return nil, nil } +func (testRequestBouncer) RevokeJWT(jti string) {} + // AddTestSecurityCookie adds a security cookie to the request func AddTestSecurityCookie(r *http.Request, jwt string) { r.AddCookie(&http.Cookie{ diff --git a/api/jwt/jwt.go b/api/jwt/jwt.go index 5fb031cfd..741b44374 100644 --- a/api/jwt/jwt.go +++ b/api/jwt/jwt.go @@ -103,7 +103,7 @@ func (service *Service) GenerateToken(data *portainer.TokenData) (string, time.T } // ParseAndVerifyToken parses a JWT token and verify its validity. It returns an error if token is invalid. -func (service *Service) ParseAndVerifyToken(token string) (*portainer.TokenData, error) { +func (service *Service) ParseAndVerifyToken(token string) (*portainer.TokenData, string, time.Time, error) { scope := parseScope(token) secret := service.secrets[scope] parsedToken, err := jwt.ParseWithClaims(token, &claims{}, func(token *jwt.Token) (interface{}, error) { @@ -119,10 +119,10 @@ func (service *Service) ParseAndVerifyToken(token string) (*portainer.TokenData, user, err := service.dataStore.User().Read(portainer.UserID(cl.UserID)) if err != nil { - return nil, errInvalidJWTToken + return nil, "", time.Time{}, errInvalidJWTToken } if user.TokenIssueAt > cl.StandardClaims.IssuedAt { - return nil, errInvalidJWTToken + return nil, "", time.Time{}, errInvalidJWTToken } return &portainer.TokenData{ @@ -131,10 +131,11 @@ func (service *Service) ParseAndVerifyToken(token string) (*portainer.TokenData, Role: portainer.UserRole(cl.Role), Token: token, ForceChangePassword: cl.ForceChangePassword, - }, nil + }, cl.Id, time.Unix(cl.ExpiresAt, 0), nil } } - return nil, errInvalidJWTToken + + return nil, "", time.Time{}, errInvalidJWTToken } // parse a JWT token, fallback to defaultScope if no scope is present in the JWT diff --git a/api/jwt/jwt_test.go b/api/jwt/jwt_test.go index e98731eb2..e8ec895ae 100644 --- a/api/jwt/jwt_test.go +++ b/api/jwt/jwt_test.go @@ -6,8 +6,10 @@ import ( "github.com/golang-jwt/jwt/v4" portainer "github.com/portainer/portainer/api" + "github.com/portainer/portainer/api/datastore" i "github.com/portainer/portainer/api/internal/testhelpers" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGenerateSignedToken(t *testing.T) { @@ -55,3 +57,56 @@ func TestGenerateSignedToken_InvalidScope(t *testing.T) { assert.Error(t, err) assert.Equal(t, "invalid scope: testing", err.Error()) } + +func TestGenerationAndParsing(t *testing.T) { + _, store := datastore.MustNewTestStore(t, true, false) + + err := store.User().Create(&portainer.User{ID: 1}) + require.NoError(t, err) + + service, err := NewService("1h", store) + require.NoError(t, err) + + expectedToken := &portainer.TokenData{ + Username: "User", + ID: 1, + Role: 1, + } + + tokenString, _, err := service.GenerateToken(expectedToken) + require.NoError(t, err) + + expectedToken.Token = tokenString + + token, _, _, err := service.ParseAndVerifyToken(tokenString) + require.NoError(t, err) + require.Equal(t, expectedToken, token) +} + +func TestExpiration(t *testing.T) { + _, store := datastore.MustNewTestStore(t, true, false) + + err := store.User().Create(&portainer.User{ID: 1}) + require.NoError(t, err) + + service, err := NewService("1h", store) + require.NoError(t, err) + + expectedToken := &portainer.TokenData{ + Username: "User", + ID: 1, + Role: 1, + } + + service.SetUserSessionDuration(time.Second) + + tokenString, _, err := service.GenerateToken(expectedToken) + require.NoError(t, err) + + expectedToken.Token = tokenString + + time.Sleep(2 * time.Second) + + _, _, _, err = service.ParseAndVerifyToken(tokenString) + require.Error(t, err) +} diff --git a/api/portainer.go b/api/portainer.go index 373d1559d..494e8b302 100644 --- a/api/portainer.go +++ b/api/portainer.go @@ -1490,7 +1490,7 @@ type ( JWTService interface { GenerateToken(data *TokenData) (string, time.Time, error) GenerateTokenForKubeconfig(data *TokenData) (string, error) - ParseAndVerifyToken(token string) (*TokenData, error) + ParseAndVerifyToken(token string) (*TokenData, string, time.Time, error) SetUserSessionDuration(userSessionDuration time.Duration) }