From e334759874fd8f5b7920091cb933984d4091d09a Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Thu, 26 Nov 2020 10:33:46 +0100 Subject: [PATCH 1/5] implement basic auth cache --- proxy/pkg/middleware/basic_auth.go | 111 ++++++++++++++++++++--------- 1 file changed, 77 insertions(+), 34 deletions(-) diff --git a/proxy/pkg/middleware/basic_auth.go b/proxy/pkg/middleware/basic_auth.go index 58e0d7a08..19f0a63e4 100644 --- a/proxy/pkg/middleware/basic_auth.go +++ b/proxy/pkg/middleware/basic_auth.go @@ -1,9 +1,13 @@ package middleware import ( + "crypto/sha1" "fmt" + "github.com/owncloud/ocis/proxy/pkg/cache" "net/http" "strings" + "sync" + "time" accounts "github.com/owncloud/ocis/accounts/pkg/proto/v0" "github.com/owncloud/ocis/ocis-pkg/log" @@ -15,60 +19,99 @@ const publicFilesEndpoint = "/remote.php/dav/public-files/" // BasicAuth provides a middleware to check if BasicAuth is provided func BasicAuth(optionSetters ...Option) func(next http.Handler) http.Handler { options := newOptions(optionSetters...) + logger := options.Logger + oidcIss := options.OIDCIss + accountsCache := cache.NewCache(cache.Size(5000)) if options.EnableBasicAuth { options.Logger.Warn().Msg("basic auth enabled, use only for testing or development") } + h := basicAuth{ + logger: logger, + enabled: options.EnableBasicAuth, + accountsClient: options.AccountsClient, + accountsCache: &accountsCache, + } + return func(next http.Handler) http.Handler { - return &basicAuth{ - next: next, - logger: options.Logger, - enabled: options.EnableBasicAuth, - accountsClient: options.AccountsClient, - oidcIss: options.OIDCIss, - } + return http.HandlerFunc( + func(w http.ResponseWriter, req *http.Request) { + if h.isPublicLink(req) || !h.isBasicAuth(req) { + next.ServeHTTP(w, req) + return + } + + account, ok := h.getAccount(req) + + if !ok { + w.WriteHeader(http.StatusUnauthorized) + return + } + + claims := &oidc.StandardClaims{ + OcisID: account.Id, + Iss: oidcIss, + } + + next.ServeHTTP(w, req.WithContext(oidc.NewContext(req.Context(), claims))) + }, + ) } } type basicAuth struct { - next http.Handler logger log.Logger enabled bool accountsClient accounts.AccountsService - oidcIss string + accountsCache *cache.Cache + m sync.Mutex } -func (m basicAuth) ServeHTTP(w http.ResponseWriter, req *http.Request) { - if m.isPublicLink(req) || !m.isBasicAuth(req) { - m.next.ServeHTTP(w, req) - return - } - - login, password, _ := req.BasicAuth() - - account, status := getAccount(m.logger, m.accountsClient, fmt.Sprintf("login eq '%s' and password eq '%s'", strings.ReplaceAll(login, "'", "''"), strings.ReplaceAll(password, "'", "''"))) - - if status != 0 { - w.WriteHeader(http.StatusUnauthorized) - return - } - - claims := &oidc.StandardClaims{ - OcisID: account.Id, - Iss: m.oidcIss, - } - - m.next.ServeHTTP(w, req.WithContext(oidc.NewContext(req.Context(), claims))) -} - -func (m basicAuth) isPublicLink(req *http.Request) bool { +func (m *basicAuth) isPublicLink(req *http.Request) bool { login, _, ok := req.BasicAuth() return ok && login == "public" && strings.HasPrefix(req.URL.Path, publicFilesEndpoint) } -func (m basicAuth) isBasicAuth(req *http.Request) bool { +func (m *basicAuth) isBasicAuth(req *http.Request) bool { login, password, ok := req.BasicAuth() return m.enabled && ok && login != "" && password != "" } + +func (m *basicAuth) getAccount(req *http.Request) (*accounts.Account, bool) { + var ok bool + var hit *cache.Entry + + login, password, _ := req.BasicAuth() + + h := sha1.New() + h.Write([]byte(login)) + + lookup := fmt.Sprintf("%x", h.Sum([]byte(password))) + + m.m.Lock() + defer m.m.Unlock() + + if hit = m.accountsCache.Get(lookup); hit == nil { + account, status := getAccount( + m.logger, + m.accountsClient, + fmt.Sprintf( + "login eq '%s' and password eq '%s'", + strings.ReplaceAll(login, "'", "''"), + strings.ReplaceAll(password, "'", "''"), + ), + ) + + if ok = status == 0; ok { + m.accountsCache.Set(lookup, account, time.Now().Add(10*time.Minute)) + } + + return account, ok + } + + account, ok := hit.V.(*accounts.Account) + + return account, ok +} From 7cbbcadbdf8792c1c5a81c8b1506ca9bcd8f3737 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Thu, 26 Nov 2020 10:51:09 +0100 Subject: [PATCH 2/5] add changelog --- changelog/unreleased/proxy-cache-basic-auth.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/unreleased/proxy-cache-basic-auth.md diff --git a/changelog/unreleased/proxy-cache-basic-auth.md b/changelog/unreleased/proxy-cache-basic-auth.md new file mode 100644 index 000000000..7a18b3035 --- /dev/null +++ b/changelog/unreleased/proxy-cache-basic-auth.md @@ -0,0 +1,8 @@ +Enhancement: Cache basic auth account id in proxy + +Tags: proxy + +The basic auth middleware now caches account ids. The entry cache gets invalidated after 10 Minutes. +This si useful for scenarios where a lot of basic auth requests with the same username and password happens, for example tests. + +https://github.com/owncloud/ocis/pull/877 From e72328d7380c8e20eb7626fee7fc672fb125c2e5 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Thu, 26 Nov 2020 11:00:10 +0100 Subject: [PATCH 3/5] fix typo --- changelog/unreleased/proxy-cache-basic-auth.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog/unreleased/proxy-cache-basic-auth.md b/changelog/unreleased/proxy-cache-basic-auth.md index 7a18b3035..b7c6ff87c 100644 --- a/changelog/unreleased/proxy-cache-basic-auth.md +++ b/changelog/unreleased/proxy-cache-basic-auth.md @@ -3,6 +3,6 @@ Enhancement: Cache basic auth account id in proxy Tags: proxy The basic auth middleware now caches account ids. The entry cache gets invalidated after 10 Minutes. -This si useful for scenarios where a lot of basic auth requests with the same username and password happens, for example tests. +This is useful for scenarios where a lot of basic auth requests with the same username and password happens, for example tests. -https://github.com/owncloud/ocis/pull/877 +https://github.com/owncloud/ocis/pull/958 From 11ba46eb884d1ea150f1cf1e3a13f56f6ee2fa9a Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Thu, 26 Nov 2020 13:52:24 +0100 Subject: [PATCH 4/5] remove accounts cache from basic auth middleware move cache to ocis-pkg add password validation cache to accounts service --- accounts/pkg/service/v0/accounts.go | 26 ++++++++-- .../unreleased/proxy-cache-basic-auth.md | 8 ---- {proxy/pkg => ocis-pkg}/cache/cache.go | 0 {proxy/pkg => ocis-pkg}/cache/option.go | 0 proxy/pkg/middleware/basic_auth.go | 48 +++++-------------- proxy/pkg/middleware/oidc_auth.go | 2 +- 6 files changed, 35 insertions(+), 49 deletions(-) delete mode 100644 changelog/unreleased/proxy-cache-basic-auth.md rename {proxy/pkg => ocis-pkg}/cache/cache.go (100%) rename {proxy/pkg => ocis-pkg}/cache/option.go (100%) diff --git a/accounts/pkg/service/v0/accounts.go b/accounts/pkg/service/v0/accounts.go index 2d88c313f..6627e3e35 100644 --- a/accounts/pkg/service/v0/accounts.go +++ b/accounts/pkg/service/v0/accounts.go @@ -2,7 +2,9 @@ package service import ( "context" + "crypto/sha256" "fmt" + "github.com/owncloud/ocis/ocis-pkg/cache" "golang.org/x/crypto/bcrypt" "path" "regexp" @@ -32,6 +34,12 @@ import ( // accLock mutually exclude readers from writers on account files var accLock sync.Mutex +// passwordValidCache caches basic auth password validations +var passwordValidCache = cache.NewCache(cache.Size(1024)) + +// passwordValidCacheExpiration defines the entry lifetime +const passwordValidCacheExpiration = 10 * time.Minute + // an auth request is currently hardcoded and has to match this regex // login eq \"teddy\" and password eq \"F&1!b90t111!\" var authQuery = regexp.MustCompile(`^login eq '(.*)' and password eq '(.*)'$`) // TODO how is ' escaped in the password? @@ -128,7 +136,6 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest teardownServiceUser := s.serviceUserToIndex() defer teardownServiceUser() - match, authRequest := getAuthQueryMatch(in.Query) if authRequest { password := match[2] @@ -152,9 +159,22 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest if err != nil || a.PasswordProfile == nil || len(a.PasswordProfile.Password) == 0 { return merrors.Unauthorized(s.id, "account not found or invalid credentials") } - if !isPasswordValid(s.log, a.PasswordProfile.Password, password) { - return merrors.Unauthorized(s.id, "account not found or invalid credentials") + + h := sha256.New() + h.Write([]byte(a.PasswordProfile.Password)) + + k := fmt.Sprintf("%x", h.Sum([]byte(password))) + + if hit := passwordValidCache.Get(k); hit == nil || !hit.V.(bool) { + var ok bool + + if ok = isPasswordValid(s.log, a.PasswordProfile.Password, password); !ok { + return merrors.Unauthorized(s.id, "account not found or invalid credentials") + } + + passwordValidCache.Set(k, ok, time.Now().Add(passwordValidCacheExpiration)) } + a.PasswordProfile.Password = "" out.Accounts = []*proto.Account{a} return nil diff --git a/changelog/unreleased/proxy-cache-basic-auth.md b/changelog/unreleased/proxy-cache-basic-auth.md deleted file mode 100644 index b7c6ff87c..000000000 --- a/changelog/unreleased/proxy-cache-basic-auth.md +++ /dev/null @@ -1,8 +0,0 @@ -Enhancement: Cache basic auth account id in proxy - -Tags: proxy - -The basic auth middleware now caches account ids. The entry cache gets invalidated after 10 Minutes. -This is useful for scenarios where a lot of basic auth requests with the same username and password happens, for example tests. - -https://github.com/owncloud/ocis/pull/958 diff --git a/proxy/pkg/cache/cache.go b/ocis-pkg/cache/cache.go similarity index 100% rename from proxy/pkg/cache/cache.go rename to ocis-pkg/cache/cache.go diff --git a/proxy/pkg/cache/option.go b/ocis-pkg/cache/option.go similarity index 100% rename from proxy/pkg/cache/option.go rename to ocis-pkg/cache/option.go diff --git a/proxy/pkg/middleware/basic_auth.go b/proxy/pkg/middleware/basic_auth.go index 19f0a63e4..22c894a41 100644 --- a/proxy/pkg/middleware/basic_auth.go +++ b/proxy/pkg/middleware/basic_auth.go @@ -1,13 +1,10 @@ package middleware import ( - "crypto/sha1" "fmt" - "github.com/owncloud/ocis/proxy/pkg/cache" "net/http" "strings" "sync" - "time" accounts "github.com/owncloud/ocis/accounts/pkg/proto/v0" "github.com/owncloud/ocis/ocis-pkg/log" @@ -21,7 +18,7 @@ func BasicAuth(optionSetters ...Option) func(next http.Handler) http.Handler { options := newOptions(optionSetters...) logger := options.Logger oidcIss := options.OIDCIss - accountsCache := cache.NewCache(cache.Size(5000)) + if options.EnableBasicAuth { options.Logger.Warn().Msg("basic auth enabled, use only for testing or development") } @@ -30,7 +27,6 @@ func BasicAuth(optionSetters ...Option) func(next http.Handler) http.Handler { logger: logger, enabled: options.EnableBasicAuth, accountsClient: options.AccountsClient, - accountsCache: &accountsCache, } return func(next http.Handler) http.Handler { @@ -63,7 +59,6 @@ type basicAuth struct { logger log.Logger enabled bool accountsClient accounts.AccountsService - accountsCache *cache.Cache m sync.Mutex } @@ -80,38 +75,17 @@ func (m *basicAuth) isBasicAuth(req *http.Request) bool { } func (m *basicAuth) getAccount(req *http.Request) (*accounts.Account, bool) { - var ok bool - var hit *cache.Entry - login, password, _ := req.BasicAuth() - h := sha1.New() - h.Write([]byte(login)) + account, status := getAccount( + m.logger, + m.accountsClient, + fmt.Sprintf( + "login eq '%s' and password eq '%s'", + strings.ReplaceAll(login, "'", "''"), + strings.ReplaceAll(password, "'", "''"), + ), + ) - lookup := fmt.Sprintf("%x", h.Sum([]byte(password))) - - m.m.Lock() - defer m.m.Unlock() - - if hit = m.accountsCache.Get(lookup); hit == nil { - account, status := getAccount( - m.logger, - m.accountsClient, - fmt.Sprintf( - "login eq '%s' and password eq '%s'", - strings.ReplaceAll(login, "'", "''"), - strings.ReplaceAll(password, "'", "''"), - ), - ) - - if ok = status == 0; ok { - m.accountsCache.Set(lookup, account, time.Now().Add(10*time.Minute)) - } - - return account, ok - } - - account, ok := hit.V.(*accounts.Account) - - return account, ok + return account, status == 0 } diff --git a/proxy/pkg/middleware/oidc_auth.go b/proxy/pkg/middleware/oidc_auth.go index de44511e7..2033d0fa5 100644 --- a/proxy/pkg/middleware/oidc_auth.go +++ b/proxy/pkg/middleware/oidc_auth.go @@ -10,9 +10,9 @@ import ( "github.com/dgrijalva/jwt-go" gOidc "github.com/coreos/go-oidc" + "github.com/owncloud/ocis/ocis-pkg/cache" "github.com/owncloud/ocis/ocis-pkg/log" "github.com/owncloud/ocis/ocis-pkg/oidc" - "github.com/owncloud/ocis/proxy/pkg/cache" "golang.org/x/oauth2" ) From cb2e2a38967c78eb4cf5a127a7445b4535945084 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Thu, 26 Nov 2020 14:46:44 +0100 Subject: [PATCH 5/5] add changelog remove unused mux cleanup k6 test --- .../accounts-cache-password-validation.md | 8 ++++++++ proxy/pkg/middleware/basic_auth.go | 13 +++++-------- tests/k6/src/test-issue-162.ts | 4 ++-- 3 files changed, 15 insertions(+), 10 deletions(-) create mode 100644 changelog/unreleased/accounts-cache-password-validation.md diff --git a/changelog/unreleased/accounts-cache-password-validation.md b/changelog/unreleased/accounts-cache-password-validation.md new file mode 100644 index 000000000..4e3363b9c --- /dev/null +++ b/changelog/unreleased/accounts-cache-password-validation.md @@ -0,0 +1,8 @@ +Change: Cache password validation + +Tags: accounts + +The password validity check for requests like `login eq '%s' and password eq '%s'` is now cached for 10 minutes. +This improves the performance for basic auth requests. + +https://github.com/owncloud/ocis/pull/958 \ No newline at end of file diff --git a/proxy/pkg/middleware/basic_auth.go b/proxy/pkg/middleware/basic_auth.go index 22c894a41..f06879b01 100644 --- a/proxy/pkg/middleware/basic_auth.go +++ b/proxy/pkg/middleware/basic_auth.go @@ -2,13 +2,11 @@ package middleware import ( "fmt" - "net/http" - "strings" - "sync" - accounts "github.com/owncloud/ocis/accounts/pkg/proto/v0" "github.com/owncloud/ocis/ocis-pkg/log" "github.com/owncloud/ocis/ocis-pkg/oidc" + "net/http" + "strings" ) const publicFilesEndpoint = "/remote.php/dav/public-files/" @@ -59,22 +57,21 @@ type basicAuth struct { logger log.Logger enabled bool accountsClient accounts.AccountsService - m sync.Mutex } -func (m *basicAuth) isPublicLink(req *http.Request) bool { +func (m basicAuth) isPublicLink(req *http.Request) bool { login, _, ok := req.BasicAuth() return ok && login == "public" && strings.HasPrefix(req.URL.Path, publicFilesEndpoint) } -func (m *basicAuth) isBasicAuth(req *http.Request) bool { +func (m basicAuth) isBasicAuth(req *http.Request) bool { login, password, ok := req.BasicAuth() return m.enabled && ok && login != "" && password != "" } -func (m *basicAuth) getAccount(req *http.Request) (*accounts.Account, bool) { +func (m basicAuth) getAccount(req *http.Request) (*accounts.Account, bool) { login, password, _ := req.BasicAuth() account, status := getAccount( diff --git a/tests/k6/src/test-issue-162.ts b/tests/k6/src/test-issue-162.ts index 11bd86263..c2e57cf5c 100644 --- a/tests/k6/src/test-issue-162.ts +++ b/tests/k6/src/test-issue-162.ts @@ -1,6 +1,6 @@ import {sleep, check} from 'k6'; import {Options} from "k6/options"; -import {defaults, api, utils} from "./lib"; +import {defaults, api} from "./lib"; const files = { 'kb_50.jpg': open('./_files/kb_50.jpg', 'b'), @@ -13,7 +13,7 @@ export let options: Options = { }; export default () => { - const res = api.uploadFile(defaults.accounts.einstein, files['kb_50.jpg'], `kb_50-${utils.randomString()}.jpg`) + const res = api.uploadFile(defaults.accounts.einstein, files['kb_50.jpg'], `kb_50-${__VU}-${__ITER}.jpg`) check(res, { 'status is 201': () => res.status === 201,