notary/internal/tlog: change TileReader.Reject to TileReader.SaveTiles
TileReader has a ReadTiles method that returns unverified tiles,
and then the implementation needs some way to distinguish tiles
that validate from ones that don't. The old approach was to let
ReadTiles assume the tiles would validate and have the validator
promise to call Reject for any tiles that did not validate.
This created uncertainty about the state of the tiles immediately
after a ReadTiles call returned - was a Reject on its way or not?
A better approach, taken in this CL, is to have ReadTiles assume
that tiles it returns are unvalidated until the client calls SaveTiles
with the ones that do pass validation.
Addresses a TODO from the initial review of TileReader.
Change-Id: I69c8cf8aecf33b7fa904646258ae5ffbb915a09e
Reviewed-on: https://go-review.googlesource.com/c/exp/+/171858
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
diff --git a/notary/internal/notecheck/main.go b/notary/internal/notecheck/main.go
index 62a0bdf..4643002 100644
--- a/notary/internal/notecheck/main.go
+++ b/notary/internal/notecheck/main.go
@@ -205,8 +205,8 @@
return *height
}
-func (r *tileReader) Reject(tile tlog.Tile) {
- log.Printf("tile rejected: %v", tile.Path())
+func (r *tileReader) SaveTiles(tiles []tlog.Tile, data [][]byte) {
+ // no on-disk cache here
}
// TODO(rsc): Move some variant of this to package tlog
diff --git a/notary/internal/tlog/tile.go b/notary/internal/tlog/tile.go
index e633958..1885e8e 100644
--- a/notary/internal/tlog/tile.go
+++ b/notary/internal/tlog/tile.go
@@ -228,24 +228,17 @@
// (len(data[i]) == tiles[i].W*HashSize).
ReadTiles(tiles []Tile) (data [][]byte, err error)
- // Reject marks the tile as invalid.
- // A caller can call Reject if a returned Tile cannot
- // be authenticated against a given Tree.
- // TODO(rsc): The point of this method is to let the cache speculatively
- // write the downloaded set of tiles to disk during ReadTiles
- // and then have the caller call back with Reject when it finds
- // out a tile was inconsistent with a known signed key.
- // It would be better to let the TileReader implementation
- // find out whether the tile is good before caching any.
- Reject(tile Tile)
+ // SaveTiles informs the TileReader that the tile data
+ // returned by ReadTiles has been confirmed as valid
+ // and can be saved in persistent storage (on disk).
+ SaveTiles(tiles []Tile, data [][]byte)
}
// TileHashReader returns a HashReader that satisfies requests
// by loading tiles of the given tree.
//
// The returned HashReader checks that loaded tiles are
-// valid for the given tree; if not, it calls tr.Reject to report them
-// and returns an error. A consequence is that any hashes returned
+// valid for the given tree. Therefore, any hashes returned
// by the HashReader are already proven to be in the tree.
func TileHashReader(tree Tree, tr TileReader) HashReader {
return &tileHashReader{tree: tree, tr: tr}
@@ -367,10 +360,7 @@
}
if th != r.tree.Hash {
// The tiles do not support the tree hash.
- // We know at least one is wrong, but not which one; reject them all.
- for _, tile := range tiles[:len(stx)] {
- r.tr.Reject(tile)
- }
+ // We know at least one is wrong, but not which one.
return nil, fmt.Errorf("downloaded inconsistent tile")
}
@@ -387,13 +377,14 @@
return nil, fmt.Errorf("bad math in tileHashReader %d %v: lost hash of %v: %v", r.tree.N, indexes, tile, err)
}
if h != tileHash(data[i]) {
- r.tr.Reject(tile)
return nil, fmt.Errorf("downloaded inconsistent tile")
}
}
// Now we have all the tiles needed for the requested hashes,
// and we've authenticated the full tile set against the trusted tree hash.
+ r.tr.SaveTiles(tiles, data)
+
// Pull out the requested hashes.
hashes := make([]Hash, len(indexes))
for i, x := range indexes {
diff --git a/notary/internal/tlog/tlog_test.go b/notary/internal/tlog/tlog_test.go
index cfc1233..3dad1a1 100644
--- a/notary/internal/tlog/tlog_test.go
+++ b/notary/internal/tlog/tlog_test.go
@@ -33,21 +33,25 @@
return out, nil
}
-type testTilesStorage map[Tile][]byte
+type testTilesStorage struct {
+ unsaved int
+ m map[Tile][]byte
+}
func (t testTilesStorage) Height() int {
return 2
}
-func (t testTilesStorage) Reject(tile Tile) {
- println("REJECT")
+func (t *testTilesStorage) SaveTiles(tiles []Tile, data [][]byte) {
+ t.unsaved -= len(tiles)
}
-func (t testTilesStorage) ReadTiles(tiles []Tile) ([][]byte, error) {
+func (t *testTilesStorage) ReadTiles(tiles []Tile) ([][]byte, error) {
out := make([][]byte, len(tiles))
for i, tile := range tiles {
- out[i] = t[tile]
+ out[i] = t.m[tile]
}
+ t.unsaved += len(tiles)
return out, nil
}
@@ -55,7 +59,7 @@
var trees []Hash
var leafhashes []Hash
var storage testHashStorage
- tiles := make(testTilesStorage)
+ tiles := make(map[Tile][]byte)
const testH = 2
for i := int64(0); i < 100; i++ {
data := []byte(fmt.Sprintf("leaf %d", i))
@@ -145,7 +149,8 @@
// Check that leaf proofs work using TileReader.
// To prove a leaf that way, all you have to do is read and verify its hash.
- thr := TileHashReader(Tree{i + 1, th}, tiles)
+ storage := &testTilesStorage{m: tiles}
+ thr := TileHashReader(Tree{i + 1, th}, storage)
for j := int64(0); j <= i; j++ {
h, err := thr.ReadHashes([]int64{StoredHashIndex(0, j)})
if err != nil {
@@ -165,11 +170,17 @@
t.Fatalf("CheckRecord(%d, %d, TileHashReader(%d)): %v", i+1, j, i+1, err)
}
}
+ if storage.unsaved != 0 {
+ t.Fatalf("TileHashReader(%d) did not save %d tiles", i+1, storage.unsaved)
+ }
// Check that ReadHashes will give an error if the index is not in the tree.
if _, err := thr.ReadHashes([]int64{(i + 1) * 2}); err == nil {
t.Fatalf("TileHashReader(%d).ReadHashes(%d) for index not in tree <nil>, want err", i, i+1)
}
+ if storage.unsaved != 0 {
+ t.Fatalf("TileHashReader(%d) did not save %d tiles", i+1, storage.unsaved)
+ }
// Check that tree proofs work, for all trees so far, using TileReader.
// To prove a tree that way, all you have to do is compute and verify its hash.
@@ -184,7 +195,7 @@
// Even though computing the subtree hash suffices,
// check that we can generate the proof too.
- p, err := ProveTree(i+1, j+1, storage)
+ p, err := ProveTree(i+1, j+1, thr)
if err != nil {
t.Fatalf("ProveTree(%d, %d): %v", i+1, j+1, err)
}
@@ -199,6 +210,9 @@
p[k][0] ^= 1
}
}
+ if storage.unsaved != 0 {
+ t.Fatalf("TileHashReader(%d) did not save %d tiles", i+1, storage.unsaved)
+ }
}
}
