Submitting review 8a2966d

Adds GPG signing and verification

Addresses #89

Hey there!

Here's my work so far w.r.t. implementing signing and verification. As it stands now, signing is available for both requests and acceptances, and we can verify those at `pull` time. I wanted to share now to see if there were any major contentions with the path I've gone down before I went through the rote repetition of adding signing functionality to the other comment-based commands.

Cheers!

commands/abandon.go

@@ -25,6 +25,7 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 	"github.com/google/git-appraise/review/request"
 )
 
@@ -33,6 +34,9 @@ var abandonFlagSet = flag.NewFlagSet("abandon", flag.ExitOnError)
 var (
 	abandonMessageFile = abandonFlagSet.String("F", "", "Take the comment from the given file. Use - to read the message from the standard input")
 	abandonMessage     = abandonFlagSet.String("m", "", "Message to attach to the review")
+
+	abandonSign = abandonFlagSet.Bool("S", false,
+		"Sign the contents of the abandonment")
 )
 
 // abandonReview adds an NMW comment to the current code review.
@@ -88,14 +92,32 @@ func abandonReview(repo repository.Repo, args []string) error {
 	c.Location = &location
 	c.Resolved = &resolved
 
-	err = r.AddComment(c)
+	var key string
+	if *abandonSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
 
+	err = r.AddComment(c)
 	if err != nil {
 		return err
 	}
 
 	// Empty target ref indicates that request was abandoned
 	r.Request.TargetRef = ""
+	// (re)sign the request after clearing out `TargetRef'.
+	if *abandonSign {
+		err = gpg.Sign(key, &r.Request)
+		if err != nil {
+			return err
+		}
+	}
 
 	note, err := r.Request.Write()
 	if err != nil {

commands/accept.go

@@ -24,13 +24,17 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 var acceptFlagSet = flag.NewFlagSet("accept", flag.ExitOnError)
 
 var (
 	acceptMessageFile = acceptFlagSet.String("F", "", "Take the comment from the given file. Use - to read the message from the standard input")
 	acceptMessage     = acceptFlagSet.String("m", "", "Message to attach to the review")
+
+	acceptSign = acceptFlagSet.Bool("S", false,
+		"sign the contents of the acceptance")
 )
 
 // acceptReview adds an LGTM comment to the current code review.
@@ -80,6 +84,16 @@ func acceptReview(repo repository.Repo, args []string) error {
 	c := comment.New(userEmail, *acceptMessage)
 	c.Location = &location
 	c.Resolved = &resolved
+	if *acceptSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
 	return r.AddComment(c)
 }
 

commands/comment.go

@@ -25,6 +25,7 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 var commentFlagSet = flag.NewFlagSet("comment", flag.ExitOnError)
@@ -37,6 +38,8 @@ var (
 	commentFile        = commentFlagSet.String("f", "", "File being commented upon")
 	commentLgtm        = commentFlagSet.Bool("lgtm", false, "'Looks Good To Me'. Set this to express your approval. This cannot be combined with nmw")
 	commentNmw         = commentFlagSet.Bool("nmw", false, "'Needs More Work'. Set this to express your disapproval. This cannot be combined with lgtm")
+	commentSign        = commentFlagSet.Bool("S", false,
+		"Sign the contents of the comment")
 )
 
 func init() {
@@ -129,6 +132,18 @@ func commentOnReview(repo repository.Repo, args []string) error {
 		resolved := *commentLgtm
 		c.Resolved = &resolved
 	}
+
+	if *commentSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
+
 	return r.AddComment(c)
 }
 

commands/pull.go

@@ -18,30 +18,73 @@ package commands
 
 import (
 	"errors"
+	"flag"
 	"fmt"
+
 	"github.com/google/git-appraise/repository"
+	"github.com/google/git-appraise/review"
+)
+
+var (
+	pullFlagSet = flag.NewFlagSet("pull", flag.ExitOnError)
+	pullVerify  = pullFlagSet.Bool("verify-signatures", false,
+		"verify the signatures of pulled reviews")
 )
 
-// pull updates the local git-notes used for reviews with those from a remote repo.
+// pull updates the local git-notes used for reviews with those from a remote
+// repo.
 func pull(repo repository.Repo, args []string) error {
-	if len(args) > 1 {
-		return errors.New("Only pulling from one remote at a time is supported.")
+	pullFlagSet.Parse(args)
+	pullArgs := pullFlagSet.Args()
+
+	if len(pullArgs) > 1 {
+		return errors.New(
+			"Only pulling from one remote at a time is supported.")
 	}
 
 	remote := "origin"
-	if len(args) == 1 {
-		remote = args[0]
+	if len(pullArgs) == 1 {
+		remote = pullArgs[0]
+	}
+	// This is the easy case. We're not checking signatures so just go the
+	// normal route.
+	if !*pullVerify {
+		return repo.PullNotesAndArchive(remote, notesRefPattern,
+			archiveRefPattern)
+	}
+
+	// Otherwise, we collect the fetched reviewed revisions (their hashes), get
+	// their reviews, and then one by one, verify them. If we make it through
+	// the set, _then_ we merge the remote reference into the local branch.
+	revisions, err := repo.FetchAndReturnNewReviewHashes(remote,
+		notesRefPattern, archiveRefPattern)
+	if err != nil {
+		return err
+	}
+	for _, revision := range revisions {
+		rvw, err := review.GetSummaryViaRefs(repo,
+			"refs/notes/"+remote+"/devtools/reviews",
+			"refs/notes/"+remote+"/devtools/discuss", revision)
+		if err != nil {
+			return err
+		}
+		err = rvw.Verify()
+		if err != nil {
+			return err
+		}
+		fmt.Println("verified review:", revision)
 	}
 
-	if err := repo.PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern); err != nil {
+	err = repo.MergeNotes(remote, notesRefPattern)
+	if err != nil {
 		return err
 	}
-	return nil
+	return repo.MergeArchives(remote, archiveRefPattern)
 }
 
 var pullCmd = &Command{
 	Usage: func(arg0 string) {
-		fmt.Printf("Usage: %s pull [<remote>]\n", arg0)
+		fmt.Printf("Usage: %s pull [<option>] [<remote>]\n", arg0)
 	},
 	RunMethod: func(repo repository.Repo, args []string) error {
 		return pull(repo, args)

commands/rebase.go

@@ -29,6 +29,8 @@ var rebaseFlagSet = flag.NewFlagSet("rebase", flag.ExitOnError)
 
 var (
 	rebaseArchive = rebaseFlagSet.Bool("archive", true, "Prevent the original commit from being garbage collected.")
+	rebaseSign    = rebaseFlagSet.Bool("S", false,
+		"Sign the contents of the request after the rebase")
 )
 
 // Validate that the user's request to rebase a review makes sense.
@@ -80,6 +82,9 @@ func rebaseReview(repo repository.Repo, args []string) error {
 	if err != nil {
 		return err
 	}
+	if *rebaseSign {
+		return r.RebaseAndSign(*rebaseArchive)
+	}
 	return r.Rebase(*rebaseArchive)
 }
 

commands/reject.go

@@ -25,13 +25,17 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 var rejectFlagSet = flag.NewFlagSet("reject", flag.ExitOnError)
 
 var (
 	rejectMessageFile = rejectFlagSet.String("F", "", "Take the comment from the given file. Use - to read the message from the standard input")
 	rejectMessage     = rejectFlagSet.String("m", "", "Message to attach to the review")
+
+	rejectSign = rejectFlagSet.Bool("S", false,
+		"Sign the contents of the rejection")
 )
 
 // rejectReview adds an NMW comment to the current code review.
@@ -90,6 +94,16 @@ func rejectReview(repo repository.Repo, args []string) error {
 	c := comment.New(userEmail, *rejectMessage)
 	c.Location = &location
 	c.Resolved = &resolved
+	if *rejectSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
 	return r.AddComment(c)
 }
 

commands/request.go

@@ -20,10 +20,12 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"strings"
+
 	"github.com/google/git-appraise/commands/input"
 	"github.com/google/git-appraise/repository"
+	"github.com/google/git-appraise/review/gpg"
 	"github.com/google/git-appraise/review/request"
-	"strings"
 )
 
 // Template for the "request" subcommand's output.
@@ -44,6 +46,8 @@ var (
 	requestTarget           = requestFlagSet.String("target", "refs/heads/master", "Revision against which to review")
 	requestQuiet            = requestFlagSet.Bool("quiet", false, "Suppress review summary output")
 	requestAllowUncommitted = requestFlagSet.Bool("allow-uncommitted", false, "Allow uncommitted local changes.")
+	requestSign             = requestFlagSet.Bool("S", false,
+		"GPG sign the content of the request")
 )
 
 // Build the template review request based solely on the parsed flag values.
@@ -145,7 +149,16 @@ func requestReview(repo repository.Repo, args []string) error {
 		}
 		r.Description = description
 	}
-
+	if *requestSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &r)
+		if err != nil {
+			return err
+		}
+	}
 	note, err := r.Write()
 	if err != nil {
 		return err

commands/submit.go

@@ -32,6 +32,9 @@ var (
 	submitFastForward = submitFlagSet.Bool("fast-forward", false, "Create a merge using the default fast-forward mode.")
 	submitTBR         = submitFlagSet.Bool("tbr", false, "(To be reviewed) Force the submission of a review that has not been accepted.")
 	submitArchive     = submitFlagSet.Bool("archive", true, "Prevent the original commit from being garbage collected; only affects rebased submits.")
+
+	submitSign = submitFlagSet.Bool("S", false,
+		"Sign the contents of the submission")
 )
 
 // Submit the current code review request.
@@ -105,9 +108,16 @@ func submitReview(repo repository.Repo, args []string) error {
 	}
 
 	if *submitRebase {
-		if err := r.Rebase(*submitArchive); err != nil {
+		var err error
+		if *submitSign {
+			err = r.RebaseAndSign(*submitArchive)
+		} else {
+			err = r.Rebase(*submitArchive)
+		}
+		if err != nil {
 			return err
 		}
+
 		source, err = r.GetHeadCommit()
 		if err != nil {
 			return err
@@ -119,9 +129,19 @@ func submitReview(repo repository.Repo, args []string) error {
 	}
 	if *submitMerge {
 		submitMessage := fmt.Sprintf("Submitting review %.12s", r.Revision)
-		return repo.MergeRef(source, false, submitMessage, r.Request.Description)
+		if *submitSign {
+			return repo.MergeAndSignRef(source, false, submitMessage,
+				r.Request.Description)
+		} else {
+			return repo.MergeRef(source, false, submitMessage,
+				r.Request.Description)
+		}
 	} else {
-		return repo.MergeRef(source, true)
+		if *submitSign {
+			return repo.MergeAndSignRef(source, true)
+		} else {
+			return repo.MergeRef(source, true)
+		}
 	}
 }