Fix C2SP compliance and simplify (#325)

api/http.go

@@ -31,6 +31,7 @@ const (
 // checkpoint byte slice and a consistency proof (slice of slices).  The logID
 // is part of the request URL.
 type UpdateRequest struct {
+	OldSize    uint64
 	Checkpoint []byte
 	Proof      [][]byte
 }

cmd/feedbastion/main.go

@@ -122,7 +122,7 @@ func (b *bastionClient) GetLatestCheckpoint(ctx context.Context, logID string) (
 // Update attempts to clock the witness forward for the given logID.
 // The latest signed checkpoint will be returned if this succeeds, or if the error is
 // http.ErrCheckpointTooOld. In all other cases no checkpoint should be expected.
-func (b *bastionClient) Update(ctx context.Context, logID string, newCP []byte, proof [][]byte) ([]byte, error) {
+func (b *bastionClient) Update(ctx context.Context, logID string, oldSize uint64, newCP []byte, proof [][]byte) ([]byte, error) {
 	// The request body MUST be a sequence of
 	// - a previous size line,
 	// - zero or more consistency proof lines,

internal/feeder/bastion/bastion_feeder.go

@@ -38,12 +38,11 @@ import (
 	"github.com/transparency-dev/formats/log"
 	"github.com/transparency-dev/witness/internal/config"
 	"github.com/transparency-dev/witness/internal/feeder"
+	"github.com/transparency-dev/witness/internal/witness"
 	"github.com/transparency-dev/witness/monitoring"
 	"golang.org/x/mod/sumdb/note"
 	"golang.org/x/net/http2"
 	"golang.org/x/time/rate"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 	"k8s.io/klog/v2"
 )
 
@@ -148,49 +147,65 @@ func (a *addHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		counterBastionIncomingResponse.Inc(bastionID, "unknown", strconv.Itoa(http.StatusNotFound))
 		return
 	}
-	signedCP, updateErr := a.w.Update(r.Context(), logID, cp, proof)
 
-	if updateErr != nil {
-		if sc := status.Code(updateErr); sc == codes.FailedPrecondition {
-			// Invalid proof
-			klog.Infof("Invalid proof: %v\noldSize: %d\nnewCP:\n%s\nProof:\n%s", updateErr, oldSize, cp, proof)
-			w.WriteHeader(http.StatusForbidden)
-			counterBastionIncomingResponse.Inc(bastionID, logCfg.Origin, strconv.Itoa(http.StatusForbidden))
-			return
-		}
-		if sc := status.Code(updateErr); sc == codes.AlreadyExists {
-			// old checkpoint is smaller than the latest the witness knows about
-			checkpoint, _, _, cpErr := log.ParseCheckpoint(signedCP, logCfg.Origin, a.witVerifier)
-			if cpErr != nil {
-				klog.V(1).Infof("invalid checkpoint: %v", cpErr)
-				w.WriteHeader(http.StatusBadRequest)
-				counterBastionIncomingResponse.Inc(bastionID, logCfg.Origin, strconv.Itoa(http.StatusBadRequest))
-				return
-			}
-			w.Header().Add("Content-Type", "text/x.tlog.size")
-			w.WriteHeader(http.StatusConflict)
-			counterBastionIncomingResponse.Inc(bastionID, logCfg.Origin, strconv.Itoa(http.StatusConflict))
-			if _, err := w.Write([]byte(fmt.Sprintf("%d\n", checkpoint.Size))); err != nil {
-				klog.V(1).Infof("Failed to write size response: %v", err)
-			}
-			return
+	sc, body, contentType, err := a.handleUpdate(r.Context(), logID, logCfg.Origin, oldSize, cp, proof)
+	if err != nil {
+		klog.Errorf("handleUpdate: %v", err)
+		w.WriteHeader(http.StatusInternalServerError)
+		counterBastionIncomingResponse.Inc(bastionID, logCfg.Origin, strconv.Itoa(http.StatusInternalServerError))
+		return
+	}
+
+	if contentType != "" {
+		w.Header().Add("Content-Type", contentType)
+	}
+	w.WriteHeader(sc)
+	if len(body) > 0 {
+		if _, err := w.Write(body); err != nil {
+			klog.Infof("Failed to write response body: %v", err)
 		}
 	}
+	counterBastionIncomingResponse.Inc(bastionID, logCfg.Origin, strconv.Itoa(sc))
+}
 
-	_, _, n, cpErr := log.ParseCheckpoint(signedCP, logCfg.Origin, a.witVerifier)
+// handleUpdate submits the provided checkpoint to the witness and interprets any errors which may result.
+//
+// Returns an appropriate HTTP status code, response body, and Content Type representing the outcome.
+func (a *addHandler) handleUpdate(ctx context.Context, logID string, origin string, oldSize uint64, newCP []byte, proof [][]byte) (int, []byte, string, error) {
+	trusted, updateErr := a.w.Update(ctx, logID, oldSize, newCP, proof)
+	// Whatever happened, we usually get the latest trusted CP from the witness (whether it's the old one or the one we've just updated to).
+	// If we get nothing at all, then something's gone quite wrong.
+	if trusted == nil {
+		return http.StatusInternalServerError, nil, "", fmt.Errorf("something went quite wrong during update: %v", updateErr)
+	}
+	// We'll need to use the old CP when sending responses, so parse it once here:
+	trustedCP, _, n, cpErr := log.ParseCheckpoint(trusted, origin, a.witVerifier)
 	if cpErr != nil {
-		klog.V(1).Infof("invalid checkpoint: %v", cpErr)
-		w.WriteHeader(http.StatusBadRequest)
-		counterBastionIncomingResponse.Inc(bastionID, logCfg.Origin, strconv.Itoa(http.StatusBadRequest))
-		return
+		return http.StatusInternalServerError, nil, "", fmt.Errorf("invalid stored checkpoint!: %v", cpErr)
 	}
 
-	if _, err := w.Write([]byte(fmt.Sprintf("— %s %s\n", n.Sigs[0].Name, n.Sigs[0].Base64))); err != nil {
-		klog.V(1).Infof("Failed to write signature response: %v", err)
+	// Finally, handle any "soft" error from the update:
+	if updateErr != nil {
+		switch updateErr {
+		case witness.ErrCheckpointStale:
+			return http.StatusConflict, []byte(fmt.Sprintf("%d\n", trustedCP.Size)), "text/x.tlog.size", nil
+		case witness.ErrUnknownLog:
+			return http.StatusNotFound, nil, "", nil
+		case witness.ErrNoValidSignature:
+			return http.StatusForbidden, nil, "", nil
+		case witness.ErrOldSizeInvalid:
+			return http.StatusBadRequest, nil, "", nil
+		case witness.ErrInvalidProof:
+			return http.StatusUnprocessableEntity, nil, "", nil
+		case witness.ErrRootMismatch:
+			return http.StatusConflict, nil, "", nil
+		default:
+			return http.StatusInternalServerError, nil, "", updateErr
+		}
 	}
 
-	counterBastionIncomingResponse.Inc(bastionID, logCfg.Origin, strconv.Itoa(200))
-
+	body := []byte(fmt.Sprintf("— %s %s\n", n.Sigs[0].Name, n.Sigs[0].Base64))
+	return http.StatusOK, body, "", nil
 }
 
 // parseBody reads the incoming request and parses into constituent parts.

internal/feeder/bastion/bastion_feeder_test.go

@@ -16,16 +16,29 @@ package bastion
 
 import (
 	"bytes"
+	"context"
 	"encoding/base64"
+	"fmt"
+	"net/http"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/transparency-dev/formats/note"
+	"github.com/transparency-dev/witness/internal/config"
+	"github.com/transparency-dev/witness/internal/witness"
 )
 
 const (
-	testCP = "56\n7azctENRYLlBCBQ5OX2qxxIKCTOeCda1KfTwjdt0wdA=\n\n— transparency.dev-aw-ftlog-ci-2 93xidocoWXVph2jEuzW2oovU+IjU71+FeVGKtKXQknSla2HCvr6RYHRSdJfxpo4kj5geqxkjrDXcbpiSo7lK96X4Dgc=\n"
+	testCPOrigin = "transparency.dev/armored-witness/firmware_transparency/ci/2"
+	testCPSize   = 56
+	testCPRoot   = "7azctENRYLlBCBQ5OX2qxxIKCTOeCda1KfTwjdt0wdA="
+	testCPSig    = "— transparency.dev-aw-ftlog-ci-2 93xidocoWXVph2jEuzW2oovU+IjU71+FeVGKtKXQknSla2HCvr6RYHRSdJfxpo4kj5geqxkjrDXcbpiSo7lK96X4Dgc=\n"
+
+	testCPVerifier = "transparency.dev-aw-ftlog-ci-2+f77c6276+AZXqiaARpwF4MoNOxx46kuiIRjrML0PDTm+c7BLaAMt6"
 )
 
+var testCP = fmt.Sprintf("%s\n%d\n%s\n\n%s", testCPOrigin, testCPSize, testCPRoot, testCPSig)
+
 func TestParseBody(t *testing.T) {
 	for _, test := range []struct {
 		name            string
@@ -75,6 +88,105 @@ func TestParseBody(t *testing.T) {
 	}
 }
 
+func TestHandler(t *testing.T) {
+	v, err := note.NewVerifier(testCPVerifier)
+	if err != nil {
+		t.Fatalf("NewVerifier: %v", err)
+	}
+	logID := "logID"
+	logs := map[string]config.Log{
+		logID: config.Log{Origin: testCPOrigin},
+	}
+	for _, test := range []struct {
+		name string
+		// fake witness control
+		witnessResp []byte
+		witnessErr  error
+		// responses
+		wantBody        string
+		wantStatus      int
+		wantContentType string
+	}{
+		{
+			name:        "Accepted by witness",
+			witnessResp: []byte(testCP),
+			wantStatus:  200,
+			wantBody:    testCPSig,
+		}, {
+			name:            "ErrCheckpointStale",
+			witnessResp:     []byte(testCP),
+			witnessErr:      witness.ErrCheckpointStale,
+			wantStatus:      http.StatusConflict,
+			wantContentType: "text/x.tlog.size",
+			wantBody:        fmt.Sprintf("%d\n", testCPSize),
+		}, {
+			name:        "ErrNoValidSignature",
+			witnessResp: []byte(testCP),
+			witnessErr:  witness.ErrNoValidSignature,
+			wantStatus:  http.StatusForbidden,
+		}, {
+			name:        "ErrUnknownLog",
+			witnessResp: []byte(testCP),
+			witnessErr:  witness.ErrUnknownLog,
+			wantStatus:  http.StatusNotFound,
+		}, {
+			name:        "ErrInvalidProof",
+			witnessResp: []byte(testCP),
+			witnessErr:  witness.ErrInvalidProof,
+			wantStatus:  http.StatusUnprocessableEntity,
+		}, {
+			name:        "ErrOldSizeInvalid",
+			witnessResp: []byte(testCP),
+			witnessErr:  witness.ErrOldSizeInvalid,
+			wantStatus:  http.StatusBadRequest,
+		}, {
+			name:        "ErrRootMismatch",
+			witnessResp: []byte(testCP),
+			witnessErr:  witness.ErrRootMismatch,
+			wantStatus:  http.StatusConflict,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			a := addHandler{
+				w:           &testWitness{updateResponse: test.witnessResp, updateErr: test.witnessErr},
+				witVerifier: v,
+				logs:        logs,
+			}
+			if err != nil {
+				t.Fatalf("NewRequest: %v", err)
+			}
+			sc, body, ct, err := a.handleUpdate(context.Background(), "logID", testCPOrigin, 0, []byte(testCP), [][]byte{})
+			if err != nil {
+				t.Fatalf("handleUpdate: %v", err)
+			}
+			if got, want := sc, test.wantStatus; got != want {
+				t.Errorf("handleUpdate got status %d, want %d", got, want)
+			}
+			if got, want := ct, test.wantContentType; got != want {
+				t.Errorf("handleUpdate got content type %q, want %q", got, want)
+			}
+			if got, want := string(body), test.wantBody; got != want {
+				t.Errorf("handleUpdate got body %q, %q", got, want)
+			}
+		})
+	}
+}
+
+type testWitness struct {
+	latestCPErr    error
+	latestCP       []byte
+	updateErr      error
+	updateResponse []byte
+}
+
+func (tw *testWitness) GetLatestCheckpoint(ctx context.Context, logID string) ([]byte, error) {
+	return tw.latestCP, tw.latestCPErr
+}
+
+func (tw *testWitness) Update(ctx context.Context, logID string, oldSize uint64, newCP []byte, proof [][]byte) ([]byte, error) {
+	return tw.updateResponse, tw.updateErr
+}
+
 func d64(t *testing.T, s string) []byte {
 	t.Helper()
 	r, err := base64.StdEncoding.DecodeString(s)

internal/feeder/feeder.go

@@ -40,7 +40,7 @@ type Witness interface {
 	// Update attempts to clock the witness forward for the given logID.
 	// The latest signed checkpoint will be returned if this succeeds, or if the error is
 	// http.ErrCheckpointTooOld. In all other cases no checkpoint should be expected.
-	Update(ctx context.Context, logID string, newCP []byte, proof [][]byte) ([]byte, error)
+	Update(ctx context.Context, logID string, oldSize uint64, newCP []byte, proof [][]byte) ([]byte, error)
 }
 
 // FeedOpts holds parameters when calling the Feed function.
@@ -150,7 +150,7 @@ func submitToWitness(ctx context.Context, cpRaw []byte, cpSubmit log.Checkpoint,
 			}
 			if latestCP.Size == cpSubmit.Size && bytes.Equal(latestCP.Hash, cpSubmit.Hash) {
 				klog.V(1).Infof("%q unchanged - @%d: %x", logName, latestCP.Size, latestCP.Hash)
-				if returnCp, err = opts.Witness.Update(ctx, opts.LogID, cpRaw, [][]byte{}); err != nil {
+				if returnCp, err = opts.Witness.Update(ctx, opts.LogID, latestCP.Size, cpRaw, [][]byte{}); err != nil {
 					e := fmt.Errorf("failed to submit fresh checkpoint to witness: %v", err)
 					klog.Warning(e.Error())
 					return e
@@ -171,7 +171,7 @@ func submitToWitness(ctx context.Context, cpRaw []byte, cpSubmit log.Checkpoint,
 		}
 		klog.V(2).Infof("%q %d -> %d proof: %x", logName, latestCP.Size, cpSubmit.Size, conP)
 
-		if returnCp, err = opts.Witness.Update(ctx, opts.LogID, cpRaw, conP); err != nil {
+		if returnCp, err = opts.Witness.Update(ctx, opts.LogID, latestCP.Size, cpRaw, conP); err != nil {
 			e := fmt.Errorf("failed to submit checkpoint to witness: %v", err)
 			klog.Warning(e.Error())
 			return e

internal/feeder/feeder_test.go

@@ -134,7 +134,7 @@ func (fw *fakeWitness) GetLatestCheckpoint(_ context.Context, logID string) ([]b
 	return fw.latestCP, nil
 }
 
-func (fw *fakeWitness) Update(_ context.Context, logID string, newCP []byte, proof [][]byte) ([]byte, error) {
+func (fw *fakeWitness) Update(_ context.Context, logID string, oldSize uint64, newCP []byte, proof [][]byte) ([]byte, error) {
 	if fw.rejectUpdate {
 		return nil, errors.New("computer says 'no'")
 	}

internal/http/server.go

@@ -18,7 +18,6 @@ package http
 import (
 	"encoding/json"
 	"fmt"
-	"io"
 	"net/http"
 
 	"github.com/gorilla/mux"
@@ -41,44 +40,6 @@ func NewServer(witness *witness.Witness) *Server {
 	}
 }
 
-// update handles requests to update checkpoints.
-// It expects a POSTed body containing a JSON-formatted api.UpdateRequest
-// statement and returns a JSON-formatted api.UpdateResponse statement.
-func (s *Server) update(w http.ResponseWriter, r *http.Request) {
-	v := mux.Vars(r)
-	logID := v["logid"]
-	body, err := io.ReadAll(r.Body)
-	if err != nil {
-		http.Error(w, fmt.Sprintf("cannot read request body: %v", err.Error()), http.StatusBadRequest)
-		return
-	}
-	var req api.UpdateRequest
-	if err := json.Unmarshal(body, &req); err != nil {
-		http.Error(w, fmt.Sprintf("cannot parse request body as proper JSON struct: %v", err.Error()), http.StatusBadRequest)
-		return
-	}
-	// Get the output from the witness.
-	chkpt, err := s.w.Update(r.Context(), logID, req.Checkpoint, req.Proof)
-	if err != nil {
-		c := status.Code(err)
-		// If there was an AlreadyExists it's possible the caller was
-		// just out of date.  Give the returned checkpoint to help them
-		// form a new request.
-		if c == codes.AlreadyExists {
-			w.Header().Set("X-Content-Type-Options", "nosniff")
-			w.WriteHeader(httpForCode(c))
-			// The checkpoint body will be written below...
-		} else {
-			http.Error(w, fmt.Sprintf("failed to update to new checkpoint: %v", err), httpForCode(c))
-			return
-		}
-	}
-	w.Header().Set("Content-Type", "text/plain; charset=utf-8")
-	if _, err := w.Write(chkpt); err != nil {
-		klog.Warningf("Error writing response: %v", err)
-	}
-}
-
 // getCheckpoint returns a checkpoint stored for a given log.
 func (s *Server) getCheckpoint(w http.ResponseWriter, r *http.Request) {
 	v := mux.Vars(r)
@@ -117,7 +78,6 @@ func (s *Server) getLogs(w http.ResponseWriter, r *http.Request) {
 func (s *Server) RegisterHandlers(r *mux.Router) {
 	logStr := "{logid:[a-zA-Z0-9-]+}"
 	r.HandleFunc(fmt.Sprintf(api.HTTPGetCheckpoint, logStr), s.getCheckpoint).Methods(http.MethodGet)
-	r.HandleFunc(fmt.Sprintf(api.HTTPUpdate, logStr), s.update).Methods(http.MethodPut)
 	r.HandleFunc(api.HTTPGetLogs, s.getLogs).Methods(http.MethodGet)
 }
 
@@ -127,7 +87,7 @@ func httpForCode(c codes.Code) int {
 		return http.StatusConflict
 	case codes.NotFound:
 		return http.StatusNotFound
-	case codes.FailedPrecondition, codes.InvalidArgument:
+	case codes.FailedPrecondition, codes.InvalidArgument, codes.Unauthenticated:
 		return http.StatusBadRequest
 	default:
 		return http.StatusInternalServerError