From ef339713c25253980f98d4c28b3fe5326538664b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 13 Nov 2024 10:26:37 +0800 Subject: [PATCH] Refactor internal routers (partial backport, auth token const time comparing) (#32473) (#32479) Partially backport #32473. LFS related changes are not in 1.22, so skip them. 1. Ignore non-existing repos during migrations 2. Improve ReadBatchLine's comment 3. Use `X-Gitea-Internal-Auth` header for internal API calls and make the comparing constant time (it wasn't a serous problem because in a real world it's nearly impossible to timing-attack the token, but indeed security related and good to fix and backport) 4. Fix route mock nil check --- models/migrations/v1_21/v276.go | 5 ++++- modules/git/batch_reader.go | 5 ++--- modules/private/internal.go | 2 +- modules/web/route.go | 13 +++++++++++-- routers/private/internal.go | 18 ++++++++++-------- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/models/migrations/v1_21/v276.go b/models/migrations/v1_21/v276.go index ed1bc3bda5..15177bf040 100644 --- a/models/migrations/v1_21/v276.go +++ b/models/migrations/v1_21/v276.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/modules/git" giturl "code.gitea.io/gitea/modules/git/url" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" "xorm.io/xorm" ) @@ -163,7 +164,9 @@ func migratePushMirrors(x *xorm.Engine) error { func getRemoteAddress(ownerName, repoName, remoteName string) (string, error) { repoPath := filepath.Join(setting.RepoRootPath, strings.ToLower(ownerName), strings.ToLower(repoName)+".git") - + if exist, _ := util.IsExist(repoPath); !exist { + return "", nil + } remoteURL, err := git.GetRemoteAddress(context.Background(), repoPath, remoteName) if err != nil { return "", fmt.Errorf("get remote %s's address of %s/%s failed: %v", remoteName, ownerName, repoName, err) diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index 3b1a466b2e..7dfda72155 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -146,9 +146,8 @@ func catFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufi } // ReadBatchLine reads the header line from cat-file --batch -// We expect: -// SP SP LF -// sha is a hex encoded here +// We expect: SP SP LF +// then leaving the rest of the stream " LF" to be read func ReadBatchLine(rd *bufio.Reader) (sha []byte, typ string, size int64, err error) { typ, err = rd.ReadString('\n') if err != nil { diff --git a/modules/private/internal.go b/modules/private/internal.go index 9c330a24a8..c7e7773524 100644 --- a/modules/private/internal.go +++ b/modules/private/internal.go @@ -43,7 +43,7 @@ Ensure you are running in the correct environment or set the correct configurati req := httplib.NewRequest(url, method). SetContext(ctx). Header("X-Real-IP", getClientIP()). - Header("Authorization", fmt.Sprintf("Bearer %s", setting.InternalToken)). + Header("X-Gitea-Internal-Auth", fmt.Sprintf("Bearer %s", setting.InternalToken)). SetTLSClientConfig(&tls.Config{ InsecureSkipVerify: true, ServerName: setting.Domain, diff --git a/modules/web/route.go b/modules/web/route.go index 805fcb4411..cda15aa0c7 100644 --- a/modules/web/route.go +++ b/modules/web/route.go @@ -5,6 +5,7 @@ package web import ( "net/http" + "reflect" "strings" "code.gitea.io/gitea/modules/web/middleware" @@ -80,15 +81,23 @@ func (r *Route) getPattern(pattern string) string { return strings.TrimSuffix(newPattern, "/") } +func isNilOrFuncNil(v any) bool { + if v == nil { + return true + } + r := reflect.ValueOf(v) + return r.Kind() == reflect.Func && r.IsNil() +} + func (r *Route) wrapMiddlewareAndHandler(h []any) ([]func(http.Handler) http.Handler, http.HandlerFunc) { handlerProviders := make([]func(http.Handler) http.Handler, 0, len(r.curMiddlewares)+len(h)+1) for _, m := range r.curMiddlewares { - if m != nil { + if !isNilOrFuncNil(m) { handlerProviders = append(handlerProviders, toHandlerProvider(m)) } } for _, m := range h { - if h != nil { + if !isNilOrFuncNil(m) { handlerProviders = append(handlerProviders, toHandlerProvider(m)) } } diff --git a/routers/private/internal.go b/routers/private/internal.go index ede310113c..439af74f16 100644 --- a/routers/private/internal.go +++ b/routers/private/internal.go @@ -5,6 +5,7 @@ package private import ( + "crypto/subtle" "net/http" "strings" @@ -18,22 +19,23 @@ import ( chi_middleware "github.com/go-chi/chi/v5/middleware" ) -// CheckInternalToken check internal token is set -func CheckInternalToken(next http.Handler) http.Handler { +func authInternal(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - tokens := req.Header.Get("Authorization") - fields := strings.SplitN(tokens, " ", 2) if setting.InternalToken == "" { log.Warn(`The INTERNAL_TOKEN setting is missing from the configuration file: %q, internal API can't work.`, setting.CustomConf) http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) return } - if len(fields) != 2 || fields[0] != "Bearer" || fields[1] != setting.InternalToken { + + tokens := req.Header.Get("X-Gitea-Internal-Auth") // TODO: use something like JWT or HMAC to avoid passing the token in the clear + after, found := strings.CutPrefix(tokens, "Bearer ") + authSucceeded := found && subtle.ConstantTimeCompare([]byte(after), []byte(setting.InternalToken)) == 1 + if !authSucceeded { log.Debug("Forbidden attempt to access internal url: Authorization header: %s", tokens) http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) - } else { - next.ServeHTTP(w, req) + return } + next.ServeHTTP(w, req) }) } @@ -51,7 +53,7 @@ func bind[T any](_ T) any { func Routes() *web.Route { r := web.NewRoute() r.Use(context.PrivateContexter()) - r.Use(CheckInternalToken) + r.Use(authInternal) // Log the real ip address of the request from SSH is really helpful for diagnosing sometimes. // Since internal API will be sent only from Gitea sub commands and it's under control (checked by InternalToken), we can trust the headers. r.Use(chi_middleware.RealIP)