From 448f8c67e043b09546ca018754ca77c3b493a0c7 Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Fri, 25 Sep 2026 15:22:04 +0200 Subject: [PATCH] fix(ssh): fetch ssh key by fingerprint (#39423) Co-authored-by: wxiaoguang --- cmd/keys.go | 2 +- cmd/serv.go | 4 +- models/asymkey/ssh_key.go | 26 +++++++----- modules/log/logger_global.go | 4 -- modules/private/key.go | 9 +--- modules/ssh/ssh.go | 82 +++++++++++++++--------------------- routers/private/internal.go | 2 +- routers/private/key.go | 7 +-- services/auth/httpsign.go | 4 +- 9 files changed, 60 insertions(+), 80 deletions(-) diff --git a/cmd/keys.go b/cmd/keys.go index 2f9f52e5dbf..75ddf2e27e8 100644 --- a/cmd/keys.go +++ b/cmd/keys.go @@ -74,7 +74,7 @@ func runKeys(ctx context.Context, c *cli.Command) error { setup(ctx, c.Bool("debug")) - authorizedString, extra := private.AuthorizedPublicKeyByContent(ctx, content) + authorizedString, extra := private.AuthorizedPublicKeyForSSH(ctx, content) // do not use handleCliResponseExtra or cli.NewExitError, if it exists immediately, it breaks some tests like Test_CmdKeys if extra.Error != nil { return extra.Error diff --git a/cmd/serv.go b/cmd/serv.go index 583d534d2bc..c6521e1590a 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -343,8 +343,8 @@ func runServ(ctx context.Context, c *cli.Command) error { // Update user key activity. if results.PublicKeyID > 0 { - if err = private.UpdatePublicKeyInRepo(ctx, results.PublicKeyID, results.RepoID); err != nil { - return fail(ctx, "Failed to update public key", "UpdatePublicKeyInRepo: %v", err) + if err = private.UpdatePublicKeyLastUsed(ctx, results.PublicKeyID, results.RepoID); err != nil { + return fail(ctx, "Failed to update public key", "UpdatePublicKeyLastUsed: %v", err) } } diff --git a/models/asymkey/ssh_key.go b/models/asymkey/ssh_key.go index f41ec04dc89..be2484a5342 100644 --- a/models/asymkey/ssh_key.go +++ b/models/asymkey/ssh_key.go @@ -175,12 +175,16 @@ func GetPublicKeyByID(ctx context.Context, keyID int64) (*PublicKey, error) { return key, nil } -// SearchPublicKeyByContent searches content as prefix (leak e-mail part) -// and returns public key found. -func SearchPublicKeyByContent(ctx context.Context, content string) (*PublicKey, error) { +func SearchPublicKeyForSSH(ctx context.Context, sshPubKey string) (*PublicKey, error) { + // this function is designed to only accept SSH public keys, + // because there might be different methods to calculate the fingerprint in the future (at the moment: "SHA256:...") + fingerprint, err := CalcFingerprint(sshPubKey) + if err != nil { + return nil, err + } key := new(PublicKey) has, err := db.GetEngine(ctx). - Where("content like ?", content+"%"). + Where("fingerprint = ?", fingerprint). Get(key) if err != nil { return nil, err @@ -190,12 +194,12 @@ func SearchPublicKeyByContent(ctx context.Context, content string) (*PublicKey, return key, nil } -// SearchPublicKeyByContentExact searches content -// and returns public key found. -func SearchPublicKeyByContentExact(ctx context.Context, content string) (*PublicKey, error) { +func SearchPrincipalKey(ctx context.Context, principalKey string) (*PublicKey, error) { + // FIXME: this function is wrong, and there is no index on the content column + // In the future, the existing principal keys should be migrated to use the fingerprint ("principal:{name}") instead of the content key := new(PublicKey) has, err := db.GetEngine(ctx). - Where("content = ?", content). + Where("content = ?", principalKey). Get(key) if err != nil { return nil, err @@ -321,10 +325,10 @@ func deleteKeysMarkedForDeletion(ctx context.Context, keys []string) (bool, erro return db.WithTx2(ctx, func(ctx context.Context) (bool, error) { // Delete keys marked for deletion var sshKeysNeedUpdate bool - for _, KeyToDelete := range keys { - key, err := SearchPublicKeyByContent(ctx, KeyToDelete) + for _, sshKeyToDelete := range keys { + key, err := SearchPublicKeyForSSH(ctx, sshKeyToDelete) if err != nil { - log.Error("SearchPublicKeyByContent: %v", err) + log.Error("SearchPublicKeyForSSH: %v", err) continue } if _, err = db.DeleteByID[PublicKey](ctx, key.ID); err != nil { diff --git a/modules/log/logger_global.go b/modules/log/logger_global.go index 08ac5d2d5f6..72746890ef7 100644 --- a/modules/log/logger_global.go +++ b/modules/log/logger_global.go @@ -34,10 +34,6 @@ func Debug(format string, v ...any) { Log(1, DEBUG, format, v...) } -func IsDebug() bool { - return GetLevel() <= DEBUG -} - func Info(format string, v ...any) { Log(1, INFO, format, v...) } diff --git a/modules/private/key.go b/modules/private/key.go index 93ea695bffe..4b4e8e3066f 100644 --- a/modules/private/key.go +++ b/modules/private/key.go @@ -10,19 +10,14 @@ import ( "gitea.dev/modules/setting" ) -// UpdatePublicKeyInRepo update public key and if necessary deploy key updates -func UpdatePublicKeyInRepo(ctx context.Context, keyID, repoID int64) error { - // Ask for running deliver hook and test pull request tasks. +func UpdatePublicKeyLastUsed(ctx context.Context, keyID, repoID int64) error { reqURL := setting.LocalURL + fmt.Sprintf("api/internal/ssh/%d/update/%d", keyID, repoID) req := newInternalRequestAPI(ctx, reqURL, "POST") _, extra := requestJSONResp(req, &ResponseText{}) return extra.Error } -// AuthorizedPublicKeyByContent searches content as prefix (leak e-mail part) -// and returns public key found. -func AuthorizedPublicKeyByContent(ctx context.Context, content string) (*ResponseText, ResponseExtra) { - // Ask for running deliver hook and test pull request tasks. +func AuthorizedPublicKeyForSSH(ctx context.Context, content string) (*ResponseText, ResponseExtra) { reqURL := setting.LocalURL + "api/internal/ssh/authorized_keys" req := newInternalRequestAPI(ctx, reqURL, "POST") req.Param("content", content) diff --git a/modules/ssh/ssh.go b/modules/ssh/ssh.go index 5995ad16abf..53b2f6f3df0 100644 --- a/modules/ssh/ssh.go +++ b/modules/ssh/ssh.go @@ -159,9 +159,9 @@ func keyPermissions(keyID int64) *gossh.Permissions { // returned Permissions to the ssh conn once it verified the signature for that key, so a user // offering keys A (with a private key) and B (without one) authenticates and is served as A. func publicKeyHandler(ctx context.Context, conn gossh.ConnMetadata, key gossh.PublicKey) (*gossh.Permissions, error) { - if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary - log.Debug("Handle Public Key: Fingerprint: %s from %s", gossh.FingerprintSHA256(key), conn.RemoteAddr()) - } + sshPubKey := string(gossh.MarshalAuthorizedKey(key)) + fingerprint := gossh.FingerprintSHA256(key) + log.Debug("Handle Public Key: Fingerprint: %s from %s", fingerprint, conn.RemoteAddr()) if conn.User() != setting.SSH.BuiltinServerUser { log.Warn("Invalid SSH username %s - must use %s for all git operations via ssh", conn.User(), setting.SSH.BuiltinServerUser) @@ -171,9 +171,7 @@ func publicKeyHandler(ctx context.Context, conn gossh.ConnMetadata, key gossh.Pu // check if we have a certificate if cert, ok := key.(*gossh.Certificate); ok { - if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary - log.Debug("Handle Certificate: %s Fingerprint: %s is a certificate", conn.RemoteAddr(), gossh.FingerprintSHA256(key)) - } + log.Debug("Handle Certificate: %s Fingerprint: %s is a certificate", conn.RemoteAddr(), fingerprint) if len(setting.SSH.TrustedUserCAKeys) == 0 { log.Warn("Certificate Rejected: No trusted certificate authorities for this server") @@ -187,78 +185,68 @@ func publicKeyHandler(ctx context.Context, conn gossh.ConnMetadata, key gossh.Pu return nil, util.ErrPermissionDenied } - // look for the exact principal - principalLoop: + certChecker := &gossh.CertChecker{ + IsUserAuthority: func(auth gossh.PublicKey) bool { + marshaled := auth.Marshal() + for _, k := range setting.SSH.TrustedUserCAKeysParsed { + if bytes.Equal(marshaled, k.Marshal()) { + return true + } + } + return false + }, + } + + // check the CA of the cert + if !certChecker.IsUserAuthority(cert.SignatureKey) { + log.Warn("Principal Rejected: %s Untrusted Authority Signature Fingerprint %s", conn.RemoteAddr(), gossh.FingerprintSHA256(cert.SignatureKey)) + log.Warn("Failed authentication attempt from %s", conn.RemoteAddr()) + return nil, util.ErrPermissionDenied + } + + principalLoop: // look for the exact principal for _, principal := range cert.ValidPrincipals { - pkey, err := asymkey_model.SearchPublicKeyByContentExact(ctx, principal) + pkey, err := asymkey_model.SearchPrincipalKey(ctx, principal) if err != nil { if asymkey_model.IsErrKeyNotExist(err) { log.Debug("Principal Rejected: %s Unknown Principal: %s", conn.RemoteAddr(), principal) continue principalLoop } - log.Error("SearchPublicKeyByContentExact: %v", err) + log.Error("SearchPrincipalKey: %v", err) return nil, util.ErrPermissionDenied } - c := &gossh.CertChecker{ - IsUserAuthority: func(auth gossh.PublicKey) bool { - marshaled := auth.Marshal() - for _, k := range setting.SSH.TrustedUserCAKeysParsed { - if bytes.Equal(marshaled, k.Marshal()) { - return true - } - } - - return false - }, - } - - // check the CA of the cert - if !c.IsUserAuthority(cert.SignatureKey) { - if log.IsDebug() { - log.Debug("Principal Rejected: %s Untrusted Authority Signature Fingerprint %s for Principal: %s", conn.RemoteAddr(), gossh.FingerprintSHA256(cert.SignatureKey), principal) - } - continue principalLoop - } - // validate the cert for this principal - if err := c.CheckCert(principal, cert); err != nil { + if err := certChecker.CheckCert(principal, cert); err != nil { // User is presenting an invalid certificate - STOP any further processing - log.Error("Invalid Certificate KeyID %s with Signature Fingerprint %s presented for Principal: %s from %s", cert.KeyId, gossh.FingerprintSHA256(cert.SignatureKey), principal, conn.RemoteAddr()) + log.Warn("Invalid Certificate KeyID %s with Signature Fingerprint %s presented for Principal: %s from %s", cert.KeyId, gossh.FingerprintSHA256(cert.SignatureKey), principal, conn.RemoteAddr()) log.Warn("Failed authentication attempt from %s", conn.RemoteAddr()) - return nil, util.ErrPermissionDenied } - if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary - log.Debug("Successfully authenticated: %s Certificate Fingerprint: %s Principal: %s", conn.RemoteAddr(), gossh.FingerprintSHA256(key), principal) - } + log.Debug("Successfully authenticated: %s Certificate Fingerprint: %s Principal: %s", conn.RemoteAddr(), fingerprint, principal) return keyPermissions(pkey.ID), nil } - log.Warn("From %s Fingerprint: %s is a certificate, but no valid principals found", conn.RemoteAddr(), gossh.FingerprintSHA256(key)) + log.Warn("From %s Fingerprint: %s is a certificate, but no valid principals found", conn.RemoteAddr(), fingerprint) log.Warn("Failed authentication attempt from %s", conn.RemoteAddr()) return nil, util.ErrPermissionDenied } - if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary - log.Debug("Handle Public Key: %s Fingerprint: %s is not a certificate", conn.RemoteAddr(), gossh.FingerprintSHA256(key)) - } + log.Debug("Handle Public Key: %s Fingerprint: %s is not a certificate", conn.RemoteAddr(), fingerprint) - pkey, err := asymkey_model.SearchPublicKeyByContent(ctx, strings.TrimSpace(string(gossh.MarshalAuthorizedKey(key)))) + pkey, err := asymkey_model.SearchPublicKeyForSSH(ctx, sshPubKey) if err != nil { if asymkey_model.IsErrKeyNotExist(err) { - log.Warn("Unknown public key: %s from %s", gossh.FingerprintSHA256(key), conn.RemoteAddr()) + log.Warn("Unknown public key: %s from %s", fingerprint, conn.RemoteAddr()) log.Warn("Failed authentication attempt from %s", conn.RemoteAddr()) return nil, util.ErrPermissionDenied } - log.Error("SearchPublicKeyByContent: %v", err) + log.Error("SearchPublicKeyForSSH: %v", err) return nil, util.ErrPermissionDenied } - if log.IsDebug() { // <- FingerprintSHA256 is kinda expensive so only calculate it if necessary - log.Debug("Successfully authenticated: %s Public Key Fingerprint: %s", conn.RemoteAddr(), gossh.FingerprintSHA256(key)) - } + log.Debug("Successfully authenticated: %s Public Key Fingerprint: %s", conn.RemoteAddr(), fingerprint) return keyPermissions(pkey.ID), nil } diff --git a/routers/private/internal.go b/routers/private/internal.go index 4c04c8e45eb..4a6c473d193 100644 --- a/routers/private/internal.go +++ b/routers/private/internal.go @@ -78,7 +78,7 @@ func Routes() *web.Router { r.AfterRouting(common.AuditOrigin(audit_model.OriginSystem)) r.Get("/dummy", misc.DummyOK) - r.Post("/ssh/authorized_keys", AuthorizedPublicKeyByContent) + r.Post("/ssh/authorized_keys", AuthorizedPublicKeyForSSH) r.Post("/ssh/{id}/update/{repoid}", UpdatePublicKeyInRepo) r.Post("/ssh/log", bind(private.SSHLogOption{}), SSHLog) r.Post("/hook/pre-receive/{owner}/{repo}", RepoAssignment, bind(private.HookOptions{}), HookPreReceive) diff --git a/routers/private/key.go b/routers/private/key.go index 46bb311bb74..e42f354497d 100644 --- a/routers/private/key.go +++ b/routers/private/key.go @@ -37,12 +37,9 @@ func UpdatePublicKeyInRepo(ctx *context.PrivateContext) { ctx.PlainText(http.StatusOK, "success") } -// AuthorizedPublicKeyByContent searches content as prefix (without comment part) -// and returns public key found. -func AuthorizedPublicKeyByContent(ctx *context.PrivateContext) { +func AuthorizedPublicKeyForSSH(ctx *context.PrivateContext) { content := ctx.FormString("content") - - publicKey, err := asymkey_model.SearchPublicKeyByContent(ctx, content) + publicKey, err := asymkey_model.SearchPublicKeyForSSH(ctx, content) if err != nil { ctx.PrivateInternalErrorf("%v", err) return diff --git a/services/auth/httpsign.go b/services/auth/httpsign.go index 7ce7cc64216..c6048b2de7f 100644 --- a/services/auth/httpsign.go +++ b/services/auth/httpsign.go @@ -167,13 +167,13 @@ func VerifyCert(r *http.Request) (*asymkey_model.PublicKey, error) { // Now for each of the certificate valid principals for _, principal := range cert.ValidPrincipals { // Look in the db for the public key - publicKey, err := asymkey_model.SearchPublicKeyByContentExact(r.Context(), principal) + publicKey, err := asymkey_model.SearchPrincipalKey(r.Context(), principal) if asymkey_model.IsErrKeyNotExist(err) { // No public key matches this principal - try the next principal continue } else if err != nil { // this error will be a db error therefore we can't solve this and we should abort - log.Error("SearchPublicKeyByContentExact: %v", err) + log.Error("SearchPrincipalKey: %v", err) return nil, err }