refactor ParseCommitWithSSHSignature

Author: wxiaoguang

models/fixtures/branch.yml

@@ -201,15 +201,3 @@
   is_deleted: false
   deleted_by_id: 0
   deleted_unix: 0
-
--
-  id: 25
-  repo_id: 63
-  name: 'master'
-  commit_id: 'b7fca3aa0d80144f9718b0486681944f9c587e33'
-  commit_message: "Initial commit with signed file"
-  commit_time: 1602935385
-  pusher_id: 2
-  is_deleted: false
-  deleted_by_id: 0
-  deleted_unix: 0

models/fixtures/repo_unit.yml

@@ -733,44 +733,3 @@
   type: 3
   config: "{\"IgnoreWhitespaceConflicts\":false,\"AllowMerge\":true,\"AllowRebase\":true,\"AllowRebaseMerge\":true,\"AllowSquash\":true}"
   created_unix: 946684810
-
--
-  id: 111
-  repo_id: 63
-  type: 4
-  config: "{}"
-  created_unix: 946684810
-
--
-  id: 112
-  repo_id: 63
-  type: 5
-  config: "{}"
-  created_unix: 946684810
-
--
-  id: 113
-  repo_id: 63
-  type: 1
-  config: "{}"
-  created_unix: 946684810
-
--
-  id: 114
-  repo_id: 63
-  type: 2
-  config: "{}"
-  created_unix: 946684810
-
--
-  id: 115
-  repo_id: 63
-  type: 8
-  created_unix: 946684810
-
--
-  id: 116
-  repo_id: 63
-  type: 3
-  config: "{}"
-  created_unix: 946684810

models/fixtures/repository.yml

@@ -1786,34 +1786,3 @@
   size: 0
   is_fsck_enabled: true
   close_issues_via_commit_in_any_branch: false
-
--
-  id: 63
-  owner_id: 2
-  owner_name: user2
-  lower_name: repo-test-trusted-ssh-keys
-  name: repo-test-trusted-ssh-keys
-  default_branch: master
-  num_watches: 0
-  num_stars: 0
-  num_forks: 0
-  num_issues: 0
-  num_closed_issues: 0
-  num_pulls: 0
-  num_closed_pulls: 0
-  num_milestones: 0
-  num_closed_milestones: 0
-  num_projects: 0
-  num_closed_projects: 0
-  is_private: false
-  is_empty: false
-  is_archived: false
-  is_mirror: false
-  status: 0
-  is_fork: false
-  fork_id: 0
-  is_template: false
-  template_id: 0
-  size: 0
-  is_fsck_enabled: true
-  close_issues_via_commit_in_any_branch: false

models/fixtures/user.yml

@@ -67,7 +67,7 @@
   num_followers: 2
   num_following: 1
   num_stars: 2
-  num_repos: 15
+  num_repos: 14
   num_teams: 0
   num_members: 0
   visibility: 0

models/repo/repo_list_test.go

@@ -138,27 +138,27 @@ func getTestCases() []struct {
 		{
 			name:  "AllPublic/PublicRepositoriesOfUserIncludingCollaborative",
 			opts:  repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, AllPublic: true, Template: optional.Some(false)},
-			count: 35,
+			count: 34,
 		},
 		{
 			name:  "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative",
 			opts:  repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true, AllLimited: true, Template: optional.Some(false)},
-			count: 40,
+			count: 39,
 		},
 		{
 			name:  "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName",
 			opts:  repo_model.SearchRepoOptions{Keyword: "test", ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true},
-			count: 16,
+			count: 15,
 		},
 		{
 			name:  "AllPublic/PublicAndPrivateRepositoriesOfUser2IncludingCollaborativeByName",
 			opts:  repo_model.SearchRepoOptions{Keyword: "test", ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 18, Private: true, AllPublic: true},
-			count: 14,
+			count: 13,
 		},
 		{
 			name:  "AllPublic/PublicRepositoriesOfOrganization",
 			opts:  repo_model.SearchRepoOptions{ListOptions: db.ListOptions{Page: 1, PageSize: 10}, OwnerID: 17, AllPublic: true, Collaborate: optional.Some(false), Template: optional.Some(false)},
-			count: 35,
+			count: 34,
 		},
 		{
 			name:  "AllTemplates",

services/asymkey/commit.go

@@ -6,6 +6,7 @@ package asymkey
 import (
 	"context"
 	"fmt"
+	"slices"
 	"strings"
 
 	asymkey_model "code.gitea.io/gitea/models/asymkey"
@@ -359,24 +360,39 @@ func VerifyWithGPGSettings(ctx context.Context, gpgSettings *git.GPGSettings, si
 	return nil
 }
 
+func verifySSHCommitVerificationByInstanceKey(c *git.Commit, committerUser, signerUser *user_model.User, committerGitEmail, publicKeyContent string) *asymkey_model.CommitVerification {
+	fingerprint, err := asymkey_model.CalcFingerprint(publicKeyContent)
+	if err != nil {
+		log.Error("Error calculating the fingerprint public key %q, err: %v", publicKeyContent, err)
+		return nil
+	}
+	sshPubKey := &asymkey_model.PublicKey{
+		Verified:    true,
+		Content:     publicKeyContent,
+		Fingerprint: fingerprint,
+		HasUsed:     true,
+	}
+	return verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, sshPubKey, committerUser, signerUser, committerGitEmail)
+}
+
 // ParseCommitWithSSHSignature check if signature is good against keystore.
-func ParseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committer *user_model.User) *asymkey_model.CommitVerification {
+func ParseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committerUser *user_model.User) *asymkey_model.CommitVerification {
 	// Now try to associate the signature with the committer, if present
-	if committer.ID != 0 {
+	if committerUser.ID != 0 {
 		keys, err := db.Find[asymkey_model.PublicKey](ctx, asymkey_model.FindPublicKeyOptions{
-			OwnerID:    committer.ID,
+			OwnerID:    committerUser.ID,
 			NotKeytype: asymkey_model.KeyTypePrincipal,
 		})
 		if err != nil { // Skipping failed to get ssh keys of user
 			log.Error("ListPublicKeys: %v", err)
 			return &asymkey_model.CommitVerification{
-				CommittingUser: committer,
+				CommittingUser: committerUser,
 				Verified:       false,
 				Reason:         "gpg.error.failed_retrieval_gpg_keys",
 			}
 		}
 
-		committerEmailAddresses, err := cache.GetWithContextCache(ctx, cachegroup.UserEmailAddresses, committer.ID, user_model.GetEmailAddresses)
+		committerEmailAddresses, err := cache.GetWithContextCache(ctx, cachegroup.UserEmailAddresses, committerUser.ID, user_model.GetEmailAddresses)
 		if err != nil {
 			log.Error("GetEmailAddresses: %v", err)
 		}
@@ -391,64 +407,51 @@ func ParseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committer *
 
 		for _, k := range keys {
 			if k.Verified && activated {
-				commitVerification := verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, k, committer, committer, c.Committer.Email)
+				commitVerification := verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, k, committerUser, committerUser, c.Committer.Email)
 				if commitVerification != nil {
 					return commitVerification
 				}
 			}
 		}
 	}
-	// Trust more than one key for every User
+
+	// Try the pre-set trusted keys (for key-rotation purpose)
 	for _, k := range setting.Repository.Signing.TrustedSSHKeys {
-		if fingerprint, err := asymkey_model.CalcFingerprint(k); err != nil {
-			log.Error("Error calculating the fingerprint public key: %s %v", k, err)
-		} else if commitVerification := verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, &asymkey_model.PublicKey{
-			Verified:    true,
-			Content:     k,
-			Fingerprint: fingerprint,
-			HasUsed:     true,
-		}, committer, committer, c.Committer.Email); commitVerification != nil {
+		// FIXME: why here uses "commiterUser" as "signerUser" but below don't? why here uses "c.Committer.Email" but below uses "gpgSettings.Email"?
+		signerUser := committerUser
+		commitVerification := verifySSHCommitVerificationByInstanceKey(c, committerUser, signerUser, c.Committer.Email, k)
+		if commitVerification != nil && commitVerification.Verified {
 			return commitVerification
 		}
 	}
 
-	defaultReason := asymkey_model.NoKeyFound
-
-	// Covers ssh verification for the default SSH signing key specified in gitea config
-	if setting.Repository.Signing.SigningFormat == git.KeyTypeSSH && setting.Repository.Signing.SigningKey != "" && setting.Repository.Signing.SigningKey != "default" && setting.Repository.Signing.SigningKey != "none" {
-		// OK we should try the default key
+	// Try the configured instance-wide SSH public key
+	if setting.Repository.Signing.SigningFormat == git.KeyTypeSSH && !slices.Contains([]string{"", "default", "none"}, setting.Repository.Signing.SigningKey) {
 		gpgSettings := git.GPGSettings{
 			Sign:   true,
 			KeyID:  setting.Repository.Signing.SigningKey,
 			Name:   setting.Repository.Signing.SigningName,
 			Email:  setting.Repository.Signing.SigningEmail,
 			Format: setting.Repository.Signing.SigningFormat,
 		}
-		if err := gpgSettings.LoadPublicKeyContent(); err != nil {
-			log.Error("Error getting default signing key: %s %v", gpgSettings.KeyID, err)
-		} else if fingerprint, err := asymkey_model.CalcFingerprint(gpgSettings.PublicKeyContent); err != nil {
-			log.Error("Error calculating the fingerprint public key: %s %v", gpgSettings.KeyID, err)
-		} else if commitVerification := verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, &asymkey_model.PublicKey{
-			Verified:    true,
-			Content:     gpgSettings.PublicKeyContent,
-			Fingerprint: fingerprint,
-			HasUsed:     true,
-		}, committer, &user_model.User{
+		signerUser := &user_model.User{
 			Name:  gpgSettings.Name,
 			Email: gpgSettings.Email,
-		}, gpgSettings.Email); commitVerification != nil {
-			if commitVerification.Reason == asymkey_model.BadSignature {
-				defaultReason = asymkey_model.BadSignature
-			} else {
+		}
+		if err := gpgSettings.LoadPublicKeyContent(); err != nil {
+			log.Error("Error getting instance-wide SSH signing key %q, err: %v", gpgSettings.KeyID, err)
+		} else {
+			commitVerification := verifySSHCommitVerificationByInstanceKey(c, committerUser, signerUser, gpgSettings.Email, gpgSettings.PublicKeyContent)
+			if commitVerification != nil && commitVerification.Verified {
 				return commitVerification
 			}
 		}
 	}
 
 	return &asymkey_model.CommitVerification{
-		CommittingUser: committer,
+		CommittingUser: committerUser,
 		Verified:       false,
-		Reason:         defaultReason,
+		Reason:         asymkey_model.NoKeyFound,
 	}
 }
 

services/asymkey/commit_test.go

@@ -0,0 +1,44 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package asymkey
+
+import (
+	"strings"
+	"testing"
+
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseCommitWithSSHSignature(t *testing.T) {
+	// Here we only test the TrustedSSHKeys. The complete signing test is in tests/integration/gpg_ssh_git_test.go
+	t.Run("TrustedSSHKey", func(t *testing.T) {
+		defer test.MockVariableValue(&setting.Repository.Signing.TrustedSSHKeys, []string{"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIH6Y4idVaW3E+bLw1uqoAfJD7o5Siu+HqS51E9oQLPE9"})()
+
+		commit, err := git.CommitFromReader(nil, git.Sha1ObjectFormat.EmptyObjectID(), strings.NewReader(`tree 9a93ffa76e8b72bdb6431910b3a506fa2b39f42e
+author User Two <[email protected]> 1749230009 +0200
+committer User Two <[email protected]> 1749230009 +0200
+gpgsig -----BEGIN SSH SIGNATURE-----
+ U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgfpjiJ1VpbcT5svDW6qgB8kPujl
+ KK74epLnUT2hAs8T0AAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5
+ AAAAQDX2t2iHuuLxEWHLJetYXKsgayv3c43r0pJNfAzdLN55Q65pC5M7rG6++gT2bxcpOu
+ Y6EXbpLqia9sunEF3+LQY=
+ -----END SSH SIGNATURE-----
+
+Initial commit with signed file
+`))
+		require.NoError(t, err)
+		ret := ParseCommitWithSSHSignature(t.Context(), commit, &user_model.User{Name: "foo", Email: "[email protected]"})
+		require.NotNil(t, ret)
+		assert.True(t, ret.Verified)
+		assert.False(t, ret.Warning)
+		assert.NotNil(t, ret.CommittingUser) // FIXME: test the CommittingUser and SigningUser correctly
+		assert.NotNil(t, ret.SigningUser)    // FIXME: test the CommittingUser and SigningUser correctly
+	})
+}

tests/gitea-repositories-meta/user2/repo-test-trusted-ssh-keys.git/HEAD

@@ -94,9 +94,9 @@ func TestAPISearchRepo(t *testing.T) {
 	}{
 		{
 			name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50&private=false", expectedResults: expectedResults{
-				nil:   {count: 37},
-				user:  {count: 37},
-				user2: {count: 37},
+				nil:   {count: 36},
+				user:  {count: 36},
+				user2: {count: 36},
 			},
 		},
 		{

tests/gitea-repositories-meta/user2/repo-test-trusted-ssh-keys.git/config

@@ -300,23 +300,3 @@ func importTestingKey() (*openpgp.Entity, error) {
 	// There should only be one entity in this file.
 	return keyring[0], nil
 }
-
-func TestTrustedSSHKeys(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-	defer test.MockVariableValue(&setting.Repository.Signing.TrustedSSHKeys, []string{"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIH6Y4idVaW3E+bLw1uqoAfJD7o5Siu+HqS51E9oQLPE9"})()
-
-	username := "user2"
-	testCtx := NewAPITestContext(t, username, "repo-test-trusted-ssh-keys", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
-	t.Run("CheckMasterBranchSignedVerified", doAPIGetBranch(testCtx, "master", func(t *testing.T, branch api.Branch) {
-		require.NotNil(t, branch.Commit, "no commit provided with branch! %v", branch)
-		require.NotNil(t, branch.Commit.Verification, "no verification provided with branch commit! %v", branch.Commit)
-		require.True(t, branch.Commit.Verification.Verified)
-	}))
-
-	setting.Repository.Signing.TrustedSSHKeys = []string{}
-	t.Run("CheckMasterBranchSignedUnverified", doAPIGetBranch(testCtx, "master", func(t *testing.T, branch api.Branch) {
-		require.NotNil(t, branch.Commit, "no commit provided with branch! %v", branch)
-		require.NotNil(t, branch.Commit.Verification, "no verification provided with branch commit! %v", branch.Commit)
-		require.False(t, branch.Commit.Verification.Verified)
-	}))
-}

tests/gitea-repositories-meta/user2/repo-test-trusted-ssh-keys.git/description

@@ -691,10 +691,6 @@ func TestOAuth_GrantScopesReadRepositoryFailOrganization(t *testing.T) {
 			FullRepoName: "user2/commitsonpr",
 			Private:      false,
 		},
-		{
-			FullRepoName: "user2/repo-test-trusted-ssh-keys",
-			Private:      false,
-		},
 	}
 	assert.Equal(t, reposExpected, reposCaptured)