Remove VerifiableReader

VerifiableReader was code that SOCI inherited from Stargz, but didn't
use. With estargz images, the TOC is embedded in the layer in the remote
store. When setting up a reader, you can optionally pass a digest of the
TOC which the stargz-snaphostter will verify against the actual TOC in
the remote registry.

SOCI stores TOCs in zTOCs which have integrity rooted in the SOCI index.
Therefore, you get equivalent verification by specifing a SOCI index
digest when pulling an image. By the time we're setting up a reader,
we've already verified the ztocs and their layer mapping which means
there is nothing to verify here.

Initially we initially just made Verify a no-op and bypassed it.
This change cleans up that unneeded code entirely.

Signed-off-by: Kern Walster <[email protected]>

fs/fs.go

@@ -560,12 +560,6 @@ func (fs *filesystem) Mount(ctx context.Context, mountpoint string, labels map[s
 		}
 	}()
 
-	// Verification is needed to instantiate reader
-	l.SkipVerify()
-	log.G(ctx).Infof("Verification forcefully skipped")
-	// Maybe we should reword the log here or remove it entirely,
-	// since the old Verify() function no longer serves any purpose.
-
 	node, err := l.RootNode(0)
 	if err != nil {
 		log.G(ctx).WithError(err).Warnf("Failed to get root node")

fs/layer/layer.go

@@ -94,15 +94,6 @@ type Layer interface {
 	// Refresh refreshes the layer connection.
 	Refresh(ctx context.Context, hosts []docker.RegistryHost, refspec reference.Spec, desc ocispec.Descriptor) error
 
-	// Verify verifies this layer using the passed TOC Digest.
-	// Nop if Verify() or SkipVerify() was already called.
-	// NOTE: Legacy stargz code, this is never called.
-	Verify(tocDigest digest.Digest) (err error)
-
-	// SkipVerify skips verification for this layer.
-	// Nop if Verify() or SkipVerify() was already called.
-	SkipVerify()
-
 	// ReadAt reads this layer.
 	ReadAt([]byte, int64, ...remote.Option) (int, error)
 
@@ -406,7 +397,7 @@ func newLayer(
 	resolver *Resolver,
 	desc ocispec.Descriptor,
 	blob *blobRef,
-	vr *reader.VerifiableReader,
+	r reader.Reader,
 	bgResolver backgroundfetcher.Resolver,
 	opCounter *FuseOperationCounter,
 	disableXAttrs bool,
@@ -415,18 +406,17 @@ func newLayer(
 		resolver:             resolver,
 		desc:                 desc,
 		blob:                 blob,
-		verifiableReader:     vr,
+		r:                    r,
 		bgResolver:           bgResolver,
 		fuseOperationCounter: opCounter,
 		disableXAttrs:        disableXAttrs,
 	}
 }
 
 type layer struct {
-	resolver         *Resolver
-	desc             ocispec.Descriptor
-	blob             *blobRef
-	verifiableReader *reader.VerifiableReader
+	resolver *Resolver
+	desc     ocispec.Descriptor
+	blob     *blobRef
 
 	bgResolver backgroundfetcher.Resolver
 
@@ -440,15 +430,11 @@ type layer struct {
 }
 
 func (l *layer) Info() Info {
-	var readTime time.Time
-	if l.r != nil {
-		readTime = l.r.LastOnDemandReadTime()
-	}
 	return Info{
 		Digest:      l.desc.Digest,
 		Size:        l.blob.Size(),
 		FetchedSize: l.blob.FetchedSize(),
-		ReadTime:    readTime,
+		ReadTime:    l.r.LastOnDemandReadTime(),
 	}
 }
 
@@ -466,25 +452,6 @@ func (l *layer) Refresh(ctx context.Context, hosts []docker.RegistryHost, refspe
 	return l.blob.Refresh(ctx, hosts, refspec, desc)
 }
 
-// Never used, should probably just have it return nil.
-func (l *layer) Verify(tocDigest digest.Digest) (err error) {
-	if l.isClosed() {
-		return fmt.Errorf("layer is already closed")
-	}
-	if l.r != nil {
-		return nil
-	}
-	l.r, err = l.verifiableReader.VerifyTOC(tocDigest)
-	return
-}
-
-func (l *layer) SkipVerify() {
-	if l.r != nil {
-		return
-	}
-	l.r = l.verifiableReader.SkipVerify()
-}
-
 func (l *layerRef) Done() {
 	l.done()
 }
@@ -493,9 +460,6 @@ func (l *layer) RootNode(baseInode uint32) (fusefs.InodeEmbedder, error) {
 	if l.isClosed() {
 		return nil, fmt.Errorf("layer is already closed")
 	}
-	if l.r == nil {
-		return nil, fmt.Errorf("layer hasn't been verified yet")
-	}
 	return newNode(l.desc.Digest, l.r, l.blob, baseInode, l.resolver.overlayOpaqueType, l.resolver.config.LogFuseOperations, l.fuseOperationCounter)
 }
 
@@ -518,11 +482,7 @@ func (l *layer) close() error {
 		l.bgResolver.Close()
 	}
 	defer l.blob.done() // Close reader first, then close the blob
-	l.verifiableReader.Close()
-	if l.r != nil {
-		return l.r.Close()
-	}
-	return nil
+	return l.r.Close()
 }
 
 func (l *layer) isClosed() bool {

fs/layer/util_test.go

@@ -168,25 +168,24 @@ func makeNodeReader(t *testing.T, contents []byte, spanSize int64, factory metad
 		t.Fatalf("failed to create reader: %v", err)
 	}
 	spanManager := spanmanager.New(ztoc, sr, cache.NewMemoryCache(), 0)
-	vr, err := reader.NewReader(mr, digest.FromString(""), spanManager, false)
+	r, err := reader.NewReader(mr, digest.FromString(""), spanManager, false)
 	if err != nil {
 		mr.Close()
 		t.Fatalf("failed to make new reader: %v", err)
 	}
-	r := vr.GetReader()
 	rootNode := getRootNode(t, r, OverlayOpaqueAll)
 	var eo fuse.EntryOut
 	inode, errno := rootNode.Lookup(context.Background(), testName, &eo)
 	if errno != 0 {
-		vr.Close()
+		r.Close()
 		t.Fatalf("failed to lookup test node; errno: %v", errno)
 	}
 	f, _, errno := inode.Operations().(fusefs.NodeOpener).Open(context.Background(), 0)
 	if errno != 0 {
-		vr.Close()
+		r.Close()
 		t.Fatalf("failed to open test file; errno: %v", errno)
 	}
-	return f.(*file), vr.Close
+	return f.(*file), r.Close
 }
 
 func testExistence(t *testing.T, factory metadata.Store) {
@@ -330,11 +329,10 @@ func testExistenceWithOpaque(t *testing.T, factory metadata.Store, opaque Overla
 				}
 				defer mr.Close()
 				spanManager := spanmanager.New(ztoc, sr, cache.NewMemoryCache(), 0)
-				vr, err := reader.NewReader(mr, digest.FromString(""), spanManager, false)
+				r, err := reader.NewReader(mr, digest.FromString(""), spanManager, false)
 				if err != nil {
 					t.Fatalf("failed to make new reader: %v", err)
 				}
-				r := vr.GetReader()
 				defer r.Close()
 				rootNode := getRootNode(t, r, opaque)
 				for _, want := range tt.want {

fs/reader/reader.go

@@ -64,76 +64,14 @@ type Reader interface {
 	LastOnDemandReadTime() time.Time
 }
 
-// VerifiableReader produces a Reader with a given verifier.
-type VerifiableReader struct {
-	r *reader
-
-	lastVerifyErr           atomic.Value
-	prohibitVerifyFailure   bool
-	prohibitVerifyFailureMu sync.RWMutex
-
-	closed   bool
-	closedMu sync.Mutex
-
-	verifier func(uint32, string) (digest.Verifier, error)
-}
-
-func (vr *VerifiableReader) SkipVerify() Reader {
-	return vr.r
-}
-
-func (vr *VerifiableReader) VerifyTOC(tocDigest digest.Digest) (Reader, error) {
-	if vr.isClosed() {
-		return nil, fmt.Errorf("reader is already closed")
-	}
-	vr.prohibitVerifyFailureMu.Lock()
-	vr.prohibitVerifyFailure = true
-	lastVerifyErr := vr.lastVerifyErr.Load()
-	vr.prohibitVerifyFailureMu.Unlock()
-	if err := lastVerifyErr; err != nil {
-		return nil, fmt.Errorf("content error occures during caching contents: %w", err.(error))
-	}
-	vr.r.verify = true
-	return vr.r, nil
-}
-
-// nolint:revive
-func (vr *VerifiableReader) GetReader() *reader {
-	return vr.r
-}
-
-func (vr *VerifiableReader) Metadata() metadata.Reader {
-	// TODO: this shouldn't be called before verified
-	return vr.r.r
-}
-
-func (vr *VerifiableReader) Close() error {
-	vr.closedMu.Lock()
-	defer vr.closedMu.Unlock()
-	if vr.closed {
-		return nil
-	}
-	vr.closed = true
-	return vr.r.Close()
-}
-
-func (vr *VerifiableReader) isClosed() bool {
-	vr.closedMu.Lock()
-	closed := vr.closed
-	vr.closedMu.Unlock()
-	return closed
-}
-
 // NewReader creates a Reader based on the given soci blob and Span Manager.
-func NewReader(r metadata.Reader, layerSha digest.Digest, spanManager *spanmanager.SpanManager, disableVerification bool) (*VerifiableReader, error) {
-	vr := &reader{
+func NewReader(r metadata.Reader, layerSha digest.Digest, spanManager *spanmanager.SpanManager, disableVerification bool) (Reader, error) {
+	return &reader{
 		spanManager:         spanManager,
 		r:                   r,
 		layerSha:            layerSha,
-		verifier:            digestVerifier,
 		disableVerification: disableVerification,
-	}
-	return &VerifiableReader{r: vr, verifier: digestVerifier}, nil
+	}, nil
 }
 
 type reader struct {
@@ -147,8 +85,6 @@ type reader struct {
 	closed   bool
 	closedMu sync.Mutex
 
-	verify              bool
-	verifier            func(uint32, string) (digest.Verifier, error)
 	disableVerification bool
 }
 
@@ -360,11 +296,3 @@ func WithReader(sr *io.SectionReader) CacheOption {
 		opts.reader = sr
 	}
 }
-
-func digestVerifier(id uint32, digestStr string) (digest.Verifier, error) {
-	digest, err := digest.Parse(digestStr)
-	if err != nil {
-		return nil, fmt.Errorf("no digset is recorded: %w", err)
-	}
-	return digest.Verifier(), nil
-}

fs/reader/reader_test.go

@@ -162,28 +162,27 @@ func makeFile(t *testing.T, contents []byte, prefix string, factory metadata.Sto
 		t.Fatalf("failed to create reader: %v", err)
 	}
 	spanManager := spanmanager.New(ztoc, sr, cache.NewMemoryCache(), 0)
-	vr, err := NewReader(mr, digest.FromString(""), spanManager, false)
+	r, err := NewReader(mr, digest.FromString(""), spanManager, false)
 	if err != nil {
 		mr.Close()
 		t.Fatalf("failed to make new reader: %v", err)
 	}
-	r := vr.GetReader()
 	tid, _, err := mr.GetChild(mr.RootID(), testName)
 	if err != nil {
-		vr.Close()
+		r.Close()
 		t.Fatalf("failed to get %q: %v", testName, err)
 	}
 	ra, err := r.OpenFile(tid)
 	if err != nil {
-		vr.Close()
+		r.Close()
 		t.Fatalf("Failed to open testing file: %v", err)
 	}
 	f, ok := ra.(*file)
 	if !ok {
-		vr.Close()
+		r.Close()
 		t.Fatalf("invalid type of file %q", tid)
 	}
-	return f, vr.Close
+	return f, r.Close
 }
 
 func testFailReader(t *testing.T, factory metadata.Store) {
@@ -218,12 +217,11 @@ func testFailReader(t *testing.T, factory metadata.Store) {
 				t.Fatalf("free ID not found")
 			}
 			spanManager := spanmanager.New(ztoc, sr, cache.NewMemoryCache(), 0)
-			vr, err := NewReader(mr, digest.FromString(""), spanManager, false)
+			r, err := NewReader(mr, digest.FromString(""), spanManager, false)
 			if err != nil {
 				mr.Close()
 				t.Fatalf("failed to make new reader: %v", err)
 			}
-			r := vr.GetReader()
 
 			_, err = r.OpenFile(notexist)
 			if err == nil {