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 | 186 +++++++++++----- services/proxy/pkg/middleware/basic_auth.go | 153 +++----------- .../proxy/pkg/middleware/basic_auth_test.go | 4 +- services/proxy/pkg/middleware/oidc_auth.go | 198 ++++++++---------- .../proxy/pkg/middleware/public_share_auth.go | 94 ++++----- .../proxy/pkg/middleware/signed_url_auth.go | 47 +++-- .../pkg/middleware/signed_url_auth_test.go | 12 +- 8 files changed, 415 insertions(+), 386 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 81342945f12..9e372e44816 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.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( - // 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), - - // 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), + 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 15fa4ec4299..651876f35f4 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/")} + + _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", + } +) - // WWWAuthenticate captures the Www-Authenticate header string. - WWWAuthenticate = "Www-Authenticate" +const ( + // WwwAuthenticate captures the Www-Authenticate header string. + WwwAuthenticate = "Www-Authenticate" ) -// 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 +// 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 options.OIDCIss == "" && options.EnableBasicAuth { - basic(next).ServeHTTP(w, r) + 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", + }) + + 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 b7d480d87bc..4771e57f217 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 +type BasicAuthenticator struct { + Logger log.Logger + UserProvider backend.UserBackend + UserCS3Claim string + UserOIDCClaim string +} - if options.EnableBasicAuth { - options.Logger.Warn().Msg("basic auth enabled, use only for testing or development") +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 } - h := basicAuth{ - logger: logger, - enabled: options.EnableBasicAuth, - userProvider: options.UserProvider, + login, password, ok := req.BasicAuth() + if !ok { + return nil, false } - 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))) - }, - ) + user, _, err := m.UserProvider.Authenticate(req.Context(), login, password) + if err != nil { + // TODO add log line + return nil, false } -} - -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 + // 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, } - 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 + 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 - for _, p := range publicPaths { - if strings.HasPrefix(req.URL.Path, p) { - isPublic = true - break - } } - - return isPublic -} - -// 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" -} - -func (m basicAuth) isBasicAuth(req *http.Request) bool { - _, _, ok := req.BasicAuth() - return m.enabled && ok + 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 46c7f48285e..77279d66b4c 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 ae3b313f086..97a71962ff4 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) - - 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{}, - } - - 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))) - }) - } +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 + + providerLock *sync.Mutex + provider OIDCProvider + + 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 9b3ddcd5ffd..6d6d0fa747d 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 +type PublicShareAuthenticator struct { + Logger log.Logger + RevaGatewayClient gateway.GatewayAPIClient +} - 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) - } +func (a PublicShareAuthenticator) Authenticate(r *http.Request) (*http.Request, bool) { + if !isPublicPath(r.URL.Path) { + return nil, false + } - // Currently we only want to authenticate app open request coming from public shares. - if shareToken == "" { - // Don't authenticate - next.ServeHTTP(w, r) - return - } + query := r.URL.Query() + shareToken := r.Header.Get(headerShareToken) + if shareToken == "" { + shareToken = query.Get(headerShareToken) + } - 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() + // Currently we only want to authenticate app open request coming from public shares. + if shareToken == "" { + // Don't authenticate + return nil, false + } - sharePassword = basicAuthPasswordPrefix - if ok { - sharePassword += password - } - } + 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() - authResp, err := options.RevaGatewayClient.Authenticate(r.Context(), &gateway.AuthenticateRequest{ - Type: authenticationType, - ClientId: shareToken, - ClientSecret: sharePassword, - }) + sharePassword = basicAuthPasswordPrefix + if ok { + sharePassword += password + } + } - 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 - } + authResp, err := a.RevaGatewayClient.Authenticate(r.Context(), &gateway.AuthenticateRequest{ + Type: authenticationType, + ClientId: shareToken, + ClientSecret: sharePassword, + }) - r.Header.Add(headerRevaAccessToken, authResp.Token) - next.ServeHTTP(w, r) - }) + 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 d71036d5a7f..094c279ad49 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 856fcdb0e2a..01311b731e8 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 | 113 ++++++++---------- 6 files changed, 127 insertions(+), 198 deletions(-) diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index 9e372e44816..7377d958290 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 651876f35f4..6c961657d10 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 4771e57f217..eaccd704254 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 97a71962ff4..5d740843d1c 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 6d6d0fa747d..795063d5d00 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 094c279ad49..80a613aace9 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...) - - return func(next http.Handler) http.Handler { - return &SignedURLAuthenticator{ - next: next, - logger: options.Logger, - preSignedURLConfig: options.PreSignedURLConfig, - store: options.Store, - userProvider: options.UserProvider, - } - } -} - -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) +const ( + _paramOCSignature = "OC-Signature" + _paramOCCredential = "OC-Credential" + _paramOCDate = "OC-Date" + _paramOCExpires = "OC-Expires" + _paramOCVerb = "OC-Verb" +) - if err := m.validate(req); err != nil { - http.Error(w, "Invalid url signature", http.StatusUnauthorized) - return +var ( + _requiredParams = [...]string{ + _paramOCSignature, + _paramOCCredential, + _paramOCDate, + _paramOCExpires, + _paramOCVerb, } +) - m.next.ServeHTTP(w, req) +type SignedURLAuthenticator struct { + 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 6c961657d10..887e116be2f 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 5d740843d1c..a6c1b5519d1 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 00000000000..1188fefa343 --- /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 77279d66b4c..00000000000 --- 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 01311b731e8..35f84e66559 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 00000000000..fcc2e6f3ce0 --- /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 00000000000..09e4e113609 --- /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 6b7e97bceb5..93612e3c48e 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) {}) - - m := OIDCAuth( - Logger(log.NewLogger()), - OIDCProviderFunc(func() (OIDCProvider, error) { - return mockOP(false), nil - }), - OIDCIss("https://localhost:9200"), - AccessTokenVerifyMethod(config.AccessTokenVerificationNone), - )(next) +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") - w := httptest.NewRecorder() - m.ServeHTTP(w, r) + r := httptest.NewRequest(http.MethodGet, "https://idp.example.com", nil) + r.Header.Set("Authorization", "Bearer sometoken") - 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 795063d5d00..b32f9ed147f 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 00000000000..98637988bd6 --- /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 93612e3c48e..00000000000 --- 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 7377d958290..e54538f7165 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 887e116be2f..53dfd414357 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 a6c1b5519d1..a59a3b24573 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 53dfd414357..f5f4d911507 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 b32f9ed147f..dabaa8a38c8 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 f5f4d911507..c90522403fd 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 c90522403fd..728a85a0a48 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 00964ecb4b8..999f00df863 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 728a85a0a48..07d5568f7f8 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 07d5568f7f8..2bdc5fe2cbc 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 2bdc5fe2cbc..2ee63fe34ba 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 2ee63fe34ba..3e851d0adae 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 eaccd704254..eb8f79847b6 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 a59a3b24573..36adeeb2964 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 3e851d0adae..84a552463f6 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 84a552463f6..a218546760b 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 00000000000..c3990743129 --- /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 dabaa8a38c8..cab9b6ebf38 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 80a613aace9..2455e8eecfc 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 a218546760b..93a023038fd 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 eb8f79847b6..913e7404865 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 36adeeb2964..c89f83d5ccd 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 cab9b6ebf38..8f82bf72031 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 2455e8eecfc..793f6d8adca 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 93a023038fd..6d430c0038b 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)) }