Merge pull request 'modules/git: Recognize SSH signed tags too' (#2520) from algernon/forgejo:ssh-exclamation-mark-one-one-exclamation-mark-space-key into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2520
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

models/asymkey/ssh_key_commit_verification_test.go

@@ -40,7 +40,7 @@ func TestParseCommitWithSSHSignature(t *testing.T) {
 			Committer: &git.Signature{
 				Email: "non-existent",
 			},
-			Signature: &git.CommitGPGSignature{
+			Signature: &git.ObjectSignature{
 				Payload: `tree 2d491b2985a7ff848d5c02748e7ea9f9f7619f9f
 parent 45b03601635a1f463b81963a4022c7f87ce96ef9
 author user2 <non-existent> 1699710556 +0100
@@ -67,7 +67,7 @@ AAAAQIMufOuSjZeDUujrkVK4sl7ICa0WwEftas8UAYxx0Thdkiw2qWjR1U1PKfTLm16/w8
 			Committer: &git.Signature{
 				Email: "user2@example.com",
 			},
-			Signature: &git.CommitGPGSignature{
+			Signature: &git.ObjectSignature{
 				Payload: `tree 853694aae8816094a0d875fee7ea26278dbf5d0f
 parent c2780d5c313da2a947eae22efd7dacf4213f4e7f
 author user2 <user2@example.com> 1699707877 +0100
@@ -89,7 +89,7 @@ Add content
 			Committer: &git.Signature{
 				Email: "user2@example.com",
 			},
-			Signature: &git.CommitGPGSignature{
+			Signature: &git.ObjectSignature{
 				Payload: `tree 853694aae8816094a0d875fee7ea26278dbf5d0f
 parent c2780d5c313da2a947eae22efd7dacf4213f4e7f
 author user2 <user2@example.com> 1699707877 +0100
@@ -120,7 +120,7 @@ fs9cMpZVM9BfIKNUSO8QY=
 			Committer: &git.Signature{
 				Email: "user2@noreply.example.com",
 			},
-			Signature: &git.CommitGPGSignature{
+			Signature: &git.ObjectSignature{
 				Payload: `tree 4836c7f639f37388bab4050ef5c97bbbd54272fc
 parent 795be1b0117ea5c65456050bb9fd84744d4fd9c6
 author user2 <user2@noreply.example.com> 1699709594 +0100

models/forgejo_migrations/migrate.go

@@ -52,6 +52,8 @@ var migrations = []*Migration{
 	NewMigration("Add wiki_branch to repository", forgejo_v1_22.AddWikiBranchToRepository),
 	// v6 -> v7
 	NewMigration("Add enable_repo_unit_hints to the user table", forgejo_v1_22.AddUserRepoUnitHintsSetting),
+	// v7 -> v8
+	NewMigration("Remove SSH signatures from Release notes", forgejo_v1_22.RemoveSSHSignaturesFromReleaseNotes),
 }
 
 // GetCurrentDBVersion returns the current Forgejo database version.

models/forgejo_migrations/v1_22/main_test.go

@@ -0,0 +1,14 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_22 //nolint
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/migrations/base"
+)
+
+func TestMain(m *testing.M) {
+	base.MainTest(m)
+}

models/forgejo_migrations/v1_22/v8.go

@@ -0,0 +1,51 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_22 //nolint
+
+import (
+	"strings"
+
+	"xorm.io/xorm"
+)
+
+func RemoveSSHSignaturesFromReleaseNotes(x *xorm.Engine) error {
+	type Release struct {
+		ID   int64  `xorm:"pk autoincr"`
+		Note string `xorm:"TEXT"`
+	}
+
+	if err := x.Sync(&Release{}); err != nil {
+		return err
+	}
+
+	var releaseNotes []struct {
+		ID   int64
+		Note string
+	}
+
+	if err := x.Table("release").Where("note LIKE '%-----BEGIN SSH SIGNATURE-----%'").Find(&releaseNotes); err != nil {
+		return err
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+
+	for _, release := range releaseNotes {
+		idx := strings.LastIndex(release.Note, "-----BEGIN SSH SIGNATURE-----")
+		if idx == -1 {
+			continue
+		}
+		release.Note = release.Note[:idx]
+		_, err := sess.Exec("UPDATE `release` SET note = ? WHERE id = ?", release.Note, release.ID)
+		if err != nil {
+			return err
+		}
+	}
+
+	return sess.Commit()
+}

models/forgejo_migrations/v1_22/v8_test.go

@@ -0,0 +1,34 @@
+// Copyright 2024 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_22 //nolint
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/migrations/base"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_RemoveSSHSignaturesFromReleaseNotes(t *testing.T) {
+	// A reduced mock of the `repo_model.Release` struct.
+	type Release struct {
+		ID   int64  `xorm:"pk autoincr"`
+		Note string `xorm:"TEXT"`
+	}
+
+	x, deferable := base.PrepareTestEnv(t, 0, new(Release))
+	defer deferable()
+
+	assert.NoError(t, RemoveSSHSignaturesFromReleaseNotes(x))
+
+	var releases []Release
+	err := x.Table("release").OrderBy("id ASC").Find(&releases)
+	assert.NoError(t, err)
+	assert.Len(t, releases, 3)
+
+	assert.Equal(t, "", releases[0].Note)
+	assert.Equal(t, "A message.\n", releases[1].Note)
+	assert.Equal(t, "no signature present here", releases[2].Note)
+}

models/migrations/fixtures/Test_RemoveSSHSignaturesFromReleaseNotes/release.yml

@@ -0,0 +1,22 @@
+# type Release struct {
+#   ID   int64  `xorm:"pk autoincr"`
+#   Note string `xorm:"TEXT"`
+# }
+-
+  id: 1
+  note: |
+    -----BEGIN SSH SIGNATURE-----
+    some signature
+    -----END SSH SIGNATURE-----
+
+-
+  id: 2
+  note: |
+    A message.
+    -----BEGIN SSH SIGNATURE-----
+    some signature
+    -----END SSH SIGNATURE-----
+
+-
+  id: 3
+  note: "no signature present here"

modules/git/commit.go

@@ -25,18 +25,12 @@ type Commit struct {
 	Author        *Signature
 	Committer     *Signature
 	CommitMessage string
-	Signature     *CommitGPGSignature
+	Signature     *ObjectSignature
 
 	Parents        []ObjectID // ID strings
 	submoduleCache *ObjectCache
 }
 
-// CommitGPGSignature represents a git commit signature part.
-type CommitGPGSignature struct {
-	Signature string
-	Payload   string // TODO check if can be reconstruct from the rest of commit information to not have duplicate data
-}
-
 // Message returns the commit message. Same as retrieving CommitMessage directly.
 func (c *Commit) Message() string {
 	return c.CommitMessage

modules/git/commit_convert_gogit.go

@@ -13,7 +13,7 @@ import (
 	"github.com/go-git/go-git/v5/plumbing/object"
 )
 
-func convertPGPSignature(c *object.Commit) *CommitGPGSignature {
+func convertPGPSignature(c *object.Commit) *ObjectSignature {
 	if c.PGPSignature == "" {
 		return nil
 	}
@@ -51,7 +51,7 @@ func convertPGPSignature(c *object.Commit) *CommitGPGSignature {
 		return nil
 	}
 
-	return &CommitGPGSignature{
+	return &ObjectSignature{
 		Signature: c.PGPSignature,
 		Payload:   w.String(),
 	}

modules/git/commit_reader.go

@@ -97,7 +97,7 @@ readLoop:
 		}
 	}
 	commit.CommitMessage = messageSB.String()
-	commit.Signature = &CommitGPGSignature{
+	commit.Signature = &ObjectSignature{
 		Signature: signatureSB.String(),
 		Payload:   payloadSB.String(),
 	}

modules/git/object_signature.go

@@ -0,0 +1,11 @@
+// Copyright 2015 The Gogs Authors. All rights reserved.
+// Copyright 2019 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package git
+
+// ObjectSignature represents a git object (commit, tag) signature part.
+type ObjectSignature struct {
+	Signature string
+	Payload   string // TODO check if can be reconstruct from the rest of commit information to not have duplicate data
+}

modules/git/repo_tag.go

@@ -185,17 +185,22 @@ func parseTagRef(ref map[string]string) (tag *Tag, err error) {
 
 	tag.Tagger = parseSignatureFromCommitLine(ref["creator"])
 	tag.Message = ref["contents"]
-	// strip PGP signature if present in contents field
+	// strip the signature if present in contents field
 	pgpStart := strings.Index(tag.Message, beginpgp)
 	if pgpStart >= 0 {
 		tag.Message = tag.Message[0:pgpStart]
+	} else {
+		sshStart := strings.Index(tag.Message, beginssh)
+		if sshStart >= 0 {
+			tag.Message = tag.Message[0:sshStart]
+		}
 	}
 
-	// annotated tag with GPG signature
+	// annotated tag with signature
 	if tag.Type == "tag" && ref["contents:signature"] != "" {
 		payload := fmt.Sprintf("object %s\ntype commit\ntag %s\ntagger %s\n\n%s\n",
 			tag.Object, tag.Name, ref["creator"], strings.TrimSpace(tag.Message))
-		tag.Signature = &CommitGPGSignature{
+		tag.Signature = &ObjectSignature{
 			Signature: ref["contents:signature"],
 			Payload:   payload,
 		}

modules/git/repo_tag_test.go

@@ -315,7 +315,7 @@ qbHDASXl
 				Type:    "tag",
 				Tagger:  parseSignatureFromCommitLine("Foo Bar <foo@bar.com> 1565789218 +0300"),
 				Message: "Add changelog of v1.9.1 (#7859)\n\n* add changelog of v1.9.1\n* Update CHANGELOG.md",
-				Signature: &CommitGPGSignature{
+				Signature: &ObjectSignature{
 					Signature: `-----BEGIN PGP SIGNATURE-----
 
 aBCGzBAABCgAdFiEEyWRwv/q1Q6IjSv+D4IPOwzt33PoFAmI8jbIACgkQ4IPOwzt3

modules/git/tag.go

@@ -14,6 +14,8 @@ import (
 const (
 	beginpgp = "\n-----BEGIN PGP SIGNATURE-----\n"
 	endpgp   = "\n-----END PGP SIGNATURE-----"
+	beginssh = "\n-----BEGIN SSH SIGNATURE-----\n"
+	endssh   = "\n-----END SSH SIGNATURE-----"
 )
 
 // Tag represents a Git tag.
@@ -24,7 +26,7 @@ type Tag struct {
 	Type      string
 	Tagger    *Signature
 	Message   string
-	Signature *CommitGPGSignature
+	Signature *ObjectSignature
 }
 
 // Commit return the commit of the tag reference
@@ -71,17 +73,36 @@ l:
 			break l
 		}
 	}
-	idx := strings.LastIndex(tag.Message, beginpgp)
-	if idx > 0 {
-		endSigIdx := strings.Index(tag.Message[idx:], endpgp)
-		if endSigIdx > 0 {
-			tag.Signature = &CommitGPGSignature{
-				Signature: tag.Message[idx+1 : idx+endSigIdx+len(endpgp)],
-				Payload:   string(data[:bytes.LastIndex(data, []byte(beginpgp))+1]),
-			}
-			tag.Message = tag.Message[:idx+1]
+
+	extractTagSignature := func(signatureBeginMark, signatureEndMark string) (bool, *ObjectSignature, string) {
+		idx := strings.LastIndex(tag.Message, signatureBeginMark)
+		if idx == -1 {
+			return false, nil, ""
 		}
+
+		endSigIdx := strings.Index(tag.Message[idx:], signatureEndMark)
+		if endSigIdx == -1 {
+			return false, nil, ""
+		}
+
+		return true, &ObjectSignature{
+			Signature: tag.Message[idx+1 : idx+endSigIdx+len(signatureEndMark)],
+			Payload:   string(data[:bytes.LastIndex(data, []byte(signatureBeginMark))+1]),
+		}, tag.Message[:idx+1]
 	}
+
+	// Try to find an OpenPGP signature
+	found, sig, message := extractTagSignature(beginpgp, endpgp)
+	if !found {
+		// If not found, try an SSH one
+		found, sig, message = extractTagSignature(beginssh, endssh)
+	}
+	// If either is found, update the tag Signature and Message
+	if found {
+		tag.Signature = sig
+		tag.Message = message
+	}
+
 	return tag, nil
 }
 

modules/git/tag_test.go

@@ -46,6 +46,41 @@ ono`), tag: Tag{
 			Message:   "test message\no\n\nono",
 			Signature: nil,
 		}},
+		{data: []byte(`object d8d1fdb5b20eaca882e34ee510eb55941a242b24
+type commit
+tag v0
+tagger Jane Doe <jane.doe@example.com> 1709146405 +0100
+
+v0
+-----BEGIN SSH SIGNATURE-----
+U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgvD4pK7baygXxoWoVoKjVEc/xZh
+6w+1FUn5hypFqJXNAAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5
+AAAAQKFeTnxi9ssRqSg+sJcmjAgpgoPq1k5SXm306+mJmkPwvhim8f9Gz6uy1AddPmXaD7
+5LVB3fV2GmmFDKGB+wCAo=
+-----END SSH SIGNATURE-----
+`), tag: Tag{
+			Name:    "",
+			ID:      Sha1ObjectFormat.EmptyObjectID(),
+			Object:  &Sha1Hash{0xd8, 0xd1, 0xfd, 0xb5, 0xb2, 0x0e, 0xac, 0xa8, 0x82, 0xe3, 0x4e, 0xe5, 0x10, 0xeb, 0x55, 0x94, 0x1a, 0x24, 0x2b, 0x24},
+			Type:    "commit",
+			Tagger:  &Signature{Name: "Jane Doe", Email: "jane.doe@example.com", When: time.Unix(1709146405, 0)},
+			Message: "v0\n",
+			Signature: &ObjectSignature{
+				Signature: `-----BEGIN SSH SIGNATURE-----
+U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgvD4pK7baygXxoWoVoKjVEc/xZh
+6w+1FUn5hypFqJXNAAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5
+AAAAQKFeTnxi9ssRqSg+sJcmjAgpgoPq1k5SXm306+mJmkPwvhim8f9Gz6uy1AddPmXaD7
+5LVB3fV2GmmFDKGB+wCAo=
+-----END SSH SIGNATURE-----`,
+				Payload: `object d8d1fdb5b20eaca882e34ee510eb55941a242b24
+type commit
+tag v0
+tagger Jane Doe <jane.doe@example.com> 1709146405 +0100
+
+v0
+`,
+			},
+		}},
 	}
 
 	for _, test := range testData {