Avoid re-entrant transactions

persistence/sqlite/sql_test.go

@@ -15,14 +15,19 @@
 package sqlite
 
 import (
-	"testing"
-
+	"context"
 	"database/sql"
+	"encoding/hex"
+	"testing"
+	"time"
 
-	_ "modernc.org/sqlite" // Load drivers for sqlite3
 	"github.com/transparency-dev/formats/log"
+	f_note "github.com/transparency-dev/formats/note"
 	"github.com/transparency-dev/witness/omniwitness"
+	"github.com/transparency-dev/witness/witness"
 	ptest "github.com/transparency-dev/witness/persistence/testonly"
+	"golang.org/x/mod/sumdb/note"
+	_ "modernc.org/sqlite" // Load drivers for sqlite3
 )
 
 func TestUpdate(t *testing.T) {
@@ -149,3 +154,79 @@ func TestDisabledLogs(t *testing.T) {
 		}
 	}
 }
+
+func TestDeadlock(t *testing.T) {
+	db, cleanup := mustCreateDB(t) // MaxOpenConns(1)
+	defer func() { _ = cleanup() }()
+
+	p := New(db)
+	if err := p.Init(t.Context()); err != nil {
+		t.Fatalf("Init(): %v", err)
+	}
+
+	mPK := "monkeys+db4d9f7e+AULaJMvTtDLHPUcUrjdDad9vDlh/PTfC2VV60JUtCfWT"
+	wSK := "PRIVATE+KEY+witness+f13a86db+AaLa/dfyBhyo/m0Z7WCi98ENVZWtrP8pxgRNrx7tIWiA"
+
+	logV, err := note.NewVerifier(mPK)
+	if err != nil {
+		t.Fatalf("NewVerifier: %v", err)
+	}
+	err = p.AddLogs(t.Context(), []omniwitness.Log{
+		{Origin: "monkeys", VKey: mPK, Verifier: logV},
+	})
+	if err != nil {
+		t.Fatalf("AddLogs: %v", err)
+	}
+
+	ns, err := f_note.NewSignerForCosignatureV1(wSK)
+	if err != nil {
+		t.Fatalf("NewSignerForCosignatureV1: %v", err)
+	}
+
+	w, err := witness.New(t.Context(), witness.Opts{
+		Persistence: p,
+		Signers:     []note.Signer{ns},
+		VerifierForLog: func(ctx context.Context, origin string) (note.Verifier, bool, error) {
+			l, ok, err := p.Log(ctx, origin)
+			if err != nil || !ok {
+				return nil, ok, err
+			}
+			return l.Verifier, true, nil
+		},
+	})
+	if err != nil {
+		t.Fatalf("witness.New: %v", err)
+	}
+
+	mInit := []byte("monkeys\n5\n41smjBUiAU70EtKlT6lIOIYtRTYxYXsDB+XHfcvu/BE=\n\n— monkeys 202fftzGl3LVoqjXfwCFZZXs8I+5G22+Ek2K0AOyBuSJ/8/CZawNF+6fNlTKOCd622pbzJNkkJFWuw9DbicZCkEx9AY=\n")
+	mNext := []byte("monkeys\n8\nV8K9aklZ4EPB+RMOk1/8VsJUdFZR77GDtZUQq84vSbo=\n\n— monkeys 202ffoUEboiQYpHzICeaFmoy3RNviHTpAxYrq/eO4QQVQMvu9UebKBMX2MJC76NLthZaKsnKbCA8GxrjePZhvDCH7Ag=\n")
+
+	dh := func(h string) []byte {
+		r, err := hex.DecodeString(h)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return r
+	}
+	consProof := [][]byte{
+		dh("b9e1d62618f7fee8034e4c5010f727ab24d8e4705cb296c374bf2025a87a10d2"),
+		dh("aac66cd7a79ce4012d80762fe8eec3a77f22d1ca4145c3f4cee022e7efcd599d"),
+		dh("89d0f753f66a290c483b39cd5e9eafb12021293395fad3d4a2ad053cfbcfdc9e"),
+		dh("29e40bb79c966f4c6fe96aff6f30acfce5f3e8d84c02215175d6e018a5dee833"),
+	}
+
+	// First update (TOFU) - should succeed.
+	_, _, err = w.Update(t.Context(), 0, mInit, nil)
+	if err != nil {
+		t.Fatalf("First Update (TOFU) failed: %v", err)
+	}
+
+	// Second update (consistent transition)
+	ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second)
+	defer cancel()
+
+	_, _, err = w.Update(ctx, 5, mNext, consProof)
+	if err != nil {
+		t.Fatalf("Second Update failed (expected success with fix): %v", err)
+	}
+}

witness/witness.go

@@ -24,6 +24,7 @@ import (
 	"encoding/binary"
 	"errors"
 	"fmt"
+	"strconv"
 	"strings"
 	"unicode"
 	"unicode/utf8"
@@ -112,24 +113,6 @@ func New(ctx context.Context, wo Opts) (*Witness, error) {
 	}, nil
 }
 
-// verifyCheckpoint verifies the checkpoint under the appropriate key for the origin and returns
-// the parsed checkpoint and the note itself.
-func (w *Witness) verifyCheckpoint(ctx context.Context, chkptRaw []byte) (*log.Checkpoint, *note.Note, string, error) {
-	origin, _, found := strings.Cut(string(chkptRaw), "\n")
-	if !found {
-		return nil, nil, "", errors.New("invalid checkpoint")
-	}
-	v, ok, err := w.VerifierForLog(ctx, origin)
-	if err != nil {
-		return nil, nil, "", err
-	}
-	if !ok {
-		return nil, nil, "", ErrUnknownLog
-	}
-	cp, _, n, err := log.ParseCheckpoint(chkptRaw, origin, v)
-	return cp, n, origin, err
-}
-
 // GetCheckpoint gets a checkpoint for a given log, which is consistent with all
 // other checkpoints for the same log signed by this witness.
 //
@@ -153,7 +136,21 @@ func (w *Witness) Update(ctx context.Context, oldSize uint64, nextRaw []byte, cP
 	//
 	// SPEC: The witness MUST verify the checkpoint signature against the public key(s) it trusts for the
 	//       checkpoint origin, and it MUST ignore signatures from unknown keys.
-	next, nextNote, origin, err := w.verifyCheckpoint(ctx, nextRaw)
+	next, nextNote, origin, err := func() (*log.Checkpoint, *note.Note, string, error) {
+		origin, _, found := strings.Cut(string(nextRaw), "\n")
+		if !found {
+			return nil, nil, "", errors.New("invalid checkpoint")
+		}
+		v, ok, err := w.VerifierForLog(ctx, origin)
+		if err != nil {
+			return nil, nil, "", err
+		}
+		if !ok {
+			return nil, nil, "", ErrUnknownLog
+		}
+		cp, _, n, err := log.ParseCheckpoint(nextRaw, origin, v)
+		return cp, n, origin, err
+	}()
 	if err != nil {
 		return nil, 0, err
 	}
@@ -179,33 +176,39 @@ func (w *Witness) Update(ctx context.Context, oldSize uint64, nextRaw []byte, cP
 			return signed, nil
 		}
 
-		prev, _, _, err := w.verifyCheckpoint(ctx, prevRaw)
+		// The persistence layer is within the TCB so we assume that whatever we read is a valid log checkpoint.
+		// Avoiding revalidating this gives us more flexibility to support log key rotation.
+		prevOrigin, prevSize, prevHash, err := checkpointUnsafe(prevRaw)
 		if err != nil {
 			retSize, retSigs = 0, nil
 			return nil, fmt.Errorf("couldn't parse stored checkpoint: %v", err)
 		}
+		if prevOrigin != origin {
+			retSize, retSigs = 0, nil
+			return nil, fmt.Errorf("origin didn't match during update. prev=%q, next=%q", prevOrigin, origin)
+		}
 
 		// SPEC: The old size MUST be equal to or lower than the (submitted) checkpoint size.
 		if oldSize > next.Size {
-			retSize, retSigs = prev.Size, nil
+			retSize, retSigs = prevSize, nil
 			return nil, ErrOldSizeInvalid
 		}
 		// SPEC: The witness MUST check that the old size matches the size of the latest checkpoint it cosigned
 		//       for the checkpoint's origin (or zero if it never cosigned a checkpoint for that origin)
-		if oldSize != prev.Size {
-			retSize, retSigs = prev.Size, nil
-			return nil, fmt.Errorf("%w (%d != %d)", ErrCheckpointStale, oldSize, prev.Size)
+		if oldSize != prevSize {
+			retSize, retSigs = prevSize, nil
+			return nil, fmt.Errorf("%w (%d != %d)", ErrCheckpointStale, oldSize, prevSize)
 		}
 		// SPEC: The old size MUST be equal to or lower than the checkpoint size.
-		if next.Size < prev.Size {
-			retSize, retSigs = prev.Size, nil
+		if next.Size < prevSize {
+			retSize, retSigs = prevSize, nil
 			return nil, ErrOldSizeInvalid
 		}
 		// SPEC:  If the old size matches the checkpoint size, the witness MUST check that the root hashes are
 		//        also identical.
-		if next.Size == prev.Size {
-			if !bytes.Equal(next.Hash, prev.Hash) {
-				klog.Errorf("%s: INCONSISTENT CHECKPOINTS!:\n%v\n%v", origin, prev, next)
+		if next.Size == prevSize {
+			if !bytes.Equal(next.Hash, prevHash) {
+				klog.Errorf("%s: INCONSISTENT CHECKPOINTS!:\n%v\n%v", origin, prevRaw, next)
 				counterInconsistentCheckpoints.Add(ctx, 1, metric.WithAttributes(originKey.String(origin)))
 
 				retSize, retSigs = 0, nil
@@ -216,7 +219,7 @@ func (w *Witness) Update(ctx context.Context, oldSize uint64, nextRaw []byte, cP
 		}
 		// Checkpoints of size 0 are really placeholders and consistency proofs can't be performed.
 		// If we initialized on a tree size of 0, then we simply ratchet forward and effectively TOFU the new checkpoint.
-		if prev.Size == 0 {
+		if prevSize == 0 {
 			// SPEC:  The proof MUST be empty if the old size is zero.
 			if len(cProof) > 0 {
 				retSize, retSigs = 0, nil
@@ -233,7 +236,7 @@ func (w *Witness) Update(ctx context.Context, oldSize uint64, nextRaw []byte, cP
 
 		// The only remaining option is next.Size > prev.Size. This might be
 		// valid so we verify the consistency proofs.
-		if err := proof.VerifyConsistency(rfc6962.DefaultHasher, prev.Size, next.Size, cProof, prev.Hash, next.Hash); err != nil {
+		if err := proof.VerifyConsistency(rfc6962.DefaultHasher, prevSize, next.Size, cProof, prevHash, next.Hash); err != nil {
 			// Complain if the checkpoints aren't consistent.
 			counterInvalidConsistency.Add(ctx, 1, metric.WithAttributes(originKey.String(origin)))
 			return nil, ErrInvalidProof
@@ -317,3 +320,25 @@ func (w *Witness) signChkpt(n *note.Note) ([]byte, []byte, error) {
 func isValidSignerName(name string) bool {
 	return name != "" && utf8.ValidString(name) && strings.IndexFunc(name, unicode.IsSpace) < 0 && !strings.Contains(name, "+")
 }
+
+// checkpointUnsafe parses a checkpoint without performing any signature verification.
+// This is intended to be as fast as possible, but sacrifices safety because it skips verifying
+// the note signature.
+func checkpointUnsafe(rawCp []byte) (string, uint64, []byte, error) {
+	parts := bytes.SplitN(rawCp, []byte{'\n'}, 4)
+	if want, got := 4, len(parts); want != got {
+		return "", 0, nil, fmt.Errorf("invalid checkpoint: %q", rawCp)
+	}
+	origin := string(parts[0])
+	sizeStr := string(parts[1])
+	hashStr := string(parts[2])
+	size, err := strconv.ParseUint(sizeStr, 10, 64)
+	if err != nil {
+		return "", 0, nil, fmt.Errorf("failed to parse checkpoint size of %q into uint64: %v", sizeStr, err)
+	}
+	hash, err := base64.StdEncoding.DecodeString(hashStr)
+	if err != nil {
+		return "", 0, nil, fmt.Errorf("failed to decode hash: %v", err)
+	}
+	return origin, size, hash, nil
+}