From 26ed995290c4bef8656a8bea3bfb50c1d7c60b7e Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 28 Feb 2024 12:52:21 +0100 Subject: [PATCH 1/3] modules/git: Recognize SSH signed tags too Just like commits, tags can be signed with either an OpenPGP, or with an SSH key. While the latter is supported already, SSH-signed tags have not been. This patch teaches the git module to recognize and handle SSH-signed tags too. This will stop the signatures appearing in release notes, but are currently unused otherwise. Signed-off-by: Gergely Nagy --- modules/git/repo_tag.go | 7 ++++++- modules/git/tag.go | 39 ++++++++++++++++++++++++++++++--------- modules/git/tag_test.go | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index e8c5ce6fb8..eab4ca5a57 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -185,10 +185,15 @@ 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 diff --git a/modules/git/tag.go b/modules/git/tag.go index 94e5cd7c63..66dc9cc08f 100644 --- a/modules/git/tag.go +++ b/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. @@ -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, *CommitGPGSignature, 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, &CommitGPGSignature{ + 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 } diff --git a/modules/git/tag_test.go b/modules/git/tag_test.go index f980b0c560..d71501929c 100644 --- a/modules/git/tag_test.go +++ b/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 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: &CommitGPGSignature{ + 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 1709146405 +0100 + +v0 +`, + }, + }}, } for _, test := range testData { From 8fdffc94ca5ceb888cc721c2ff5d9682d95f994e Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Thu, 29 Feb 2024 09:14:50 +0100 Subject: [PATCH 2/3] Add a migration to remove SSH signatures from release notes Because the `git` module did not recognize SSH signed tags, those signatures ended up in the `notes` column of the `release` table. While future signatures will not end up there, Forgejo should clean up the old ones. This migration does just that: finds all releases that have an SSH signature, and removes those signatures, preserving the rest of the note (if any). While this may seem like an expensive operation, it's only done once, and even on the largest known Forgejo instance as of this writing (Codeberg), the number of affected rows are just over a hundred, a tiny amount all things considered. Signed-off-by: Gergely Nagy --- models/forgejo_migrations/migrate.go | 2 + models/forgejo_migrations/v1_22/main_test.go | 14 +++++ models/forgejo_migrations/v1_22/v8.go | 51 +++++++++++++++++++ models/forgejo_migrations/v1_22/v8_test.go | 34 +++++++++++++ .../release.yml | 22 ++++++++ 5 files changed, 123 insertions(+) create mode 100644 models/forgejo_migrations/v1_22/main_test.go create mode 100644 models/forgejo_migrations/v1_22/v8.go create mode 100644 models/forgejo_migrations/v1_22/v8_test.go create mode 100644 models/migrations/fixtures/Test_RemoveSSHSignaturesFromReleaseNotes/release.yml diff --git a/models/forgejo_migrations/migrate.go b/models/forgejo_migrations/migrate.go index 39c8c8a142..3c85031e9a 100644 --- a/models/forgejo_migrations/migrate.go +++ b/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. diff --git a/models/forgejo_migrations/v1_22/main_test.go b/models/forgejo_migrations/v1_22/main_test.go new file mode 100644 index 0000000000..8ca5395a26 --- /dev/null +++ b/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) +} diff --git a/models/forgejo_migrations/v1_22/v8.go b/models/forgejo_migrations/v1_22/v8.go new file mode 100644 index 0000000000..2d3c0c594b --- /dev/null +++ b/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() +} diff --git a/models/forgejo_migrations/v1_22/v8_test.go b/models/forgejo_migrations/v1_22/v8_test.go new file mode 100644 index 0000000000..b8cd478daa --- /dev/null +++ b/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) +} diff --git a/models/migrations/fixtures/Test_RemoveSSHSignaturesFromReleaseNotes/release.yml b/models/migrations/fixtures/Test_RemoveSSHSignaturesFromReleaseNotes/release.yml new file mode 100644 index 0000000000..caa0b40b8a --- /dev/null +++ b/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" From 40c357bbc51311db54ddf2b8309d9fe7592f9056 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Thu, 29 Feb 2024 09:44:35 +0100 Subject: [PATCH 3/3] Rename CommitGPGSignature to ObjectSignature `CommitGPGSignature` was originally made to store information about a commit's GPG signature. Nowadays, it is used to store information about SSH signatures too, and not just commit signatures, but tag signatures too. As such, rename it to `ObjectSignature`, because that describes what it does a whole lot better. Signed-off-by: Gergely Nagy --- models/asymkey/ssh_key_commit_verification_test.go | 8 ++++---- modules/git/commit.go | 8 +------- modules/git/commit_convert_gogit.go | 4 ++-- modules/git/commit_reader.go | 2 +- modules/git/object_signature.go | 11 +++++++++++ modules/git/repo_tag.go | 4 ++-- modules/git/repo_tag_test.go | 2 +- modules/git/tag.go | 6 +++--- modules/git/tag_test.go | 2 +- 9 files changed, 26 insertions(+), 21 deletions(-) create mode 100644 modules/git/object_signature.go diff --git a/models/asymkey/ssh_key_commit_verification_test.go b/models/asymkey/ssh_key_commit_verification_test.go index e1ed409bd7..320e114b3d 100644 --- a/models/asymkey/ssh_key_commit_verification_test.go +++ b/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 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 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 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 1699709594 +0100 diff --git a/modules/git/commit.go b/modules/git/commit.go index 3140d1f302..c30f1e35e5 100644 --- a/modules/git/commit.go +++ b/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 diff --git a/modules/git/commit_convert_gogit.go b/modules/git/commit_convert_gogit.go index 819ea0d1db..abf65bf008 100644 --- a/modules/git/commit_convert_gogit.go +++ b/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(), } diff --git a/modules/git/commit_reader.go b/modules/git/commit_reader.go index 4809d6c7ed..bf7412aa1d 100644 --- a/modules/git/commit_reader.go +++ b/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(), } diff --git a/modules/git/object_signature.go b/modules/git/object_signature.go new file mode 100644 index 0000000000..35fa671a9e --- /dev/null +++ b/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 +} diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index eab4ca5a57..638c508e4b 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -196,11 +196,11 @@ func parseTagRef(ref map[string]string) (tag *Tag, err error) { } } - // 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, } diff --git a/modules/git/repo_tag_test.go b/modules/git/repo_tag_test.go index 785c3442a7..8f0875c60d 100644 --- a/modules/git/repo_tag_test.go +++ b/modules/git/repo_tag_test.go @@ -315,7 +315,7 @@ qbHDASXl Type: "tag", Tagger: parseSignatureFromCommitLine("Foo Bar 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 diff --git a/modules/git/tag.go b/modules/git/tag.go index 66dc9cc08f..1fe4c16b5d 100644 --- a/modules/git/tag.go +++ b/modules/git/tag.go @@ -26,7 +26,7 @@ type Tag struct { Type string Tagger *Signature Message string - Signature *CommitGPGSignature + Signature *ObjectSignature } // Commit return the commit of the tag reference @@ -74,7 +74,7 @@ l: } } - extractTagSignature := func(signatureBeginMark, signatureEndMark string) (bool, *CommitGPGSignature, string) { + extractTagSignature := func(signatureBeginMark, signatureEndMark string) (bool, *ObjectSignature, string) { idx := strings.LastIndex(tag.Message, signatureBeginMark) if idx == -1 { return false, nil, "" @@ -85,7 +85,7 @@ l: return false, nil, "" } - return true, &CommitGPGSignature{ + 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] diff --git a/modules/git/tag_test.go b/modules/git/tag_test.go index d71501929c..79796bbdc2 100644 --- a/modules/git/tag_test.go +++ b/modules/git/tag_test.go @@ -65,7 +65,7 @@ AAAAQKFeTnxi9ssRqSg+sJcmjAgpgoPq1k5SXm306+mJmkPwvhim8f9Gz6uy1AddPmXaD7 Type: "commit", Tagger: &Signature{Name: "Jane Doe", Email: "jane.doe@example.com", When: time.Unix(1709146405, 0)}, Message: "v0\n", - Signature: &CommitGPGSignature{ + Signature: &ObjectSignature{ Signature: `-----BEGIN SSH SIGNATURE----- U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgvD4pK7baygXxoWoVoKjVEc/xZh 6w+1FUn5hypFqJXNAAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5