Skip to content

Commit 6599efb

Browse files
wxiaoguangwxiaoguang
authored
Fix user's sign email check (#35045)
Fix #21692
1 parent 6090d70 commit 6599efb

File tree

4 files changed

+55
-30
lines changed

4 files changed

+55
-30
lines changed

‎models/asymkey/ssh_key_fingerprint.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@ import (
1313
"xorm.io/builder"
1414
)
1515

16-
// The database is used in checkKeyFingerprint however most of these functions probably belong in a module
16+
// The database is used in checkKeyFingerprint. However, most of these functions probably belong in a module
1717

18-
// checkKeyFingerprint only checks if key fingerprint has been used as public key,
18+
// checkKeyFingerprint only checks if key fingerprint has been used as a public key,
1919
// it is OK to use same key as deploy key for multiple repositories/users.
2020
func checkKeyFingerprint(ctx context.Context, fingerprint string) error {
2121
has, err := db.Exist[PublicKey](ctx, builder.Eq{"fingerprint": fingerprint})

‎models/fixtures/public_key.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,4 @@
99
created_unix: 1559593109
1010
updated_unix: 1565224552
1111
login_source_id: 0
12+
verified: false

‎services/asymkey/commit.go

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,9 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *asymkey_model
3636
}
3737

3838
// ParseCommitWithSignatureCommitter parses a commit's GPG or SSH signature.
39+
// The caller guarantees that the committer user is related to the commit by checking its activated email addresses or no-reply address.
3940
// If the commit is singed by an instance key, then committer can be nil.
40-
// If the signature exists, even if committer is nil, the returned CommittingUser will be a non-nil fake user.
41+
// If the signature exists, even if committer is nil, the returned CommittingUser will be a non-nil fake user (e.g.: instance key)
4142
func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, committer *user_model.User) *asymkey_model.CommitVerification {
4243
// If no signature, just report the committer
4344
if c.Signature == nil {
@@ -114,20 +115,11 @@ func parseCommitWithGPGSignature(ctx context.Context, c *git.Commit, committer *
114115
}
115116
}
116117

117-
committerEmailAddresses, _ := cache.GetWithContextCache(ctx, cachegroup.UserEmailAddresses, committer.ID, user_model.GetEmailAddresses)
118-
activated := false
119-
for _, e := range committerEmailAddresses {
120-
if e.IsActivated && strings.EqualFold(e.Email, c.Committer.Email) {
121-
activated = true
122-
break
123-
}
124-
}
125-
126118
for _, k := range keys {
127119
// Pre-check (& optimization) that emails attached to key can be attached to the committer email and can validate
128120
canValidate := false
129121
email := ""
130-
if k.Verified && activated {
122+
if k.Verified {
131123
canValidate = true
132124
email = c.Committer.Email
133125
}
@@ -217,8 +209,8 @@ func checkKeyEmails(ctx context.Context, email string, keys ...*asymkey_model.GP
217209
return true, e.Email
218210
}
219211
}
220-
if user.KeepEmailPrivate && strings.EqualFold(email, user.GetEmail()) {
221-
return true, user.GetEmail()
212+
if user != nil && strings.EqualFold(email, user.GetPlaceholderEmail()) {
213+
return true, user.GetPlaceholderEmail()
222214
}
223215
}
224216
}
@@ -388,21 +380,8 @@ func parseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committerUs
388380
}
389381
}
390382

391-
committerEmailAddresses, err := cache.GetWithContextCache(ctx, cachegroup.UserEmailAddresses, committerUser.ID, user_model.GetEmailAddresses)
392-
if err != nil {
393-
log.Error("GetEmailAddresses: %v", err)
394-
}
395-
396-
activated := false
397-
for _, e := range committerEmailAddresses {
398-
if e.IsActivated && strings.EqualFold(e.Email, c.Committer.Email) {
399-
activated = true
400-
break
401-
}
402-
}
403-
404383
for _, k := range keys {
405-
if k.Verified && activated {
384+
if k.Verified {
406385
commitVerification := verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, k, committerUser, committerUser, c.Committer.Email)
407386
if commitVerification != nil {
408387
return commitVerification

‎services/asymkey/commit_test.go

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ import (
77
"strings"
88
"testing"
99

10+
asymkey_model "code.gitea.io/gitea/models/asymkey"
11+
"code.gitea.io/gitea/models/db"
12+
"code.gitea.io/gitea/models/unittest"
1013
user_model "code.gitea.io/gitea/models/user"
1114
"code.gitea.io/gitea/modules/git"
1215
"code.gitea.io/gitea/modules/setting"
@@ -17,7 +20,49 @@ import (
1720
)
1821

1922
func TestParseCommitWithSSHSignature(t *testing.T) {
20-
// Here we only test the TrustedSSHKeys. The complete signing test is in tests/integration/gpg_ssh_git_test.go
23+
assert.NoError(t, unittest.PrepareTestDatabase())
24+
25+
// Here we only need to do some tests that "tests/integration/gpg_ssh_git_test.go" doesn't cover
26+
27+
// -----BEGIN OPENSSH PRIVATE KEY-----
28+
// b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
29+
// QyNTUxOQAAACC6T6zF0oPak8dOIzzT1kXB7LrcsVo04SKc3GjuvMllZwAAAJgy08upMtPL
30+
// qQAAAAtzc2gtZWQyNTUxOQAAACC6T6zF0oPak8dOIzzT1kXB7LrcsVo04SKc3GjuvMllZw
31+
// AAAEDWqPHTH51xb4hy1y1f1VeWL/2A9Q0b6atOyv5fx8x5prpPrMXSg9qTx04jPNPWRcHs
32+
// utyxWjThIpzcaO68yWVnAAAAEXVzZXIyQGV4YW1wbGUuY29tAQIDBA==
33+
// -----END OPENSSH PRIVATE KEY-----
34+
sshPubKey, err := asymkey_model.AddPublicKey(t.Context(), 999, "user-ssh-key-any-name", "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILpPrMXSg9qTx04jPNPWRcHsutyxWjThIpzcaO68yWVn", 0)
35+
require.NoError(t, err)
36+
_, err = db.GetEngine(t.Context()).ID(sshPubKey.ID).Cols("verified").Update(&asymkey_model.PublicKey{Verified: true})
37+
require.NoError(t, err)
38+
39+
t.Run("UserSSHKey", func(t *testing.T) {
40+
commit, err := git.CommitFromReader(nil, git.Sha1ObjectFormat.EmptyObjectID(), strings.NewReader(`tree a3b1fad553e0f9a2b4a58327bebde36c7da75aa2
41+
author user2 <[email protected]> 1752194028 -0700
42+
committer user2 <[email protected]> 1752194028 -0700
43+
gpgsig -----BEGIN SSH SIGNATURE-----
44+
U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAguk+sxdKD2pPHTiM809ZFwey63L
45+
FaNOEinNxo7rzJZWcAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5
46+
AAAAQBfX+6mcKZBnXckwHcBFqRuXMD3vTKi1yv5wgrqIxTyr2LWB97xxmO92cvjsr0POQ2
47+
2YA7mQS510Cg2s1uU1XAk=
48+
-----END SSH SIGNATURE-----
49+
50+
init project
51+
`))
52+
require.NoError(t, err)
53+
54+
// the committingUser is guaranteed by the caller, parseCommitWithSSHSignature doesn't do any more checks
55+
committingUser := &user_model.User{ID: 999, Name: "user-x"}
56+
ret := parseCommitWithSSHSignature(t.Context(), commit, committingUser)
57+
require.NotNil(t, ret)
58+
assert.True(t, ret.Verified)
59+
assert.Equal(t, committingUser.Name+" / "+sshPubKey.Fingerprint, ret.Reason)
60+
assert.False(t, ret.Warning)
61+
assert.Equal(t, committingUser, ret.SigningUser)
62+
assert.Equal(t, committingUser, ret.CommittingUser)
63+
assert.Equal(t, sshPubKey.ID, ret.SigningSSHKey.ID)
64+
})
65+
2166
t.Run("TrustedSSHKey", func(t *testing.T) {
2267
defer test.MockVariableValue(&setting.Repository.Signing.SigningName, "gitea")()
2368
defer test.MockVariableValue(&setting.Repository.Signing.SigningEmail, "[email protected]")()

0 commit comments

Comments
 (0)