Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evict user from auth-cache on reset password #3407

Merged
merged 11 commits into from
May 24, 2022
10 changes: 6 additions & 4 deletions cmd/lakefs/cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -236,7 +237,8 @@ var runCmd = &cobra.Command{
apiHandler := api.Serve(
cfg,
c,
authenticator,
middlewareAuthenticator,
controllerAuthenticator,
authService,
blockStore,
authMetadataManager,
Expand Down
13 changes: 13 additions & 0 deletions design/open/evict-user-from-auth-cache-on-reset-password.md
Original file line number Diff line number Diff line change
@@ -0,0 +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 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.
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why is that so bad?
We already state in the docs that email+password should be used only for the UI.
If the user decides to use them for the API, an extra DB call will happen for every request - I think it will not cause a bottleneck. If it does in the future - we can solve it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think of a Spark app reading from multiple workers. One user in one org makes a mistake, and all users get rate limited.

A shared auth server will have this problem in trumps, even more so if it fails to protect its DB access.

7 changes: 4 additions & 3 deletions pkg/api/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/serve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 0 additions & 11 deletions pkg/auth/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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),
}
}
Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 7 additions & 9 deletions pkg/auth/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.GetUserByEmail(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) {
Expand Down
1 change: 1 addition & 0 deletions pkg/loadtest/local_load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ func TestLocalLoad(t *testing.T) {
conf,
c,
authenticator,
authenticator,
authService,
blockAdapter,
meta,
Expand Down