From e4e55157e808e60a82bf9704359d241a9e2e11ed Mon Sep 17 00:00:00 2001 From: andres-portainer <91705312+andres-portainer@users.noreply.github.com> Date: Fri, 30 Aug 2024 20:24:05 -0300 Subject: [PATCH] fix(bouncer): add support for JWT revocation BE-11179 (#12164) --- api/http/handler/auth/authenticate.go | 23 ++-- api/http/handler/auth/logout.go | 2 + api/http/security/bouncer.go | 71 ++++++++++-- api/http/security/bouncer_test.go | 62 ++++++++++- api/internal/testhelpers/request_bouncer.go | 2 + api/jwt/jwt.go | 114 ++++++++++---------- api/jwt/jwt_test.go | 64 ++++++++++- api/portainer.go | 2 +- 8 files changed, 254 insertions(+), 86 deletions(-) diff --git a/api/http/handler/auth/authenticate.go b/api/http/handler/auth/authenticate.go index 783d28691..989949daa 100644 --- a/api/http/handler/auth/authenticate.go +++ b/api/http/handler/auth/authenticate.go @@ -154,7 +154,6 @@ func (handler *Handler) persistAndWriteToken(w http.ResponseWriter, tokenData *p security.AddAuthCookie(w, token, expirationTime) return response.JSON(w, &authenticateResponse{JWT: token}) - } func (handler *Handler) syncUserTeamsWithLDAPGroups(user *portainer.User, settings *portainer.LDAPSettings) error { @@ -179,20 +178,18 @@ func (handler *Handler) syncUserTeamsWithLDAPGroups(user *portainer.User, settin } for _, team := range teams { - if teamExists(team.Name, userGroups) { - if teamMembershipExists(team.ID, userMemberships) { - continue - } + if !teamExists(team.Name, userGroups) || teamMembershipExists(team.ID, userMemberships) { + continue + } - membership := &portainer.TeamMembership{ - UserID: user.ID, - TeamID: team.ID, - Role: portainer.TeamMember, - } + membership := &portainer.TeamMembership{ + UserID: user.ID, + TeamID: team.ID, + Role: portainer.TeamMember, + } - if err := handler.DataStore.TeamMembership().Create(membership); err != nil { - return err - } + if err := handler.DataStore.TeamMembership().Create(membership); err != nil { + return err } } diff --git a/api/http/handler/auth/logout.go b/api/http/handler/auth/logout.go index d4117b283..977fafa69 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 a4d1b2c95..2b740de4c 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/rs/zerolog/log" ) +const apiKeyHeader = "X-API-KEY" +const jwtTokenHeader = "Authorization" + type ( BouncerService interface { PublicAccess(http.Handler) http.Handler @@ -27,9 +31,10 @@ type ( AuthorizedEndpointOperation(*http.Request, *portainer.Endpoint) error AuthorizedEdgeEndpointOperation(*http.Request, *portainer.Endpoint) error - TrustedEdgeEnvironmentAccess(dataservices.DataStoreTx, *portainer.Endpoint) error CookieAuthLookup(*http.Request) (*portainer.TokenData, error) JWTAuthLookup(*http.Request) (*portainer.TokenData, error) + TrustedEdgeEnvironmentAccess(dataservices.DataStoreTx, *portainer.Endpoint) 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. @@ -80,6 +92,7 @@ func (bouncer *RequestBouncer) AdminAccess(h http.Handler) http.Handler { h = bouncer.mwUpgradeToRestrictedRequest(h) h = bouncer.mwCheckPortainerAuthorizations(h, true) h = bouncer.mwAuthenticatedUser(h) + return h } @@ -92,6 +105,7 @@ func (bouncer *RequestBouncer) RestrictedAccess(h http.Handler) http.Handler { h = bouncer.mwUpgradeToRestrictedRequest(h) h = bouncer.mwCheckPortainerAuthorizations(h, false) h = bouncer.mwAuthenticatedUser(h) + return h } @@ -105,6 +119,7 @@ func (bouncer *RequestBouncer) TeamLeaderAccess(h http.Handler) http.Handler { h = bouncer.mwIsTeamLeader(h) h = bouncer.mwUpgradeToRestrictedRequest(h) h = bouncer.mwAuthenticatedUser(h) + return h } @@ -116,6 +131,7 @@ func (bouncer *RequestBouncer) TeamLeaderAccess(h http.Handler) http.Handler { func (bouncer *RequestBouncer) AuthenticatedAccess(h http.Handler) http.Handler { h = bouncer.mwUpgradeToRestrictedRequest(h) h = bouncer.mwAuthenticatedUser(h) + return h } @@ -197,6 +213,7 @@ func (bouncer *RequestBouncer) mwAuthenticatedUser(h http.Handler) http.Handler bouncer.JWTAuthLookup, }, h) h = mwSecureHeaders(h) + return h } @@ -284,23 +301,27 @@ func (bouncer *RequestBouncer) mwAuthenticateFirst(tokenLookups []tokenLookup, n resultToken, err := lookup(r) if err != nil { httperror.WriteError(w, http.StatusUnauthorized, "Invalid JWT token", httperrors.ErrUnauthorized) + return } if resultToken != nil { token = resultToken + break } } if token == nil { httperror.WriteError(w, http.StatusUnauthorized, "A valid authorization token is missing", httperrors.ErrUnauthorized) + return } user, _ := bouncer.dataStore.User().Read(token.ID) if user == nil { httperror.WriteError(w, http.StatusUnauthorized, "An authorization token is invalid", httperrors.ErrUnauthorized) + return } @@ -317,11 +338,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 +358,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 @@ -392,6 +446,7 @@ func extractBearerToken(r *http.Request) (string, bool) { if token != "" { query.Del("token") r.URL.RawQuery = query.Encode() + return token, true } @@ -505,11 +560,13 @@ func (bouncer *RequestBouncer) EdgeComputeOperation(next http.Handler) http.Hand settings, err := bouncer.dataStore.Settings().Settings() if err != nil { httperror.WriteError(w, http.StatusServiceUnavailable, "Unable to retrieve settings", err) + return } if !settings.EnableEdgeComputeFeatures { httperror.WriteError(w, http.StatusServiceUnavailable, "Edge compute features are disabled", errors.New("Edge compute features are disabled")) + return } diff --git a/api/http/security/bouncer_test.go b/api/http/security/bouncer_test.go index b202d63c9..7b0e5e0a4 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 @@ -39,7 +41,6 @@ func tokenLookupEmpty(r *http.Request) (*portainer.TokenData, error) { } func Test_mwAuthenticateFirst(t *testing.T) { - _, store := datastore.MustNewTestStore(t, true, true) jwtService, err := jwt.NewService("1h", store) @@ -154,7 +155,6 @@ func Test_extractKeyFromCookie(t *testing.T) { } func Test_extractBearerToken(t *testing.T) { - tt := []struct { name string requestHeader string @@ -384,7 +384,6 @@ func Test_apiKeyLookup(t *testing.T) { } func Test_ShouldSkipCSRFCheck(t *testing.T) { - tt := []struct { name string cookieValue string @@ -459,3 +458,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 661036574..e05d5e420 100644 --- a/api/jwt/jwt.go +++ b/api/jwt/jwt.go @@ -13,7 +13,14 @@ import ( "github.com/rs/zerolog/log" ) -const year = time.Hour * 24 * 365 +const ( + year = time.Hour * 24 * 365 + + keyLen = 32 + + defaultScope = scope("default") + kubeConfigScope = scope("kubeconfig") +) // scope represents JWT scopes that are supported in JWT claims. type scope string @@ -35,13 +42,8 @@ type claims struct { } var ( - errSecretGeneration = errors.New("Unable to generate secret key") - errInvalidJWTToken = errors.New("Invalid JWT token") -) - -const ( - defaultScope = scope("default") - kubeConfigScope = scope("kubeconfig") + errSecretGeneration = errors.New("unable to generate secret key") + errInvalidJWTToken = errors.New("invalid JWT token") ) // NewService initializes a new service. It will generate a random key that will be used to sign JWT tokens. @@ -51,7 +53,7 @@ func NewService(userSessionDuration string, dataStore dataservices.DataStore) (* return nil, err } - secret := apikey.GenerateRandomKey(32) + secret := apikey.GenerateRandomKey(keyLen) if secret == nil { return nil, errSecretGeneration } @@ -61,16 +63,14 @@ func NewService(userSessionDuration string, dataStore dataservices.DataStore) (* return nil, err } - service := &Service{ + return &Service{ map[scope][]byte{ defaultScope: secret, kubeConfigScope: kubeSecret, }, userSessionTimeout, dataStore, - } - - return service, nil + }, nil } func getOrCreateKubeSecret(dataStore dataservices.DataStore) ([]byte, error) { @@ -80,17 +80,19 @@ func getOrCreateKubeSecret(dataStore dataservices.DataStore) ([]byte, error) { } kubeSecret := settings.OAuthSettings.KubeSecretKey + if kubeSecret != nil { + return kubeSecret, nil + } + + kubeSecret = apikey.GenerateRandomKey(keyLen) if kubeSecret == nil { - kubeSecret = apikey.GenerateRandomKey(32) - if kubeSecret == nil { - return nil, errSecretGeneration - } + return nil, errSecretGeneration + } - settings.OAuthSettings.KubeSecretKey = kubeSecret + settings.OAuthSettings.KubeSecretKey = kubeSecret - if err := dataStore.Settings().UpdateSettings(settings); err != nil { - return nil, err - } + if err := dataStore.Settings().UpdateSettings(settings); err != nil { + return nil, err } return kubeSecret, nil @@ -104,53 +106,54 @@ func (service *Service) defaultExpireAt() time.Time { func (service *Service) GenerateToken(data *portainer.TokenData) (string, time.Time, error) { expiryTime := service.defaultExpireAt() token, err := service.generateSignedToken(data, expiryTime, defaultScope) + return token, expiryTime, err } // 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) (any, error) { if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { - msg := fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) - return nil, msg + return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) } + return secret, nil }) - - if err == nil && parsedToken != nil { - if cl, ok := parsedToken.Claims.(*claims); ok && parsedToken.Valid { - - user, err := service.dataStore.User().Read(portainer.UserID(cl.UserID)) - if err != nil { - return nil, errInvalidJWTToken - } - if user.TokenIssueAt > cl.RegisteredClaims.IssuedAt.Unix() { - return nil, errInvalidJWTToken - } - - return &portainer.TokenData{ - ID: portainer.UserID(cl.UserID), - Username: cl.Username, - Role: portainer.UserRole(cl.Role), - Token: token, - ForceChangePassword: cl.ForceChangePassword, - }, nil - } + if err != nil || parsedToken == nil { + return nil, "", time.Time{}, errInvalidJWTToken } - return nil, errInvalidJWTToken + + cl, ok := parsedToken.Claims.(*claims) + if !ok || !parsedToken.Valid { + return nil, "", time.Time{}, errInvalidJWTToken + } + + user, err := service.dataStore.User().Read(portainer.UserID(cl.UserID)) + if err != nil || user.TokenIssueAt > cl.RegisteredClaims.IssuedAt.Unix() { + return nil, "", time.Time{}, errInvalidJWTToken + } + + return &portainer.TokenData{ + ID: portainer.UserID(cl.UserID), + Username: cl.Username, + Role: portainer.UserRole(cl.Role), + Token: token, + ForceChangePassword: cl.ForceChangePassword, + }, cl.ID, cl.ExpiresAt.Time, nil } // parse a JWT token, fallback to defaultScope if no scope is present in the JWT func parseScope(token string) scope { unverifiedToken, _, _ := new(jwt.Parser).ParseUnverified(token, &claims{}) - if unverifiedToken != nil { - if cl, ok := unverifiedToken.Claims.(*claims); ok { - if cl.Scope == kubeConfigScope { - return kubeConfigScope - } - } + if unverifiedToken == nil { + return defaultScope + } + + if cl, ok := unverifiedToken.Claims.(*claims); ok && cl.Scope == kubeConfigScope { + return kubeConfigScope } return defaultScope @@ -173,9 +176,8 @@ func (service *Service) generateSignedToken(data *portainer.TokenData, expiresAt } if settings.IsDockerDesktopExtension { - // Set expiration to 99 years for docker desktop extension. log.Info().Msg("detected docker desktop extension mode") - expiresAt = time.Now().Add(year * 99) + expiresAt = time.Now().Add(99 * year) } cl := claims{ @@ -196,10 +198,6 @@ func (service *Service) generateSignedToken(data *portainer.TokenData, expiresAt } token := jwt.NewWithClaims(jwt.SigningMethodHS256, cl) - signedToken, err := token.SignedString(secret) - if err != nil { - return "", err - } - return signedToken, nil + return token.SignedString(secret) } diff --git a/api/jwt/jwt_test.go b/api/jwt/jwt_test.go index 6e008d1ca..ba79b8ef2 100644 --- a/api/jwt/jwt_test.go +++ b/api/jwt/jwt_test.go @@ -4,14 +4,17 @@ import ( "testing" "time" - "github.com/golang-jwt/jwt/v4" portainer "github.com/portainer/portainer/api" - i "github.com/portainer/portainer/api/internal/testhelpers" + "github.com/portainer/portainer/api/datastore" + "github.com/portainer/portainer/api/internal/testhelpers" + + "github.com/golang-jwt/jwt/v4" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGenerateSignedToken(t *testing.T) { - dataStore := i.NewDatastore(i.WithSettingsService(&portainer.Settings{})) + dataStore := testhelpers.NewDatastore(testhelpers.WithSettingsService(&portainer.Settings{})) svc, err := NewService("24h", dataStore) assert.NoError(t, err, "failed to create a copy of service") @@ -40,7 +43,7 @@ func TestGenerateSignedToken(t *testing.T) { } func TestGenerateSignedToken_InvalidScope(t *testing.T) { - dataStore := i.NewDatastore(i.WithSettingsService(&portainer.Settings{})) + dataStore := testhelpers.NewDatastore(testhelpers.WithSettingsService(&portainer.Settings{})) svc, err := NewService("24h", dataStore) assert.NoError(t, err, "failed to create a copy of service") @@ -55,3 +58,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 e074d6d6e..9460fad14 100644 --- a/api/portainer.go +++ b/api/portainer.go @@ -1458,7 +1458,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) }