From 900e1580648e153d40f9da005eb745705410ac73 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 29 Dec 2022 13:50:09 +0800 Subject: [PATCH] refactor auth interface to return error when verify failure (#22119) (#22259) backport #22119 This PR changed the Auth interface signature from `Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User` to `Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error)`. There is a new return argument `error` which means the verification condition matched but verify process failed, we should stop the auth process. Before this PR, when return a `nil` user, we don't know the reason why it returned `nil`. If the match condition is not satisfied or it verified failure? For these two different results, we should have different handler. If the match condition is not satisfied, we should try next auth method and if there is no more auth method, it's an anonymous user. If the condition matched but verify failed, the auth process should be stop and return immediately. This will fix #20563 Co-authored-by: KN4CK3R Co-authored-by: Jason Song --- modules/context/api.go | 8 +++++++- modules/context/context.go | 8 +++++++- routers/api/packages/api.go | 17 +++++++++++++++-- routers/api/packages/conan/auth.go | 10 +++++----- routers/api/packages/container/auth.go | 12 ++++++------ routers/api/packages/nuget/auth.go | 9 +++++---- services/auth/basic.go | 22 +++++++++++----------- services/auth/group.go | 17 ++++++++--------- services/auth/httpsign.go | 14 +++++++------- services/auth/interface.go | 5 +++-- services/auth/oauth2.go | 14 +++++--------- services/auth/reverseproxy.go | 19 +++++++++++-------- services/auth/session.go | 6 +++--- services/auth/sspi_windows.go | 18 +++++++++--------- services/packages/auth.go | 11 +++++++++-- 15 files changed, 111 insertions(+), 79 deletions(-) diff --git a/modules/context/api.go b/modules/context/api.go index 3eb1de8939..d95e580321 100644 --- a/modules/context/api.go +++ b/modules/context/api.go @@ -220,7 +220,13 @@ func (ctx *APIContext) CheckForOTP() { func APIAuth(authMethod auth_service.Method) func(*APIContext) { return func(ctx *APIContext) { // Get user from session if logged in. - ctx.Doer = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + var err error + ctx.Doer, err = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + if err != nil { + ctx.Error(http.StatusUnauthorized, "APIAuth", err) + return + } + if ctx.Doer != nil { if ctx.Locale.Language() != ctx.Doer.Language { ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req) diff --git a/modules/context/context.go b/modules/context/context.go index 47368bb280..b761f7b803 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -663,7 +663,13 @@ func getCsrfOpts() CsrfOptions { // Auth converts auth.Auth as a middleware func Auth(authMethod auth.Method) func(*Context) { return func(ctx *Context) { - ctx.Doer = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + var err error + ctx.Doer, err = authMethod.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + if err != nil { + log.Error("Failed to verify user %v: %v", ctx.Req.RemoteAddr, err) + ctx.Error(http.StatusUnauthorized, "Verify") + return + } if ctx.Doer != nil { if ctx.Locale.Language() != ctx.Doer.Language { ctx.Locale = middleware.Locale(ctx.Resp, ctx.Req) diff --git a/routers/api/packages/api.go b/routers/api/packages/api.go index 2b3d7de3d1..7d1acb5f53 100644 --- a/routers/api/packages/api.go +++ b/routers/api/packages/api.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/packages/composer" @@ -57,7 +58,13 @@ func Routes(ctx gocontext.Context) *web.Route { authGroup := auth.NewGroup(authMethods...) r.Use(func(ctx *context.Context) { - ctx.Doer = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + var err error + ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + if err != nil { + log.Error("Verify: %v", err) + ctx.Error(http.StatusUnauthorized, "authGroup.Verify") + return + } ctx.IsSigned = ctx.Doer != nil }) @@ -317,7 +324,13 @@ func ContainerRoutes(ctx gocontext.Context) *web.Route { authGroup := auth.NewGroup(authMethods...) r.Use(func(ctx *context.Context) { - ctx.Doer = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + var err error + ctx.Doer, err = authGroup.Verify(ctx.Req, ctx.Resp, ctx, ctx.Session) + if err != nil { + log.Error("Failed to verify user: %v", err) + ctx.Error(http.StatusUnauthorized, "Verify") + return + } ctx.IsSigned = ctx.Doer != nil }) diff --git a/routers/api/packages/conan/auth.go b/routers/api/packages/conan/auth.go index 00855a97a4..9d3ed98361 100644 --- a/routers/api/packages/conan/auth.go +++ b/routers/api/packages/conan/auth.go @@ -20,22 +20,22 @@ func (a *Auth) Name() string { } // Verify extracts the user from the Bearer token -func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) *user_model.User { +func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) (*user_model.User, error) { uid, err := packages.ParseAuthorizationToken(req) if err != nil { log.Trace("ParseAuthorizationToken: %v", err) - return nil + return nil, err } if uid == 0 { - return nil + return nil, nil } u, err := user_model.GetUserByID(uid) if err != nil { log.Error("GetUserByID: %v", err) - return nil + return nil, err } - return u + return u, nil } diff --git a/routers/api/packages/container/auth.go b/routers/api/packages/container/auth.go index 770068a3bf..9f99e5b0fb 100644 --- a/routers/api/packages/container/auth.go +++ b/routers/api/packages/container/auth.go @@ -21,25 +21,25 @@ func (a *Auth) Name() string { // Verify extracts the user from the Bearer token // If it's an anonymous session a ghost user is returned -func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) *user_model.User { +func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) (*user_model.User, error) { uid, err := packages.ParseAuthorizationToken(req) if err != nil { log.Trace("ParseAuthorizationToken: %v", err) - return nil + return nil, err } if uid == 0 { - return nil + return nil, nil } if uid == -1 { - return user_model.NewGhostUser() + return user_model.NewGhostUser(), nil } u, err := user_model.GetUserByID(uid) if err != nil { log.Error("GetUserByID: %v", err) - return nil + return nil, err } - return u + return u, nil } diff --git a/routers/api/packages/nuget/auth.go b/routers/api/packages/nuget/auth.go index 1dad452648..859ac16fe6 100644 --- a/routers/api/packages/nuget/auth.go +++ b/routers/api/packages/nuget/auth.go @@ -21,19 +21,20 @@ func (a *Auth) Name() string { } // https://docs.microsoft.com/en-us/nuget/api/package-publish-resource#request-parameters -func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) *user_model.User { +func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataStore, sess auth.SessionStore) (*user_model.User, error) { token, err := auth_model.GetAccessTokenBySHA(req.Header.Get("X-NuGet-ApiKey")) if err != nil { if !(auth_model.IsErrAccessTokenNotExist(err) || auth_model.IsErrAccessTokenEmpty(err)) { log.Error("GetAccessTokenBySHA: %v", err) + return nil, err } - return nil + return nil, nil } u, err := user_model.GetUserByID(token.UID) if err != nil { log.Error("GetUserByID: %v", err) - return nil + return nil, err } token.UpdatedUnix = timeutil.TimeStampNow() @@ -41,5 +42,5 @@ func (a *Auth) Verify(req *http.Request, w http.ResponseWriter, store auth.DataS log.Error("UpdateAccessToken: %v", err) } - return u + return u, nil } diff --git a/services/auth/basic.go b/services/auth/basic.go index 9b32ad29af..a6294dbee0 100644 --- a/services/auth/basic.go +++ b/services/auth/basic.go @@ -41,20 +41,20 @@ func (b *Basic) Name() string { // "Authorization" header of the request and returns the corresponding user object for that // name/token on successful validation. // Returns nil if header is empty or validation fails. -func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { +func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { // Basic authentication should only fire on API, Download or on Git or LFSPaths if !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawReleaseOrLFSPath(req) { - return nil + return nil, nil } baHead := req.Header.Get("Authorization") if len(baHead) == 0 { - return nil + return nil, nil } auths := strings.SplitN(baHead, " ", 2) if len(auths) != 2 || (strings.ToLower(auths[0]) != "basic") { - return nil + return nil, nil } uname, passwd, _ := base.BasicAuthDecode(auths[1]) @@ -78,11 +78,11 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore u, err := user_model.GetUserByID(uid) if err != nil { log.Error("GetUserByID: %v", err) - return nil + return nil, err } store.GetData()["IsApiToken"] = true - return u + return u, nil } token, err := auth_model.GetAccessTokenBySHA(authToken) @@ -91,7 +91,7 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore u, err := user_model.GetUserByID(token.UID) if err != nil { log.Error("GetUserByID: %v", err) - return nil + return nil, err } token.UpdatedUnix = timeutil.TimeStampNow() @@ -100,13 +100,13 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore } store.GetData()["IsApiToken"] = true - return u + return u, nil } else if !auth_model.IsErrAccessTokenNotExist(err) && !auth_model.IsErrAccessTokenEmpty(err) { log.Error("GetAccessTokenBySha: %v", err) } if !setting.Service.EnableBasicAuth { - return nil + return nil, nil } log.Trace("Basic Authorization: Attempting SignIn for %s", uname) @@ -115,7 +115,7 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore if !user_model.IsErrUserNotExist(err) { log.Error("UserSignIn: %v", err) } - return nil + return nil, err } if skipper, ok := source.Cfg.(LocalTwoFASkipper); ok && skipper.IsSkipLocalTwoFA() { @@ -124,5 +124,5 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, store DataStore log.Trace("Basic Authorization: Logged in user %-v", u) - return u + return u, nil } diff --git a/services/auth/group.go b/services/auth/group.go index bbafe64b49..ef68fc35a6 100644 --- a/services/auth/group.go +++ b/services/auth/group.go @@ -10,7 +10,6 @@ import ( "reflect" "strings" - "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" ) @@ -81,23 +80,23 @@ func (b *Group) Free() error { } // Verify extracts and validates -func (b *Group) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { - if !db.HasEngine { - return nil - } - +func (b *Group) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { // Try to sign in with each of the enabled plugins for _, ssoMethod := range b.methods { - user := ssoMethod.Verify(req, w, store, sess) + user, err := ssoMethod.Verify(req, w, store, sess) + if err != nil { + return nil, err + } + if user != nil { if store.GetData()["AuthedMethod"] == nil { if named, ok := ssoMethod.(Named); ok { store.GetData()["AuthedMethod"] = named.Name() } } - return user + return user, nil } } - return nil + return nil, nil } diff --git a/services/auth/httpsign.go b/services/auth/httpsign.go index 67053d2b77..4e01bc29c6 100644 --- a/services/auth/httpsign.go +++ b/services/auth/httpsign.go @@ -40,10 +40,10 @@ func (h *HTTPSign) Name() string { // Verify extracts and validates HTTPsign from the Signature header of the request and returns // the corresponding user object on successful validation. // Returns nil if header is empty or validation fails. -func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { +func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { sigHead := req.Header.Get("Signature") if len(sigHead) == 0 { - return nil + return nil, nil } var ( @@ -54,14 +54,14 @@ func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataSt if len(req.Header.Get("X-Ssh-Certificate")) != 0 { // Handle Signature signed by SSH certificates if len(setting.SSH.TrustedUserCAKeys) == 0 { - return nil + return nil, nil } publicKey, err = VerifyCert(req) if err != nil { log.Debug("VerifyCert on request from %s: failed: %v", req.RemoteAddr, err) log.Warn("Failed authentication attempt from %s", req.RemoteAddr) - return nil + return nil, nil } } else { // Handle Signature signed by Public Key @@ -69,21 +69,21 @@ func (h *HTTPSign) Verify(req *http.Request, w http.ResponseWriter, store DataSt if err != nil { log.Debug("VerifyPubKey on request from %s: failed: %v", req.RemoteAddr, err) log.Warn("Failed authentication attempt from %s", req.RemoteAddr) - return nil + return nil, nil } } u, err := user_model.GetUserByID(publicKey.OwnerID) if err != nil { log.Error("GetUserByID: %v", err) - return nil + return nil, err } store.GetData()["IsApiToken"] = true log.Trace("HTTP Sign: Logged in user %-v", u) - return u + return u, nil } func VerifyPubKey(r *http.Request) (*asymkey_model.PublicKey, error) { diff --git a/services/auth/interface.go b/services/auth/interface.go index ecc9ad2ca6..18d2eadb52 100644 --- a/services/auth/interface.go +++ b/services/auth/interface.go @@ -25,8 +25,9 @@ type Method interface { // If verification is successful returns either an existing user object (with id > 0) // or a new user object (with id = 0) populated with the information that was found // in the authentication data (username or email). - // Returns nil if verification fails. - Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User + // Second argument returns err if verification fails, otherwise + // First return argument returns nil if no matched verification condition + Verify(http *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) } // Initializable represents a structure that requires initialization diff --git a/services/auth/oauth2.go b/services/auth/oauth2.go index 8f038d6104..2f1154ebbe 100644 --- a/services/auth/oauth2.go +++ b/services/auth/oauth2.go @@ -109,18 +109,14 @@ func (o *OAuth2) userIDFromToken(req *http.Request, store DataStore) int64 { // or the "Authorization" header and returns the corresponding user object for that ID. // If verification is successful returns an existing user object. // Returns nil if verification fails. -func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { - if !db.HasEngine { - return nil - } - +func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) { - return nil + return nil, nil } id := o.userIDFromToken(req, store) if id <= 0 { - return nil + return nil, nil } log.Trace("OAuth2 Authorization: Found token for user[%d]", id) @@ -129,11 +125,11 @@ func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, store DataStor if !user_model.IsErrUserNotExist(err) { log.Error("GetUserByName: %v", err) } - return nil + return nil, err } log.Trace("OAuth2 Authorization: Logged in user %-v", user) - return user + return user, nil } func isAuthenticatedTokenRequest(req *http.Request) bool { diff --git a/services/auth/reverseproxy.go b/services/auth/reverseproxy.go index 8dec1c8ea7..59cd1fe05d 100644 --- a/services/auth/reverseproxy.go +++ b/services/auth/reverseproxy.go @@ -52,10 +52,10 @@ func (r *ReverseProxy) Name() string { // If a username is available in the "setting.ReverseProxyAuthUser" header an existing // user object is returned (populated with username or email found in header). // Returns nil if header is empty. -func (r *ReverseProxy) getUserFromAuthUser(req *http.Request) *user_model.User { +func (r *ReverseProxy) getUserFromAuthUser(req *http.Request) (*user_model.User, error) { username := r.getUserName(req) if len(username) == 0 { - return nil + return nil, nil } log.Trace("ReverseProxy Authorization: Found username: %s", username) @@ -63,11 +63,11 @@ func (r *ReverseProxy) getUserFromAuthUser(req *http.Request) *user_model.User { if err != nil { if !user_model.IsErrUserNotExist(err) || !r.isAutoRegisterAllowed() { log.Error("GetUserByName: %v", err) - return nil + return nil, err } user = r.newUser(req) } - return user + return user, nil } // getEmail extracts the email from the "setting.ReverseProxyAuthEmail" header @@ -107,12 +107,15 @@ func (r *ReverseProxy) getUserFromAuthEmail(req *http.Request) *user_model.User // First it will attempt to load it based on the username (see docs for getUserFromAuthUser), // and failing that it will attempt to load it based on the email (see docs for getUserFromAuthEmail). // Returns nil if the headers are empty or the user is not found. -func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { - user := r.getUserFromAuthUser(req) +func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { + user, err := r.getUserFromAuthUser(req) + if err != nil { + return nil, err + } if user == nil { user = r.getUserFromAuthEmail(req) if user == nil { - return nil + return nil, nil } } @@ -125,7 +128,7 @@ func (r *ReverseProxy) Verify(req *http.Request, w http.ResponseWriter, store Da store.GetData()["IsReverseProxy"] = true log.Trace("ReverseProxy Authorization: Logged in user %-v", user) - return user + return user, nil } // isAutoRegisterAllowed checks if EnableReverseProxyAutoRegister setting is true diff --git a/services/auth/session.go b/services/auth/session.go index 1ec94aa0af..adc4be5399 100644 --- a/services/auth/session.go +++ b/services/auth/session.go @@ -29,12 +29,12 @@ func (s *Session) Name() string { // Verify checks if there is a user uid stored in the session and returns the user // object for that uid. // Returns nil if there is no user uid stored in the session. -func (s *Session) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { +func (s *Session) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { user := SessionUser(sess) if user != nil { - return user + return user, nil } - return nil + return nil, nil } // SessionUser returns the user object corresponding to the "uid" session variable. diff --git a/services/auth/sspi_windows.go b/services/auth/sspi_windows.go index 757d596c4c..4bcee1203a 100644 --- a/services/auth/sspi_windows.go +++ b/services/auth/sspi_windows.go @@ -78,15 +78,15 @@ func (s *SSPI) Free() error { // If authentication is successful, returns the corresponding user object. // If negotiation should continue or authentication fails, immediately returns a 401 HTTP // response code, as required by the SPNEGO protocol. -func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) *user_model.User { +func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) { if !s.shouldAuthenticate(req) { - return nil + return nil, nil } cfg, err := s.getConfig() if err != nil { log.Error("could not get SSPI config: %v", err) - return nil + return nil, err } log.Trace("SSPI Authorization: Attempting to authenticate") @@ -109,7 +109,7 @@ func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, log.Error("%v", err) } - return nil + return nil, err } if outToken != "" { sspiAuth.AppendAuthenticateHeader(w, outToken) @@ -117,7 +117,7 @@ func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, username := sanitizeUsername(userInfo.Username, cfg) if len(username) == 0 { - return nil + return nil, nil } log.Info("Authenticated as %s\n", username) @@ -125,16 +125,16 @@ func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, if err != nil { if !user_model.IsErrUserNotExist(err) { log.Error("GetUserByName: %v", err) - return nil + return nil, err } if !cfg.AutoCreateUsers { log.Error("User '%s' not found", username) - return nil + return nil, nil } user, err = s.newUser(username, cfg) if err != nil { log.Error("CreateUser: %v", err) - return nil + return nil, err } } @@ -144,7 +144,7 @@ func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, } log.Trace("SSPI Authorization: Logged in user %-v", user) - return user + return user, nil } // getConfig retrieves the SSPI configuration from login sources diff --git a/services/packages/auth.go b/services/packages/auth.go index 50212fccfd..49ff6c0a3d 100644 --- a/services/packages/auth.go +++ b/services/packages/auth.go @@ -11,6 +11,7 @@ import ( "time" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "github.com/golang-jwt/jwt/v4" @@ -42,9 +43,15 @@ func CreateAuthorizationToken(u *user_model.User) (string, error) { } func ParseAuthorizationToken(req *http.Request) (int64, error) { - parts := strings.SplitN(req.Header.Get("Authorization"), " ", 2) + h := req.Header.Get("Authorization") + if h == "" { + return 0, nil + } + + parts := strings.SplitN(h, " ", 2) if len(parts) != 2 { - return 0, fmt.Errorf("no token") + log.Error("split token failed: %s", h) + return 0, fmt.Errorf("split token failed") } token, err := jwt.ParseWithClaims(parts[1], &packageClaims{}, func(t *jwt.Token) (interface{}, error) {