From e96819bce83dde85aeba7c3602bf2afb59eead38 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Thu, 4 Aug 2022 17:38:55 +0200 Subject: [PATCH 01/20] rewrite the auth middleware The old approach of the authentication middlewares had the problem that when an authenticator could not authenticate a request it would still send it to the next handler, in case that the next one can authenticate it. But if no authenticator could successfully authenticate the request, it would still be handled, which leads to unauthorized access. --- services/proxy/pkg/command/server.go | 107 ++++++---- .../proxy/pkg/middleware/authentication.go | 184 ++++++++++++----- services/proxy/pkg/middleware/basic_auth.go | 159 +++----------- .../proxy/pkg/middleware/basic_auth_test.go | 4 +- services/proxy/pkg/middleware/oidc_auth.go | 194 ++++++++---------- .../proxy/pkg/middleware/public_share_auth.go | 110 +++++----- .../proxy/pkg/middleware/signed_url_auth.go | 47 +++-- .../pkg/middleware/signed_url_auth_test.go | 12 +- 8 files changed, 423 insertions(+), 394 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 81342945f1..9e372e4481 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -16,9 +16,9 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/log" pkgmiddleware "github.com/owncloud/ocis/v2/ocis-pkg/middleware" "github.com/owncloud/ocis/v2/ocis-pkg/service/grpc" + "github.com/owncloud/ocis/v2/ocis-pkg/sync" "github.com/owncloud/ocis/v2/ocis-pkg/version" settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" - storesvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/store/v0" "github.com/owncloud/ocis/v2/services/proxy/pkg/config" "github.com/owncloud/ocis/v2/services/proxy/pkg/config/parser" "github.com/owncloud/ocis/v2/services/proxy/pkg/cs3" @@ -149,7 +149,7 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) logger.Fatal().Msgf("Invalid accounts backend type '%s'", cfg.AccountBackend) } - storeClient := storesvc.NewStoreService("com.owncloud.api.store", grpc.DefaultClient) + // storeClient := storesvc.NewStoreService("com.owncloud.api.store", grpc.DefaultClient) if err != nil { logger.Error().Err(err). Str("gateway", cfg.Reva.Address). @@ -166,6 +166,38 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) Timeout: time.Second * 10, } + var authenticators []middleware.Authenticator + if cfg.EnableBasicAuth { + logger.Warn().Msg("basic auth enabled, use only for testing or development") + authenticators = append(authenticators, middleware.BasicAuthenticator{ + Logger: logger, + UserProvider: userProvider, + }) + } + tokenCache := sync.NewCache(cfg.OIDC.UserinfoCache.Size) + authenticators = append(authenticators, middleware.OIDCAuthenticator{ + Logger: logger, + TokenCache: &tokenCache, + TokenCacheTTL: time.Duration(cfg.OIDC.UserinfoCache.TTL), + HTTPClient: oidcHTTPClient, + OIDCIss: cfg.OIDC.Issuer, + ProviderFunc: func() (middleware.OIDCProvider, error) { + // Initialize a provider by specifying the issuer URL. + // it will fetch the keys from the issuer using the .well-known + // endpoint + return oidc.NewProvider( + context.WithValue(ctx, oauth2.HTTPClient, oidcHTTPClient), + cfg.OIDC.Issuer, + ) + }, + JWKSOptions: cfg.OIDC.JWKS, + AccessTokenVerifyMethod: cfg.OIDC.AccessTokenVerifyMethod, + }) + // authenticators = append(authenticators, middleware.PublicShareAuthenticator{ + // Logger: logger, + // RevaGatewayClient: revaClient, + // }) + return alice.New( // first make sure we log all requests and redirect to https if necessary pkgmiddleware.TraceContext, @@ -179,39 +211,44 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) oidcHTTPClient, ), - // now that we established the basics, on with authentication middleware - middleware.Authentication( - // OIDC Options - middleware.OIDCProviderFunc(func() (middleware.OIDCProvider, error) { - // Initialize a provider by specifying the issuer URL. - // it will fetch the keys from the issuer using the .well-known - // endpoint - return oidc.NewProvider( - context.WithValue(ctx, oauth2.HTTPClient, oidcHTTPClient), - cfg.OIDC.Issuer, - ) - }), - middleware.HTTPClient(oidcHTTPClient), - middleware.TokenCacheSize(cfg.OIDC.UserinfoCache.Size), - middleware.TokenCacheTTL(time.Second*time.Duration(cfg.OIDC.UserinfoCache.TTL)), - middleware.AccessTokenVerifyMethod(cfg.OIDC.AccessTokenVerifyMethod), - middleware.JWKSOptions(cfg.OIDC.JWKS), + // middleware.AuthenticationOld( + // // OIDC Options + // middleware.OIDCProviderFunc(func() (middleware.OIDCProvider, error) { + // // Initialize a provider by specifying the issuer URL. + // // it will fetch the keys from the issuer using the .well-known + // // endpoint + // return oidc.NewProvider( + // context.WithValue(ctx, oauth2.HTTPClient, oidcHTTPClient), + // cfg.OIDC.Issuer, + // ) + // }), - // basic Options - middleware.Logger(logger), - middleware.EnableBasicAuth(cfg.EnableBasicAuth), - middleware.UserProvider(userProvider), - middleware.OIDCIss(cfg.OIDC.Issuer), - middleware.UserOIDCClaim(cfg.UserOIDCClaim), - middleware.UserCS3Claim(cfg.UserCS3Claim), + middleware.Authentication( + authenticators, middleware.CredentialsByUserAgent(cfg.AuthMiddleware.CredentialsByUserAgent), - ), - middleware.SignedURLAuth( middleware.Logger(logger), - middleware.PreSignedURLConfig(cfg.PreSignedURL), - middleware.UserProvider(userProvider), - middleware.Store(storeClient), + middleware.OIDCIss(cfg.OIDC.Issuer), + middleware.EnableBasicAuth(cfg.EnableBasicAuth), ), + // middleware.HTTPClient(oidcHTTPClient), + // middleware.TokenCacheSize(cfg.OIDC.UserinfoCache.Size), + // middleware.TokenCacheTTL(time.Second*time.Duration(cfg.OIDC.UserinfoCache.TTL)), + // + // // basic Options + // middleware.Logger(logger), + // middleware.EnableBasicAuth(cfg.EnableBasicAuth), + // middleware.UserProvider(userProvider), + // middleware.OIDCIss(cfg.OIDC.Issuer), + // middleware.UserOIDCClaim(cfg.UserOIDCClaim), + // middleware.UserCS3Claim(cfg.UserCS3Claim), + // middleware.CredentialsByUserAgent(cfg.AuthMiddleware.CredentialsByUserAgent), + // ), + // middleware.SignedURLAuth( + // middleware.Logger(logger), + // middleware.PreSignedURLConfig(cfg.PreSignedURL), + // middleware.UserProvider(userProvider), + // middleware.Store(storeClient), + // ), middleware.AccountResolver( middleware.Logger(logger), middleware.UserProvider(userProvider), @@ -233,9 +270,9 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) middleware.TokenManagerConfig(*cfg.TokenManager), middleware.RevaGatewayClient(revaClient), ), - middleware.PublicShareAuth( - middleware.Logger(logger), - middleware.RevaGatewayClient(revaClient), - ), + // middleware.PublicShareAuth( + // middleware.Logger(logger), + // middleware.RevaGatewayClient(revaClient), + // ), ) } diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 15fa4ec429..651876f35f 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -5,6 +5,8 @@ import ( "net/http" "regexp" "strings" + + "github.com/owncloud/ocis/v2/services/proxy/pkg/webdav" ) var ( @@ -12,48 +14,97 @@ var ( SupportedAuthStrategies []string // ProxyWwwAuthenticate is a list of endpoints that do not rely on reva underlying authentication, such as ocs. - // services that fallback to reva authentication are declared in the "frontend" command on oCIS. It is a list of strings - // to be regexp compiled. - ProxyWwwAuthenticate = []string{"/ocs/v[12].php/cloud/"} + // services that fallback to reva authentication are declared in the "frontend" command on oCIS. It is a list of + // regexp.Regexp which are safe to use concurrently. + ProxyWwwAuthenticate = []regexp.Regexp{*regexp.MustCompile("/ocs/v[12].php/cloud/")} - // WWWAuthenticate captures the Www-Authenticate header string. - WWWAuthenticate = "Www-Authenticate" + _publicPaths = []string{ + "/dav/public-files/", + "/remote.php/dav/public-files/", + "/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected", + "/ocs/v1.php/cloud/capabilities", + "/data", + } ) -// userAgentLocker aids in dependency injection for helper methods. The set of fields is arbitrary and the only relation -// they share is to fulfill their duty and lock a User-Agent to its correct challenge if configured. -type userAgentLocker struct { - w http.ResponseWriter - r *http.Request - locks map[string]string // locks represents a reva user-agent:challenge mapping. - fallback string +const ( + // WwwAuthenticate captures the Www-Authenticate header string. + WwwAuthenticate = "Www-Authenticate" +) + +// Authenticator is the common interface implemented by all request authenticators. +// The Authenticator may augment the request with user info or anything related to the +// authentication and return the augmented request. +type Authenticator interface { + Authenticate(*http.Request) (*http.Request, bool) } // Authentication is a higher order authentication middleware. -func Authentication(opts ...Option) func(next http.Handler) http.Handler { +func Authentication(auths []Authenticator, opts ...Option) func(next http.Handler) http.Handler { options := newOptions(opts...) - configureSupportedChallenges(options) - oidc := newOIDCAuth(options) - basic := newBasicAuth(options) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if options.OIDCIss != "" && options.EnableBasicAuth { - oidc(basic(next)).ServeHTTP(w, r) + if isOIDCTokenAuth(r) || + r.URL.Path == "/" || + strings.HasPrefix(r.URL.Path, "/.well-known") || + r.URL.Path == "/login" || + strings.HasPrefix(r.URL.Path, "/js") || + strings.HasPrefix(r.URL.Path, "/themes") || + strings.HasPrefix(r.URL.Path, "/signin") || + strings.HasPrefix(r.URL.Path, "/konnect") || + r.URL.Path == "/config.json" || + r.URL.Path == "/oidc-callback.html" || + r.URL.Path == "/oidc-callback" || + r.URL.Path == "/settings.js" { + // The authentication for this request is handled by the IdP. + next.ServeHTTP(w, r) + return } - if options.OIDCIss != "" && !options.EnableBasicAuth { - oidc(next).ServeHTTP(w, r) + for _, a := range auths { + if req, ok := a.Authenticate(r); ok { + next.ServeHTTP(w, req) + return + } } + if !isPublicPath(r.URL.Path) { + for _, s := range SupportedAuthStrategies { + userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, s) + } + } + w.WriteHeader(http.StatusUnauthorized) + // if the request is a PROPFIND return a WebDAV error code. + // TODO: The proxy has to be smart enough to detect when a request is directed towards a webdav server + // and react accordingly. + if webdav.IsWebdavRequest(r) { + b, err := webdav.Marshal(webdav.Exception{ + Code: webdav.SabredavPermissionDenied, + Message: "Authentication error", + }) - if options.OIDCIss == "" && options.EnableBasicAuth { - basic(next).ServeHTTP(w, r) + webdav.HandleWebdavError(w, b, err) } }) } } +// The token auth endpoint uses basic auth for clients, see https://openid.net/specs/openid-connect-basic-1_0.html#TokenRequest +// > The Client MUST authenticate to the Token Endpoint using the HTTP Basic method, as described in 2.3.1 of OAuth 2.0. +func isOIDCTokenAuth(req *http.Request) bool { + return req.URL.Path == "/konnect/v1/token" +} + +func isPublicPath(p string) bool { + for _, pp := range _publicPaths { + if strings.HasPrefix(p, pp) { + return true + } + } + return false +} + // configureSupportedChallenges adds known authentication challenges to the current session. func configureSupportedChallenges(options Options) { if options.OIDCIss != "" { @@ -66,13 +117,22 @@ func configureSupportedChallenges(options Options) { } func writeSupportedAuthenticateHeader(w http.ResponseWriter, r *http.Request) { - for i := 0; i < len(SupportedAuthStrategies); i++ { - w.Header().Add(WWWAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(SupportedAuthStrategies[i]), r.Host)) + for _, s := range SupportedAuthStrategies { + w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(s), r.Host)) } } func removeSuperfluousAuthenticate(w http.ResponseWriter) { - w.Header().Del(WWWAuthenticate) + w.Header().Del(WwwAuthenticate) +} + +// userAgentLocker aids in dependency injection for helper methods. The set of fields is arbitrary and the only relation +// they share is to fulfill their duty and lock a User-Agent to its correct challenge if configured. +type userAgentLocker struct { + w http.ResponseWriter + r *http.Request + locks map[string]string // locks represents a reva user-agent:challenge mapping. + fallback string } // userAgentAuthenticateLockIn sets Www-Authenticate according to configured user agents. This is useful for the case of @@ -86,22 +146,48 @@ func userAgentAuthenticateLockIn(w http.ResponseWriter, r *http.Request, locks m fallback: fallback, } - for i := 0; i < len(ProxyWwwAuthenticate); i++ { - evalRequestURI(&u, i) + for _, r := range ProxyWwwAuthenticate { + evalRequestURI(u, r) } } -func evalRequestURI(l *userAgentLocker, i int) { - r := regexp.MustCompile(ProxyWwwAuthenticate[i]) - if r.Match([]byte(l.r.RequestURI)) { - for k, v := range l.locks { - if strings.Contains(k, l.r.UserAgent()) { - removeSuperfluousAuthenticate(l.w) - l.w.Header().Add(WWWAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), l.r.Host)) - return - } +func evalRequestURI(l userAgentLocker, r regexp.Regexp) { + if !r.MatchString(l.r.RequestURI) { + return + } + for k, v := range l.locks { + if strings.Contains(k, l.r.UserAgent()) { + removeSuperfluousAuthenticate(l.w) + l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), l.r.Host)) + return } - l.w.Header().Add(WWWAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(l.fallback), l.r.Host)) + } + l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(l.fallback), l.r.Host)) +} + +// AuthenticationOld is a higher order authentication middleware. +func AuthenticationOld(opts ...Option) func(next http.Handler) http.Handler { + options := newOptions(opts...) + + configureSupportedChallenges(options) + oidc := newOIDCAuth(options) + // basic := newBasicAuth(options) + + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if options.OIDCIss != "" && options.EnableBasicAuth { + //oidc(basic(next)).ServeHTTP(w, r) + oidc(next).ServeHTTP(w, r) + } + + if options.OIDCIss != "" && !options.EnableBasicAuth { + oidc(next).ServeHTTP(w, r) + } + + // if options.OIDCIss == "" && options.EnableBasicAuth { + // basic(next).ServeHTTP(w, r) + // } + }) } } @@ -120,15 +206,15 @@ func newOIDCAuth(options Options) func(http.Handler) http.Handler { ) } -// newBasicAuth returns a configured basic middleware -func newBasicAuth(options Options) func(http.Handler) http.Handler { - return BasicAuth( - UserProvider(options.UserProvider), - Logger(options.Logger), - EnableBasicAuth(options.EnableBasicAuth), - OIDCIss(options.OIDCIss), - UserOIDCClaim(options.UserOIDCClaim), - UserCS3Claim(options.UserCS3Claim), - CredentialsByUserAgent(options.CredentialsByUserAgent), - ) -} +// // newBasicAuth returns a configured basic middleware +// func newBasicAuth(options Options) func(http.Handler) http.Handler { +// return BasicAuth( +// UserProvider(options.UserProvider), +// Logger(options.Logger), +// EnableBasicAuth(options.EnableBasicAuth), +// OIDCIss(options.OIDCIss), +// UserOIDCClaim(options.UserOIDCClaim), +// UserCS3Claim(options.UserCS3Claim), +// CredentialsByUserAgent(options.CredentialsByUserAgent), +// ) +// } diff --git a/services/proxy/pkg/middleware/basic_auth.go b/services/proxy/pkg/middleware/basic_auth.go index b7d480d87b..4771e57f21 100644 --- a/services/proxy/pkg/middleware/basic_auth.go +++ b/services/proxy/pkg/middleware/basic_auth.go @@ -1,145 +1,52 @@ package middleware import ( - "fmt" "net/http" - "strings" "github.com/owncloud/ocis/v2/ocis-pkg/log" "github.com/owncloud/ocis/v2/ocis-pkg/oidc" "github.com/owncloud/ocis/v2/services/proxy/pkg/user/backend" - "github.com/owncloud/ocis/v2/services/proxy/pkg/webdav" ) -// 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 - - if options.EnableBasicAuth { - options.Logger.Warn().Msg("basic auth enabled, use only for testing or development") - } - - h := basicAuth{ - logger: logger, - enabled: options.EnableBasicAuth, - userProvider: options.UserProvider, - } - - return func(next http.Handler) http.Handler { - return http.HandlerFunc( - func(w http.ResponseWriter, req *http.Request) { - if h.isPublicLink(req) || !h.isBasicAuth(req) || h.isOIDCTokenAuth(req) { - if !h.isPublicLink(req) { - userAgentAuthenticateLockIn(w, req, options.CredentialsByUserAgent, "basic") - } - next.ServeHTTP(w, req) - return - } - - removeSuperfluousAuthenticate(w) - login, password, _ := req.BasicAuth() - user, _, err := h.userProvider.Authenticate(req.Context(), login, password) - - // touch is a user agent locking guard, when touched changes to true it indicates the User-Agent on the - // request is configured to support only one challenge, it it remains untouched, there are no considera- - // tions and we should write all available authentication challenges to the response. - touch := false - - if err != nil { - for k, v := range options.CredentialsByUserAgent { - if strings.Contains(k, req.UserAgent()) { - removeSuperfluousAuthenticate(w) - w.Header().Add("Www-Authenticate", fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), req.Host)) - touch = true - break - } - } - - // if the request is not bound to any user agent, write all available challenges - if !touch { - writeSupportedAuthenticateHeader(w, req) - } - - // if the request is a PROPFIND return a WebDAV error code. - // TODO: The proxy has to be smart enough to detect when a request is directed towards a webdav server - // and react accordingly. - - w.WriteHeader(http.StatusUnauthorized) - - if webdav.IsWebdavRequest(req) { - b, err := webdav.Marshal(webdav.Exception{ - Code: webdav.SabredavPermissionDenied, - Message: "Authentication error", - }) - - webdav.HandleWebdavError(w, b, err) - return - } - - return - } - - // fake oidc claims - claims := map[string]interface{}{ - oidc.Iss: user.Id.Idp, - oidc.PreferredUsername: user.Username, - oidc.Email: user.Mail, - oidc.OwncloudUUID: user.Id.OpaqueId, - } - - if options.UserCS3Claim == "userid" { - // set the custom user claim only if users will be looked up by the userid on the CS3api - // OpaqueId contains the userid configured in STORAGE_LDAP_USER_SCHEMA_UID - claims[options.UserOIDCClaim] = user.Id.OpaqueId - - } - - next.ServeHTTP(w, req.WithContext(oidc.NewContext(req.Context(), claims))) - }, - ) - } +type BasicAuthenticator struct { + Logger log.Logger + UserProvider backend.UserBackend + UserCS3Claim string + UserOIDCClaim string } -type basicAuth struct { - logger log.Logger - enabled bool - userProvider backend.UserBackend -} - -func (m basicAuth) isPublicLink(req *http.Request) bool { - login, _, ok := req.BasicAuth() - - if !ok || login != "public" { - return false +func (m BasicAuthenticator) Authenticate(req *http.Request) (*http.Request, bool) { + if isPublicPath(req.URL.Path) { + // The authentication of public path requests is handled by another authenticator. + // Since we can't guarantee the order of execution of the authenticators, we better + // implement an early return here for paths we can't authenticate in this authenticator. + return nil, false } - publicPaths := []string{ - "/dav/public-files/", - "/remote.php/dav/public-files/", - "/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected", - "/ocs/v1.php/cloud/capabilities", - "/data", - } - isPublic := false - - for _, p := range publicPaths { - if strings.HasPrefix(req.URL.Path, p) { - isPublic = true - break - } + login, password, ok := req.BasicAuth() + if !ok { + return nil, false } - return isPublic -} + user, _, err := m.UserProvider.Authenticate(req.Context(), login, password) + if err != nil { + // TODO add log line + return nil, false + } -// The token auth endpoint uses basic auth for clients, see https://openid.net/specs/openid-connect-basic-1_0.html#TokenRequest -// > The Client MUST authenticate to the Token Endpoint using the HTTP Basic method, as described in 2.3.1 of OAuth 2.0. -func (m basicAuth) isOIDCTokenAuth(req *http.Request) bool { - return req.URL.Path == "/konnect/v1/token" -} + // fake oidc claims + claims := map[string]interface{}{ + oidc.Iss: user.Id.Idp, + oidc.PreferredUsername: user.Username, + oidc.Email: user.Mail, + oidc.OwncloudUUID: user.Id.OpaqueId, + } -func (m basicAuth) isBasicAuth(req *http.Request) bool { - _, _, ok := req.BasicAuth() - return m.enabled && ok + if m.UserCS3Claim == "userid" { + // set the custom user claim only if users will be looked up by the userid on the CS3api + // OpaqueId contains the userid configured in STORAGE_LDAP_USER_SCHEMA_UID + claims[m.UserOIDCClaim] = user.Id.OpaqueId + + } + return req.WithContext(oidc.NewContext(req.Context(), claims)), true } diff --git a/services/proxy/pkg/middleware/basic_auth_test.go b/services/proxy/pkg/middleware/basic_auth_test.go index 46c7f48285..77279d66b4 100644 --- a/services/proxy/pkg/middleware/basic_auth_test.go +++ b/services/proxy/pkg/middleware/basic_auth_test.go @@ -23,8 +23,6 @@ func TestBasicAuth__isPublicLink(t *testing.T) { {url: "/ocs/v1.php/cloud/capabilities", username: "public", expected: true}, {url: "/ocs/v1.php/cloud/users/admin", username: "public", expected: false}, } - ba := basicAuth{} - for _, tt := range tests { req := httptest.NewRequest("", tt.url, nil) @@ -32,7 +30,7 @@ func TestBasicAuth__isPublicLink(t *testing.T) { req.SetBasicAuth(tt.username, "") } - result := ba.isPublicLink(req) + result := isPublicPath(req.URL.Path) if result != tt.expected { t.Errorf("with %s expected %t got %t", tt.url, tt.expected, result) } diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index ae3b313f08..97a71962ff 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -25,80 +25,29 @@ type OIDCProvider interface { UserInfo(ctx context.Context, ts oauth2.TokenSource) (*gOidc.UserInfo, error) } -// OIDCAuth provides a middleware to check access secured by a static token. -func OIDCAuth(optionSetters ...Option) func(next http.Handler) http.Handler { - options := newOptions(optionSetters...) - tokenCache := osync.NewCache(options.UserinfoCacheSize) +type OIDCAuthenticator struct { + Logger log.Logger + HTTPClient *http.Client + OIDCIss string + TokenCache *osync.Cache + TokenCacheTTL time.Duration + ProviderFunc func() (OIDCProvider, error) + AccessTokenVerifyMethod string + JWKSOptions config.JWKS - h := oidcAuth{ - logger: options.Logger, - providerFunc: options.OIDCProviderFunc, - httpClient: options.HTTPClient, - oidcIss: options.OIDCIss, - tokenCache: &tokenCache, - tokenCacheTTL: options.UserinfoCacheTTL, - accessTokenVerifyMethod: options.AccessTokenVerifyMethod, - jwksOptions: options.JWKS, - jwksLock: &sync.Mutex{}, - providerLock: &sync.Mutex{}, - } + providerLock *sync.Mutex + provider OIDCProvider - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - // there is no bearer token on the request, - if !h.shouldServe(req) { - // oidc supported but token not present, add header and handover to the next middleware. - userAgentAuthenticateLockIn(w, req, options.CredentialsByUserAgent, "bearer") - next.ServeHTTP(w, req) - return - } - - if h.getProvider() == nil { - w.WriteHeader(http.StatusInternalServerError) - return - } - - // Force init of jwks keyfunc if needed (contacts the .well-known and jwks endpoints on first call) - if h.accessTokenVerifyMethod == config.AccessTokenVerificationJWT && h.getKeyfunc() == nil { - w.WriteHeader(http.StatusInternalServerError) - return - } - - token := strings.TrimPrefix(req.Header.Get("Authorization"), "Bearer ") - - claims, status := h.getClaims(token, req) - if status != 0 { - w.WriteHeader(status) - return - } - - // inject claims to the request context for the account_resolver middleware. - next.ServeHTTP(w, req.WithContext(oidc.NewContext(req.Context(), claims))) - }) - } + jwksLock *sync.Mutex + JWKS *keyfunc.JWKS } -type oidcAuth struct { - logger log.Logger - provider OIDCProvider - providerLock *sync.Mutex - jwksOptions config.JWKS - jwks *keyfunc.JWKS - jwksLock *sync.Mutex - providerFunc func() (OIDCProvider, error) - httpClient *http.Client - oidcIss string - tokenCache *osync.Cache - tokenCacheTTL time.Duration - accessTokenVerifyMethod string -} - -func (m oidcAuth) getClaims(token string, req *http.Request) (claims map[string]interface{}, status int) { - hit := m.tokenCache.Load(token) +func (m OIDCAuthenticator) getClaims(token string, req *http.Request) (claims map[string]interface{}, status int) { + hit := m.TokenCache.Load(token) if hit == nil { aClaims, err := m.verifyAccessToken(token) if err != nil { - m.logger.Error().Err(err).Msg("Failed to verify access token") + m.Logger.Error().Err(err).Msg("Failed to verify access token") status = http.StatusUnauthorized return } @@ -108,25 +57,25 @@ func (m oidcAuth) getClaims(token string, req *http.Request) (claims map[string] } userInfo, err := m.getProvider().UserInfo( - context.WithValue(req.Context(), oauth2.HTTPClient, m.httpClient), + context.WithValue(req.Context(), oauth2.HTTPClient, m.HTTPClient), oauth2.StaticTokenSource(oauth2Token), ) if err != nil { - m.logger.Error().Err(err).Msg("Failed to get userinfo") + m.Logger.Error().Err(err).Msg("Failed to get userinfo") status = http.StatusUnauthorized return } if err := userInfo.Claims(&claims); err != nil { - m.logger.Error().Err(err).Interface("userinfo", userInfo).Msg("failed to unmarshal userinfo claims") + m.Logger.Error().Err(err).Interface("userinfo", userInfo).Msg("failed to unmarshal userinfo claims") status = http.StatusInternalServerError return } expiration := m.extractExpiration(aClaims) - m.tokenCache.Store(token, claims, expiration) + m.TokenCache.Store(token, claims, expiration) - m.logger.Debug().Interface("claims", claims).Interface("userInfo", userInfo).Time("expiration", expiration.UTC()).Msg("unmarshalled and cached userinfo") + m.Logger.Debug().Interface("claims", claims).Interface("userInfo", userInfo).Time("expiration", expiration.UTC()).Msg("unmarshalled and cached userinfo") return } @@ -135,25 +84,25 @@ func (m oidcAuth) getClaims(token string, req *http.Request) (claims map[string] status = http.StatusInternalServerError return } - m.logger.Debug().Interface("claims", claims).Msg("cache hit for userinfo") + m.Logger.Debug().Interface("claims", claims).Msg("cache hit for userinfo") return } -func (m oidcAuth) verifyAccessToken(token string) (jwt.RegisteredClaims, error) { - switch m.accessTokenVerifyMethod { +func (m OIDCAuthenticator) verifyAccessToken(token string) (jwt.RegisteredClaims, error) { + switch m.AccessTokenVerifyMethod { case config.AccessTokenVerificationJWT: return m.verifyAccessTokenJWT(token) case config.AccessTokenVerificationNone: - m.logger.Debug().Msg("Access Token verification disabled") + m.Logger.Debug().Msg("Access Token verification disabled") return jwt.RegisteredClaims{}, nil default: - m.logger.Error().Str("access_token_verify_method", m.accessTokenVerifyMethod).Msg("Unknown Access Token verification setting") + m.Logger.Error().Str("access_token_verify_method", m.AccessTokenVerifyMethod).Msg("Unknown Access Token verification setting") return jwt.RegisteredClaims{}, errors.New("Unknown Access Token Verification method") } } // verifyAccessTokenJWT tries to parse and verify the access token as a JWT. -func (m oidcAuth) verifyAccessTokenJWT(token string) (jwt.RegisteredClaims, error) { +func (m OIDCAuthenticator) verifyAccessTokenJWT(token string) (jwt.RegisteredClaims, error) { var claims jwt.RegisteredClaims jwks := m.getKeyfunc() if jwks == nil { @@ -161,13 +110,13 @@ func (m oidcAuth) verifyAccessTokenJWT(token string) (jwt.RegisteredClaims, erro } _, err := jwt.ParseWithClaims(token, &claims, jwks.Keyfunc) - m.logger.Debug().Interface("access token", &claims).Msg("parsed access token") + m.Logger.Debug().Interface("access token", &claims).Msg("parsed access token") if err != nil { - m.logger.Info().Err(err).Msg("Failed to parse/verify the access token.") + m.Logger.Info().Err(err).Msg("Failed to parse/verify the access token.") return claims, err } - if !claims.VerifyIssuer(m.oidcIss, true) { + if !claims.VerifyIssuer(m.OIDCIss, true) { vErr := jwt.ValidationError{} vErr.Inner = jwt.ErrTokenInvalidIssuer vErr.Errors |= jwt.ValidationErrorIssuer @@ -180,19 +129,19 @@ func (m oidcAuth) verifyAccessTokenJWT(token string) (jwt.RegisteredClaims, erro // extractExpiration tries to extract the expriration time from the access token // If the access token does not have an exp claim it will fallback to the configured // default expiration -func (m oidcAuth) extractExpiration(aClaims jwt.RegisteredClaims) time.Time { - defaultExpiration := time.Now().Add(m.tokenCacheTTL) +func (m OIDCAuthenticator) extractExpiration(aClaims jwt.RegisteredClaims) time.Time { + defaultExpiration := time.Now().Add(m.TokenCacheTTL) if aClaims.ExpiresAt != nil { - m.logger.Debug().Str("exp", aClaims.ExpiresAt.String()).Msg("Expiration Time from access_token") + m.Logger.Debug().Str("exp", aClaims.ExpiresAt.String()).Msg("Expiration Time from access_token") return aClaims.ExpiresAt.Time } return defaultExpiration } -func (m oidcAuth) shouldServe(req *http.Request) bool { +func (m OIDCAuthenticator) shouldServe(req *http.Request) bool { header := req.Header.Get("Authorization") - if m.oidcIss == "" { + if m.OIDCIss == "" { return false } @@ -211,58 +160,58 @@ type jwksJSON struct { JWKSURL string `json:"jwks_uri"` } -func (m *oidcAuth) getKeyfunc() *keyfunc.JWKS { +func (m *OIDCAuthenticator) getKeyfunc() *keyfunc.JWKS { m.jwksLock.Lock() defer m.jwksLock.Unlock() - if m.jwks == nil { - wellKnown := strings.TrimSuffix(m.oidcIss, "/") + "/.well-known/openid-configuration" + if m.JWKS == nil { + wellKnown := strings.TrimSuffix(m.OIDCIss, "/") + "/.well-known/openid-configuration" - resp, err := m.httpClient.Get(wellKnown) + resp, err := m.HTTPClient.Get(wellKnown) if err != nil { - m.logger.Error().Err(err).Msg("Failed to set request for .well-known/openid-configuration") + m.Logger.Error().Err(err).Msg("Failed to set request for .well-known/openid-configuration") return nil } defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) if err != nil { - m.logger.Error().Err(err).Msg("unable to read discovery response body") + m.Logger.Error().Err(err).Msg("unable to read discovery response body") return nil } if resp.StatusCode != http.StatusOK { - m.logger.Error().Str("status", resp.Status).Str("body", string(body)).Msg("error requesting openid-configuration") + m.Logger.Error().Str("status", resp.Status).Str("body", string(body)).Msg("error requesting openid-configuration") return nil } var j jwksJSON err = json.Unmarshal(body, &j) if err != nil { - m.logger.Error().Err(err).Msg("failed to decode provider openid-configuration") + m.Logger.Error().Err(err).Msg("failed to decode provider openid-configuration") return nil } - m.logger.Debug().Str("jwks", j.JWKSURL).Msg("discovered jwks endpoint") + m.Logger.Debug().Str("jwks", j.JWKSURL).Msg("discovered jwks endpoint") options := keyfunc.Options{ - Client: m.httpClient, + Client: m.HTTPClient, RefreshErrorHandler: func(err error) { - m.logger.Error().Err(err).Msg("There was an error with the jwt.Keyfunc") + m.Logger.Error().Err(err).Msg("There was an error with the jwt.Keyfunc") }, - RefreshInterval: time.Minute * time.Duration(m.jwksOptions.RefreshInterval), - RefreshRateLimit: time.Second * time.Duration(m.jwksOptions.RefreshRateLimit), - RefreshTimeout: time.Second * time.Duration(m.jwksOptions.RefreshTimeout), - RefreshUnknownKID: m.jwksOptions.RefreshUnknownKID, + RefreshInterval: time.Minute * time.Duration(m.JWKSOptions.RefreshInterval), + RefreshRateLimit: time.Second * time.Duration(m.JWKSOptions.RefreshRateLimit), + RefreshTimeout: time.Second * time.Duration(m.JWKSOptions.RefreshTimeout), + RefreshUnknownKID: m.JWKSOptions.RefreshUnknownKID, } - m.jwks, err = keyfunc.Get(j.JWKSURL, options) + m.JWKS, err = keyfunc.Get(j.JWKSURL, options) if err != nil { - m.jwks = nil - m.logger.Error().Err(err).Msg("Failed to create JWKS from resource at the given URL.") + m.JWKS = nil + m.Logger.Error().Err(err).Msg("Failed to create JWKS from resource at the given URL.") return nil } } - return m.jwks + return m.JWKS } -func (m *oidcAuth) getProvider() OIDCProvider { +func (m *OIDCAuthenticator) getProvider() OIDCProvider { m.providerLock.Lock() defer m.providerLock.Unlock() if m.provider == nil { @@ -271,9 +220,9 @@ func (m *oidcAuth) getProvider() OIDCProvider { // provider needs to be cached as when it is created // it will fetch the keys from the issuer using the .well-known // endpoint - provider, err := m.providerFunc() + provider, err := m.ProviderFunc() if err != nil { - m.logger.Error().Err(err).Msg("could not initialize oidcAuth provider") + m.Logger.Error().Err(err).Msg("could not initialize oidcAuth provider") return nil } @@ -281,3 +230,32 @@ func (m *oidcAuth) getProvider() OIDCProvider { } return m.provider } + +func (m OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { + // there is no bearer token on the request, + if !m.shouldServe(r) { + // // oidc supported but token not present, add header and handover to the next middleware. + // userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, "bearer") + // next.ServeHTTP(w, r) + return nil, false + } + + if m.getProvider() == nil { + // w.WriteHeader(http.StatusInternalServerError) + return nil, false + } + // Force init of jwks keyfunc if needed (contacts the .well-known and jwks endpoints on first call) + if m.AccessTokenVerifyMethod == config.AccessTokenVerificationJWT && m.getKeyfunc() == nil { + return nil, false + } + + token := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + + claims, status := m.getClaims(token, r) + if status != 0 { + // w.WriteHeader(status) + // TODO log + return nil, false + } + return r.WithContext(oidc.NewContext(r.Context(), claims)), true +} diff --git a/services/proxy/pkg/middleware/public_share_auth.go b/services/proxy/pkg/middleware/public_share_auth.go index 9b3ddcd5ff..6d6d0fa747 100644 --- a/services/proxy/pkg/middleware/public_share_auth.go +++ b/services/proxy/pkg/middleware/public_share_auth.go @@ -5,6 +5,7 @@ import ( "strings" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + "github.com/owncloud/ocis/v2/ocis-pkg/log" ) const ( @@ -14,59 +15,58 @@ const ( authenticationType = "publicshares" ) -// PublicShareAuth ... -func PublicShareAuth(opts ...Option) func(next http.Handler) http.Handler { - options := newOptions(opts...) - logger := options.Logger - - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - shareToken := r.Header.Get(headerShareToken) - if shareToken == "" { - shareToken = r.URL.Query().Get(headerShareToken) - } - - // Currently we only want to authenticate app open request coming from public shares. - if shareToken == "" { - // Don't authenticate - next.ServeHTTP(w, r) - return - } - - var sharePassword string - if signature := r.URL.Query().Get("signature"); signature != "" { - expiration := r.URL.Query().Get("expiration") - if expiration == "" { - logger.Warn().Str("signature", signature).Msg("cannot do signature auth without the expiration") - next.ServeHTTP(w, r) - return - } - sharePassword = strings.Join([]string{"signature", signature, expiration}, "|") - } else { - // We can ignore the username since it is always set to "public" in public shares. - _, password, ok := r.BasicAuth() - - sharePassword = basicAuthPasswordPrefix - if ok { - sharePassword += password - } - } - - authResp, err := options.RevaGatewayClient.Authenticate(r.Context(), &gateway.AuthenticateRequest{ - Type: authenticationType, - ClientId: shareToken, - ClientSecret: sharePassword, - }) - - if err != nil { - logger.Debug().Err(err).Str("public_share_token", shareToken).Msg("could not authenticate public share") - // try another middleware - next.ServeHTTP(w, r) - return - } - - r.Header.Add(headerRevaAccessToken, authResp.Token) - next.ServeHTTP(w, r) - }) - } +type PublicShareAuthenticator struct { + Logger log.Logger + RevaGatewayClient gateway.GatewayAPIClient +} + +func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { + if !isPublicPath(r.URL.Path) { + return nil, false + } + + query := r.URL.Query() + shareToken := r.Header.Get(headerShareToken) + if shareToken == "" { + shareToken = query.Get(headerShareToken) + } + + // Currently we only want to authenticate app open request coming from public shares. + if shareToken == "" { + // Don't authenticate + return nil, false + } + + var sharePassword string + if signature := query.Get("signature"); signature != "" { + expiration := query.Get("expiration") + if expiration == "" { + a.Logger.Warn().Str("signature", signature).Msg("cannot do signature auth without the expiration") + return nil, false + } + sharePassword = strings.Join([]string{"signature", signature, expiration}, "|") + } else { + // We can ignore the username since it is always set to "public" in public shares. + _, password, ok := r.BasicAuth() + + sharePassword = basicAuthPasswordPrefix + if ok { + sharePassword += password + } + } + + authResp, err := a.RevaGatewayClient.Authenticate(r.Context(), &gateway.AuthenticateRequest{ + Type: authenticationType, + ClientId: shareToken, + ClientSecret: sharePassword, + }) + + if err != nil { + a.Logger.Debug().Err(err).Str("public_share_token", shareToken).Msg("could not authenticate public share") + // try another middleware + return nil, false + } + + r.Header.Add(headerRevaAccessToken, authResp.Token) + return r, false } diff --git a/services/proxy/pkg/middleware/signed_url_auth.go b/services/proxy/pkg/middleware/signed_url_auth.go index d71036d5a7..094c279ad4 100644 --- a/services/proxy/pkg/middleware/signed_url_auth.go +++ b/services/proxy/pkg/middleware/signed_url_auth.go @@ -25,7 +25,7 @@ func SignedURLAuth(optionSetters ...Option) func(next http.Handler) http.Handler options := newOptions(optionSetters...) return func(next http.Handler) http.Handler { - return &signedURLAuth{ + return &SignedURLAuthenticator{ next: next, logger: options.Logger, preSignedURLConfig: options.PreSignedURLConfig, @@ -35,7 +35,7 @@ func SignedURLAuth(optionSetters ...Option) func(next http.Handler) http.Handler } } -type signedURLAuth struct { +type SignedURLAuthenticator struct { next http.Handler logger log.Logger preSignedURLConfig config.PreSignedURL @@ -43,7 +43,7 @@ type signedURLAuth struct { store storesvc.StoreService } -func (m signedURLAuth) ServeHTTP(w http.ResponseWriter, req *http.Request) { +func (m SignedURLAuthenticator) ServeHTTP(w http.ResponseWriter, req *http.Request) { if !m.shouldServe(req) { m.next.ServeHTTP(w, req) return @@ -67,14 +67,14 @@ func (m signedURLAuth) ServeHTTP(w http.ResponseWriter, req *http.Request) { m.next.ServeHTTP(w, req) } -func (m signedURLAuth) shouldServe(req *http.Request) bool { +func (m SignedURLAuthenticator) shouldServe(req *http.Request) bool { if !m.preSignedURLConfig.Enabled { return false } return req.URL.Query().Get("OC-Signature") != "" } -func (m signedURLAuth) validate(req *http.Request) (err error) { +func (m SignedURLAuthenticator) validate(req *http.Request) (err error) { query := req.URL.Query() if ok, err := m.allRequiredParametersArePresent(query); !ok { @@ -100,7 +100,7 @@ func (m signedURLAuth) validate(req *http.Request) (err error) { return nil } -func (m signedURLAuth) allRequiredParametersArePresent(query url.Values) (ok bool, err error) { +func (m SignedURLAuthenticator) allRequiredParametersArePresent(query url.Values) (ok bool, err error) { // check if required query parameters exist in given request query parameters // OC-Signature - the computed signature - server will verify the request upon this REQUIRED // OC-Credential - defines the user scope (shall we use the owncloud user id here - this might leak internal data ....) REQUIRED @@ -122,7 +122,7 @@ func (m signedURLAuth) allRequiredParametersArePresent(query url.Values) (ok boo return true, nil } -func (m signedURLAuth) requestMethodMatches(meth string, query url.Values) (ok bool, err error) { +func (m SignedURLAuthenticator) requestMethodMatches(meth string, query url.Values) (ok bool, err error) { // check if given url query parameter OC-Verb matches given request method if !strings.EqualFold(meth, query.Get("OC-Verb")) { return false, errors.New("required OC-Verb parameter did not match request method") @@ -131,7 +131,7 @@ func (m signedURLAuth) requestMethodMatches(meth string, query url.Values) (ok b return true, nil } -func (m signedURLAuth) requestMethodIsAllowed(meth string) (ok bool, err error) { +func (m SignedURLAuthenticator) requestMethodIsAllowed(meth string) (ok bool, err error) { // check if given request method is allowed methodIsAllowed := false for _, am := range m.preSignedURLConfig.AllowedHTTPMethods { @@ -147,7 +147,7 @@ func (m signedURLAuth) requestMethodIsAllowed(meth string) (ok bool, err error) return true, nil } -func (m signedURLAuth) urlIsExpired(query url.Values, now func() time.Time) (expired bool, err error) { +func (m SignedURLAuthenticator) urlIsExpired(query url.Values, now func() time.Time) (expired bool, err error) { // check if url is expired by checking if given date (OC-Date) + expires in seconds (OC-Expires) is after now validFrom, err := time.Parse(time.RFC3339, query.Get("OC-Date")) if err != nil { @@ -164,7 +164,7 @@ func (m signedURLAuth) urlIsExpired(query url.Values, now func() time.Time) (exp return !(now().After(validFrom) && now().Before(validTo)), nil } -func (m signedURLAuth) signatureIsValid(req *http.Request) (ok bool, err error) { +func (m SignedURLAuthenticator) signatureIsValid(req *http.Request) (ok bool, err error) { u := revactx.ContextMustGetUser(req.Context()) signingKey, err := m.getSigningKey(req.Context(), u.Id.OpaqueId) if err != nil { @@ -187,7 +187,7 @@ func (m signedURLAuth) signatureIsValid(req *http.Request) (ok bool, err error) return m.createSignature(url, signingKey) == signature, nil } -func (m signedURLAuth) createSignature(url string, signingKey []byte) string { +func (m SignedURLAuthenticator) createSignature(url string, signingKey []byte) string { // the oc10 signature check: $hash = \hash_pbkdf2("sha512", $url, $signingKey, 10000, 64, false); // - sets the length of the output string to 64 // - sets raw output to false -> if raw_output is FALSE length corresponds to twice the byte-length of the derived key (as every byte of the key is returned as two hexits). @@ -197,7 +197,7 @@ func (m signedURLAuth) createSignature(url string, signingKey []byte) string { return hex.EncodeToString(hash) } -func (m signedURLAuth) getSigningKey(ctx context.Context, ocisID string) ([]byte, error) { +func (m SignedURLAuthenticator) getSigningKey(ctx context.Context, ocisID string) ([]byte, error) { res, err := m.store.Read(ctx, &storesvc.ReadRequest{ Options: &storemsg.ReadOptions{ Database: "proxy", @@ -211,3 +211,26 @@ func (m signedURLAuth) getSigningKey(ctx context.Context, ocisID string) ([]byte return res.Records[0].Value, nil } + +func (m SignedURLAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { + if !m.shouldServe(r) { + return nil, false + } + + user, _, err := m.userProvider.GetUserByClaims(r.Context(), "username", r.URL.Query().Get("OC-Credential"), true) + if err != nil { + m.logger.Error().Err(err).Msg("Could not get user by claim") + return nil, false + } + + ctx := revactx.ContextSetUser(r.Context(), user) + + r = r.WithContext(ctx) + + if err := m.validate(r); err != nil { + // http.Error(w, "Invalid url signature", http.StatusUnauthorized) + return nil, false + } + + return r, true +} diff --git a/services/proxy/pkg/middleware/signed_url_auth_test.go b/services/proxy/pkg/middleware/signed_url_auth_test.go index 856fcdb0e2..01311b731e 100644 --- a/services/proxy/pkg/middleware/signed_url_auth_test.go +++ b/services/proxy/pkg/middleware/signed_url_auth_test.go @@ -7,7 +7,7 @@ import ( ) func TestSignedURLAuth_shouldServe(t *testing.T) { - pua := signedURLAuth{} + pua := SignedURLAuthenticator{} tests := []struct { url string enabled bool @@ -31,7 +31,7 @@ func TestSignedURLAuth_shouldServe(t *testing.T) { } func TestSignedURLAuth_allRequiredParametersPresent(t *testing.T) { - pua := signedURLAuth{} + pua := SignedURLAuthenticator{} baseURL := "https://example.com/example.jpg?" tests := []struct { params string @@ -54,7 +54,7 @@ func TestSignedURLAuth_allRequiredParametersPresent(t *testing.T) { } func TestSignedURLAuth_requestMethodMatches(t *testing.T) { - pua := signedURLAuth{} + pua := SignedURLAuthenticator{} tests := []struct { method string url string @@ -75,7 +75,7 @@ func TestSignedURLAuth_requestMethodMatches(t *testing.T) { } func TestSignedURLAuth_requestMethodIsAllowed(t *testing.T) { - pua := signedURLAuth{} + pua := SignedURLAuthenticator{} tests := []struct { method string allowed []string @@ -99,7 +99,7 @@ func TestSignedURLAuth_requestMethodIsAllowed(t *testing.T) { } func TestSignedURLAuth_urlIsExpired(t *testing.T) { - pua := signedURLAuth{} + pua := SignedURLAuthenticator{} nowFunc := func() time.Time { t, _ := time.Parse(time.RFC3339, "2020-02-02T12:30:00.000Z") return t @@ -126,7 +126,7 @@ func TestSignedURLAuth_urlIsExpired(t *testing.T) { } func TestSignedURLAuth_createSignature(t *testing.T) { - pua := signedURLAuth{} + pua := SignedURLAuthenticator{} expected := "27d2ebea381384af3179235114801dcd00f91e46f99fca72575301cf3948101d" s := pua.createSignature("something", []byte("somerandomkey")) From f35c8b9205c79470da0bdc3d7ed7188886740711 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 8 Aug 2022 15:49:22 +0200 Subject: [PATCH 02/20] clean up the authenticators middlewares --- services/proxy/pkg/command/server.go | 53 ++------- .../proxy/pkg/middleware/authentication.go | 56 +-------- services/proxy/pkg/middleware/basic_auth.go | 20 +++- services/proxy/pkg/middleware/oidc_auth.go | 63 +++++----- .../proxy/pkg/middleware/public_share_auth.go | 20 +++- .../proxy/pkg/middleware/signed_url_auth.go | 111 ++++++++---------- 6 files changed, 126 insertions(+), 197 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 9e372e4481..7377d95829 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -19,6 +19,7 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/sync" "github.com/owncloud/ocis/v2/ocis-pkg/version" settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" + storesvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/store/v0" "github.com/owncloud/ocis/v2/services/proxy/pkg/config" "github.com/owncloud/ocis/v2/services/proxy/pkg/config/parser" "github.com/owncloud/ocis/v2/services/proxy/pkg/cs3" @@ -149,7 +150,7 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) logger.Fatal().Msgf("Invalid accounts backend type '%s'", cfg.AccountBackend) } - // storeClient := storesvc.NewStoreService("com.owncloud.api.store", grpc.DefaultClient) + storeClient := storesvc.NewStoreService("com.owncloud.api.store", grpc.DefaultClient) if err != nil { logger.Error().Err(err). Str("gateway", cfg.Reva.Address). @@ -193,10 +194,17 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) JWKSOptions: cfg.OIDC.JWKS, AccessTokenVerifyMethod: cfg.OIDC.AccessTokenVerifyMethod, }) - // authenticators = append(authenticators, middleware.PublicShareAuthenticator{ - // Logger: logger, - // RevaGatewayClient: revaClient, - // }) + authenticators = append(authenticators, middleware.PublicShareAuthenticator{ + Logger: logger, + RevaGatewayClient: revaClient, + }) + + authenticators = append(authenticators, middleware.SignedURLAuthenticator{ + Logger: logger, + PreSignedURLConfig: cfg.PreSignedURL, + UserProvider: userProvider, + Store: storeClient, + }) return alice.New( // first make sure we log all requests and redirect to https if necessary @@ -211,18 +219,6 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) oidcHTTPClient, ), - // middleware.AuthenticationOld( - // // OIDC Options - // middleware.OIDCProviderFunc(func() (middleware.OIDCProvider, error) { - // // Initialize a provider by specifying the issuer URL. - // // it will fetch the keys from the issuer using the .well-known - // // endpoint - // return oidc.NewProvider( - // context.WithValue(ctx, oauth2.HTTPClient, oidcHTTPClient), - // cfg.OIDC.Issuer, - // ) - // }), - middleware.Authentication( authenticators, middleware.CredentialsByUserAgent(cfg.AuthMiddleware.CredentialsByUserAgent), @@ -230,25 +226,6 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) middleware.OIDCIss(cfg.OIDC.Issuer), middleware.EnableBasicAuth(cfg.EnableBasicAuth), ), - // middleware.HTTPClient(oidcHTTPClient), - // middleware.TokenCacheSize(cfg.OIDC.UserinfoCache.Size), - // middleware.TokenCacheTTL(time.Second*time.Duration(cfg.OIDC.UserinfoCache.TTL)), - // - // // basic Options - // middleware.Logger(logger), - // middleware.EnableBasicAuth(cfg.EnableBasicAuth), - // middleware.UserProvider(userProvider), - // middleware.OIDCIss(cfg.OIDC.Issuer), - // middleware.UserOIDCClaim(cfg.UserOIDCClaim), - // middleware.UserCS3Claim(cfg.UserCS3Claim), - // middleware.CredentialsByUserAgent(cfg.AuthMiddleware.CredentialsByUserAgent), - // ), - // middleware.SignedURLAuth( - // middleware.Logger(logger), - // middleware.PreSignedURLConfig(cfg.PreSignedURL), - // middleware.UserProvider(userProvider), - // middleware.Store(storeClient), - // ), middleware.AccountResolver( middleware.Logger(logger), middleware.UserProvider(userProvider), @@ -270,9 +247,5 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) middleware.TokenManagerConfig(*cfg.TokenManager), middleware.RevaGatewayClient(revaClient), ), - // middleware.PublicShareAuth( - // middleware.Logger(logger), - // middleware.RevaGatewayClient(revaClient), - // ), ) } diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 651876f35f..6c961657d1 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -18,7 +18,7 @@ var ( // regexp.Regexp which are safe to use concurrently. ProxyWwwAuthenticate = []regexp.Regexp{*regexp.MustCompile("/ocs/v[12].php/cloud/")} - _publicPaths = []string{ + _publicPaths = [...]string{ "/dav/public-files/", "/remote.php/dav/public-files/", "/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected", @@ -164,57 +164,3 @@ func evalRequestURI(l userAgentLocker, r regexp.Regexp) { } l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(l.fallback), l.r.Host)) } - -// AuthenticationOld is a higher order authentication middleware. -func AuthenticationOld(opts ...Option) func(next http.Handler) http.Handler { - options := newOptions(opts...) - - configureSupportedChallenges(options) - oidc := newOIDCAuth(options) - // basic := newBasicAuth(options) - - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if options.OIDCIss != "" && options.EnableBasicAuth { - //oidc(basic(next)).ServeHTTP(w, r) - oidc(next).ServeHTTP(w, r) - } - - if options.OIDCIss != "" && !options.EnableBasicAuth { - oidc(next).ServeHTTP(w, r) - } - - // if options.OIDCIss == "" && options.EnableBasicAuth { - // basic(next).ServeHTTP(w, r) - // } - }) - } -} - -// newOIDCAuth returns a configured oidc middleware -func newOIDCAuth(options Options) func(http.Handler) http.Handler { - return OIDCAuth( - Logger(options.Logger), - OIDCProviderFunc(options.OIDCProviderFunc), - HTTPClient(options.HTTPClient), - OIDCIss(options.OIDCIss), - TokenCacheSize(options.UserinfoCacheSize), - TokenCacheTTL(options.UserinfoCacheTTL), - CredentialsByUserAgent(options.CredentialsByUserAgent), - AccessTokenVerifyMethod(options.AccessTokenVerifyMethod), - JWKSOptions(options.JWKS), - ) -} - -// // newBasicAuth returns a configured basic middleware -// func newBasicAuth(options Options) func(http.Handler) http.Handler { -// return BasicAuth( -// UserProvider(options.UserProvider), -// Logger(options.Logger), -// EnableBasicAuth(options.EnableBasicAuth), -// OIDCIss(options.OIDCIss), -// UserOIDCClaim(options.UserOIDCClaim), -// UserCS3Claim(options.UserCS3Claim), -// CredentialsByUserAgent(options.CredentialsByUserAgent), -// ) -// } diff --git a/services/proxy/pkg/middleware/basic_auth.go b/services/proxy/pkg/middleware/basic_auth.go index 4771e57f21..eaccd70425 100644 --- a/services/proxy/pkg/middleware/basic_auth.go +++ b/services/proxy/pkg/middleware/basic_auth.go @@ -15,22 +15,26 @@ type BasicAuthenticator struct { UserOIDCClaim string } -func (m BasicAuthenticator) Authenticate(req *http.Request) (*http.Request, bool) { - if isPublicPath(req.URL.Path) { +func (m BasicAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { + if isPublicPath(r.URL.Path) { // The authentication of public path requests is handled by another authenticator. // Since we can't guarantee the order of execution of the authenticators, we better // implement an early return here for paths we can't authenticate in this authenticator. return nil, false } - login, password, ok := req.BasicAuth() + login, password, ok := r.BasicAuth() if !ok { return nil, false } - user, _, err := m.UserProvider.Authenticate(req.Context(), login, password) + user, _, err := m.UserProvider.Authenticate(r.Context(), login, password) if err != nil { - // TODO add log line + m.Logger.Error(). + Err(err). + Str("authenticator", "basic"). + Str("path", r.URL.Path). + Msg("failed to authenticate request") return nil, false } @@ -48,5 +52,9 @@ func (m BasicAuthenticator) Authenticate(req *http.Request) (*http.Request, bool claims[m.UserOIDCClaim] = user.Id.OpaqueId } - return req.WithContext(oidc.NewContext(req.Context(), claims)), true + m.Logger.Debug(). + Str("authenticator", "basic"). + Str("path", r.URL.Path). + Msg("successfully authenticated request") + return r.WithContext(oidc.NewContext(r.Context(), claims)), true } diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 97a71962ff..5d740843d1 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -3,7 +3,6 @@ package middleware import ( "context" "encoding/json" - "errors" "io/ioutil" "net/http" "strings" @@ -17,9 +16,20 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/oidc" osync "github.com/owncloud/ocis/v2/ocis-pkg/sync" "github.com/owncloud/ocis/v2/services/proxy/pkg/config" + "github.com/pkg/errors" "golang.org/x/oauth2" ) +const ( + _headerAuthorization = "Authorization" + _bearerPrefix = "Bearer " +) + +var ( + // _unauthenticatePaths contains paths which don't need to be authenticated. + _unauthenticatePaths = [...]string{"/konnect/v1/userinfo", "/status.php"} +) + // OIDCProvider used to mock the oidc provider during tests type OIDCProvider interface { UserInfo(ctx context.Context, ts oauth2.TokenSource) (*gOidc.UserInfo, error) @@ -42,14 +52,13 @@ type OIDCAuthenticator struct { JWKS *keyfunc.JWKS } -func (m OIDCAuthenticator) getClaims(token string, req *http.Request) (claims map[string]interface{}, status int) { +func (m OIDCAuthenticator) getClaims(token string, req *http.Request) (map[string]interface{}, error) { + var claims map[string]interface{} hit := m.TokenCache.Load(token) if hit == nil { aClaims, err := m.verifyAccessToken(token) if err != nil { - m.Logger.Error().Err(err).Msg("Failed to verify access token") - status = http.StatusUnauthorized - return + return nil, errors.Wrap(err, "failed to verify access token") } oauth2Token := &oauth2.Token{ @@ -61,31 +70,25 @@ func (m OIDCAuthenticator) getClaims(token string, req *http.Request) (claims ma oauth2.StaticTokenSource(oauth2Token), ) if err != nil { - m.Logger.Error().Err(err).Msg("Failed to get userinfo") - status = http.StatusUnauthorized - return + return nil, errors.Wrap(err, "failed to get userinfo") } - if err := userInfo.Claims(&claims); err != nil { - m.Logger.Error().Err(err).Interface("userinfo", userInfo).Msg("failed to unmarshal userinfo claims") - status = http.StatusInternalServerError - return + return nil, errors.Wrap(err, "failed to unmarshal userinfo claims") } expiration := m.extractExpiration(aClaims) m.TokenCache.Store(token, claims, expiration) m.Logger.Debug().Interface("claims", claims).Interface("userInfo", userInfo).Time("expiration", expiration.UTC()).Msg("unmarshalled and cached userinfo") - return + return claims, nil } var ok bool if claims, ok = hit.V.(map[string]interface{}); !ok { - status = http.StatusInternalServerError - return + return nil, errors.New("failed to cast claims from the cache") } m.Logger.Debug().Interface("claims", claims).Msg("cache hit for userinfo") - return + return claims, nil } func (m OIDCAuthenticator) verifyAccessToken(token string) (jwt.RegisteredClaims, error) { @@ -139,21 +142,20 @@ func (m OIDCAuthenticator) extractExpiration(aClaims jwt.RegisteredClaims) time. } func (m OIDCAuthenticator) shouldServe(req *http.Request) bool { - header := req.Header.Get("Authorization") - if m.OIDCIss == "" { return false } // todo: looks dirty, check later // TODO: make a PR to coreos/go-oidc for exposing userinfo endpoint on provider, see https://github.com/coreos/go-oidc/issues/248 - for _, ignoringPath := range []string{"/konnect/v1/userinfo", "/status.php"} { + for _, ignoringPath := range _unauthenticatePaths { if req.URL.Path == ignoringPath { return false } } - return strings.HasPrefix(header, "Bearer ") + header := req.Header.Get(_headerAuthorization) + return strings.HasPrefix(header, _bearerPrefix) } type jwksJSON struct { @@ -234,14 +236,10 @@ func (m *OIDCAuthenticator) getProvider() OIDCProvider { func (m OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { // there is no bearer token on the request, if !m.shouldServe(r) { - // // oidc supported but token not present, add header and handover to the next middleware. - // userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, "bearer") - // next.ServeHTTP(w, r) return nil, false } if m.getProvider() == nil { - // w.WriteHeader(http.StatusInternalServerError) return nil, false } // Force init of jwks keyfunc if needed (contacts the .well-known and jwks endpoints on first call) @@ -249,13 +247,20 @@ func (m OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { return nil, false } - token := strings.TrimPrefix(r.Header.Get("Authorization"), "Bearer ") + token := strings.TrimPrefix(r.Header.Get(_headerAuthorization), _bearerPrefix) - claims, status := m.getClaims(token, r) - if status != 0 { - // w.WriteHeader(status) - // TODO log + claims, err := m.getClaims(token, r) + if err != nil { + m.Logger.Error(). + Err(err). + Str("authenticator", "oidc"). + Str("path", r.URL.Path). + Msg("failed to authenticate the request") return nil, false } + m.Logger.Debug(). + Str("authenticator", "oidc"). + Str("path", r.URL.Path). + Msg("successfully authenticated request") return r.WithContext(oidc.NewContext(r.Context(), claims)), true } diff --git a/services/proxy/pkg/middleware/public_share_auth.go b/services/proxy/pkg/middleware/public_share_auth.go index 6d6d0fa747..795063d5d0 100644 --- a/services/proxy/pkg/middleware/public_share_auth.go +++ b/services/proxy/pkg/middleware/public_share_auth.go @@ -13,6 +13,9 @@ const ( headerShareToken = "public-token" basicAuthPasswordPrefix = "password|" authenticationType = "publicshares" + + _paramSignature = "signature" + _paramExpiration = "expiration" ) type PublicShareAuthenticator struct { @@ -38,8 +41,8 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, } var sharePassword string - if signature := query.Get("signature"); signature != "" { - expiration := query.Get("expiration") + if signature := query.Get(_paramSignature); signature != "" { + expiration := query.Get(_paramExpiration) if expiration == "" { a.Logger.Warn().Str("signature", signature).Msg("cannot do signature auth without the expiration") return nil, false @@ -62,11 +65,20 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, }) if err != nil { - a.Logger.Debug().Err(err).Str("public_share_token", shareToken).Msg("could not authenticate public share") - // try another middleware + a.Logger.Error(). + Err(err). + Str("authenticator", "public_share"). + Str("public_share_token", shareToken). + Str("path", r.URL.Path). + Msg("failed to authenticate request") return nil, false } r.Header.Add(headerRevaAccessToken, authResp.Token) + + a.Logger.Debug(). + Str("authenticator", "public_share"). + Str("path", r.URL.Path). + Msg("successfully authenticated request") return r, false } diff --git a/services/proxy/pkg/middleware/signed_url_auth.go b/services/proxy/pkg/middleware/signed_url_auth.go index 094c279ad4..80a613aace 100644 --- a/services/proxy/pkg/middleware/signed_url_auth.go +++ b/services/proxy/pkg/middleware/signed_url_auth.go @@ -20,58 +20,36 @@ import ( "golang.org/x/crypto/pbkdf2" ) -// SignedURLAuth provides a middleware to check access secured by a signed URL. -func SignedURLAuth(optionSetters ...Option) func(next http.Handler) http.Handler { - options := newOptions(optionSetters...) +const ( + _paramOCSignature = "OC-Signature" + _paramOCCredential = "OC-Credential" + _paramOCDate = "OC-Date" + _paramOCExpires = "OC-Expires" + _paramOCVerb = "OC-Verb" +) - return func(next http.Handler) http.Handler { - return &SignedURLAuthenticator{ - next: next, - logger: options.Logger, - preSignedURLConfig: options.PreSignedURLConfig, - store: options.Store, - userProvider: options.UserProvider, - } +var ( + _requiredParams = [...]string{ + _paramOCSignature, + _paramOCCredential, + _paramOCDate, + _paramOCExpires, + _paramOCVerb, } -} +) type SignedURLAuthenticator struct { - next http.Handler - logger log.Logger - preSignedURLConfig config.PreSignedURL - userProvider backend.UserBackend - store storesvc.StoreService -} - -func (m SignedURLAuthenticator) ServeHTTP(w http.ResponseWriter, req *http.Request) { - if !m.shouldServe(req) { - m.next.ServeHTTP(w, req) - return - } - - user, _, err := m.userProvider.GetUserByClaims(req.Context(), "username", req.URL.Query().Get("OC-Credential"), true) - if err != nil { - m.logger.Error().Err(err).Msg("Could not get user by claim") - w.WriteHeader(http.StatusInternalServerError) - } - - ctx := revactx.ContextSetUser(req.Context(), user) - - req = req.WithContext(ctx) - - if err := m.validate(req); err != nil { - http.Error(w, "Invalid url signature", http.StatusUnauthorized) - return - } - - m.next.ServeHTTP(w, req) + Logger log.Logger + PreSignedURLConfig config.PreSignedURL + UserProvider backend.UserBackend + Store storesvc.StoreService } func (m SignedURLAuthenticator) shouldServe(req *http.Request) bool { - if !m.preSignedURLConfig.Enabled { + if !m.PreSignedURLConfig.Enabled { return false } - return req.URL.Query().Get("OC-Signature") != "" + return req.URL.Query().Get(_paramOCSignature) != "" } func (m SignedURLAuthenticator) validate(req *http.Request) (err error) { @@ -107,13 +85,7 @@ func (m SignedURLAuthenticator) allRequiredParametersArePresent(query url.Values // OC-Date - defined the date the url was signed (ISO 8601 UTC) REQUIRED // OC-Expires - defines the expiry interval in seconds (between 1 and 604800 = 7 days) REQUIRED // TODO OC-Verb - defines for which http verb the request is valid - defaults to GET OPTIONAL - for _, p := range []string{ - "OC-Signature", - "OC-Credential", - "OC-Date", - "OC-Expires", - "OC-Verb", - } { + for _, p := range _requiredParams { if query.Get(p) == "" { return false, fmt.Errorf("required %s parameter not found", p) } @@ -124,7 +96,7 @@ func (m SignedURLAuthenticator) allRequiredParametersArePresent(query url.Values func (m SignedURLAuthenticator) requestMethodMatches(meth string, query url.Values) (ok bool, err error) { // check if given url query parameter OC-Verb matches given request method - if !strings.EqualFold(meth, query.Get("OC-Verb")) { + if !strings.EqualFold(meth, query.Get(_paramOCVerb)) { return false, errors.New("required OC-Verb parameter did not match request method") } @@ -134,7 +106,7 @@ func (m SignedURLAuthenticator) requestMethodMatches(meth string, query url.Valu func (m SignedURLAuthenticator) requestMethodIsAllowed(meth string) (ok bool, err error) { // check if given request method is allowed methodIsAllowed := false - for _, am := range m.preSignedURLConfig.AllowedHTTPMethods { + for _, am := range m.PreSignedURLConfig.AllowedHTTPMethods { if strings.EqualFold(meth, am) { methodIsAllowed = true break @@ -147,14 +119,15 @@ func (m SignedURLAuthenticator) requestMethodIsAllowed(meth string) (ok bool, er return true, nil } + func (m SignedURLAuthenticator) urlIsExpired(query url.Values, now func() time.Time) (expired bool, err error) { // check if url is expired by checking if given date (OC-Date) + expires in seconds (OC-Expires) is after now - validFrom, err := time.Parse(time.RFC3339, query.Get("OC-Date")) + validFrom, err := time.Parse(time.RFC3339, query.Get(_paramOCDate)) if err != nil { return true, err } - requestExpiry, err := time.ParseDuration(query.Get("OC-Expires") + "s") + requestExpiry, err := time.ParseDuration(query.Get(_paramOCExpires) + "s") if err != nil { return true, err } @@ -168,16 +141,16 @@ func (m SignedURLAuthenticator) signatureIsValid(req *http.Request) (ok bool, er u := revactx.ContextMustGetUser(req.Context()) signingKey, err := m.getSigningKey(req.Context(), u.Id.OpaqueId) if err != nil { - m.logger.Error().Err(err).Msg("could not retrieve signing key") + m.Logger.Error().Err(err).Msg("could not retrieve signing key") return false, err } if len(signingKey) == 0 { - m.logger.Error().Err(err).Msg("signing key empty") + m.Logger.Error().Err(err).Msg("signing key empty") return false, err } q := req.URL.Query() - signature := q.Get("OC-Signature") - q.Del("OC-Signature") + signature := q.Get(_paramOCSignature) + q.Del(_paramOCSignature) req.URL.RawQuery = q.Encode() url := req.URL.String() if !req.URL.IsAbs() { @@ -198,7 +171,7 @@ func (m SignedURLAuthenticator) createSignature(url string, signingKey []byte) s } func (m SignedURLAuthenticator) getSigningKey(ctx context.Context, ocisID string) ([]byte, error) { - res, err := m.store.Read(ctx, &storesvc.ReadRequest{ + res, err := m.Store.Read(ctx, &storesvc.ReadRequest{ Options: &storemsg.ReadOptions{ Database: "proxy", Table: "signing-keys", @@ -206,7 +179,7 @@ func (m SignedURLAuthenticator) getSigningKey(ctx context.Context, ocisID string Key: ocisID, }) if err != nil || len(res.Records) < 1 { - return []byte{}, err + return nil, err } return res.Records[0].Value, nil @@ -217,9 +190,13 @@ func (m SignedURLAuthenticator) Authenticate(r *http.Request) (*http.Request, bo return nil, false } - user, _, err := m.userProvider.GetUserByClaims(r.Context(), "username", r.URL.Query().Get("OC-Credential"), true) + user, _, err := m.UserProvider.GetUserByClaims(r.Context(), "username", r.URL.Query().Get(_paramOCCredential), true) if err != nil { - m.logger.Error().Err(err).Msg("Could not get user by claim") + m.Logger.Error(). + Err(err). + Str("authenticator", "signed_url"). + Str("path", r.URL.Path). + Msg("Could not get user by claim") return nil, false } @@ -228,9 +205,17 @@ func (m SignedURLAuthenticator) Authenticate(r *http.Request) (*http.Request, bo r = r.WithContext(ctx) if err := m.validate(r); err != nil { - // http.Error(w, "Invalid url signature", http.StatusUnauthorized) + m.Logger.Error(). + Err(err). + Str("authenticator", "signed_url"). + Str("path", r.URL.Path). + Msg("Could not get user by claim") return nil, false } + m.Logger.Debug(). + Str("authenticator", "signed_url"). + Str("path", r.URL.Path). + Msg("successfully authenticated request") return r, true } From ddfc01bff9242db5042f751ee91ef41d8cbfb5ad Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 8 Aug 2022 16:59:05 +0200 Subject: [PATCH 03/20] refactor unprotected paths check --- .../proxy/pkg/middleware/authentication.go | 51 ++++++++++++++----- services/proxy/pkg/middleware/oidc_auth.go | 13 ----- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 6c961657d1..887e116be2 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -23,7 +23,31 @@ var ( "/remote.php/dav/public-files/", "/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected", "/ocs/v1.php/cloud/capabilities", - "/data", + } + // _unprotectedPaths contains paths which don't need to be authenticated. + _unprotectedPaths = map[string]struct{}{ + "/": {}, + "/login": {}, + "/app/list": {}, + "/config.json": {}, + "/oidc-callback.html": {}, + "/oidc-callback": {}, + "/settings.js": {}, + "/data": {}, + "/konnect/v1/userinfo": {}, + "/status.php": {}, + } + // _unprotectedPathPrefixes contains paths which don't need to be authenticated. + _unprotectedPathPrefixes = [...]string{ + "/files", + "/settings", + "/user-management", + "/.well-known", + "/js", + "/icons", + "/themes", + "/signin", + "/konnect", } ) @@ -46,18 +70,7 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if isOIDCTokenAuth(r) || - r.URL.Path == "/" || - strings.HasPrefix(r.URL.Path, "/.well-known") || - r.URL.Path == "/login" || - strings.HasPrefix(r.URL.Path, "/js") || - strings.HasPrefix(r.URL.Path, "/themes") || - strings.HasPrefix(r.URL.Path, "/signin") || - strings.HasPrefix(r.URL.Path, "/konnect") || - r.URL.Path == "/config.json" || - r.URL.Path == "/oidc-callback.html" || - r.URL.Path == "/oidc-callback" || - r.URL.Path == "/settings.js" { + if isOIDCTokenAuth(r) || isUnprotectedPath(r) { // The authentication for this request is handled by the IdP. next.ServeHTTP(w, r) return @@ -96,6 +109,18 @@ func isOIDCTokenAuth(req *http.Request) bool { return req.URL.Path == "/konnect/v1/token" } +func isUnprotectedPath(r *http.Request) bool { + if _, ok := _unprotectedPaths[r.URL.Path]; ok { + return true + } + for _, p := range _unprotectedPathPrefixes { + if strings.HasPrefix(r.URL.Path, p) { + return true + } + } + return false +} + func isPublicPath(p string) bool { for _, pp := range _publicPaths { if strings.HasPrefix(p, pp) { diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 5d740843d1..a6c1b5519d 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -25,11 +25,6 @@ const ( _bearerPrefix = "Bearer " ) -var ( - // _unauthenticatePaths contains paths which don't need to be authenticated. - _unauthenticatePaths = [...]string{"/konnect/v1/userinfo", "/status.php"} -) - // OIDCProvider used to mock the oidc provider during tests type OIDCProvider interface { UserInfo(ctx context.Context, ts oauth2.TokenSource) (*gOidc.UserInfo, error) @@ -146,14 +141,6 @@ func (m OIDCAuthenticator) shouldServe(req *http.Request) bool { return false } - // todo: looks dirty, check later - // TODO: make a PR to coreos/go-oidc for exposing userinfo endpoint on provider, see https://github.com/coreos/go-oidc/issues/248 - for _, ignoringPath := range _unauthenticatePaths { - if req.URL.Path == ignoringPath { - return false - } - } - header := req.Header.Get(_headerAuthorization) return strings.HasPrefix(header, _bearerPrefix) } From ef020920e8155d442b5c538ab10a296d4c08da74 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 8 Aug 2022 17:23:47 +0200 Subject: [PATCH 04/20] update authentication tests --- .../pkg/middleware/authentication_test.go | 19 ++++++++++ .../proxy/pkg/middleware/basic_auth_test.go | 38 ------------------- .../pkg/middleware/signed_url_auth_test.go | 4 +- 3 files changed, 21 insertions(+), 40 deletions(-) create mode 100644 services/proxy/pkg/middleware/authentication_test.go delete mode 100644 services/proxy/pkg/middleware/basic_auth_test.go diff --git a/services/proxy/pkg/middleware/authentication_test.go b/services/proxy/pkg/middleware/authentication_test.go new file mode 100644 index 0000000000..1188fefa34 --- /dev/null +++ b/services/proxy/pkg/middleware/authentication_test.go @@ -0,0 +1,19 @@ +package middleware + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("authentication helpers", func() { + DescribeTable("isPublicPath should recognize public paths", + func(input string, expected bool) { + isPublic := isPublicPath(input) + Expect(isPublic).To(Equal(expected)) + }, + Entry("public files path", "/remote.php/dav/public-files/", true), + Entry("public files path without remote.php", "/remote.php/dav/public-files/", true), + Entry("token info path", "/remote.php/ocs/apps/files_sharing/api/v1/tokeninfo/unprotected", true), + Entry("capabilities", "/ocs/v1.php/cloud/capabilities", true), + ) +}) diff --git a/services/proxy/pkg/middleware/basic_auth_test.go b/services/proxy/pkg/middleware/basic_auth_test.go deleted file mode 100644 index 77279d66b4..0000000000 --- a/services/proxy/pkg/middleware/basic_auth_test.go +++ /dev/null @@ -1,38 +0,0 @@ -package middleware - -import ( - "net/http/httptest" - "testing" -) - -/**/ - -func TestBasicAuth__isPublicLink(t *testing.T) { - tests := []struct { - url string - username string - expected bool - }{ - {url: "/remote.php/dav/public-files/", username: "", expected: false}, - {url: "/remote.php/dav/public-files/", username: "abc", expected: false}, - {url: "/remote.php/dav/public-files/", username: "private", expected: false}, - {url: "/remote.php/dav/public-files/", username: "public", expected: true}, - {url: "/ocs/v1.php/cloud/capabilities", username: "", expected: false}, - {url: "/ocs/v1.php/cloud/capabilities", username: "abc", expected: false}, - {url: "/ocs/v1.php/cloud/capabilities", username: "private", expected: false}, - {url: "/ocs/v1.php/cloud/capabilities", username: "public", expected: true}, - {url: "/ocs/v1.php/cloud/users/admin", username: "public", expected: false}, - } - for _, tt := range tests { - req := httptest.NewRequest("", tt.url, nil) - - if tt.username != "" { - req.SetBasicAuth(tt.username, "") - } - - result := isPublicPath(req.URL.Path) - if result != tt.expected { - t.Errorf("with %s expected %t got %t", tt.url, tt.expected, result) - } - } -} diff --git a/services/proxy/pkg/middleware/signed_url_auth_test.go b/services/proxy/pkg/middleware/signed_url_auth_test.go index 01311b731e..35f84e6655 100644 --- a/services/proxy/pkg/middleware/signed_url_auth_test.go +++ b/services/proxy/pkg/middleware/signed_url_auth_test.go @@ -20,7 +20,7 @@ func TestSignedURLAuth_shouldServe(t *testing.T) { } for _, tt := range tests { - pua.preSignedURLConfig.Enabled = tt.enabled + pua.PreSignedURLConfig.Enabled = tt.enabled r := httptest.NewRequest("", tt.url, nil) result := pua.shouldServe(r) @@ -89,7 +89,7 @@ func TestSignedURLAuth_requestMethodIsAllowed(t *testing.T) { } for _, tt := range tests { - pua.preSignedURLConfig.AllowedHTTPMethods = tt.allowed + pua.PreSignedURLConfig.AllowedHTTPMethods = tt.allowed ok, _ := pua.requestMethodIsAllowed(tt.method) if ok != tt.expected { From 7bc1305cb4a5f0de769d0c357328174e071a144a Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 9 Aug 2022 15:32:35 +0200 Subject: [PATCH 05/20] add tests for the basic auth middleware --- .../proxy/pkg/middleware/basic_auth_test.go | 68 +++++++++++++++++++ .../pkg/middleware/middleware_suite_test.go | 13 ++++ .../proxy/pkg/middleware/oidc_auth_test.go | 36 ++++------ 3 files changed, 95 insertions(+), 22 deletions(-) create mode 100644 services/proxy/pkg/middleware/basic_auth_test.go create mode 100644 services/proxy/pkg/middleware/middleware_suite_test.go diff --git a/services/proxy/pkg/middleware/basic_auth_test.go b/services/proxy/pkg/middleware/basic_auth_test.go new file mode 100644 index 0000000000..fcc2e6f3ce --- /dev/null +++ b/services/proxy/pkg/middleware/basic_auth_test.go @@ -0,0 +1,68 @@ +package middleware + +import ( + "context" + "net/http" + "net/http/httptest" + + userv1beta1 "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + . "github.com/onsi/ginkgo/v2" + + . "github.com/onsi/gomega" + "github.com/owncloud/ocis/v2/ocis-pkg/log" + "github.com/owncloud/ocis/v2/ocis-pkg/oidc" + "github.com/owncloud/ocis/v2/services/proxy/pkg/user/backend" + "github.com/owncloud/ocis/v2/services/proxy/pkg/user/backend/test" +) + +var _ = Describe("Authenticating requests", Label("BasicAuthenticator"), func() { + var authenticator Authenticator + BeforeEach(func() { + authenticator = BasicAuthenticator{ + Logger: log.NewLogger(), + UserProvider: &test.UserBackendMock{ + AuthenticateFunc: func(ctx context.Context, username, password string) (*userv1beta1.User, string, error) { + var user *userv1beta1.User + if username == "testuser" && password == "testpassword" { + user = &userv1beta1.User{ + Id: &userv1beta1.UserId{ + Idp: "IdpId", + OpaqueId: "OpaqueId", + }, + Username: "testuser", + Mail: "testuser@example.com", + } + return user, "", nil + } + return nil, "", backend.ErrAccountNotFound + }, + }, + } + }) + + When("the request contains correct data", func() { + It("should successfully authenticate", func() { + req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) + req.SetBasicAuth("testuser", "testpassword") + + req2, valid := authenticator.Authenticate(req) + + Expect(valid).To(Equal(true)) + Expect(req2).ToNot(BeNil()) + }) + It("adds claims to the request context", func() { + req := httptest.NewRequest(http.MethodGet, "http://example.com/example/path", http.NoBody) + req.SetBasicAuth("testuser", "testpassword") + + req2, valid := authenticator.Authenticate(req) + Expect(valid).To(Equal(true)) + + claims := oidc.FromContext(req2.Context()) + Expect(claims).ToNot(BeNil()) + Expect(claims[oidc.Iss]).To(Equal("IdpId")) + Expect(claims[oidc.PreferredUsername]).To(Equal("testuser")) + Expect(claims[oidc.Email]).To(Equal("testuser@example.com")) + Expect(claims[oidc.OwncloudUUID]).To(Equal("OpaqueId")) + }) + }) +}) diff --git a/services/proxy/pkg/middleware/middleware_suite_test.go b/services/proxy/pkg/middleware/middleware_suite_test.go new file mode 100644 index 0000000000..09e4e11360 --- /dev/null +++ b/services/proxy/pkg/middleware/middleware_suite_test.go @@ -0,0 +1,13 @@ +package middleware_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestMiddleware(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Middleware Suite") +} diff --git a/services/proxy/pkg/middleware/oidc_auth_test.go b/services/proxy/pkg/middleware/oidc_auth_test.go index 6b7e97bceb..93612e3c48 100644 --- a/services/proxy/pkg/middleware/oidc_auth_test.go +++ b/services/proxy/pkg/middleware/oidc_auth_test.go @@ -5,35 +5,27 @@ import ( "fmt" "net/http" "net/http/httptest" - "testing" "github.com/coreos/go-oidc/v3/oidc" - "github.com/owncloud/ocis/v2/ocis-pkg/log" - "github.com/owncloud/ocis/v2/services/proxy/pkg/config" + . "github.com/onsi/ginkgo/v2" "golang.org/x/oauth2" ) -func TestOIDCAuthMiddleware(t *testing.T) { - next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) +var _ = Describe("Test OIDC Authenticator", func() { + It("should authenticate requests", func() { + m := OIDCAuthenticator{ + ProviderFunc: func() (OIDCProvider, error) { return mockOP(false), nil }, + } - m := OIDCAuth( - Logger(log.NewLogger()), - OIDCProviderFunc(func() (OIDCProvider, error) { - return mockOP(false), nil - }), - OIDCIss("https://localhost:9200"), - AccessTokenVerifyMethod(config.AccessTokenVerificationNone), - )(next) + r := httptest.NewRequest(http.MethodGet, "https://idp.example.com", nil) + r.Header.Set("Authorization", "Bearer sometoken") - r := httptest.NewRequest(http.MethodGet, "https://idp.example.com", nil) - r.Header.Set("Authorization", "Bearer sometoken") - w := httptest.NewRecorder() - m.ServeHTTP(w, r) - - if w.Code != http.StatusInternalServerError { - t.Errorf("expected an internal server error") - } -} + _, ok := m.Authenticate(r) + if ok { + Fail("expected an internal server error") + } + }) +}) type mockOIDCProvider struct { UserInfoFunc func(ctx context.Context, ts oauth2.TokenSource) (*oidc.UserInfo, error) From 17a4e65f082601baaf1560722a24f55a42ff7918 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 9 Aug 2022 16:50:05 +0200 Subject: [PATCH 06/20] add tests for the public share auth middleware --- .../proxy/pkg/middleware/public_share_auth.go | 6 +- .../pkg/middleware/public_share_auth_test.go | 78 +++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 services/proxy/pkg/middleware/public_share_auth_test.go diff --git a/services/proxy/pkg/middleware/public_share_auth.go b/services/proxy/pkg/middleware/public_share_auth.go index 795063d5d0..b32f9ed147 100644 --- a/services/proxy/pkg/middleware/public_share_auth.go +++ b/services/proxy/pkg/middleware/public_share_auth.go @@ -9,7 +9,7 @@ import ( ) const ( - headerRevaAccessToken = "x-access-token" + _headerRevaAccessToken = "x-access-token" headerShareToken = "public-token" basicAuthPasswordPrefix = "password|" authenticationType = "publicshares" @@ -74,11 +74,11 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, return nil, false } - r.Header.Add(headerRevaAccessToken, authResp.Token) + r.Header.Add(_headerRevaAccessToken, authResp.Token) a.Logger.Debug(). Str("authenticator", "public_share"). Str("path", r.URL.Path). Msg("successfully authenticated request") - return r, false + return r, true } diff --git a/services/proxy/pkg/middleware/public_share_auth_test.go b/services/proxy/pkg/middleware/public_share_auth_test.go new file mode 100644 index 0000000000..98637988bd --- /dev/null +++ b/services/proxy/pkg/middleware/public_share_auth_test.go @@ -0,0 +1,78 @@ +package middleware + +import ( + "context" + "net/http" + "net/http/httptest" + + gatewayv1beta1 "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" + rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/owncloud/ocis/v2/ocis-pkg/log" + "google.golang.org/grpc" +) + +var _ = Describe("Authenticating requests", Label("PublicShareAuthenticator"), func() { + var authenticator Authenticator + BeforeEach(func() { + authenticator = PublicShareAuthenticator{ + Logger: log.NewLogger(), + RevaGatewayClient: mockGatewayClient{ + AuthenticateFunc: func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) { + if authType != "publicshares" { + return "", rpcv1beta1.Code_CODE_NOT_FOUND + } + + if clientID == "sharetoken" && (clientSecret == "password|examples3cr3t" || clientSecret == "signature|examplesignature|exampleexpiration") { + return "exampletoken", rpcv1beta1.Code_CODE_OK + } + + return "", rpcv1beta1.Code_CODE_NOT_FOUND + }, + }, + } + }) + When("the request contains correct data", func() { + Context("using password authentication", func() { + It("should successfully authenticate", func() { + req := httptest.NewRequest(http.MethodGet, "http://example.com/dav/public-files/?public-token=sharetoken", http.NoBody) + req.SetBasicAuth("public", "examples3cr3t") + + req2, valid := authenticator.Authenticate(req) + + Expect(valid).To(Equal(true)) + Expect(req2).ToNot(BeNil()) + + h := req2.Header + Expect(h.Get(_headerRevaAccessToken)).To(Equal("exampletoken")) + }) + }) + Context("using signature authentication", func() { + It("should successfully authenticate", func() { + req := httptest.NewRequest(http.MethodGet, "http://example.com/dav/public-files/?public-token=sharetoken&signature=examplesignature&expiration=exampleexpiration", http.NoBody) + + req2, valid := authenticator.Authenticate(req) + + Expect(valid).To(Equal(true)) + Expect(req2).ToNot(BeNil()) + + h := req2.Header + Expect(h.Get(_headerRevaAccessToken)).To(Equal("exampletoken")) + }) + }) + }) +}) + +type mockGatewayClient struct { + gatewayv1beta1.GatewayAPIClient + AuthenticateFunc func(authType, clientID, clientSecret string) (string, rpcv1beta1.Code) +} + +func (c mockGatewayClient) Authenticate(ctx context.Context, in *gatewayv1beta1.AuthenticateRequest, opts ...grpc.CallOption) (*gatewayv1beta1.AuthenticateResponse, error) { + token, code := c.AuthenticateFunc(in.GetType(), in.GetClientId(), in.GetClientSecret()) + return &gatewayv1beta1.AuthenticateResponse{ + Status: &rpcv1beta1.Status{Code: code}, + Token: token, + }, nil +} From 9347657370fc3e316949974e8394a998a9e249fe Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 9 Aug 2022 17:17:37 +0200 Subject: [PATCH 07/20] remove the oidc tests since they aren't testing anything at the moment I admit it would be better to implement the tests but I tried and it is a bit tricky since we can't mock everything we would need to mock. I'll wan to get these changes in first and later in the near future we should revisit the auth middleware architecture and refactor it a bit more to be more testable and future proof. --- .../proxy/pkg/middleware/oidc_auth_test.go | 61 ------------------- 1 file changed, 61 deletions(-) delete mode 100644 services/proxy/pkg/middleware/oidc_auth_test.go diff --git a/services/proxy/pkg/middleware/oidc_auth_test.go b/services/proxy/pkg/middleware/oidc_auth_test.go deleted file mode 100644 index 93612e3c48..0000000000 --- a/services/proxy/pkg/middleware/oidc_auth_test.go +++ /dev/null @@ -1,61 +0,0 @@ -package middleware - -import ( - "context" - "fmt" - "net/http" - "net/http/httptest" - - "github.com/coreos/go-oidc/v3/oidc" - . "github.com/onsi/ginkgo/v2" - "golang.org/x/oauth2" -) - -var _ = Describe("Test OIDC Authenticator", func() { - It("should authenticate requests", func() { - m := OIDCAuthenticator{ - ProviderFunc: func() (OIDCProvider, error) { return mockOP(false), nil }, - } - - r := httptest.NewRequest(http.MethodGet, "https://idp.example.com", nil) - r.Header.Set("Authorization", "Bearer sometoken") - - _, ok := m.Authenticate(r) - if ok { - Fail("expected an internal server error") - } - }) -}) - -type mockOIDCProvider struct { - UserInfoFunc func(ctx context.Context, ts oauth2.TokenSource) (*oidc.UserInfo, error) -} - -// UserInfo will panic if the function has been called, but not mocked -func (m mockOIDCProvider) UserInfo(ctx context.Context, ts oauth2.TokenSource) (*oidc.UserInfo, error) { - if m.UserInfoFunc != nil { - return m.UserInfoFunc(ctx, ts) - } - - panic("UserInfo was called in test but not mocked") -} - -func mockOP(retErr bool) OIDCProvider { - if retErr { - return &mockOIDCProvider{ - UserInfoFunc: func(ctx context.Context, ts oauth2.TokenSource) (*oidc.UserInfo, error) { - return nil, fmt.Errorf("error returned by mockOIDCProvider UserInfo") - }, - } - - } - return &mockOIDCProvider{ - UserInfoFunc: func(ctx context.Context, ts oauth2.TokenSource) (*oidc.UserInfo, error) { - ui := &oidc.UserInfo{ - // claims: private ... - } - return ui, nil - }, - } - -} From 06ffd9cf8a66f3c747a362c90f0db1911da246a2 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Wed, 10 Aug 2022 13:39:15 +0200 Subject: [PATCH 08/20] some more cleaning up --- services/proxy/pkg/command/server.go | 21 ++++++-------- .../proxy/pkg/middleware/authentication.go | 2 ++ services/proxy/pkg/middleware/oidc_auth.go | 29 +++++++++++++++---- 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 7377d95829..e54538f716 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -16,7 +16,6 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/log" pkgmiddleware "github.com/owncloud/ocis/v2/ocis-pkg/middleware" "github.com/owncloud/ocis/v2/ocis-pkg/service/grpc" - "github.com/owncloud/ocis/v2/ocis-pkg/sync" "github.com/owncloud/ocis/v2/ocis-pkg/version" settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" storesvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/store/v0" @@ -175,14 +174,12 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) UserProvider: userProvider, }) } - tokenCache := sync.NewCache(cfg.OIDC.UserinfoCache.Size) - authenticators = append(authenticators, middleware.OIDCAuthenticator{ - Logger: logger, - TokenCache: &tokenCache, - TokenCacheTTL: time.Duration(cfg.OIDC.UserinfoCache.TTL), - HTTPClient: oidcHTTPClient, - OIDCIss: cfg.OIDC.Issuer, - ProviderFunc: func() (middleware.OIDCProvider, error) { + authenticators = append(authenticators, middleware.NewOIDCAuthenticator( + logger, + cfg.OIDC.UserinfoCache.TTL, + oidcHTTPClient, + cfg.OIDC.Issuer, + func() (middleware.OIDCProvider, error) { // Initialize a provider by specifying the issuer URL. // it will fetch the keys from the issuer using the .well-known // endpoint @@ -191,9 +188,9 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) cfg.OIDC.Issuer, ) }, - JWKSOptions: cfg.OIDC.JWKS, - AccessTokenVerifyMethod: cfg.OIDC.AccessTokenVerifyMethod, - }) + cfg.OIDC.JWKS, + cfg.OIDC.AccessTokenVerifyMethod, + )) authenticators = append(authenticators, middleware.PublicShareAuthenticator{ Logger: logger, RevaGatewayClient: revaClient, diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 887e116be2..53dfd41435 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -36,6 +36,7 @@ var ( "/data": {}, "/konnect/v1/userinfo": {}, "/status.php": {}, + "/favicon.ico": {}, } // _unprotectedPathPrefixes contains paths which don't need to be authenticated. _unprotectedPathPrefixes = [...]string{ @@ -44,6 +45,7 @@ var ( "/user-management", "/.well-known", "/js", + "/css", "/icons", "/themes", "/signin", diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index a6c1b5519d..a59a3b2457 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -30,11 +30,28 @@ type OIDCProvider interface { UserInfo(ctx context.Context, ts oauth2.TokenSource) (*gOidc.UserInfo, error) } +func NewOIDCAuthenticator(logger log.Logger, tokenCacheTTL int, oidcHTTPClient *http.Client, oidcIss string, providerFunc func() (OIDCProvider, error), + jwksOptions config.JWKS, accessTokenVerifyMethod string) OIDCAuthenticator { + tokenCache := osync.NewCache(tokenCacheTTL) + return OIDCAuthenticator{ + Logger: logger, + tokenCache: &tokenCache, + TokenCacheTTL: time.Duration(tokenCacheTTL), + HTTPClient: oidcHTTPClient, + OIDCIss: oidcIss, + ProviderFunc: providerFunc, + JWKSOptions: jwksOptions, + AccessTokenVerifyMethod: accessTokenVerifyMethod, + providerLock: &sync.Mutex{}, + jwksLock: &sync.Mutex{}, + } +} + type OIDCAuthenticator struct { Logger log.Logger HTTPClient *http.Client OIDCIss string - TokenCache *osync.Cache + tokenCache *osync.Cache TokenCacheTTL time.Duration ProviderFunc func() (OIDCProvider, error) AccessTokenVerifyMethod string @@ -49,7 +66,7 @@ type OIDCAuthenticator struct { func (m OIDCAuthenticator) getClaims(token string, req *http.Request) (map[string]interface{}, error) { var claims map[string]interface{} - hit := m.TokenCache.Load(token) + hit := m.tokenCache.Load(token) if hit == nil { aClaims, err := m.verifyAccessToken(token) if err != nil { @@ -72,7 +89,7 @@ func (m OIDCAuthenticator) getClaims(token string, req *http.Request) (map[strin } expiration := m.extractExpiration(aClaims) - m.TokenCache.Store(token, claims, expiration) + m.tokenCache.Store(token, claims, expiration) m.Logger.Debug().Interface("claims", claims).Interface("userInfo", userInfo).Time("expiration", expiration.UTC()).Msg("unmarshalled and cached userinfo") return claims, nil @@ -149,7 +166,7 @@ type jwksJSON struct { JWKSURL string `json:"jwks_uri"` } -func (m *OIDCAuthenticator) getKeyfunc() *keyfunc.JWKS { +func (m OIDCAuthenticator) getKeyfunc() *keyfunc.JWKS { m.jwksLock.Lock() defer m.jwksLock.Unlock() if m.JWKS == nil { @@ -200,7 +217,7 @@ func (m *OIDCAuthenticator) getKeyfunc() *keyfunc.JWKS { return m.JWKS } -func (m *OIDCAuthenticator) getProvider() OIDCProvider { +func (m OIDCAuthenticator) getProvider() OIDCProvider { m.providerLock.Lock() defer m.providerLock.Unlock() if m.provider == nil { @@ -229,11 +246,11 @@ func (m OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { if m.getProvider() == nil { return nil, false } + // Force init of jwks keyfunc if needed (contacts the .well-known and jwks endpoints on first call) if m.AccessTokenVerifyMethod == config.AccessTokenVerificationJWT && m.getKeyfunc() == nil { return nil, false } - token := strings.TrimPrefix(r.Header.Get(_headerAuthorization), _bearerPrefix) claims, err := m.getClaims(token, r) From d271ae2451199ba3ef7f2f252c8a13b41b824b9b Mon Sep 17 00:00:00 2001 From: David Christofas Date: Wed, 10 Aug 2022 16:29:41 +0200 Subject: [PATCH 09/20] fix some authentication cases --- services/proxy/pkg/middleware/authentication.go | 2 ++ services/proxy/pkg/middleware/public_share_auth.go | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 53dfd41435..f5f4d91150 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -41,6 +41,8 @@ var ( // _unprotectedPathPrefixes contains paths which don't need to be authenticated. _unprotectedPathPrefixes = [...]string{ "/files", + "/data", + "/s/", "/settings", "/user-management", "/.well-known", diff --git a/services/proxy/pkg/middleware/public_share_auth.go b/services/proxy/pkg/middleware/public_share_auth.go index b32f9ed147..dabaa8a38c 100644 --- a/services/proxy/pkg/middleware/public_share_auth.go +++ b/services/proxy/pkg/middleware/public_share_auth.go @@ -34,10 +34,10 @@ func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, shareToken = query.Get(headerShareToken) } - // Currently we only want to authenticate app open request coming from public shares. if shareToken == "" { - // Don't authenticate - return nil, false + // If the share token is not set then we don't need to inject the user to + // the request context so we can just continue with the request. + return r, true } var sharePassword string From 32f68f91ffdc14ade38373b42ef4880ee185fc5e Mon Sep 17 00:00:00 2001 From: David Christofas Date: Wed, 10 Aug 2022 23:59:33 +0200 Subject: [PATCH 10/20] add missing www-authentication header on failed authentication --- services/proxy/pkg/middleware/authentication.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index f5f4d91150..c90522403f 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -87,6 +87,7 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle } } if !isPublicPath(r.URL.Path) { + writeSupportedAuthenticateHeader(w, r) for _, s := range SupportedAuthStrategies { userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, s) } @@ -178,6 +179,7 @@ func userAgentAuthenticateLockIn(w http.ResponseWriter, r *http.Request, locks m for _, r := range ProxyWwwAuthenticate { evalRequestURI(u, r) } + fmt.Println(w.Header()) } func evalRequestURI(l userAgentLocker, r regexp.Regexp) { From 036c4664259634243a1ffc9880523a7d64eeb161 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Thu, 11 Aug 2022 10:41:18 +0200 Subject: [PATCH 11/20] add missing unprotected paths --- services/proxy/pkg/middleware/authentication.go | 3 ++- vendor-bin/behat/composer.json | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index c90522403f..728a85a0a4 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -37,6 +37,8 @@ var ( "/konnect/v1/userinfo": {}, "/status.php": {}, "/favicon.ico": {}, + "/ocs/v1.php/config": {}, + "/ocs/v2.php/config": {}, } // _unprotectedPathPrefixes contains paths which don't need to be authenticated. _unprotectedPathPrefixes = [...]string{ @@ -179,7 +181,6 @@ func userAgentAuthenticateLockIn(w http.ResponseWriter, r *http.Request, locks m for _, r := range ProxyWwwAuthenticate { evalRequestURI(u, r) } - fmt.Println(w.Header()) } func evalRequestURI(l userAgentLocker, r regexp.Regexp) { diff --git a/vendor-bin/behat/composer.json b/vendor-bin/behat/composer.json index 00964ecb4b..999f00df86 100644 --- a/vendor-bin/behat/composer.json +++ b/vendor-bin/behat/composer.json @@ -2,7 +2,10 @@ "config" : { "platform": { "php": "7.4" - } + }, + "allow-plugins": { + "composer/package-versions-deprecated": true + } }, "require": { "behat/behat": "^3.9", From 5d45f0e856201f6cb3afbe6d8a6477b05b10f523 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Thu, 11 Aug 2022 12:28:02 +0200 Subject: [PATCH 12/20] fix logic of when to add the www-authenticate headers --- .../proxy/pkg/middleware/authentication.go | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 728a85a0a4..07d5568f7f 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -89,10 +89,31 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle } } if !isPublicPath(r.URL.Path) { - writeSupportedAuthenticateHeader(w, r) - for _, s := range SupportedAuthStrategies { - userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, s) + // Failed basic authentication attempts receive the Www-Authenticate header in the response + var touch bool + for k, v := range options.CredentialsByUserAgent { + if strings.Contains(k, r.UserAgent()) { + removeSuperfluousAuthenticate(w) + w.Header().Add("Www-Authenticate", fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), r.Host)) + touch = true + break + } } + + // if the request is not bound to any user agent, write all available challenges + if !touch && + // This is a temporary hack... Before the authentication middleware rewrite all + // unauthenticated requests were still handled. The reva http services then did add + // the supported authentication headers to the response. Since we are not allowing the + // requests to continue so far we have to do it here. But we shouldn't do it for the graph service. + // That's the reason for this hard check here. + !strings.HasPrefix(r.URL.Path, "/graph") { + writeSupportedAuthenticateHeader(w, r) + } + } + + for _, s := range SupportedAuthStrategies { + userAgentAuthenticateLockIn(w, r, options.CredentialsByUserAgent, s) } w.WriteHeader(http.StatusUnauthorized) // if the request is a PROPFIND return a WebDAV error code. From 3ebfcbff1e3793bf806c4b11e6cbd9ffd4d0ebb4 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 15 Aug 2022 11:38:04 +0200 Subject: [PATCH 13/20] add missing unprotected path --- services/proxy/pkg/middleware/authentication.go | 1 + 1 file changed, 1 insertion(+) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 07d5568f7f..2bdc5fe2cb 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -30,6 +30,7 @@ var ( "/login": {}, "/app/list": {}, "/config.json": {}, + "/manifest.json": {}, "/oidc-callback.html": {}, "/oidc-callback": {}, "/settings.js": {}, From 864438b2de2744a57a2145cce644b9b2880598d6 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 15 Aug 2022 16:04:14 +0200 Subject: [PATCH 14/20] add missing unprotected path --- services/proxy/pkg/middleware/authentication.go | 1 + 1 file changed, 1 insertion(+) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 2bdc5fe2cb..2ee63fe34b 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -45,6 +45,7 @@ var ( _unprotectedPathPrefixes = [...]string{ "/files", "/data", + "/account", "/s/", "/settings", "/user-management", From 78d85b683da0805c5a04f1738a09de5715534ae4 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 16 Aug 2022 12:47:44 +0200 Subject: [PATCH 15/20] add missing comments --- services/proxy/pkg/middleware/authentication.go | 6 ++++-- services/proxy/pkg/middleware/basic_auth.go | 1 + services/proxy/pkg/middleware/oidc_auth.go | 7 ++++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 2ee63fe34b..3e851d0ada 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -65,9 +65,11 @@ const ( ) // Authenticator is the common interface implemented by all request authenticators. -// The Authenticator may augment the request with user info or anything related to the -// authentication and return the augmented request. + type Authenticator interface { + // Authenticate is used to authenticate incoming HTTP requests. + // The Authenticator may augment the request with user info or anything related to the + // authentication and return the augmented request. Authenticate(*http.Request) (*http.Request, bool) } diff --git a/services/proxy/pkg/middleware/basic_auth.go b/services/proxy/pkg/middleware/basic_auth.go index eaccd70425..eb8f79847b 100644 --- a/services/proxy/pkg/middleware/basic_auth.go +++ b/services/proxy/pkg/middleware/basic_auth.go @@ -8,6 +8,7 @@ import ( "github.com/owncloud/ocis/v2/services/proxy/pkg/user/backend" ) +// BasicAuthenticator is the authenticator responsible for HTTP Basic authentication. type BasicAuthenticator struct { Logger log.Logger UserProvider backend.UserBackend diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index a59a3b2457..36adeeb296 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -30,6 +30,7 @@ type OIDCProvider interface { UserInfo(ctx context.Context, ts oauth2.TokenSource) (*gOidc.UserInfo, error) } +// NewOIDCAuthenticator returns a ready to use authenticator which can handle OIDC authentication. func NewOIDCAuthenticator(logger log.Logger, tokenCacheTTL int, oidcHTTPClient *http.Client, oidcIss string, providerFunc func() (OIDCProvider, error), jwksOptions config.JWKS, accessTokenVerifyMethod string) OIDCAuthenticator { tokenCache := osync.NewCache(tokenCacheTTL) @@ -47,6 +48,7 @@ func NewOIDCAuthenticator(logger log.Logger, tokenCacheTTL int, oidcHTTPClient * } } +// OIDCAuthenticator is an authenticator responsible for OIDC authentication. type OIDCAuthenticator struct { Logger log.Logger HTTPClient *http.Client @@ -239,7 +241,10 @@ func (m OIDCAuthenticator) getProvider() OIDCProvider { func (m OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { // there is no bearer token on the request, - if !m.shouldServe(r) { + if !m.shouldServe(r) || isPublicPath(r.URL.Path) { + // The authentication of public path requests is handled by another authenticator. + // Since we can't guarantee the order of execution of the authenticators, we better + // implement an early return here for paths we can't authenticate in this authenticator. return nil, false } From 905ead629c9143e9f74273276a8b0eb2ad1d56ff Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 16 Aug 2022 15:16:48 +0200 Subject: [PATCH 16/20] add unprotected path prefix for external apps --- services/proxy/pkg/middleware/authentication.go | 1 + 1 file changed, 1 insertion(+) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 3e851d0ada..84a552463f 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -47,6 +47,7 @@ var ( "/data", "/account", "/s/", + "/external/spaces", "/settings", "/user-management", "/.well-known", From 3f25ca2059d48a58682d092408b9e47427bfd327 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 16 Aug 2022 15:45:22 +0200 Subject: [PATCH 17/20] add unprotected path prefix for parallel deployment --- services/proxy/pkg/middleware/authentication.go | 1 + 1 file changed, 1 insertion(+) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 84a552463f..a218546760 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -48,6 +48,7 @@ var ( "/account", "/s/", "/external/spaces", + "/apps/openidconnect/redirect", "/settings", "/user-management", "/.well-known", From b5ef10dc2b82fc9a083fad594a00af055068b6e2 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 16 Aug 2022 16:53:27 +0200 Subject: [PATCH 18/20] add missing comments and changelog --- changelog/unreleased/rewrite-authentication.md | 7 +++++++ services/proxy/pkg/middleware/public_share_auth.go | 2 ++ services/proxy/pkg/middleware/signed_url_auth.go | 1 + 3 files changed, 10 insertions(+) create mode 100644 changelog/unreleased/rewrite-authentication.md diff --git a/changelog/unreleased/rewrite-authentication.md b/changelog/unreleased/rewrite-authentication.md new file mode 100644 index 0000000000..c399074312 --- /dev/null +++ b/changelog/unreleased/rewrite-authentication.md @@ -0,0 +1,7 @@ +Enhancement: Rewrite of the request authentication middleware + +There were some flaws in the authentication middleware which were resolved by this rewrite. +This rewrite also introduced the need to manually mark certain paths as "unprotected" if +requests to these paths must not be authenticated. + +https://github.com/owncloud/ocis/pull/4374 diff --git a/services/proxy/pkg/middleware/public_share_auth.go b/services/proxy/pkg/middleware/public_share_auth.go index dabaa8a38c..cab9b6ebf3 100644 --- a/services/proxy/pkg/middleware/public_share_auth.go +++ b/services/proxy/pkg/middleware/public_share_auth.go @@ -18,6 +18,8 @@ const ( _paramExpiration = "expiration" ) +// PublicShareAuthenticator is the authenticator which can authenticate public share requests. +// It will add the share owner into the request context. type PublicShareAuthenticator struct { Logger log.Logger RevaGatewayClient gateway.GatewayAPIClient diff --git a/services/proxy/pkg/middleware/signed_url_auth.go b/services/proxy/pkg/middleware/signed_url_auth.go index 80a613aace..2455e8eecf 100644 --- a/services/proxy/pkg/middleware/signed_url_auth.go +++ b/services/proxy/pkg/middleware/signed_url_auth.go @@ -38,6 +38,7 @@ var ( } ) +// SignedURLAuthenticator is the authenticator responsible for authenticating signed URL requests. type SignedURLAuthenticator struct { Logger log.Logger PreSignedURLConfig config.PreSignedURL From 12d42e00746fb6f4050f645bb864403549ce4de5 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 22 Aug 2022 14:15:13 +0200 Subject: [PATCH 19/20] add missing comments --- services/proxy/pkg/middleware/authentication.go | 1 - services/proxy/pkg/middleware/basic_auth.go | 1 + services/proxy/pkg/middleware/oidc_auth.go | 1 + services/proxy/pkg/middleware/public_share_auth.go | 1 + services/proxy/pkg/middleware/signed_url_auth.go | 1 + 5 files changed, 4 insertions(+), 1 deletion(-) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index a218546760..93a023038f 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -67,7 +67,6 @@ const ( ) // Authenticator is the common interface implemented by all request authenticators. - type Authenticator interface { // Authenticate is used to authenticate incoming HTTP requests. // The Authenticator may augment the request with user info or anything related to the diff --git a/services/proxy/pkg/middleware/basic_auth.go b/services/proxy/pkg/middleware/basic_auth.go index eb8f79847b..913e740486 100644 --- a/services/proxy/pkg/middleware/basic_auth.go +++ b/services/proxy/pkg/middleware/basic_auth.go @@ -16,6 +16,7 @@ type BasicAuthenticator struct { UserOIDCClaim string } +// Authenticate implements the authenticator interface to authenticate requests via basic auth. func (m BasicAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { if isPublicPath(r.URL.Path) { // The authentication of public path requests is handled by another authenticator. diff --git a/services/proxy/pkg/middleware/oidc_auth.go b/services/proxy/pkg/middleware/oidc_auth.go index 36adeeb296..c89f83d5cc 100644 --- a/services/proxy/pkg/middleware/oidc_auth.go +++ b/services/proxy/pkg/middleware/oidc_auth.go @@ -239,6 +239,7 @@ func (m OIDCAuthenticator) getProvider() OIDCProvider { return m.provider } +// Authenticate implements the authenticator interface to authenticate requests via oidc auth. func (m OIDCAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { // there is no bearer token on the request, if !m.shouldServe(r) || isPublicPath(r.URL.Path) { diff --git a/services/proxy/pkg/middleware/public_share_auth.go b/services/proxy/pkg/middleware/public_share_auth.go index cab9b6ebf3..8f82bf7203 100644 --- a/services/proxy/pkg/middleware/public_share_auth.go +++ b/services/proxy/pkg/middleware/public_share_auth.go @@ -25,6 +25,7 @@ type PublicShareAuthenticator struct { RevaGatewayClient gateway.GatewayAPIClient } +// Authenticate implements the authenticator interface to authenticate requests via public share auth. func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { if !isPublicPath(r.URL.Path) { return nil, false diff --git a/services/proxy/pkg/middleware/signed_url_auth.go b/services/proxy/pkg/middleware/signed_url_auth.go index 2455e8eecf..793f6d8adc 100644 --- a/services/proxy/pkg/middleware/signed_url_auth.go +++ b/services/proxy/pkg/middleware/signed_url_auth.go @@ -186,6 +186,7 @@ func (m SignedURLAuthenticator) getSigningKey(ctx context.Context, ocisID string return res.Records[0].Value, nil } +// Authenticate implements the authenticator interface to authenticate requests via signed URL auth. func (m SignedURLAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { if !m.shouldServe(r) { return nil, false From dfe703291f09243b2aab2e931da08d33225f5ebe Mon Sep 17 00:00:00 2001 From: David Christofas Date: Mon, 22 Aug 2022 15:08:01 +0200 Subject: [PATCH 20/20] replace strings.Title with cases.Title --- services/proxy/pkg/middleware/authentication.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/services/proxy/pkg/middleware/authentication.go b/services/proxy/pkg/middleware/authentication.go index 93a023038f..6d430c0038 100644 --- a/services/proxy/pkg/middleware/authentication.go +++ b/services/proxy/pkg/middleware/authentication.go @@ -7,6 +7,8 @@ import ( "strings" "github.com/owncloud/ocis/v2/services/proxy/pkg/webdav" + "golang.org/x/text/cases" + "golang.org/x/text/language" ) var ( @@ -96,10 +98,11 @@ func Authentication(auths []Authenticator, opts ...Option) func(next http.Handle if !isPublicPath(r.URL.Path) { // Failed basic authentication attempts receive the Www-Authenticate header in the response var touch bool + caser := cases.Title(language.Und) for k, v := range options.CredentialsByUserAgent { if strings.Contains(k, r.UserAgent()) { removeSuperfluousAuthenticate(w) - w.Header().Add("Www-Authenticate", fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), r.Host)) + w.Header().Add("Www-Authenticate", fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", caser.String(v), r.Host)) touch = true break } @@ -175,8 +178,9 @@ func configureSupportedChallenges(options Options) { } func writeSupportedAuthenticateHeader(w http.ResponseWriter, r *http.Request) { + caser := cases.Title(language.Und) for _, s := range SupportedAuthStrategies { - w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(s), r.Host)) + w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", caser.String(s), r.Host)) } } @@ -213,12 +217,13 @@ func evalRequestURI(l userAgentLocker, r regexp.Regexp) { if !r.MatchString(l.r.RequestURI) { return } + caser := cases.Title(language.Und) for k, v := range l.locks { if strings.Contains(k, l.r.UserAgent()) { removeSuperfluousAuthenticate(l.w) - l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(v), l.r.Host)) + l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", caser.String(v), l.r.Host)) return } } - l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", strings.Title(l.fallback), l.r.Host)) + l.w.Header().Add(WwwAuthenticate, fmt.Sprintf("%v realm=\"%s\", charset=\"UTF-8\"", caser.String(l.fallback), l.r.Host)) }