From ba15ed9ef30f2630141c66bab710b265ecc2d985 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 11 Jan 2020 17:56:59 +0000 Subject: [PATCH 01/18] Add require signed commit for protected branch --- models/branches.go | 1 + models/migrations/migrations.go | 2 + models/migrations/v120.go | 18 ++ modules/auth/repo_form.go | 1 + modules/git/commit.go | 10 +- options/locale/locale_en-US.ini | 2 + routers/private/hook.go | 161 ++++++++++++++++++ routers/repo/setting_protected_branch.go | 1 + templates/repo/settings/protected_branch.tmpl | 9 +- 9 files changed, 199 insertions(+), 6 deletions(-) create mode 100644 models/migrations/v120.go diff --git a/models/branches.go b/models/branches.go index b6398f56942fb..75f5c0a3a7be8 100644 --- a/models/branches.go +++ b/models/branches.go @@ -46,6 +46,7 @@ type ProtectedBranch struct { RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"` DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` + RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` CreatedUnix timeutil.TimeStamp `xorm:"created"` UpdatedUnix timeutil.TimeStamp `xorm:"updated"` diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index dc5cc48c64e35..18a0fda1f747d 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -294,6 +294,8 @@ var migrations = []Migration{ NewMigration("Add commit id and stale to reviews", addReviewCommitAndStale), // v119 -> v120 NewMigration("Fix migrated repositories' git service type", fixMigratedRepositoryServiceType), + // v120 -> v121 + NewMigration("Add Require Signed Commits to ProtectedBranch", addRequireSignedCommits), } // Migrate database to current version diff --git a/models/migrations/v120.go b/models/migrations/v120.go new file mode 100644 index 0000000000000..84de6b35c7273 --- /dev/null +++ b/models/migrations/v120.go @@ -0,0 +1,18 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +func addRequireSignedCommits(x *xorm.Engine) error { + + type ProtectedBranch struct { + RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` + } + + return x.Sync2(new(ProtectedBranch)) +} diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 0086419b650f8..a5071de47eec8 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -173,6 +173,7 @@ type ProtectBranchForm struct { ApprovalsWhitelistTeams string BlockOnRejectedReviews bool DismissStaleApprovals bool + RequireSignedCommits bool } // Validate validates the fields diff --git a/modules/git/commit.go b/modules/git/commit.go index dfb7adcd1a2d9..9646d560633f8 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -33,7 +33,7 @@ type Commit struct { CommitMessage string Signature *CommitGPGSignature - parents []SHA1 // SHA1 strings + Parents []SHA1 // SHA1 strings submoduleCache *ObjectCache } @@ -94,7 +94,7 @@ func convertCommit(c *object.Commit) *Commit { Committer: &c.Committer, Author: &c.Author, Signature: convertPGPSignature(c), - parents: c.ParentHashes, + Parents: c.ParentHashes, } } @@ -111,10 +111,10 @@ func (c *Commit) Summary() string { // ParentID returns oid of n-th parent (0-based index). // It returns nil if no such parent exists. func (c *Commit) ParentID(n int) (SHA1, error) { - if n >= len(c.parents) { + if n >= len(c.Parents) { return SHA1{}, ErrNotExist{"", ""} } - return c.parents[n], nil + return c.Parents[n], nil } // Parent returns n-th parent (0-based index) of the commit. @@ -133,7 +133,7 @@ func (c *Commit) Parent(n int) (*Commit, error) { // ParentCount returns number of parents of the commit. // 0 if this is the root commit, otherwise 1,2, etc. func (c *Commit) ParentCount() int { - return len(c.parents) + return len(c.Parents) } func isImageFile(data []byte) (string, bool) { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 4dc0b92234c35..eae0e8ccfaf34 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1416,6 +1416,8 @@ settings.protect_approvals_whitelist_users = Whitelisted reviewers: settings.protect_approvals_whitelist_teams = Whitelisted teams for reviews: settings.dismiss_stale_approvals = Dismiss stale approvals settings.dismiss_stale_approvals_desc = When new commits that change the content of the pull request are pushed to the branch, old approvals will be dismissed. +settings.require_signed_commits = Require Signed Commits +settings.require_signed_commits_desc = Reject pushes to this branch if they are unsigned or unverifiable settings.add_protected_branch = Enable protection settings.delete_protected_branch = Disable protection settings.update_protect_branch_success = Branch protection for branch '%s' has been updated. diff --git a/routers/private/hook.go b/routers/private/hook.go index b4626fddf4272..9df8e21a0343b 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -6,6 +6,9 @@ package private import ( + "bufio" + "bytes" + "context" "fmt" "net/http" "os" @@ -18,6 +21,7 @@ import ( "code.gitea.io/gitea/modules/repofiles" "code.gitea.io/gitea/modules/util" pull_service "code.gitea.io/gitea/services/pull" + "gopkg.in/src-d/go-git.v4/plumbing" "gitea.com/macaron/macaron" ) @@ -35,6 +39,15 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { return } repo.OwnerName = ownerName + gitRepo, err := git.OpenRepository(repo.RepoPath()) + if err != nil { + log.Error("Unable to get git repository for: %s/%s Error: %v", ownerName, repoName, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": err.Error(), + }) + return + } + defer gitRepo.Close() for i := range opts.OldCommitIDs { oldCommitID := opts.OldCommitIDs[i] @@ -92,6 +105,154 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { } } + + // Require signed commits + if protectBranch.RequireSignedCommits { + env := os.Environ() + if opts.GitAlternativeObjectDirectories != "" { + env = append(env, + private.GitAlternativeObjectDirectories+"="+opts.GitAlternativeObjectDirectories) + } + if opts.GitObjectDirectory != "" { + env = append(env, + private.GitObjectDirectory+"="+opts.GitObjectDirectory) + } + if opts.GitQuarantinePath != "" { + env = append(env, + private.GitQuarantinePath+"="+opts.GitQuarantinePath) + } + + stdoutReader, stdoutWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to create pipe: %v", err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to create pipe: %v", err), + }) + return + } + defer func() { + _ = stdoutReader.Close() + _ = stdoutWriter.Close() + }() + + stderr := new(bytes.Buffer) + commits := make([]*git.Commit, 0, 10) + + var finalErr error + if err := git.NewCommand("rev-list", oldCommitID+"..."+newCommitID). + RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.RepoPath(), + stdoutWriter, stderr, nil, + func(ctx context.Context, cancel context.CancelFunc) { + _ = stdoutWriter.Close() + scanner := bufio.NewScanner(stdoutReader) + for scanner.Scan() { + line := scanner.Text() + // TODO: Consider whether we really want to read these completely in to memory + var commitStr string + commitStr, finalErr = git.NewCommand("cat-file", "commit", line).RunInDirWithEnv(repo.RepoPath(), env) + if finalErr != nil { + cancel() + } + commits = append(commits, &git.Commit{ + ID: plumbing.NewHash(line), + CommitMessage: commitStr, + }) + } + _ = stdoutReader.Close() + }); err != nil { + if finalErr != nil { + err = finalErr + } + log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err), + }) + return + } + + // TODO: We should batch read a few commits in at a time: use -n and --skip + // Now parse the commitStrs + for _, commit := range commits { + payloadSB := new(strings.Builder) + signatureSB := new(strings.Builder) + messageSB := new(strings.Builder) + message := false + pgpsig := false + + scanner := bufio.NewScanner(strings.NewReader(commit.CommitMessage)) + for scanner.Scan() { + line := scanner.Bytes() + if pgpsig { + if len(line) > 0 && line[0] == ' ' { + line = bytes.TrimLeft(line, " ") + _, _ = signatureSB.Write(line) + _ = signatureSB.WriteByte('\n') + continue + } else { + pgpsig = false + } + } + + if !message { + trimmed := bytes.TrimSpace(line) + if len(trimmed) == 0 { + message = true + _, _ = payloadSB.WriteString("\n") + continue + } + + split := bytes.SplitN(trimmed, []byte{' '}, 2) + + switch string(split[0]) { + case "tree": + commit.Tree = *git.NewTree(gitRepo, plumbing.NewHash(string(split[1]))) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "parent": + commit.Parents = append(commit.Parents, plumbing.NewHash(string(split[1]))) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "author": + commit.Author = &git.Signature{} + commit.Author.Decode(split[1]) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "committer": + commit.Committer = &git.Signature{} + commit.Committer.Decode(split[1]) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "gpgsig": + _, _ = signatureSB.Write(split[1]) + _ = signatureSB.WriteByte('\n') + pgpsig = true + } + } else { + _, _ = messageSB.Write(line) + _ = messageSB.WriteByte('\n') + } + } + commit.CommitMessage = messageSB.String() + _, _ = payloadSB.WriteString(commit.CommitMessage) + commit.Signature = &git.CommitGPGSignature{ + Signature: signatureSB.String(), + Payload: payloadSB.String(), + } + if len(commit.Signature.Signature) == 0 { + commit.Signature = nil + } + + verification := models.ParseCommitWithSignature(commit) + if !verification.Verified { + log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, commit.ID.String()) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, commit.ID.String()), + }) + return + } + } + } + canPush := false if opts.IsDeployKey { canPush = protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys) diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index da28ac50be844..e8902ed8acbe9 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -246,6 +246,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) } protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews protectBranch.DismissStaleApprovals = f.DismissStaleApprovals + protectBranch.RequireSignedCommits = f.RequireSignedCommits err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{ UserIDs: whitelistUsers, diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index c6701ce8a99e4..b4c65f78302c3 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -210,7 +210,7 @@

{{.i18n.Tr "repo.settings.block_rejected_reviews_desc"}}

- +
@@ -218,6 +218,13 @@

{{.i18n.Tr "repo.settings.dismiss_stale_approvals_desc"}}

+
+
+ + +

{{.i18n.Tr "repo.settings.require_signed_commits_desc"}}

+
+
From f482e28de6075bce32e8717cc4aca37fca287e54 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 11 Jan 2020 18:38:25 +0000 Subject: [PATCH 02/18] Fix fmt --- models/migrations/v120.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/migrations/v120.go b/models/migrations/v120.go index 84de6b35c7273..e28adc1d822e3 100644 --- a/models/migrations/v120.go +++ b/models/migrations/v120.go @@ -11,7 +11,7 @@ import ( func addRequireSignedCommits(x *xorm.Engine) error { type ProtectedBranch struct { - RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` + RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` } return x.Sync2(new(ProtectedBranch)) From f9f855734a70c64d4eb0538a77fcb9bce45dc5ff Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 11 Jan 2020 20:43:44 +0000 Subject: [PATCH 03/18] Make editor show if they will be signed --- modules/context/repo.go | 35 +++++++++++++++++++++++--- options/locale/locale_en-US.ini | 5 ++++ routers/repo/editor.go | 7 +++--- templates/repo/editor/commit_form.tmpl | 17 +++++++++++-- 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index 4c32e846eb1cb..4c8f78fdb3ea3 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -74,14 +74,41 @@ func RepoMustNotBeArchived() macaron.Handler { } } +// CanCommitToBranchResults represents the results of CanCommitToBranch +type CanCommitToBranchResults struct { + CanCommitToBranch bool + EditorEnabled bool + UserCanPush bool + RequireSigned bool + WillSign bool + SigningKey string +} + // CanCommitToBranch returns true if repository is editable and user has proper access level // and branch is not protected for push -func (r *Repository) CanCommitToBranch(doer *models.User) (bool, error) { - protectedBranch, err := r.Repository.IsProtectedBranchForPush(r.BranchName, doer) +func (r *Repository) CanCommitToBranch(doer *models.User) (CanCommitToBranchResults, error) { + protectedBranch, err := models.GetProtectedBranchBy(r.Repository.ID, r.BranchName) + if err != nil { - return false, err + return CanCommitToBranchResults{}, err } - return r.CanEnableEditor() && !protectedBranch, nil + userCanPush := protectedBranch.CanUserPush(doer.ID) + + sign, keyID := r.Repository.SignCRUDAction(doer, r.Repository.RepoPath(), git.BranchPrefix+r.BranchName) + + canCommit := r.CanEnableEditor() && userCanPush + if protectedBranch.RequireSignedCommits { + canCommit = canCommit && sign + } + + return CanCommitToBranchResults{ + CanCommitToBranch: canCommit, + EditorEnabled: r.CanEnableEditor(), + UserCanPush: userCanPush, + RequireSigned: protectedBranch.RequireSignedCommits, + WillSign: sign, + SigningKey: keyID, + }, nil } // CanUseTimetracker returns whether or not a user can use the timetracker. diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index eae0e8ccfaf34..08ca8328e7932 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -748,6 +748,7 @@ editor.name_your_file = Name your file… editor.filename_help = Add a directory by typing its name followed by a slash ('/'). Remove a directory by typing backspace at the beginning of the input field. editor.or = or editor.cancel_lower = Cancel +editor.commit_signed_changes = Commit Signed Changes editor.commit_changes = Commit Changes editor.add_tmpl = Add '' editor.add = Add '%s' @@ -780,6 +781,10 @@ editor.unable_to_upload_files = Failed to upload files to '%s' with error: %v editor.upload_file_is_locked = File '%s' is locked by %s. editor.upload_files_to_dir = Upload files to '%s' editor.cannot_commit_to_protected_branch = Cannot commit to protected branch '%s'. +editor.no_commit_to_branch = Unable to commit directly to branch because: +editor.user_no_push_to_branch = User cannot push to branch +editor.require_signed_commit = Branch requires a signed commit +editor.will_sign = This commit will be signed with key '%s' commits.desc = Browse source code change history. commits.commits = Commits diff --git a/routers/repo/editor.go b/routers/repo/editor.go index fec572852567d..aa4bfc59659ad 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -35,12 +35,13 @@ const ( ) func renderCommitRights(ctx *context.Context) bool { - canCommit, err := ctx.Repo.CanCommitToBranch(ctx.User) + canCommitToBranch, err := ctx.Repo.CanCommitToBranch(ctx.User) if err != nil { log.Error("CanCommitToBranch: %v", err) } - ctx.Data["CanCommitToBranch"] = canCommit - return canCommit + ctx.Data["CanCommitToBranch"] = canCommitToBranch + + return canCommitToBranch.CanCommitToBranch } // getParentTreeFields returns list of parent tree names and corresponding tree paths diff --git a/templates/repo/editor/commit_form.tmpl b/templates/repo/editor/commit_form.tmpl index 2ff08e3931303..fa9c60b22123e 100644 --- a/templates/repo/editor/commit_form.tmpl +++ b/templates/repo/editor/commit_form.tmpl @@ -1,7 +1,11 @@
-

{{.i18n.Tr "repo.editor.commit_changes"}}

+

{{- if .CanCommitToBranch.WillSign}} + {{.i18n.Tr "repo.editor.commit_signed_changes"}} + {{- else}} + {{.i18n.Tr "repo.editor.commit_changes"}} + {{- end}}

@@ -10,11 +14,20 @@
-
+
From f2a28e8ecb4d6d41ceb9e2d48e9175a76b444639 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 11 Jan 2020 21:45:30 +0000 Subject: [PATCH 04/18] bugfix --- modules/context/repo.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index 4c8f78fdb3ea3..496f8e76860a8 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -92,12 +92,17 @@ func (r *Repository) CanCommitToBranch(doer *models.User) (CanCommitToBranchResu if err != nil { return CanCommitToBranchResults{}, err } - userCanPush := protectedBranch.CanUserPush(doer.ID) + userCanPush := true + requireSigned := false + if protectedBranch != nil { + userCanPush = protectedBranch.CanUserPush(doer.ID) + requireSigned = protectedBranch.RequireSignedCommits + } sign, keyID := r.Repository.SignCRUDAction(doer, r.Repository.RepoPath(), git.BranchPrefix+r.BranchName) canCommit := r.CanEnableEditor() && userCanPush - if protectedBranch.RequireSignedCommits { + if requireSigned { canCommit = canCommit && sign } @@ -105,7 +110,7 @@ func (r *Repository) CanCommitToBranch(doer *models.User) (CanCommitToBranchResu CanCommitToBranch: canCommit, EditorEnabled: r.CanEnableEditor(), UserCanPush: userCanPush, - RequireSigned: protectedBranch.RequireSignedCommits, + RequireSigned: requireSigned, WillSign: sign, SigningKey: keyID, }, nil From 594126a51a559fa5498751f4ae24712f02892d4d Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Jan 2020 14:07:16 +0000 Subject: [PATCH 05/18] Add basic merge check and better information for CRUD --- models/pull_sign.go | 59 ++++++----- models/repo.go | 2 +- models/repo_sign.go | 105 +++++++++++++------- modules/context/repo.go | 15 ++- modules/repofiles/temp_repo.go | 2 +- options/locale/locale_en-US.ini | 15 ++- routers/repo/issue.go | 8 ++ services/pull/merge.go | 2 +- services/wiki/wiki.go | 4 +- templates/repo/editor/commit_form.tmpl | 4 +- templates/repo/issue/view_content/pull.tmpl | 15 ++- 11 files changed, 159 insertions(+), 72 deletions(-) diff --git a/models/pull_sign.go b/models/pull_sign.go index 19d8907c3dbfe..1d3474abe77ea 100644 --- a/models/pull_sign.go +++ b/models/pull_sign.go @@ -11,16 +11,16 @@ import ( ) // SignMerge determines if we should sign a PR merge commit to the base repository -func (pr *PullRequest) SignMerge(u *User, tmpBasePath, baseCommit, headCommit string) (bool, string) { +func (pr *PullRequest) SignMerge(u *User, tmpBasePath, baseCommit, headCommit string) (bool, string, error) { if err := pr.GetBaseRepo(); err != nil { log.Error("Unable to get Base Repo for pull request") - return false, "" + return false, "", err } repo := pr.BaseRepo signingKey := signingKey(repo.RepoPath()) if signingKey == "" { - return false, "" + return false, "", &ErrWontSign{noKey} } rules := signingModeFromStrings(setting.Repository.Signing.Merges) @@ -30,92 +30,101 @@ func (pr *PullRequest) SignMerge(u *User, tmpBasePath, baseCommit, headCommit st for _, rule := range rules { switch rule { case never: - return false, "" + return false, "", &ErrWontSign{never} case always: break case pubkey: keys, err := ListGPGKeys(u.ID) - if err != nil || len(keys) == 0 { - return false, "" + if err != nil { + return false, "", err + } + if len(keys) == 0 { + return false, "", &ErrWontSign{pubkey} } case twofa: - twofa, err := GetTwoFactorByUID(u.ID) - if err != nil || twofa == nil { - return false, "" + twofaModel, err := GetTwoFactorByUID(u.ID) + if err != nil { + return false, "", err + } + if twofaModel == nil { + return false, "", &ErrWontSign{twofa} } case approved: protectedBranch, err := GetProtectedBranchBy(repo.ID, pr.BaseBranch) - if err != nil || protectedBranch == nil { - return false, "" + if err != nil { + return false, "", err + } + if protectedBranch == nil { + return false, "", &ErrWontSign{approved} } if protectedBranch.GetGrantedApprovalsCount(pr) < 1 { - return false, "" + return false, "", &ErrWontSign{approved} } case baseSigned: if gitRepo == nil { gitRepo, err = git.OpenRepository(tmpBasePath) if err != nil { - return false, "" + return false, "", err } defer gitRepo.Close() } commit, err := gitRepo.GetCommit(baseCommit) if err != nil { - return false, "" + return false, "", err } verification := ParseCommitWithSignature(commit) if !verification.Verified { - return false, "" + return false, "", &ErrWontSign{baseSigned} } case headSigned: if gitRepo == nil { gitRepo, err = git.OpenRepository(tmpBasePath) if err != nil { - return false, "" + return false, "", err } defer gitRepo.Close() } commit, err := gitRepo.GetCommit(headCommit) if err != nil { - return false, "" + return false, "", err } verification := ParseCommitWithSignature(commit) if !verification.Verified { - return false, "" + return false, "", &ErrWontSign{headSigned} } case commitsSigned: if gitRepo == nil { gitRepo, err = git.OpenRepository(tmpBasePath) if err != nil { - return false, "" + return false, "", err } defer gitRepo.Close() } commit, err := gitRepo.GetCommit(headCommit) if err != nil { - return false, "" + return false, "", err } verification := ParseCommitWithSignature(commit) if !verification.Verified { - return false, "" + return false, "", &ErrWontSign{commitsSigned} } // need to work out merge-base mergeBaseCommit, _, err := gitRepo.GetMergeBase("", baseCommit, headCommit) if err != nil { - return false, "" + return false, "", err } commitList, err := commit.CommitsBeforeUntil(mergeBaseCommit) if err != nil { - return false, "" + return false, "", err } for e := commitList.Front(); e != nil; e = e.Next() { commit = e.Value.(*git.Commit) verification := ParseCommitWithSignature(commit) if !verification.Verified { - return false, "" + return false, "", &ErrWontSign{commitsSigned} } } } } - return true, signingKey + return true, signingKey, nil } diff --git a/models/repo.go b/models/repo.go index 6c9623ea2c5d2..740a283e15ee1 100644 --- a/models/repo.go +++ b/models/repo.go @@ -1044,7 +1044,7 @@ func initRepoCommit(tmpPath string, repo *Repository, u *User) (err error) { } if version.Compare(binVersion, "1.7.9", ">=") { - sign, keyID := SignInitialCommit(tmpPath, u) + sign, keyID, _ := SignInitialCommit(tmpPath, u) if sign { args = append(args, "-S"+keyID) } else if version.Compare(binVersion, "2.0.0", ">=") { diff --git a/models/repo_sign.go b/models/repo_sign.go index a684efb55fc44..ca1642b19c204 100644 --- a/models/repo_sign.go +++ b/models/repo_sign.go @@ -5,6 +5,7 @@ package models import ( + "fmt" "strings" "code.gitea.io/gitea/modules/git" @@ -25,6 +26,7 @@ const ( headSigned signingMode = "headsigned" commitsSigned signingMode = "commitssigned" approved signingMode = "approved" + noKey signingMode = "nokey" ) func signingModeFromStrings(modeStrings []string) []signingMode { @@ -78,6 +80,21 @@ func signingKey(repoPath string) string { return setting.Repository.Signing.SigningKey } +// ErrWontSign explains the first reason why a commit would not be signed +// There may be other reasons - this is just the first reason found +type ErrWontSign struct { + Reason signingMode +} + +func (e *ErrWontSign) Error() string { + return fmt.Sprintf("wont sign: %s", e.Reason) +} + +func IsErrWontSign(err error) bool { + _, ok := err.(*ErrWontSign) + return ok +} + // PublicSigningKey gets the public signing key within a provided repository directory func PublicSigningKey(repoPath string) (string, error) { signingKey := signingKey(repoPath) @@ -95,122 +112,140 @@ func PublicSigningKey(repoPath string) (string, error) { } // SignInitialCommit determines if we should sign the initial commit to this repository -func SignInitialCommit(repoPath string, u *User) (bool, string) { +func SignInitialCommit(repoPath string, u *User) (bool, string, error) { rules := signingModeFromStrings(setting.Repository.Signing.InitialCommit) signingKey := signingKey(repoPath) if signingKey == "" { - return false, "" + return false, "", &ErrWontSign{noKey} } for _, rule := range rules { switch rule { case never: - return false, "" + return false, "", &ErrWontSign{never} case always: break case pubkey: keys, err := ListGPGKeys(u.ID) - if err != nil || len(keys) == 0 { - return false, "" + if err != nil { + return false, "", err + } + if len(keys) == 0 { + return false, "", &ErrWontSign{pubkey} } case twofa: - twofa, err := GetTwoFactorByUID(u.ID) - if err != nil || twofa == nil { - return false, "" + twofaModel, err := GetTwoFactorByUID(u.ID) + if err != nil { + return false, "", err + } + if twofaModel == nil { + return false, "", &ErrWontSign{twofa} } } } - return true, signingKey + return true, signingKey, nil } // SignWikiCommit determines if we should sign the commits to this repository wiki -func (repo *Repository) SignWikiCommit(u *User) (bool, string) { +func (repo *Repository) SignWikiCommit(u *User) (bool, string, error) { rules := signingModeFromStrings(setting.Repository.Signing.Wiki) signingKey := signingKey(repo.WikiPath()) if signingKey == "" { - return false, "" + return false, "", &ErrWontSign{noKey} } for _, rule := range rules { switch rule { case never: - return false, "" + return false, "", &ErrWontSign{never} case always: break case pubkey: keys, err := ListGPGKeys(u.ID) - if err != nil || len(keys) == 0 { - return false, "" + if err != nil { + return false, "", err + } + if len(keys) == 0 { + return false, "", &ErrWontSign{pubkey} } case twofa: - twofa, err := GetTwoFactorByUID(u.ID) - if err != nil || twofa == nil { - return false, "" + twofaModel, err := GetTwoFactorByUID(u.ID) + if err != nil { + return false, "", err + } + if twofaModel == nil { + return false, "", &ErrWontSign{twofa} } case parentSigned: gitRepo, err := git.OpenRepository(repo.WikiPath()) if err != nil { - return false, "" + return false, "", err } defer gitRepo.Close() commit, err := gitRepo.GetCommit("HEAD") if err != nil { - return false, "" + return false, "", err } if commit.Signature == nil { - return false, "" + return false, "", &ErrWontSign{parentSigned} } verification := ParseCommitWithSignature(commit) if !verification.Verified { - return false, "" + return false, "", &ErrWontSign{parentSigned} } } } - return true, signingKey + return true, signingKey, nil } // SignCRUDAction determines if we should sign a CRUD commit to this repository -func (repo *Repository) SignCRUDAction(u *User, tmpBasePath, parentCommit string) (bool, string) { +func (repo *Repository) SignCRUDAction(u *User, tmpBasePath, parentCommit string) (bool, string, error) { rules := signingModeFromStrings(setting.Repository.Signing.CRUDActions) signingKey := signingKey(repo.RepoPath()) if signingKey == "" { - return false, "" + return false, "", &ErrWontSign{noKey} } for _, rule := range rules { switch rule { case never: - return false, "" + return false, "", &ErrWontSign{never} case always: break case pubkey: keys, err := ListGPGKeys(u.ID) - if err != nil || len(keys) == 0 { - return false, "" + if err != nil { + return false, "", err + } + if len(keys) == 0 { + return false, "", &ErrWontSign{pubkey} } case twofa: - twofa, err := GetTwoFactorByUID(u.ID) - if err != nil || twofa == nil { - return false, "" + twofaModel, err := GetTwoFactorByUID(u.ID) + if err != nil { + return false, "", err + } + if twofaModel == nil { + return false, "", &ErrWontSign{twofa} } case parentSigned: gitRepo, err := git.OpenRepository(tmpBasePath) if err != nil { - return false, "" + return false, "", err } defer gitRepo.Close() commit, err := gitRepo.GetCommit(parentCommit) if err != nil { - return false, "" + return false, "", err } if commit.Signature == nil { - return false, "" + return false, "", &ErrWontSign{parentSigned} } verification := ParseCommitWithSignature(commit) if !verification.Verified { - return false, "" + return false, "", &ErrWontSign{parentSigned} } } } - return true, signingKey + return true, signingKey, nil } diff --git a/modules/context/repo.go b/modules/context/repo.go index 496f8e76860a8..0e80187e7d268 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -82,6 +82,7 @@ type CanCommitToBranchResults struct { RequireSigned bool WillSign bool SigningKey string + WontSignReason string } // CanCommitToBranch returns true if repository is editable and user has proper access level @@ -99,12 +100,21 @@ func (r *Repository) CanCommitToBranch(doer *models.User) (CanCommitToBranchResu requireSigned = protectedBranch.RequireSignedCommits } - sign, keyID := r.Repository.SignCRUDAction(doer, r.Repository.RepoPath(), git.BranchPrefix+r.BranchName) + sign, keyID, err := r.Repository.SignCRUDAction(doer, r.Repository.RepoPath(), git.BranchPrefix+r.BranchName) canCommit := r.CanEnableEditor() && userCanPush if requireSigned { canCommit = canCommit && sign } + wontSignReason := "" + if err != nil { + if models.IsErrWontSign(err) { + wontSignReason = string(err.(*models.ErrWontSign).Reason) + err = nil + } else { + wontSignReason = "error" + } + } return CanCommitToBranchResults{ CanCommitToBranch: canCommit, @@ -113,7 +123,8 @@ func (r *Repository) CanCommitToBranch(doer *models.User) (CanCommitToBranchResu RequireSigned: requireSigned, WillSign: sign, SigningKey: keyID, - }, nil + WontSignReason: wontSignReason, + }, err } // CanUseTimetracker returns whether or not a user can use the timetracker. diff --git a/modules/repofiles/temp_repo.go b/modules/repofiles/temp_repo.go index f9ea4ba1554aa..348551e4c5888 100644 --- a/modules/repofiles/temp_repo.go +++ b/modules/repofiles/temp_repo.go @@ -219,7 +219,7 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(author, committer *models // Determine if we should sign if version.Compare(binVersion, "1.7.9", ">=") { - sign, keyID := t.repo.SignCRUDAction(author, t.basePath, "HEAD") + sign, keyID, _ := t.repo.SignCRUDAction(author, t.basePath, "HEAD") if sign { args = append(args, "-S"+keyID) } else if version.Compare(binVersion, "2.0.0", ">=") { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 08ca8328e7932..12d95fd9693b4 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -784,7 +784,6 @@ editor.cannot_commit_to_protected_branch = Cannot commit to protected branch '%s editor.no_commit_to_branch = Unable to commit directly to branch because: editor.user_no_push_to_branch = User cannot push to branch editor.require_signed_commit = Branch requires a signed commit -editor.will_sign = This commit will be signed with key '%s' commits.desc = Browse source code change history. commits.commits = Commits @@ -1073,6 +1072,7 @@ pulls.merge_pull_request = Merge Pull Request pulls.rebase_merge_pull_request = Rebase and Merge pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff) pulls.squash_merge_pull_request = Squash and Merge +pulls.require_signed_wont_sign = The branch requires signed commits but this merge will not be signed pulls.invalid_merge_option = You cannot use this merge option for this pull request. pulls.merge_conflict = Merge Failed: There was a conflict whilst merging: %[1]s
%[2]s
Hint: Try a different strategy pulls.rebase_conflict = Merge Failed: There was a conflict whilst rebasing commit: %[1]s
%[2]s
%[3]s
Hint:Try a different strategy @@ -1114,6 +1114,19 @@ milestones.filter_sort.most_complete = Most complete milestones.filter_sort.most_issues = Most issues milestones.filter_sort.least_issues = Least issues +signing.will_sign = This commit will be signed with key '%s' +signing.wont_sign.error = There was an error whilst checking if we could sign +signing.wont_sign.nokey = There is no key available to sign this commit +signing.wont_sign.never = Commits are never signed +signing.wont_sign.always = Commits are always signed +signing.wont_sign.pubkey = Cannot sign commit as you do not have a public key associated with your account +signing.wont_sign.twofa = You must have two factor authentication enabled to have commits signed +signing.wont_sign.parentsigned = Cannot sign commit as the parent commit is not signed +signing.wont_sign.basesigned = Cannot sign commit as the base commit is not signed +signing.wont_sign.headsigned = Cannot sign commit as the head commit is not signed +signing.wont_sign.commitssigned = Cannot sign commit as all the commits are not signed +signing.wont_sign.approved = Cannot sign commit as the commit is not approved + ext_wiki = Ext. Wiki ext_wiki.desc = Link to an external wiki. diff --git a/routers/repo/issue.go b/routers/repo/issue.go index ce3eb5bd2c2ed..c16617695d2fc 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -971,6 +971,14 @@ func ViewIssue(ctx *context.Context) { ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull) ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull) ctx.Data["GrantedApprovals"] = cnt + ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits + } + ctx.Data["WillSign"] = false + if ctx.User != nil { + sign, key, err := pull.SignMerge(ctx.User, pull.BaseRepo.RepoPath(), pull.BaseBranch, pull.GetGitRefName()) + ctx.Data["WillSign"] = sign + ctx.Data["SigningKey"] = key + ctx.Data["SigningError"] = err } ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && diff --git a/services/pull/merge.go b/services/pull/merge.go index e825c3fdd1836..44d6773dc08a3 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -158,7 +158,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor // Determine if we should sign signArg := "" if version.Compare(binVersion, "1.7.9", ">=") { - sign, keyID := pr.SignMerge(doer, tmpBasePath, "HEAD", trackingBranch) + sign, keyID, _ := pr.SignMerge(doer, tmpBasePath, "HEAD", trackingBranch) if sign { signArg = "-S" + keyID } else if version.Compare(binVersion, "2.0.0", ">=") { diff --git a/services/wiki/wiki.go b/services/wiki/wiki.go index 58af203cfdc03..e2b04ade7798e 100644 --- a/services/wiki/wiki.go +++ b/services/wiki/wiki.go @@ -184,7 +184,7 @@ func updateWikiPage(doer *models.User, repo *models.Repository, oldWikiName, new Message: message, } - sign, signingKey := repo.SignWikiCommit(doer) + sign, signingKey, _ := repo.SignWikiCommit(doer) if sign { commitTreeOpts.KeyID = signingKey } else { @@ -298,7 +298,7 @@ func DeleteWikiPage(doer *models.User, repo *models.Repository, wikiName string) Parents: []string{"HEAD"}, } - sign, signingKey := repo.SignWikiCommit(doer) + sign, signingKey, _ := repo.SignWikiCommit(doer) if sign { commitTreeOpts.KeyID = signingKey } else { diff --git a/templates/repo/editor/commit_form.tmpl b/templates/repo/editor/commit_form.tmpl index fa9c60b22123e..1915e9be21b9c 100644 --- a/templates/repo/editor/commit_form.tmpl +++ b/templates/repo/editor/commit_form.tmpl @@ -2,9 +2,9 @@

{{- if .CanCommitToBranch.WillSign}} - {{.i18n.Tr "repo.editor.commit_signed_changes"}} + {{.i18n.Tr "repo.editor.commit_signed_changes"}} {{- else}} - {{.i18n.Tr "repo.editor.commit_changes"}} + {{.i18n.Tr "repo.editor.commit_changes"}} {{- end}}

diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 7073312b1fbac..9e7a519e7dfa5 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -48,6 +48,7 @@ {{else if .IsBlockedByApprovals}}red {{else if .IsBlockedByRejection}}red {{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red + {{else if and .RequireSigned (not .WillSign)}}}red {{else if .Issue.PullRequest.IsChecking}}yellow {{else if .Issue.PullRequest.CanAutoMerge}}green {{else}}red{{end}}"> @@ -122,9 +123,14 @@ {{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
+ {{else if and .RequireSigned (not .WillSign)}} +
+ + {{$.i18n.Tr "repo.pulls.require_signed_wont_sign"}} +
{{end}} - {{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} - {{if or $.IsRepoAdmin (not $notAllOk)}} + {{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} + {{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}} {{if $notAllOk}}
@@ -282,6 +288,11 @@ {{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
+ {{else if and .RequireSigned (not .WillSign)}} +
+ + {{$.i18n.Tr "repo.pulls.require_signed_wont_sign"}} +
{{else}}
From dd05f6a725f50b52cf799e44e8fa3fb3f1f8544a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Jan 2020 14:18:17 +0000 Subject: [PATCH 06/18] linting comment --- models/repo_sign.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/repo_sign.go b/models/repo_sign.go index ca1642b19c204..116a8c4f387d3 100644 --- a/models/repo_sign.go +++ b/models/repo_sign.go @@ -90,6 +90,7 @@ func (e *ErrWontSign) Error() string { return fmt.Sprintf("wont sign: %s", e.Reason) } +// IsErrWontSign checks if an error is a ErrWontSign func IsErrWontSign(err error) bool { _, ok := err.(*ErrWontSign) return ok From de67865471c03709509919a8a9f9b6914270bb12 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Jan 2020 15:15:35 +0000 Subject: [PATCH 07/18] Add descriptors to merge signing --- options/locale/locale_en-US.ini | 12 ++++++------ routers/repo/issue.go | 9 ++++++++- templates/repo/issue/view_content/pull.tmpl | 15 +++++++++++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 12d95fd9693b4..fc83082ef273b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1119,13 +1119,13 @@ signing.wont_sign.error = There was an error whilst checking if we could sign signing.wont_sign.nokey = There is no key available to sign this commit signing.wont_sign.never = Commits are never signed signing.wont_sign.always = Commits are always signed -signing.wont_sign.pubkey = Cannot sign commit as you do not have a public key associated with your account +signing.wont_sign.pubkey = The commit will not be signed because you do not have a public key associated with your account signing.wont_sign.twofa = You must have two factor authentication enabled to have commits signed -signing.wont_sign.parentsigned = Cannot sign commit as the parent commit is not signed -signing.wont_sign.basesigned = Cannot sign commit as the base commit is not signed -signing.wont_sign.headsigned = Cannot sign commit as the head commit is not signed -signing.wont_sign.commitssigned = Cannot sign commit as all the commits are not signed -signing.wont_sign.approved = Cannot sign commit as the commit is not approved +signing.wont_sign.parentsigned = The commit will not be signed as the parent commit is not signed +signing.wont_sign.basesigned = The merge will not be signed as the base commit is not signed +signing.wont_sign.headsigned = The merge will not be signed as the head commit is not signed +signing.wont_sign.commitssigned = The merge will not be signed as all the associated commits are not signed +signing.wont_sign.approved = The merge will not be signed as the PR is not approved ext_wiki = Ext. Wiki ext_wiki.desc = Link to an external wiki. diff --git a/routers/repo/issue.go b/routers/repo/issue.go index c16617695d2fc..afc115c6e2cc9 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -978,7 +978,14 @@ func ViewIssue(ctx *context.Context) { sign, key, err := pull.SignMerge(ctx.User, pull.BaseRepo.RepoPath(), pull.BaseBranch, pull.GetGitRefName()) ctx.Data["WillSign"] = sign ctx.Data["SigningKey"] = key - ctx.Data["SigningError"] = err + if err != nil { + if models.IsErrWontSign(err) { + ctx.Data["WontSignReason"] = err.(*models.ErrWontSign).Reason + } else { + ctx.Data["WontSignReason"] = "error" + log.Error("Error whilst checking if could sign pr %d in repo %s. Error: %v", pull.ID, pull.BaseRepo.FullName(), err) + } + } } ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 9e7a519e7dfa5..fad10a2aa47a3 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -128,6 +128,10 @@ {{$.i18n.Tr "repo.pulls.require_signed_wont_sign"}}
+
+ + {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} +
{{end}} {{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} {{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}} @@ -142,6 +146,17 @@ {{$.i18n.Tr "repo.pulls.can_auto_merge_desc"}}
{{end}} + {{if .WillSign}} +
+ + {{$.i18n.Tr "repo.signing.will_sign" .SigningKey}} +
+ {{else}} +
+ + {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} +
+ {{end}} {{if .AllowMerge}} {{$prUnit := .Repository.MustGetUnit $.UnitTypePullRequests}} {{$approvers := .Issue.PullRequest.GetApprovers}} From 97b811db76a561ad62544120638aaa7b04e4233e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Jan 2020 17:59:21 +0000 Subject: [PATCH 08/18] Slight refactor --- modules/git/command.go | 8 +- modules/git/commit_reader.go | 93 +++++++ modules/repofiles/temp_repo.go | 3 +- routers/private/hook.go | 272 +++++++++----------- services/pull/patch.go | 3 +- templates/repo/issue/view_content/pull.tmpl | 6 +- 6 files changed, 224 insertions(+), 161 deletions(-) create mode 100644 modules/git/commit_reader.go diff --git a/modules/git/command.go b/modules/git/command.go index 33143dbd754ea..3cb676c8daac2 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -97,7 +97,7 @@ func (c *Command) RunInDirTimeoutEnvFullPipeline(env []string, timeout time.Dura // RunInDirTimeoutEnvFullPipelineFunc executes the command in given directory with given timeout, // it pipes stdout and stderr to given io.Writer and passes in an io.Reader as stdin. Between cmd.Start and cmd.Wait the passed in function is run. -func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader, fn func(context.Context, context.CancelFunc)) error { +func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader, fn func(context.Context, context.CancelFunc) error) error { if timeout == -1 { timeout = DefaultCommandExecutionTimeout @@ -135,7 +135,11 @@ func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time. defer process.GetManager().Remove(pid) if fn != nil { - fn(ctx, cancel) + err := fn(ctx, cancel) + if err != nil { + cancel() + return err + } } if err := cmd.Wait(); err != nil && ctx.Err() != context.DeadlineExceeded { diff --git a/modules/git/commit_reader.go b/modules/git/commit_reader.go new file mode 100644 index 0000000000000..968b456854639 --- /dev/null +++ b/modules/git/commit_reader.go @@ -0,0 +1,93 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package git + +import ( + "bufio" + "bytes" + "io" + "strings" + + "gopkg.in/src-d/go-git.v4/plumbing" +) + +// CommitFromReader will generate a Commit from a provided reader +// We will need this to interpret commits from cat-file +func CommitFromReader(gitRepo *Repository, sha plumbing.Hash, reader io.Reader) (*Commit, error) { + commit := &Commit{ + ID: sha, + } + + payloadSB := new(strings.Builder) + signatureSB := new(strings.Builder) + messageSB := new(strings.Builder) + message := false + pgpsig := false + + scanner := bufio.NewScanner(reader) + for scanner.Scan() { + line := scanner.Bytes() + if pgpsig { + if len(line) > 0 && line[0] == ' ' { + line = bytes.TrimLeft(line, " ") + _, _ = signatureSB.Write(line) + _ = signatureSB.WriteByte('\n') + continue + } else { + pgpsig = false + } + } + + if !message { + trimmed := bytes.TrimSpace(line) + if len(trimmed) == 0 { + message = true + _, _ = payloadSB.WriteString("\n") + continue + } + + split := bytes.SplitN(trimmed, []byte{' '}, 2) + + switch string(split[0]) { + case "tree": + commit.Tree = *NewTree(gitRepo, plumbing.NewHash(string(split[1]))) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "parent": + commit.Parents = append(commit.Parents, plumbing.NewHash(string(split[1]))) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "author": + commit.Author = &Signature{} + commit.Author.Decode(split[1]) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "committer": + commit.Committer = &Signature{} + commit.Committer.Decode(split[1]) + _, _ = payloadSB.Write(line) + _ = payloadSB.WriteByte('\n') + case "gpgsig": + _, _ = signatureSB.Write(split[1]) + _ = signatureSB.WriteByte('\n') + pgpsig = true + } + } else { + _, _ = messageSB.Write(line) + _ = messageSB.WriteByte('\n') + } + } + commit.CommitMessage = messageSB.String() + _, _ = payloadSB.WriteString(commit.CommitMessage) + commit.Signature = &CommitGPGSignature{ + Signature: signatureSB.String(), + Payload: payloadSB.String(), + } + if len(commit.Signature.Signature) == 0 { + commit.Signature = nil + } + + return commit, scanner.Err() +} diff --git a/modules/repofiles/temp_repo.go b/modules/repofiles/temp_repo.go index 348551e4c5888..a1cc37e8c64a0 100644 --- a/modules/repofiles/temp_repo.go +++ b/modules/repofiles/temp_repo.go @@ -268,7 +268,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { var finalErr error if err := git.NewCommand("diff-index", "--cached", "-p", "HEAD"). - RunInDirTimeoutEnvFullPipelineFunc(nil, 30*time.Second, t.basePath, stdoutWriter, stderr, nil, func(ctx context.Context, cancel context.CancelFunc) { + RunInDirTimeoutEnvFullPipelineFunc(nil, 30*time.Second, t.basePath, stdoutWriter, stderr, nil, func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() diff, finalErr = gitdiff.ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader) if finalErr != nil { @@ -276,6 +276,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { cancel() } _ = stdoutReader.Close() + return finalErr }); err != nil { if finalErr != nil { log.Error("Unable to ParsePatch in temporary repo %s (%s). Error: %v", t.repo.FullName(), t.basePath, finalErr) diff --git a/routers/private/hook.go b/routers/private/hook.go index 9df8e21a0343b..4d4bf878f33db 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -7,9 +7,9 @@ package private import ( "bufio" - "bytes" "context" "fmt" + "io" "net/http" "os" "strings" @@ -26,6 +26,96 @@ import ( "gitea.com/macaron/macaron" ) +func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []string) error { + stdoutReader, stdoutWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to create os.Pipe in %s", repo.Path) + return err + } + defer func() { + _ = stdoutReader.Close() + _ = stdoutWriter.Close() + }() + + err = git.NewCommand("rev-list", oldCommitID+"..."+newCommitID). + RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, + stdoutWriter, nil, nil, + func(ctx context.Context, cancel context.CancelFunc) error { + _ = stdoutWriter.Close() + err := readAndVerifyCommitsFromShaReader(stdoutReader, repo, env) + if err != nil { + log.Error("%v", err) + cancel() + } + _ = stdoutReader.Close() + return err + }) + if err != nil && !isErrUnverifiedCommit(err) { + log.Error("Unable to check commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) + } + return err +} + +func readAndVerifyCommitsFromShaReader(input io.ReadCloser, repo *git.Repository, env []string) error { + scanner := bufio.NewScanner(input) + for scanner.Scan() { + line := scanner.Text() + err := readAndVerifyCommit(line, repo, env) + if err != nil { + log.Error("%v", err) + return err + } + } + return scanner.Err() +} + +func readAndVerifyCommit(sha string, repo *git.Repository, env []string) error { + stdoutReader, stdoutWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to create pipe in %s: %v", repo.Path, err) + return err + } + defer func() { + _ = stdoutReader.Close() + _ = stdoutWriter.Close() + }() + hash := plumbing.NewHash(sha) + + return git.NewCommand("cat-file", "commit", sha). + RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, + stdoutWriter, nil, nil, + func(ctx context.Context, cancel context.CancelFunc) error { + _ = stdoutWriter.Close() + commit, err := git.CommitFromReader(repo, hash, stdoutReader) + if err != nil { + return err + } + log.Info("have commit %s", commit.ID.String()) + verification := models.ParseCommitWithSignature(commit) + if !verification.Verified { + log.Info("unverified commit %s", commit.ID.String()) + cancel() + return &errUnverifiedCommit{ + commit.ID.String(), + } + } + return nil + }) +} + +type errUnverifiedCommit struct { + sha string +} + +func (e *errUnverifiedCommit) Error() string { + return fmt.Sprintf("Unverified commit: %s", e.sha) +} + +func isErrUnverifiedCommit(err error) bool { + _, ok := err.(*errUnverifiedCommit) + return ok +} + // HookPreReceive checks whether a individual commit is acceptable func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { ownerName := ctx.Params(":owner") @@ -49,6 +139,21 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { } defer gitRepo.Close() + // Generate git environment for checking commits + env := os.Environ() + if opts.GitAlternativeObjectDirectories != "" { + env = append(env, + private.GitAlternativeObjectDirectories+"="+opts.GitAlternativeObjectDirectories) + } + if opts.GitObjectDirectory != "" { + env = append(env, + private.GitObjectDirectory+"="+opts.GitObjectDirectory) + } + if opts.GitQuarantinePath != "" { + env = append(env, + private.GitQuarantinePath+"="+opts.GitQuarantinePath) + } + for i := range opts.OldCommitIDs { oldCommitID := opts.OldCommitIDs[i] newCommitID := opts.NewCommitIDs[i] @@ -64,7 +169,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { return } if protectBranch != nil && protectBranch.IsProtected() { - // check and deletion + // detect and prevent deletion if newCommitID == git.EmptySHA { log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo) ctx.JSON(http.StatusForbidden, map[string]interface{}{ @@ -75,20 +180,6 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { // detect force push if git.EmptySHA != oldCommitID { - env := os.Environ() - if opts.GitAlternativeObjectDirectories != "" { - env = append(env, - private.GitAlternativeObjectDirectories+"="+opts.GitAlternativeObjectDirectories) - } - if opts.GitObjectDirectory != "" { - env = append(env, - private.GitObjectDirectory+"="+opts.GitObjectDirectory) - } - if opts.GitQuarantinePath != "" { - env = append(env, - private.GitQuarantinePath+"="+opts.GitQuarantinePath) - } - output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env) if err != nil { log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err) @@ -108,148 +199,21 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { // Require signed commits if protectBranch.RequireSignedCommits { - env := os.Environ() - if opts.GitAlternativeObjectDirectories != "" { - env = append(env, - private.GitAlternativeObjectDirectories+"="+opts.GitAlternativeObjectDirectories) - } - if opts.GitObjectDirectory != "" { - env = append(env, - private.GitObjectDirectory+"="+opts.GitObjectDirectory) - } - if opts.GitQuarantinePath != "" { - env = append(env, - private.GitQuarantinePath+"="+opts.GitQuarantinePath) - } - - stdoutReader, stdoutWriter, err := os.Pipe() + err := verifyCommits(oldCommitID, newCommitID, gitRepo, env) if err != nil { - log.Error("Unable to create pipe: %v", err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to create pipe: %v", err), - }) - return - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - - stderr := new(bytes.Buffer) - commits := make([]*git.Commit, 0, 10) - - var finalErr error - if err := git.NewCommand("rev-list", oldCommitID+"..."+newCommitID). - RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.RepoPath(), - stdoutWriter, stderr, nil, - func(ctx context.Context, cancel context.CancelFunc) { - _ = stdoutWriter.Close() - scanner := bufio.NewScanner(stdoutReader) - for scanner.Scan() { - line := scanner.Text() - // TODO: Consider whether we really want to read these completely in to memory - var commitStr string - commitStr, finalErr = git.NewCommand("cat-file", "commit", line).RunInDirWithEnv(repo.RepoPath(), env) - if finalErr != nil { - cancel() - } - commits = append(commits, &git.Commit{ - ID: plumbing.NewHash(line), - CommitMessage: commitStr, - }) - } - _ = stdoutReader.Close() - }); err != nil { - if finalErr != nil { - err = finalErr - } - log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err), - }) - return - } - - // TODO: We should batch read a few commits in at a time: use -n and --skip - // Now parse the commitStrs - for _, commit := range commits { - payloadSB := new(strings.Builder) - signatureSB := new(strings.Builder) - messageSB := new(strings.Builder) - message := false - pgpsig := false - - scanner := bufio.NewScanner(strings.NewReader(commit.CommitMessage)) - for scanner.Scan() { - line := scanner.Bytes() - if pgpsig { - if len(line) > 0 && line[0] == ' ' { - line = bytes.TrimLeft(line, " ") - _, _ = signatureSB.Write(line) - _ = signatureSB.WriteByte('\n') - continue - } else { - pgpsig = false - } - } - - if !message { - trimmed := bytes.TrimSpace(line) - if len(trimmed) == 0 { - message = true - _, _ = payloadSB.WriteString("\n") - continue - } - - split := bytes.SplitN(trimmed, []byte{' '}, 2) - - switch string(split[0]) { - case "tree": - commit.Tree = *git.NewTree(gitRepo, plumbing.NewHash(string(split[1]))) - _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') - case "parent": - commit.Parents = append(commit.Parents, plumbing.NewHash(string(split[1]))) - _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') - case "author": - commit.Author = &git.Signature{} - commit.Author.Decode(split[1]) - _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') - case "committer": - commit.Committer = &git.Signature{} - commit.Committer.Decode(split[1]) - _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') - case "gpgsig": - _, _ = signatureSB.Write(split[1]) - _ = signatureSB.WriteByte('\n') - pgpsig = true - } - } else { - _, _ = messageSB.Write(line) - _ = messageSB.WriteByte('\n') - } - } - commit.CommitMessage = messageSB.String() - _, _ = payloadSB.WriteString(commit.CommitMessage) - commit.Signature = &git.CommitGPGSignature{ - Signature: signatureSB.String(), - Payload: payloadSB.String(), - } - if len(commit.Signature.Signature) == 0 { - commit.Signature = nil - } - - verification := models.ParseCommitWithSignature(commit) - if !verification.Verified { - log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, commit.ID.String()) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, commit.ID.String()), + if !isErrUnverifiedCommit(err) { + log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err), }) return } + unverifiedCommit := err.(*errUnverifiedCommit).sha + log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit), + }) + return } } diff --git a/services/pull/patch.go b/services/pull/patch.go index 57a2997b36ee7..6535383a7e378 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -162,7 +162,7 @@ func TestPatch(pr *models.PullRequest) error { RunInDirTimeoutEnvFullPipelineFunc( nil, -1, tmpBasePath, nil, stderrWriter, nil, - func(ctx context.Context, cancel context.CancelFunc) { + func(ctx context.Context, cancel context.CancelFunc) error { _ = stderrWriter.Close() const prefix = "error: patch failed:" const errorPrefix = "error: " @@ -199,6 +199,7 @@ func TestPatch(pr *models.PullRequest) error { } } _ = stderrReader.Close() + return nil }) if err != nil { diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index fad10a2aa47a3..e373e11003af9 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -130,7 +130,7 @@
- {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} + {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
{{end}} {{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} @@ -149,12 +149,12 @@ {{if .WillSign}}
- {{$.i18n.Tr "repo.signing.will_sign" .SigningKey}} + {{$.i18n.Tr "repo.signing.will_sign" .SigningKey}}
{{else}}
- {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} + {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
{{end}} {{if .AllowMerge}} From 68a8e11142410d777bf23f9fe8923bd4fc20a46c Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Jan 2020 20:17:16 +0000 Subject: [PATCH 09/18] Slight improvement to appearances --- templates/repo/issue/view_content/pull.tmpl | 24 ++++++++++----------- web_src/less/_repository.less | 3 +++ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index e373e11003af9..1bf0fa7864a7f 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -94,67 +94,67 @@
{{else if .IsPullRequestBroken}}
- + {{$.i18n.Tr "repo.pulls.data_broken"}}
{{else if .IsPullWorkInProgress}}
- + {{$.i18n.Tr "repo.pulls.cannot_merge_work_in_progress" .WorkInProgressPrefix | Str2html}}
{{else if .Issue.PullRequest.IsChecking}}
- + {{$.i18n.Tr "repo.pulls.is_checking"}}
{{else if .Issue.PullRequest.CanAutoMerge}} {{if .IsBlockedByApprovals}}
- + {{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}}
{{else if .IsBlockedByRejection}}
- + {{$.i18n.Tr "repo.pulls.blocked_by_rejection"}}
{{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}
- + {{$.i18n.Tr "repo.pulls.required_status_check_failed"}}
{{else if and .RequireSigned (not .WillSign)}}
- + {{$.i18n.Tr "repo.pulls.require_signed_wont_sign"}}
- {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} + {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
{{end}} {{$notAllOk := or .IsBlockedByApprovals .IsBlockedByRejection (and .RequireSigned (not .WillSign)) (and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess))}} {{if and (or $.IsRepoAdmin (not $notAllOk)) (or (not .RequireSigned) .WillSign)}} {{if $notAllOk}}
- + {{$.i18n.Tr "repo.pulls.required_status_check_administrator"}}
{{else}}
- + {{$.i18n.Tr "repo.pulls.can_auto_merge_desc"}}
{{end}} {{if .WillSign}}
- {{$.i18n.Tr "repo.signing.will_sign" .SigningKey}} + {{$.i18n.Tr "repo.signing.will_sign" .SigningKey}}
{{else}}
- {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }} + {{$.i18n.Tr (printf "repo.signing.wont_sign.%s" .WontSignReason) }}
{{end}} {{if .AllowMerge}} diff --git a/web_src/less/_repository.less b/web_src/less/_repository.less index cd35b88f3ba39..27a0698f7b6c8 100644 --- a/web_src/less/_repository.less +++ b/web_src/less/_repository.less @@ -652,6 +652,9 @@ margin-left: 10px; margin-top: 10px; } + .icon-octicon { + padding-left: 2px; + } } .review-item { From acf011a140eac1f91980a7bd35d702f54190e801 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Jan 2020 20:51:23 +0000 Subject: [PATCH 10/18] Handle Merge API --- routers/api/v1/repo/pull.go | 8 ++++++++ services/pull/merge.go | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 6b643371e5de5..75e1ab3e243af 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -639,6 +639,14 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { } } + if _, err := pull_service.IsSignedIfRequired(pr, ctx.User); err != nil { + if !models.IsErrWontSign(err) { + ctx.Error(http.StatusInternalServerError, "IsSignedIfRequired", err) + return + } + ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err) + } + if len(form.Do) == 0 { form.Do = string(models.MergeStyleMerge) } diff --git a/services/pull/merge.go b/services/pull/merge.go index 44d6773dc08a3..d1cd4af9c4735 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -470,6 +470,23 @@ func getDiffTree(repoPath, baseBranch, headBranch string) (string, error) { return out.String(), nil } +// IsSignedIfRequired check if merge will be signed if required +func IsSignedIfRequired(pr *models.PullRequest, doer *models.User) (bool, error) { + if pr.ProtectedBranch == nil { + if err := pr.LoadProtectedBranch(); err != nil { + return false, err + } + } + + if pr.ProtectedBranch == nil || !pr.ProtectedBranch.RequireSignedCommits { + return true, nil + } + + sign, _, err := pr.SignMerge(doer, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitRefName()) + + return sign, err +} + // IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *models.User) (bool, error) { if p.IsAdmin() { From 24eb4a2789e555bcfa9afb1474962eb9ff1e487b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 12 Jan 2020 21:08:15 +0000 Subject: [PATCH 11/18] manage CRUD API --- modules/repofiles/delete.go | 25 +++++++++++++++++++++---- modules/repofiles/update.go | 23 +++++++++++++++++++++-- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/modules/repofiles/delete.go b/modules/repofiles/delete.go index 43937c49e19c8..d84931da9293e 100644 --- a/modules/repofiles/delete.go +++ b/modules/repofiles/delete.go @@ -46,7 +46,7 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo // If we aren't branching to a new branch, make sure user can commit to the given branch if opts.NewBranch != opts.OldBranch { newBranch, err := repo.GetBranch(opts.NewBranch) - if git.IsErrNotExist(err) { + if err != nil && !git.IsErrNotExist(err) { return nil, err } if newBranch != nil { @@ -54,9 +54,26 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo BranchName: opts.NewBranch, } } - } else if protected, _ := repo.IsProtectedBranchForPush(opts.OldBranch, doer); protected { - return nil, models.ErrUserCannotCommit{ - UserName: doer.LowerName, + } else { + protectedBranch, err := repo.GetBranchProtection(opts.OldBranch) + if err != nil { + return nil, err + } + if protectedBranch != nil && !protectedBranch.CanUserPush(doer.ID) { + return nil, models.ErrUserCannotCommit{ + UserName: doer.LowerName, + } + } + if protectedBranch != nil && protectedBranch.RequireSignedCommits { + _, _, err := repo.SignCRUDAction(doer, repo.RepoPath(), opts.OldBranch) + if err != nil { + if !models.IsErrWontSign(err) { + return nil, err + } + return nil, models.ErrUserCannotCommit{ + UserName: doer.LowerName, + } + } } } diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 812649af367cc..297164a537bd5 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -151,8 +151,27 @@ func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *Up if err != nil && !git.IsErrBranchNotExist(err) { return nil, err } - } else if protected, _ := repo.IsProtectedBranchForPush(opts.OldBranch, doer); protected { - return nil, models.ErrUserCannotCommit{UserName: doer.LowerName} + } else { + protectedBranch, err := repo.GetBranchProtection(opts.OldBranch) + if err != nil { + return nil, err + } + if protectedBranch != nil && !protectedBranch.CanUserPush(doer.ID) { + return nil, models.ErrUserCannotCommit{ + UserName: doer.LowerName, + } + } + if protectedBranch != nil && protectedBranch.RequireSignedCommits { + _, _, err := repo.SignCRUDAction(doer, repo.RepoPath(), opts.OldBranch) + if err != nil { + if !models.IsErrWontSign(err) { + return nil, err + } + return nil, models.ErrUserCannotCommit{ + UserName: doer.LowerName, + } + } + } } // If FromTreePath is not set, set it to the opts.TreePath From bd8d89ff787c518762a8cd7eaab1f884a4b4dbed Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 13 Jan 2020 23:09:17 +0000 Subject: [PATCH 12/18] Move error to error.go --- models/error.go | 16 ++++++++++++++++ models/repo_sign.go | 17 ----------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/models/error.go b/models/error.go index f0d5699aad6b8..fe9af70f3a49e 100644 --- a/models/error.go +++ b/models/error.go @@ -916,6 +916,22 @@ func (err ErrUserDoesNotHaveAccessToRepo) Error() string { return fmt.Sprintf("user doesn't have acces to repo [user_id: %d, repo_name: %s]", err.UserID, err.RepoName) } +// ErrWontSign explains the first reason why a commit would not be signed +// There may be other reasons - this is just the first reason found +type ErrWontSign struct { + Reason signingMode +} + +func (e *ErrWontSign) Error() string { + return fmt.Sprintf("wont sign: %s", e.Reason) +} + +// IsErrWontSign checks if an error is a ErrWontSign +func IsErrWontSign(err error) bool { + _, ok := err.(*ErrWontSign) + return ok +} + // __________ .__ // \______ \____________ ____ ____ | |__ // | | _/\_ __ \__ \ / \_/ ___\| | \ diff --git a/models/repo_sign.go b/models/repo_sign.go index 116a8c4f387d3..64f70ac7bdb62 100644 --- a/models/repo_sign.go +++ b/models/repo_sign.go @@ -5,7 +5,6 @@ package models import ( - "fmt" "strings" "code.gitea.io/gitea/modules/git" @@ -80,22 +79,6 @@ func signingKey(repoPath string) string { return setting.Repository.Signing.SigningKey } -// ErrWontSign explains the first reason why a commit would not be signed -// There may be other reasons - this is just the first reason found -type ErrWontSign struct { - Reason signingMode -} - -func (e *ErrWontSign) Error() string { - return fmt.Sprintf("wont sign: %s", e.Reason) -} - -// IsErrWontSign checks if an error is a ErrWontSign -func IsErrWontSign(err error) bool { - _, ok := err.(*ErrWontSign) - return ok -} - // PublicSigningKey gets the public signing key within a provided repository directory func PublicSigningKey(repoPath string) (string, error) { signingKey := signingKey(repoPath) From bfb7df01afa4fe417c633d4a586a14b67d747677 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 13 Jan 2020 23:12:59 +0000 Subject: [PATCH 13/18] Remove fix to delete.go --- modules/repofiles/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/repofiles/delete.go b/modules/repofiles/delete.go index d84931da9293e..4a0eb3d0e7e29 100644 --- a/modules/repofiles/delete.go +++ b/modules/repofiles/delete.go @@ -46,7 +46,7 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo // If we aren't branching to a new branch, make sure user can commit to the given branch if opts.NewBranch != opts.OldBranch { newBranch, err := repo.GetBranch(opts.NewBranch) - if err != nil && !git.IsErrNotExist(err) { + if git.IsErrNotExist(err) { return nil, err } if newBranch != nil { From 50cf49256c023cd17181f7a31642f59fc555b129 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 13 Jan 2020 23:20:23 +0000 Subject: [PATCH 14/18] prep for merge --- models/migrations/{v121.go => v122.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename models/migrations/{v121.go => v122.go} (100%) diff --git a/models/migrations/v121.go b/models/migrations/v122.go similarity index 100% rename from models/migrations/v121.go rename to models/migrations/v122.go From 7a04a475a086a092f03d9228908d35a61bbbf093 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 14 Jan 2020 09:30:26 +0000 Subject: [PATCH 15/18] need to tolerate \r\n in message --- modules/git/commit_reader.go | 44 ++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/modules/git/commit_reader.go b/modules/git/commit_reader.go index 968b456854639..62d0d5cccb297 100644 --- a/modules/git/commit_reader.go +++ b/modules/git/commit_reader.go @@ -27,13 +27,28 @@ func CommitFromReader(gitRepo *Repository, sha plumbing.Hash, reader io.Reader) pgpsig := false scanner := bufio.NewScanner(reader) + // Split by '\n' but include the '\n' + scanner.Split(func(data []byte, atEOF bool) (advance int, token []byte, err error) { + if atEOF && len(data) == 0 { + return 0, nil, nil + } + if i := bytes.IndexByte(data, '\n'); i >= 0 { + // We have a full newline-terminated line. + return i + 1, data[0 : i+1], nil + } + // If we're at EOF, we have a final, non-terminated line. Return it. + if atEOF { + return len(data), data, nil + } + // Request more data. + return 0, nil, nil + }) + for scanner.Scan() { line := scanner.Bytes() if pgpsig { if len(line) > 0 && line[0] == ' ' { - line = bytes.TrimLeft(line, " ") - _, _ = signatureSB.Write(line) - _ = signatureSB.WriteByte('\n') + _, _ = signatureSB.Write(line[1:]) continue } else { pgpsig = false @@ -41,42 +56,41 @@ func CommitFromReader(gitRepo *Repository, sha plumbing.Hash, reader io.Reader) } if !message { + // This is probably not correct but is copied from go-gits interpretation... trimmed := bytes.TrimSpace(line) if len(trimmed) == 0 { message = true - _, _ = payloadSB.WriteString("\n") + _, _ = payloadSB.Write(line) continue } split := bytes.SplitN(trimmed, []byte{' '}, 2) + var data []byte + if len(split) > 1 { + data = split[1] + } switch string(split[0]) { case "tree": - commit.Tree = *NewTree(gitRepo, plumbing.NewHash(string(split[1]))) + commit.Tree = *NewTree(gitRepo, plumbing.NewHash(string(data))) _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') case "parent": - commit.Parents = append(commit.Parents, plumbing.NewHash(string(split[1]))) + commit.Parents = append(commit.Parents, plumbing.NewHash(string(data))) _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') case "author": commit.Author = &Signature{} - commit.Author.Decode(split[1]) + commit.Author.Decode(data) _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') case "committer": commit.Committer = &Signature{} - commit.Committer.Decode(split[1]) + commit.Committer.Decode(data) _, _ = payloadSB.Write(line) - _ = payloadSB.WriteByte('\n') case "gpgsig": - _, _ = signatureSB.Write(split[1]) - _ = signatureSB.WriteByte('\n') + _, _ = signatureSB.Write(data) pgpsig = true } } else { _, _ = messageSB.Write(line) - _ = messageSB.WriteByte('\n') } } commit.CommitMessage = messageSB.String() From 161d1105e966f81bd1cec5417032d0f9efb5539f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 14 Jan 2020 09:41:02 +0000 Subject: [PATCH 16/18] check protected branch before trying to load it --- models/pull.go | 18 ++++++++++-------- services/pull/merge.go | 6 ++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/models/pull.go b/models/pull.go index 0435311e4e5de..1edd890035e39 100644 --- a/models/pull.go +++ b/models/pull.go @@ -152,16 +152,18 @@ func (pr *PullRequest) LoadProtectedBranch() (err error) { } func (pr *PullRequest) loadProtectedBranch(e Engine) (err error) { - if pr.BaseRepo == nil { - if pr.BaseRepoID == 0 { - return nil - } - pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID) - if err != nil { - return + if pr.ProtectedBranch == nil { + if pr.BaseRepo == nil { + if pr.BaseRepoID == 0 { + return nil + } + pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID) + if err != nil { + return + } } + pr.ProtectedBranch, err = getProtectedBranchBy(e, pr.BaseRepo.ID, pr.BaseBranch) } - pr.ProtectedBranch, err = getProtectedBranchBy(e, pr.BaseRepo.ID, pr.BaseBranch) return } diff --git a/services/pull/merge.go b/services/pull/merge.go index d1cd4af9c4735..f6f0abe8362c8 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -472,10 +472,8 @@ func getDiffTree(repoPath, baseBranch, headBranch string) (string, error) { // IsSignedIfRequired check if merge will be signed if required func IsSignedIfRequired(pr *models.PullRequest, doer *models.User) (bool, error) { - if pr.ProtectedBranch == nil { - if err := pr.LoadProtectedBranch(); err != nil { - return false, err - } + if err := pr.LoadProtectedBranch(); err != nil { + return false, err } if pr.ProtectedBranch == nil || !pr.ProtectedBranch.RequireSignedCommits { From 335ec5080557e923e8bda6d9b30c9d019a74c6b5 Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 14 Jan 2020 09:43:03 +0000 Subject: [PATCH 17/18] Apply suggestions from code review Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> --- options/locale/locale_en-US.ini | 2 +- routers/api/v1/repo/pull.go | 1 + routers/private/hook.go | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 8be5b17cd7042..140c1bd2e3c0b 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1115,7 +1115,7 @@ milestones.filter_sort.most_issues = Most issues milestones.filter_sort.least_issues = Least issues signing.will_sign = This commit will be signed with key '%s' -signing.wont_sign.error = There was an error whilst checking if we could sign +signing.wont_sign.error = There was an error whilst checking if the commit could be signed signing.wont_sign.nokey = There is no key available to sign this commit signing.wont_sign.never = Commits are never signed signing.wont_sign.always = Commits are always signed diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 75e1ab3e243af..bca756aea160a 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -645,6 +645,7 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err) + return } if len(form.Do) == 0 { diff --git a/routers/private/hook.go b/routers/private/hook.go index 4d4bf878f33db..6a07de15ff939 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -29,7 +29,7 @@ import ( func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []string) error { stdoutReader, stdoutWriter, err := os.Pipe() if err != nil { - log.Error("Unable to create os.Pipe in %s", repo.Path) + log.Error("Unable to create os.Pipe for %s", repo.Path) return err } defer func() { @@ -72,7 +72,7 @@ func readAndVerifyCommitsFromShaReader(input io.ReadCloser, repo *git.Repository func readAndVerifyCommit(sha string, repo *git.Repository, env []string) error { stdoutReader, stdoutWriter, err := os.Pipe() if err != nil { - log.Error("Unable to create pipe in %s: %v", repo.Path, err) + log.Error("Unable to create pipe for %s: %v", repo.Path, err) return err } defer func() { From ea40bd63ced46edd60a8a07021ac31f9651a9285 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 14 Jan 2020 22:03:54 +0000 Subject: [PATCH 18/18] fix commit-reader --- modules/git/commit_reader.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/git/commit_reader.go b/modules/git/commit_reader.go index 62d0d5cccb297..06d8f426d75bb 100644 --- a/modules/git/commit_reader.go +++ b/modules/git/commit_reader.go @@ -87,6 +87,7 @@ func CommitFromReader(gitRepo *Repository, sha plumbing.Hash, reader io.Reader) _, _ = payloadSB.Write(line) case "gpgsig": _, _ = signatureSB.Write(data) + _ = signatureSB.WriteByte('\n') pgpsig = true } } else {