Use SHA256 sums to verify discovery cache integrity

This is a little more computationally expensive but reduces the
likelihood of a potentially malicious cache collision.

Signed-off-by: Nic Cope <nicc@rk0n.org>

Kubernetes-commit: c5957c284e1d23bdadc98fbbe2bb481fc1f345d4

discovery/cached/disk/round_tripper.go

@@ -17,10 +17,9 @@ limitations under the License.
 package disk
 
 import (
+	"bytes"
 	"crypto/sha256"
-	"encoding/binary"
 	"fmt"
-	"hash/crc32"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -44,7 +43,7 @@ func newCacheRoundTripper(cacheDir string, rt http.RoundTripper) http.RoundTripp
 		BasePath: cacheDir,
 		TempDir:  filepath.Join(cacheDir, ".diskv-temp"),
 	})
-	t := httpcache.NewTransport(&crcDiskCache{disk: d})
+	t := httpcache.NewTransport(&sumDiskCache{disk: d})
 	t.Transport = rt
 
 	return &cacheRoundTripper{rt: t}
@@ -67,28 +66,30 @@ func (rt *cacheRoundTripper) CancelRequest(req *http.Request) {
 
 func (rt *cacheRoundTripper) WrappedRoundTripper() http.RoundTripper { return rt.rt.Transport }
 
-// A crcDiskCache is a cache backend for github.com/gregjones/httpcache. It is
+// A sumDiskCache is a cache backend for github.com/gregjones/httpcache. It is
-// similar to httpcache's diskcache package, but uses checksums to ensure cache
+// similar to httpcache's diskcache package, but uses SHA256 sums to ensure
-// integrity rather than fsyncing each cache entry in order to avoid performance
+// cache integrity at read time rather than fsyncing each cache entry to
-// degradation on MacOS.
+// increase the likelihood they will be persisted at write time. This avoids
+// significant performance degradation on MacOS.
 //
 // See https://github.com/kubernetes/kubernetes/issues/110753 for more.
-type crcDiskCache struct {
+type sumDiskCache struct {
 	disk *diskv.Diskv
 }
 
 // Get the requested key from the cache on disk. If Get encounters an error, or
-// the returned value is not a CRC-32 checksum followed by bytes with a matching
+// the returned value is not a SHA256 sum followed by bytes with a matching
 // checksum it will return false to indicate a cache miss.
-func (c *crcDiskCache) Get(key string) ([]byte, bool) {
+func (c *sumDiskCache) Get(key string) ([]byte, bool) {
 	b, err := c.disk.Read(sanitize(key))
-	if err != nil || len(b) < binary.MaxVarintLen32 {
+	if err != nil || len(b) < sha256.Size {
 		return []byte{}, false
 	}
 
-	response := b[binary.MaxVarintLen32:]
+	response := b[sha256.Size:]
-	sum, _ := binary.Uvarint(b[:binary.MaxVarintLen32])
+	want := b[:sha256.Size] // The first 32 bytes of the file should be the SHA256 sum.
-	if crc32.ChecksumIEEE(response) != uint32(sum) {
+	got := sha256.Sum256(response)
+	if !bytes.Equal(want, got[:]) {
 		return []byte{}, false
 	}
 
@@ -96,15 +97,14 @@ func (c *crcDiskCache) Get(key string) ([]byte, bool) {
 }
 
 // Set writes the response to a file on disk. The filename will be the SHA256
-// hash of the key. The file will contain the CRC-32 checksum of the response
+// sum of the key. The file will contain a SHA256 sum of the response bytes,
-// bytes, followed by said response bytes.
+// followed by said response bytes.
-func (c *crcDiskCache) Set(key string, response []byte) {
+func (c *sumDiskCache) Set(key string, response []byte) {
-	sum := make([]byte, binary.MaxVarintLen32)
+	s := sha256.Sum256(response)
-	_ = binary.PutUvarint(sum, uint64(crc32.ChecksumIEEE(response)))
+	_ = c.disk.Write(sanitize(key), append(s[:], response...)) // Nothing we can do with this error.
-	_ = c.disk.Write(sanitize(key), append(sum, response...)) // Nothing we can do with this error.
 }
 
-func (c *crcDiskCache) Delete(key string) {
+func (c *sumDiskCache) Delete(key string) {
 	_ = c.disk.Erase(sanitize(key)) // Nothing we can do with this error.
 }
 

discovery/cached/disk/round_tripper_test.go

@@ -18,8 +18,7 @@ package disk
 
 import (
 	"bytes"
-	"encoding/binary"
+	"crypto/sha256"
-	"hash/crc32"
 	"io/ioutil"
 	"net/http"
 	"net/url"
@@ -63,7 +62,7 @@ func BenchmarkDiskCache(b *testing.B) {
 		b.Fatal(err)
 	}
 
-	c := crcDiskCache{disk: d}
+	c := sumDiskCache{disk: d}
 
 	for n := 0; n < b.N; n++ {
 		c.Set(k, v)
@@ -178,35 +177,35 @@ func TestCacheRoundTripperPathPerm(t *testing.T) {
 	assert.NoError(err)
 }
 
-func TestCRCDiskCache(t *testing.T) {
+func TestSumDiskCache(t *testing.T) {
 	assert := assert.New(t)
 
 	// Ensure that we'll return a cache miss if the backing file doesn't exist.
 	t.Run("NoSuchKey", func(t *testing.T) {
-		cacheDir, err := ioutil.TempDir("", "cache-crc")
+		cacheDir, err := ioutil.TempDir("", "cache-test")
 		if err != nil {
 			t.Fatal(err)
 		}
 		defer os.RemoveAll(cacheDir)
 		d := diskv.New(diskv.Options{BasePath: cacheDir, TempDir: filepath.Join(cacheDir, ".diskv-temp")})
-		crc := &crcDiskCache{disk: d}
+		c := &sumDiskCache{disk: d}
 
 		key := "testing"
 
-		got, ok := crc.Get(key)
+		got, ok := c.Get(key)
 		assert.False(ok)
 		assert.Equal([]byte{}, got)
 	})
 
 	// Ensure that we'll return a cache miss if the backing file is empty.
 	t.Run("EmptyFile", func(t *testing.T) {
-		cacheDir, err := ioutil.TempDir("", "cache-crc")
+		cacheDir, err := ioutil.TempDir("", "cache-test")
 		if err != nil {
 			t.Fatal(err)
 		}
 		defer os.RemoveAll(cacheDir)
 		d := diskv.New(diskv.Options{BasePath: cacheDir, TempDir: filepath.Join(cacheDir, ".diskv-temp")})
-		crc := &crcDiskCache{disk: d}
+		c := &sumDiskCache{disk: d}
 
 		key := "testing"
 
@@ -216,7 +215,7 @@ func TestCRCDiskCache(t *testing.T) {
 		}
 		f.Close()
 
-		got, ok := crc.Get(key)
+		got, ok := c.Get(key)
 		assert.False(ok)
 		assert.Equal([]byte{}, got)
 	})
@@ -224,32 +223,31 @@ func TestCRCDiskCache(t *testing.T) {
 	// Ensure that we'll return a cache miss if the backing has an invalid
 	// checksum.
 	t.Run("InvalidChecksum", func(t *testing.T) {
-		cacheDir, err := ioutil.TempDir("", "cache-crc")
+		cacheDir, err := ioutil.TempDir("", "cache-test")
 		if err != nil {
 			t.Fatal(err)
 		}
 		defer os.RemoveAll(cacheDir)
 		d := diskv.New(diskv.Options{BasePath: cacheDir, TempDir: filepath.Join(cacheDir, ".diskv-temp")})
-		crc := &crcDiskCache{disk: d}
+		c := &sumDiskCache{disk: d}
 
 		key := "testing"
 		value := []byte("testing")
 		mismatchedValue := []byte("testink")
-		sum := make([]byte, binary.MaxVarintLen32)
+		sum := sha256.Sum256(value)
-		binary.PutUvarint(sum, uint64(crc32.ChecksumIEEE(value)))
 
-		// Create a file with the checksum of 'value' followed by the bytes of
+		// Create a file with the sum of 'value' followed by the bytes of
 		// 'mismatchedValue'.
 		f, err := os.Create(filepath.Join(cacheDir, sanitize(key)))
 		if err != nil {
 			t.Fatal(err)
 		}
-		f.Write(sum)
+		f.Write(sum[:])
 		f.Write(mismatchedValue)
 		f.Close()
 
 		// The mismatched checksum should result in a cache miss.
-		got, ok := crc.Get(key)
+		got, ok := c.Get(key)
 		assert.False(ok)
 		assert.Equal([]byte{}, got)
 	})
@@ -260,20 +258,20 @@ func TestCRCDiskCache(t *testing.T) {
 	// This should cause httpcache to fall back to its underlying transport and
 	// to subsequently cache the new value, overwriting the corrupt one.
 	t.Run("OverwriteExistingKey", func(t *testing.T) {
-		cacheDir, err := ioutil.TempDir("", "cache-crc")
+		cacheDir, err := ioutil.TempDir("", "cache-test")
 		if err != nil {
 			t.Fatal(err)
 		}
 		defer os.RemoveAll(cacheDir)
 		d := diskv.New(diskv.Options{BasePath: cacheDir, TempDir: filepath.Join(cacheDir, ".diskv-temp")})
-		crc := &crcDiskCache{disk: d}
+		c := &sumDiskCache{disk: d}
 
 		key := "testing"
 		value := []byte("cool value!")
 
 		// Write a value.
-		crc.Set(key, value)
+		c.Set(key, value)
-		got, ok := crc.Get(key)
+		got, ok := c.Get(key)
 
 		// Ensure we can read back what we wrote.
 		assert.True(ok)
@@ -282,8 +280,8 @@ func TestCRCDiskCache(t *testing.T) {
 		differentValue := []byte("I'm different!")
 
 		// Write a different value.
-		crc.Set(key, differentValue)
+		c.Set(key, differentValue)
-		got, ok = crc.Get(key)
+		got, ok = c.Get(key)
 
 		// Ensure we can read back the different value.
 		assert.True(ok)
@@ -292,32 +290,32 @@ func TestCRCDiskCache(t *testing.T) {
 
 	// Ensure that deleting a key does in fact delete it.
 	t.Run("DeleteKey", func(t *testing.T) {
-		cacheDir, err := ioutil.TempDir("", "cache-crc")
+		cacheDir, err := ioutil.TempDir("", "cache-test")
 		if err != nil {
 			t.Fatal(err)
 		}
 		defer os.RemoveAll(cacheDir)
 		d := diskv.New(diskv.Options{BasePath: cacheDir, TempDir: filepath.Join(cacheDir, ".diskv-temp")})
-		crc := &crcDiskCache{disk: d}
+		c := &sumDiskCache{disk: d}
 
 		key := "testing"
 		value := []byte("coolValue")
 
-		crc.Set(key, value)
+		c.Set(key, value)
 
 		// Ensure we successfully set the value.
-		got, ok := crc.Get(key)
+		got, ok := c.Get(key)
 		assert.True(ok)
 		assert.Equal(value, got)
 
-		crc.Delete(key)
+		c.Delete(key)
 
 		// Ensure the value is gone.
-		got, ok = crc.Get(key)
+		got, ok = c.Get(key)
 		assert.False(ok)
 		assert.Equal([]byte{}, got)
 
 		// Ensure that deleting a non-existent value is a no-op.
-		crc.Delete(key)
+		c.Delete(key)
 	})
 }