From 1a30b6f09fddb6a841b3e3b31bd4b7763b99a088 Mon Sep 17 00:00:00 2001 From: Idan Novogroder Date: Sun, 22 May 2022 12:36:02 +0300 Subject: [PATCH 1/9] Evict user from auth-cache on reset password design --- .../evict-user-from-auth-cache-on-reset-password.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 design/open/evict-user-from-auth-cache-on-reset-password.md diff --git a/design/open/evict-user-from-auth-cache-on-reset-password.md b/design/open/evict-user-from-auth-cache-on-reset-password.md new file mode 100644 index 00000000000..b4d950089b7 --- /dev/null +++ b/design/open/evict-user-from-auth-cache-on-reset-password.md @@ -0,0 +1,13 @@ +## The problem: +When a user logs in to lakeFS we save his credentials in the auth-cache. +This way, we avoid checking the usr credentials through the DB on each call the user makes. +Currently, when a user tries to reset his password, his old password is still stored in the auth-cache, so he has to wait till it is evicted to log in using the new password. + + +## The suggested solution: +The intuitive solution is just to evict the user from the auth cache when resetting his password. +The problem with this solution is that we might not necessarily have a centralized cache, so we might need to evict the user for every auth cache he exists in. +This is why the simpler solution here might be just to not use auth cache for email-password authentication. +This way, we still won't need to authenticate when a user uses API calls that use access-key and secret for authentication and won't need to do the effort of evicting the user auth from all the auth cache instances. +In case we decide to go with this solution, we have to enable email/password authentication only through the UI. +Otherwise, user might try to use email/password authentication with our Python/Java client and won't have their credentials cached. \ No newline at end of file From b4d0bcf1f9a10342b94d561e523515cf916df85e Mon Sep 17 00:00:00 2001 From: Idan Novogroder <43949240+idanovo@users.noreply.github.com> Date: Sun, 22 May 2022 17:22:34 +0300 Subject: [PATCH 2/9] Update design/open/evict-user-from-auth-cache-on-reset-password.md Co-authored-by: arielshaqed --- design/open/evict-user-from-auth-cache-on-reset-password.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design/open/evict-user-from-auth-cache-on-reset-password.md b/design/open/evict-user-from-auth-cache-on-reset-password.md index b4d950089b7..55b23a33381 100644 --- a/design/open/evict-user-from-auth-cache-on-reset-password.md +++ b/design/open/evict-user-from-auth-cache-on-reset-password.md @@ -1,5 +1,5 @@ ## The problem: -When a user logs in to lakeFS we save his credentials in the auth-cache. +When a user logs in to lakeFS we save their credentials in the auth-cache. This way, we avoid checking the usr credentials through the DB on each call the user makes. Currently, when a user tries to reset his password, his old password is still stored in the auth-cache, so he has to wait till it is evicted to log in using the new password. From 35bd2cd7a2fa0b6cb7600115c4b581f3464c24d4 Mon Sep 17 00:00:00 2001 From: Idan Novogroder <43949240+idanovo@users.noreply.github.com> Date: Sun, 22 May 2022 19:18:33 +0300 Subject: [PATCH 3/9] Update design/open/evict-user-from-auth-cache-on-reset-password.md Co-authored-by: johnnyaug --- design/open/evict-user-from-auth-cache-on-reset-password.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design/open/evict-user-from-auth-cache-on-reset-password.md b/design/open/evict-user-from-auth-cache-on-reset-password.md index 55b23a33381..c44aac38f7b 100644 --- a/design/open/evict-user-from-auth-cache-on-reset-password.md +++ b/design/open/evict-user-from-auth-cache-on-reset-password.md @@ -1,7 +1,7 @@ ## The problem: When a user logs in to lakeFS we save their credentials in the auth-cache. This way, we avoid checking the usr credentials through the DB on each call the user makes. -Currently, when a user tries to reset his password, his old password is still stored in the auth-cache, so he has to wait till it is evicted to log in using the new password. +Currently, when a user tries to reset their password, their old password is still stored in the auth-cache, so they have to wait till it is evicted to log in using the new password. ## The suggested solution: From 9ddb9a7f1d34f5d1d98ebc51acd8623dcc57052b Mon Sep 17 00:00:00 2001 From: Idan Novogroder Date: Sun, 22 May 2022 19:33:41 +0300 Subject: [PATCH 4/9] Fixed tesxt --- design/open/evict-user-from-auth-cache-on-reset-password.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/design/open/evict-user-from-auth-cache-on-reset-password.md b/design/open/evict-user-from-auth-cache-on-reset-password.md index c44aac38f7b..e79bb0fb31b 100644 --- a/design/open/evict-user-from-auth-cache-on-reset-password.md +++ b/design/open/evict-user-from-auth-cache-on-reset-password.md @@ -1,13 +1,13 @@ ## The problem: When a user logs in to lakeFS we save their credentials in the auth-cache. This way, we avoid checking the usr credentials through the DB on each call the user makes. -Currently, when a user tries to reset their password, their old password is still stored in the auth-cache, so they have to wait till it is evicted to log in using the new password. +Currently, when users try to reset their password, their old password is still stored in the auth-cache, so they have to wait till it's evicted before they log in using the new password. ## The suggested solution: -The intuitive solution is just to evict the user from the auth cache when resetting his password. +The intuitive solution is just to evict the users from the auth cache when resetting their password. The problem with this solution is that we might not necessarily have a centralized cache, so we might need to evict the user for every auth cache he exists in. This is why the simpler solution here might be just to not use auth cache for email-password authentication. -This way, we still won't need to authenticate when a user uses API calls that use access-key and secret for authentication and won't need to do the effort of evicting the user auth from all the auth cache instances. +This way, we still won't need to authenticate when users use API calls that use access-key and secret for authentication and won't need to do the effort of evicting the user auth from all the auth cache instances. In case we decide to go with this solution, we have to enable email/password authentication only through the UI. Otherwise, user might try to use email/password authentication with our Python/Java client and won't have their credentials cached. \ No newline at end of file From 4578540265ec79120bd8389bbc145f7707c96dd6 Mon Sep 17 00:00:00 2001 From: Idan Novogroder Date: Mon, 23 May 2022 11:22:40 +0300 Subject: [PATCH 5/9] Not using auth cache for email authentication --- pkg/auth/service.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/auth/service.go b/pkg/auth/service.go index f1b5606457d..5d563307dc1 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -299,15 +299,13 @@ func (s *DBAuthService) GetUser(ctx context.Context, username string) (*model.Us } func (s *DBAuthService) GetUserByEmail(ctx context.Context, email string) (*model.User, error) { - return s.cache.GetUser(email, func() (*model.User, error) { - user, err := s.db.Transact(ctx, func(tx db.Tx) (interface{}, error) { - return getUserByEmail(tx, email) - }, db.ReadOnly()) - if err != nil { - return nil, err - } - return user.(*model.User), nil - }) + user, err := s.db.Transact(ctx, func(tx db.Tx) (interface{}, error) { + return getUserByEmail(tx, email) + }, db.ReadOnly()) + if err != nil { + return nil, err + } + return user.(*model.User), nil } func (s *DBAuthService) GetUserByID(ctx context.Context, userID int64) (*model.User, error) { From f6fcc28d95c5f01a695fc7f687aaa01feebc9401 Mon Sep 17 00:00:00 2001 From: Idan Novogroder Date: Mon, 23 May 2022 12:37:44 +0300 Subject: [PATCH 6/9] Split authenticator to middleware and controller authenticators --- cmd/lakefs/cmd/run.go | 10 ++++++---- pkg/api/serve.go | 7 ++++--- pkg/loadtest/local_load_test.go | 1 + 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/cmd/lakefs/cmd/run.go b/cmd/lakefs/cmd/run.go index bd8b5cd06eb..97f66284609 100644 --- a/cmd/lakefs/cmd/run.go +++ b/cmd/lakefs/cmd/run.go @@ -164,14 +164,15 @@ var runCmd = &cobra.Command{ cfg.GetAuthCacheConfig(), logger.WithField("service", "auth_service")) } - authenticator := auth.ChainAuthenticator{ + middlewareAuthenticator := auth.ChainAuthenticator{ auth.NewBuiltinAuthenticator(authService), - auth.NewEmailAuthenticator(authService), } ldapConfig := cfg.GetLDAPConfiguration() if ldapConfig != nil { - authenticator = append(authenticator, newLDAPAuthenticator(ldapConfig, authService)) + middlewareAuthenticator = append(middlewareAuthenticator, newLDAPAuthenticator(ldapConfig, authService)) } + controllerAuthenticator := append(middlewareAuthenticator, auth.NewEmailAuthenticator(authService)) + authMetadataManager := auth.NewDBMetadataManager(version.Version, cfg.GetFixedInstallationID(), dbPool) cloudMetadataProvider := stats.BuildMetadataProvider(logger, cfg) blockstoreType := cfg.GetBlockstoreType() @@ -236,7 +237,8 @@ var runCmd = &cobra.Command{ apiHandler := api.Serve( cfg, c, - authenticator, + middlewareAuthenticator, + controllerAuthenticator, authService, blockStore, authMetadataManager, diff --git a/pkg/api/serve.go b/pkg/api/serve.go index 3aa6d94cfe7..6972faf23ff 100644 --- a/pkg/api/serve.go +++ b/pkg/api/serve.go @@ -41,7 +41,8 @@ type responseError struct { func Serve( cfg *config.Config, catalog catalog.Interface, - authenticator auth.Authenticator, + middlewareAuthenticator auth.Authenticator, + controllerAuthenticator auth.Authenticator, authService auth.Service, blockAdapter block.Adapter, metadataManager auth.MetadataManager, @@ -68,14 +69,14 @@ func Serve( RequestIDHeaderName, logging.Fields{logging.ServiceNameFieldKey: LoggerServiceName}, cfg.GetLoggingTraceRequestHeaders()), - AuthMiddleware(logger, swagger, authenticator, authService), + AuthMiddleware(logger, swagger, middlewareAuthenticator, authService), MetricsMiddleware(swagger), ) controller := NewController( cfg, catalog, - authenticator, + controllerAuthenticator, authService, blockAdapter, metadataManager, diff --git a/pkg/loadtest/local_load_test.go b/pkg/loadtest/local_load_test.go index 8b73dbb98f6..1124990d3ba 100644 --- a/pkg/loadtest/local_load_test.go +++ b/pkg/loadtest/local_load_test.go @@ -104,6 +104,7 @@ func TestLocalLoad(t *testing.T) { conf, c, authenticator, + authenticator, authService, blockAdapter, meta, From a591e2ad323268a5dd0bca3f7ed13a99346105a4 Mon Sep 17 00:00:00 2001 From: Idan Novogroder Date: Mon, 23 May 2022 14:19:25 +0300 Subject: [PATCH 7/9] Fix tests --- pkg/api/serve_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/api/serve_test.go b/pkg/api/serve_test.go index c13a11910af..2c857ec6b79 100644 --- a/pkg/api/serve_test.go +++ b/pkg/api/serve_test.go @@ -117,7 +117,7 @@ func setupHandlerWithWalkerFactory(t testing.TB, factory catalog.WalkerFactory, emailParams, _ := cfg.GetEmailParams() emailer, err := email.NewEmailer(emailParams) testutil.Must(t, err) - handler := api.Serve(cfg, c, authenticator, authService, c.BlockAdapter, meta, migrator, collector, nil, actionsService, auditChecker, logging.Default(), emailer, nil) + handler := api.Serve(cfg, c, authenticator, authenticator, authService, c.BlockAdapter, meta, migrator, collector, nil, actionsService, auditChecker, logging.Default(), emailer, nil) return handler, &dependencies{ blocks: c.BlockAdapter, From b578190bfe49c2c606338a5b948878889d794779 Mon Sep 17 00:00:00 2001 From: Idan Novogroder Date: Mon, 23 May 2022 17:18:39 +0300 Subject: [PATCH 8/9] Removed unused method from cache interface --- pkg/auth/cache.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/pkg/auth/cache.go b/pkg/auth/cache.go index a79ee44291b..a4fa0b2288d 100644 --- a/pkg/auth/cache.go +++ b/pkg/auth/cache.go @@ -15,14 +15,12 @@ type Cache interface { GetCredential(accessKeyID string, setFn CredentialSetFn) (*model.Credential, error) GetUser(username string, setFn UserSetFn) (*model.User, error) GetUserByID(userID int64, setFn UserSetFn) (*model.User, error) - GetUserByEmail(userEmail string, setFn UserSetFn) (*model.User, error) GetUserPolicies(userID string, setFn UserPoliciesSetFn) ([]*model.Policy, error) } type LRUCache struct { credentialsCache cache.Cache userCache cache.Cache - userEmailCache cache.Cache policyCache cache.Cache } @@ -31,7 +29,6 @@ func NewLRUCache(size int, expiry, jitter time.Duration) *LRUCache { return &LRUCache{ credentialsCache: cache.NewCache(size, expiry, jitterFn), userCache: cache.NewCache(size, expiry, jitterFn), - userEmailCache: cache.NewCache(size, expiry, jitterFn), policyCache: cache.NewCache(size, expiry, jitterFn), } } @@ -60,14 +57,6 @@ func (c *LRUCache) GetUserByID(userID int64, setFn UserSetFn) (*model.User, erro return v.(*model.User), nil } -func (c *LRUCache) GetUserByEmail(userEmail string, setFn UserSetFn) (*model.User, error) { - v, err := c.userEmailCache.GetOrSet(userEmail, func() (interface{}, error) { return setFn() }) - if err != nil { - return nil, err - } - return v.(*model.User), nil -} - func (c *LRUCache) GetUserPolicies(userID string, setFn UserPoliciesSetFn) ([]*model.Policy, error) { v, err := c.policyCache.GetOrSet(userID, func() (interface{}, error) { return setFn() }) if err != nil { From 44bc2a4fc1cc54bd95ad3946ba4a393a8acc4914 Mon Sep 17 00:00:00 2001 From: Idan Novogroder Date: Tue, 24 May 2022 11:01:29 +0300 Subject: [PATCH 9/9] Moved design to accepted --- .../evict-user-from-auth-cache-on-reset-password.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) rename design/{open => accepted}/evict-user-from-auth-cache-on-reset-password.md (52%) diff --git a/design/open/evict-user-from-auth-cache-on-reset-password.md b/design/accepted/evict-user-from-auth-cache-on-reset-password.md similarity index 52% rename from design/open/evict-user-from-auth-cache-on-reset-password.md rename to design/accepted/evict-user-from-auth-cache-on-reset-password.md index e79bb0fb31b..5a36aab41a6 100644 --- a/design/open/evict-user-from-auth-cache-on-reset-password.md +++ b/design/accepted/evict-user-from-auth-cache-on-reset-password.md @@ -1,13 +1,11 @@ ## The problem: When a user logs in to lakeFS we save their credentials in the auth-cache. -This way, we avoid checking the usr credentials through the DB on each call the user makes. +This way, we avoid checking the user credentials through the DB on each call the user make. Currently, when users try to reset their password, their old password is still stored in the auth-cache, so they have to wait till it's evicted before they log in using the new password. ## The suggested solution: The intuitive solution is just to evict the users from the auth cache when resetting their password. -The problem with this solution is that we might not necessarily have a centralized cache, so we might need to evict the user for every auth cache he exists in. +The problem with this solution is that we might not necessarily have a centralized cache, so we might need to evict the user from every auth cache he exists in. This is why the simpler solution here might be just to not use auth cache for email-password authentication. -This way, we still won't need to authenticate when users use API calls that use access-key and secret for authentication and won't need to do the effort of evicting the user auth from all the auth cache instances. -In case we decide to go with this solution, we have to enable email/password authentication only through the UI. -Otherwise, user might try to use email/password authentication with our Python/Java client and won't have their credentials cached. \ No newline at end of file +This way, we still won't need to authenticate when users use API calls that use access-key and secret for authentication, and we won't need to do the effort of evicting the user auth from all the auth cache instances.