Add require signed commit for protected branch (#9708)

* Add require signed commit for protected branch

* Fix fmt

* Make editor show if they will be signed

* bugfix

* Add basic merge check and better information for CRUD

* linting comment

* Add descriptors to merge signing

* Slight refactor

* Slight improvement to appearances

* Handle Merge API

* manage CRUD API

* Move error to error.go

* Remove fix to delete.go

* prep for merge

* need to tolerate \r\n in message

* check protected branch before trying to load it

* Apply suggestions from code review

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

* fix commit-reader

Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>

5 files changed, 169 insertions, 18 deletions

diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index 6b643371e5..bca756aea1 100644
--- a/routers/api/v1/repo/pull.go
+++ b/routers/api/v1/repo/pull.go
@@ -639,6 +639,15 @@ 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)
+		return
+	}
+
 	if len(form.Do) == 0 {
 		form.Do = string(models.MergeStyleMerge)
 	}
diff --git a/routers/private/hook.go b/routers/private/hook.go
index b4626fddf4..6a07de15ff 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -6,7 +6,10 @@
 package private
 
 import (
+	"bufio"
+	"context"
 	"fmt"
+	"io"
 	"net/http"
 	"os"
 	"strings"
@@ -18,10 +21,101 @@ 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"
 )
 
+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 for %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 for %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")
@@ -35,6 +129,30 @@ 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()
+
+	// 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]
@@ -51,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{}{
@@ -62,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)
@@ -92,6 +196,27 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
 
 				}
 			}
+
+			// Require signed commits
+			if protectBranch.RequireSignedCommits {
+				err := verifyCommits(oldCommitID, newCommitID, gitRepo, env)
+				if err != nil {
+					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
+				}
+			}
+
 			canPush := false
 			if opts.IsDeployKey {
 				canPush = protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys)
diff --git a/routers/repo/editor.go b/routers/repo/editor.go
index 82c74ba75f..8d4f1f8827 100644
--- a/routers/repo/editor.go
+++ b/routers/repo/editor.go
@@ -36,12 +36,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/routers/repo/issue.go b/routers/repo/issue.go
index ce3eb5bd2c..afc115c6e2 100644
--- a/routers/repo/issue.go
+++ b/routers/repo/issue.go
@@ -971,6 +971,21 @@ 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
+			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/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go
index da28ac50be..e8902ed8ac 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,