Update cache to use tamper proofing #1847

pkwarren · 2023-02-17T22:54:40Z

Update the buf CLI's cache to take advantage of tamper proofing features (manifest, blobs, and digests) to store files as content addressable storage. When enabled, the new cache directory is stored as:

  ~/.cache/buf/v2/module/{remote}/{owner}/{repo}
      blobs/
        {digest[:2]}/
          {digest[2:]} => The contents of the module's blob/manifest.
      commits/
        {commit} => The manifest digest

Update the buf CLI's cache to take advantage of tamper proofing features (manifest, blobs, and digests) to store files as content addressable storage. When enabled, the new cache directory is stored as: ~/.cache/buf/v1/module cas/ blobs/ {digest[:2]}/ {digest[2:] commits/ {commit} => The manifest hex digest manifests/ {digest[:2]}/ {digest[2:]

unmultimedio · 2023-02-17T22:57:51Z

Great! I'll take a loot at this work. Some questions:

  ~/.cache/buf/v1/module                      // what's current module cache dir structure?
    cas/
      blobs/
        {digest[:2]}/
          {digest[2:]}
      commits/
        {commit} => The manifest hex digest   // a file with just that content, right?
      manifests/
        {digest[:2]}/
          {digest[2:]}                        // does these manifest files live also in the blobs dir? or just here?

.golangci.yml

pkwarren · 2023-02-17T22:57:53Z

private/bufpkg/bufmodule/bufmodulecache/cache_stats.go

+
+import "sync"
+
+type cacheStats struct {


Broke this out into a separate struct so it could be re-used across implementations.

pkwarren · 2023-02-17T22:58:55Z

private/bufpkg/bufmodule/bufmodulecache/repository_deprecation.go

+	"go.uber.org/zap"
+)
+
+func warnIfDeprecated(


Broke this out into a utility method that could be called by both implementations.

private/pkg/manifest/manifest.go

bufdev

Some preliminary comments - some may be off-base, this is just a quick pass and I have less context, but wanted to get it to you ASAP

private/buf/bufcli/bufcli.go

bufdev · 2023-02-17T22:59:41Z

private/bufpkg/bufmodule/bufmodule.go

@@ -110,6 +111,10 @@ type Module interface {
 	//
 	// This may be nil, since older versions of the module would not have this stored.
 	LintConfig() *buflintconfig.Config
+	// Manifest returns the manifest for the module (possibly empty).
+	Manifest() manifest.Manifest


This breaks the module abstraction (interface) - a module should not have to expose its underlying data in this way, the only access should be through GetModuleFile - if we do this, it needs to be strongly justified

The manifest wraps the entire module - it isn't a 1-1 with a file within the module. This was the simplest way I could find to provide additional data at the time of caching to the cache layer. I can look again but I think some other interfaces (like bucket / path abstractions) make it harder to return a more specialized type here.

bufdev · 2023-02-17T23:02:15Z

private/bufpkg/bufmodule/bufmodulecache/module_cacher.go

@@ -25,13 +25,25 @@ import (
 	"go.uber.org/zap"
 )

+type moduleCache interface {


What is this for? If we need to keep it, it should not be in this file

This was to ensure that the cas_module_cacher and module_cacher didn't diverge. When tamper proofing is enabled, we can remove the module_cacher_* entirely and remove this interface.

private/pkg/manifest/storage.go

.golangci.yml

unmultimedio · 2023-02-17T23:01:12Z

private/bufpkg/bufmodule/bufmodule.go

+	// Manifest returns the manifest for the module (possibly empty).
+	Manifest() manifest.Manifest
+	// Blobs return the raw data for the module (possibly empty).
+	Blobs() manifest.BlobSet


I guess we'll need to have some backend PR as well to cover this change, ready to be merged when this one is?

Will we rely on having these available in the backend for anything? We're only using them in the caching logic right now.

private/bufpkg/bufmodule/bufmodule.go

.golangci.yml

private/bufpkg/bufmodule/bufmodulecache/cas_module_cacher.go

private/bufpkg/bufmodule/module.go

private/bufpkg/bufmodule/bufmodulecache/cas_module_reader.go

private/bufpkg/bufmodule/bufmodulecache/cas_module_cacher.go

private/bufpkg/bufmodule/bufmodule.go

private/pkg/storage/bucket.go

private/bufpkg/bufmodule/bufmodulecache/cas_module_reader.go

private/bufpkg/bufmodule/bufmodulecache/cas_module_cacher.go

private/pkg/manifest/manifest.go

twilly

+1 :)

bufdev · 2023-02-27T17:33:28Z

private/buf/bufcli/bufcli.go

@@ -549,19 +556,22 @@ func newModuleReaderAndCreateCacheDirs(
 	cacheModuleDataDirPath := normalpath.Join(container.CacheDirPath(), v1CacheModuleDataRelDirPath)
 	cacheModuleLockDirPath := normalpath.Join(container.CacheDirPath(), v1CacheModuleLockRelDirPath)
 	cacheModuleSumDirPath := normalpath.Join(container.CacheDirPath(), v1CacheModuleSumRelDirPath)
+	cacheModuleDirPathV2 := normalpath.Join(container.CacheDirPath(), v2CacheModuleRelDirPath)
 	if err := checkExistingCacheDirs(


Just a question - in theory, should a new version of buf need to create the v1 cache dirs at all? Will it once tamper proofing is everywhere?

Nope - we'd only need the v2 directory. I can update to create only the necessary directories.

bufdev · 2023-02-27T17:34:26Z

private/bufpkg/bufmodule/bufmodule.go

@@ -110,6 +111,10 @@ type Module interface {
 	//
 	// This may be nil, since older versions of the module would not have this stored.
 	LintConfig() *buflintconfig.Config
+	// Manifest returns the manifest for the module (possibly nil).


Can we extend this comment to say what the manifest is for (all files including license, buf.md, etc)? Same for BlobSet, what is in the BlobSet, since BlobSet is a generic concept

Added some more documentation here.

private/bufpkg/bufmodule/bufmodulecache/repository_deprecation.go

unmultimedio

Looking great. No blocking comments, a few nits, if there are any upcoming PR those could be addressed there if you want.

unmultimedio · 2023-02-27T17:18:37Z

private/bufpkg/bufmodule/bufmodulecache/cas_module_cacher.go

+	blob, err := c.readBlob(ctx, moduleBasedir, manifestDigest)
+	if err != nil {
+		return nil, err
+	}
+	f, err := blob.Open(ctx)
+	if err != nil {
+		return nil, err
+	}
+	defer func() {
+		retErr = multierr.Append(retErr, f.Close())
+	}()
+	moduleManifest, err := manifest.NewFromReader(f)


Feels weird to be reading the blob twice here. It's an interface issue I guess. Food for thought.

We ended up doing it this way because we first validate the blob's contents against the digest before attempting to consume it as a manifest. I don't think we are really reading it twice here since the second open just returns an in-memory view of the blob's contents.

unmultimedio · 2023-02-27T17:20:39Z

private/bufpkg/bufmodule/bufmodulecache/cas_module_cacher.go

+	moduleManifest := module.Manifest()
+	manifestBlob, err := moduleManifest.Blob()
+	if err != nil {
+		return err
+	}


Nit: You documented that .Manifest could be nil, you could check that here before asking its .Blob to avoid nil panic?

We check it a layer higher in the cas_module_reader, but I added an extra check here.

private/bufpkg/bufmodule/bufmodulecache/cas_module_cacher.go

unmultimedio · 2023-02-27T17:26:59Z

private/bufpkg/bufmodule/bufmodulecache/cas_module_reader.go

+		}
+		manifestDigest := manifestBlob.Digest().String()
+		if manifestDigest != modulePinDigest {
+			// buf.lock module digest and BSR module don't match - fail without overwriting cache


Nit

Suggested change

// buf.lock module digest and BSR module don't match - fail without overwriting cache

// buf.lock module digest and cached BSR module don't match - fail without overwriting cache

This isn't the cached module - it is the one we've just retrieved from the BSR due to a cache miss.

private/bufpkg/bufmodule/bufmodulecache/cas_module_reader.go

private/pkg/storage/storageos/bucket.go

twilly

+1

Update the buf CLI's cache to take advantage of tamper proofing features (manifest, blobs, and digests) to store files as content addressable storage. When enabled, the new cache directory is stored as: ``` ~/.cache/buf/v2/module/{remote}/{owner}/{repo} blobs/ {digest[:2]}/ {digest[2:]} => The contents of the module's blob/manifest. commits/ {commit} => The manifest digest ```

pkwarren requested review from twilly, bufdev and unmultimedio February 17, 2023 22:54

pkwarren commented Feb 17, 2023

View reviewed changes

bufdev reviewed Feb 17, 2023

View reviewed changes

address review comments

d267905

unmultimedio reviewed Feb 17, 2023

View reviewed changes

update

2bcaad2

twilly reviewed Feb 18, 2023

View reviewed changes

private/bufpkg/bufmodule/bufmodulecache/cas_module_reader.go Outdated Show resolved Hide resolved

private/bufpkg/bufmodule/bufmodulecache/cas_module_cacher.go Outdated Show resolved Hide resolved

Merge branch 'main' into pkw/TCN-539-manifest-cache

c59dce7

bufdev reviewed Feb 21, 2023

View reviewed changes

private/bufpkg/bufmodule/bufmodule.go Outdated Show resolved Hide resolved

pkwarren and others added 4 commits February 22, 2023 11:38

switch to storage abstraction

8d79a14

additional PR feedback

5cd139a

Merge branch 'main' into pkw/TCN-539-manifest-cache

16c85a1

update

0e27b66

bufdev reviewed Feb 22, 2023

View reviewed changes

private/pkg/storage/bucket.go Outdated Show resolved Hide resolved

twilly reviewed Feb 23, 2023

View reviewed changes

pkwarren and others added 11 commits February 22, 2023 18:42

Merge branch 'main' into pkw/TCN-539-manifest-cache

6da6cbf

fix error handling and ensure close

87dfec7

more cleanups

ee0891b

fix lint failure

c37da03

Merge branch 'main' into pkw/TCN-539-manifest-cache

7d3c5d4

PutAtomic -> PutWithAtomic

ef3ea36

improve error message

c59cd2d

pass manifest and blobs directly to module constructor

d74fc69

write digest including type to commit

56df1c9

Merge branch 'main' into pkw/TCN-539-manifest-cache

d6196e2

additional cleanup

41550af

pkwarren added 4 commits February 23, 2023 13:03

additional cleanup

76914db

more updates

3d0e293

use digest consistently

c1e908d

remove unused methods

6a5eec8

pkwarren requested review from twilly and unmultimedio February 23, 2023 19:24

final tweaks

d9b0b8c

pkwarren requested a review from bufdev February 24, 2023 22:53

twilly approved these changes Feb 24, 2023

View reviewed changes

Merge branch 'main' into pkw/TCN-539-manifest-cache

6b749ec

bufdev approved these changes Feb 27, 2023

View reviewed changes

unmultimedio approved these changes Feb 27, 2023

View reviewed changes

address review comments

57f6850

pkwarren requested review from twilly, unmultimedio and bufdev February 27, 2023 21:19

twilly approved these changes Feb 27, 2023

View reviewed changes

unmultimedio approved these changes Feb 27, 2023

View reviewed changes

pkwarren merged commit e15702f into main Feb 27, 2023

pkwarren deleted the pkw/TCN-539-manifest-cache branch February 27, 2023 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cache to use tamper proofing #1847

Update cache to use tamper proofing #1847

pkwarren commented Feb 17, 2023 •

edited

Loading

unmultimedio commented Feb 17, 2023 •

edited

Loading

pkwarren Feb 17, 2023

pkwarren Feb 17, 2023

bufdev left a comment

bufdev Feb 17, 2023

pkwarren Feb 17, 2023

bufdev Feb 17, 2023

pkwarren Feb 17, 2023

unmultimedio Feb 17, 2023

pkwarren Feb 21, 2023

twilly left a comment

bufdev Feb 27, 2023

pkwarren Feb 27, 2023 •

edited

Loading

bufdev Feb 27, 2023

pkwarren Feb 27, 2023

unmultimedio left a comment

unmultimedio Feb 27, 2023

pkwarren Feb 27, 2023

unmultimedio Feb 27, 2023

pkwarren Feb 27, 2023

unmultimedio Feb 27, 2023

pkwarren Feb 27, 2023

twilly left a comment

	// buf.lock module digest and BSR module don't match - fail without overwriting cache
	// buf.lock module digest and cached BSR module don't match - fail without overwriting cache

Update cache to use tamper proofing #1847

Update cache to use tamper proofing #1847

Conversation

pkwarren commented Feb 17, 2023 • edited Loading

unmultimedio commented Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkwarren Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unmultimedio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilly left a comment

Choose a reason for hiding this comment

pkwarren commented Feb 17, 2023 •

edited

Loading

unmultimedio commented Feb 17, 2023 •

edited

Loading

pkwarren Feb 27, 2023 •

edited

Loading