From 9a0c8bdd20a5d119919ea08ae994e53694b67ce1 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Mon, 19 Jul 2021 22:49:51 -0700 Subject: [PATCH] util/deephash: make hash type opaque The fact that Hash returns a [sha256.Size]byte leaks details about the underlying hash implementation. This could very well be any other hashing algorithm with a possible different block size. Abstract this implementation detail away by declaring an opaque type that is comparable. While we are changing the signature of UpdateHash, rename it to just Update to reduce stutter (e.g., deephash.Update). Signed-off-by: Joe Tsai --- ipn/ipnlocal/local.go | 4 ++-- util/deephash/deephash.go | 43 ++++++++++++++-------------------- util/deephash/deephash_test.go | 17 +------------- wgengine/userspace.go | 12 +++++----- 4 files changed, 27 insertions(+), 49 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 6d1eeeaee..eb7bf4cf5 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -95,7 +95,7 @@ type LocalBackend struct { serverURL string // tailcontrol URL newDecompressor func() (controlclient.Decompressor, error) - filterHash string + filterHash deephash.Sum // The mutex protects the following elements. mu sync.Mutex @@ -944,7 +944,7 @@ func (b *LocalBackend) updateFilter(netMap *netmap.NetworkMap, prefs *ipn.Prefs) localNets, _ := localNetsB.IPSet() logNets, _ := logNetsB.IPSet() - changed := deephash.UpdateHash(&b.filterHash, haveNetmap, addrs, packetFilter, localNets.Ranges(), logNets.Ranges(), shieldsUp) + changed := deephash.Update(&b.filterHash, haveNetmap, addrs, packetFilter, localNets.Ranges(), logNets.Ranges(), shieldsUp) if !changed { return } diff --git a/util/deephash/deephash.go b/util/deephash/deephash.go index f688a26de..1fe154fdb 100644 --- a/util/deephash/deephash.go +++ b/util/deephash/deephash.go @@ -53,8 +53,17 @@ func (h *hasher) setBufioWriter(w *bufio.Writer) (old *bufio.Writer) { return old } -// Hash returns the raw SHA-256 (not hex) of v. -func (h *hasher) Hash(v interface{}) (hash [sha256.Size]byte) { +// Sum is an opaque checksum type that is comparable. +type Sum struct { + sum [sha256.Size]byte +} + +func (s Sum) String() string { + return hex.EncodeToString(s.sum[:]) +} + +// Hash returns the hash of v. +func (h *hasher) Hash(v interface{}) (hash Sum) { h.bw.Flush() h.h.Reset() h.print(reflect.ValueOf(v)) @@ -64,7 +73,7 @@ func (h *hasher) Hash(v interface{}) (hash [sha256.Size]byte) { // concrete type exported and we don't want the 'hash' result // parameter to escape to the heap: h.h.Sum(h.scratch[:0]) - copy(hash[:], h.scratch[:]) + copy(hash.sum[:], h.scratch[:]) return } @@ -72,8 +81,8 @@ var hasherPool = &sync.Pool{ New: func() interface{} { return newHasher() }, } -// Hash returns the raw SHA-256 hash of v. -func Hash(v interface{}) [sha256.Size]byte { +// Hash returns the hash of v. +func Hash(v interface{}) Sum { h := hasherPool.Get().(*hasher) defer hasherPool.Put(h) for k := range h.visited { @@ -82,30 +91,14 @@ func Hash(v interface{}) [sha256.Size]byte { return h.Hash(v) } -// UpdateHash sets last to the hex-encoded hash of v and reports whether its value changed. -func UpdateHash(last *string, v ...interface{}) (changed bool) { +// Update sets last to the hash of v and reports whether its value changed. +func Update(last *Sum, v ...interface{}) (changed bool) { sum := Hash(v) - if sha256EqualHex(sum, *last) { + if sum == *last { // unchanged. return false } - *last = hex.EncodeToString(sum[:]) - return true -} - -// sha256EqualHex reports whether hx is the hex encoding of sum. -func sha256EqualHex(sum [sha256.Size]byte, hx string) bool { - if len(hx) != len(sum)*2 { - return false - } - const hextable = "0123456789abcdef" - j := 0 - for _, v := range sum { - if hx[j] != hextable[v>>4] || hx[j+1] != hextable[v&0x0f] { - return false - } - j += 2 - } + *last = sum return true } diff --git a/util/deephash/deephash_test.go b/util/deephash/deephash_test.go index b26414f7b..0a2b28a14 100644 --- a/util/deephash/deephash_test.go +++ b/util/deephash/deephash_test.go @@ -7,8 +7,6 @@ package deephash import ( "bufio" "bytes" - "crypto/sha256" - "encoding/hex" "fmt" "reflect" "testing" @@ -233,7 +231,7 @@ func BenchmarkTailcfgNode(b *testing.B) { } func TestExhaustive(t *testing.T) { - seen := make(map[[sha256.Size]byte]bool) + seen := make(map[Sum]bool) for i := 0; i < 100000; i++ { s := Hash(i) if seen[s] { @@ -243,19 +241,6 @@ func TestExhaustive(t *testing.T) { } } -func TestSHA256EqualHex(t *testing.T) { - for i := 0; i < 1000; i++ { - sum := Hash(i) - hx := hex.EncodeToString(sum[:]) - if !sha256EqualHex(sum, hx) { - t.Fatal("didn't match, should've") - } - if sha256EqualHex(sum, hx[:len(hx)-1]) { - t.Fatal("matched on wrong length") - } - } -} - // verify this doesn't loop forever, as it used to (Issue 2340) func TestMapCyclicFallback(t *testing.T) { type T struct { diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 2972726e9..9f9228fc9 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -107,9 +107,9 @@ type userspaceEngine struct { wgLock sync.Mutex // serializes all wgdev operations; see lock order comment below lastCfgFull wgcfg.Config - lastRouterSig string // of router.Config - lastEngineSigFull string // of full wireguard config - lastEngineSigTrim string // of trimmed wireguard config + lastRouterSig deephash.Sum // of router.Config + lastEngineSigFull deephash.Sum // of full wireguard config + lastEngineSigTrim deephash.Sum // of trimmed wireguard config recvActivityAt map[tailcfg.DiscoKey]time.Time trimmedDisco map[tailcfg.DiscoKey]bool // set of disco keys of peers currently excluded from wireguard config sentActivityAt map[netaddr.IP]*int64 // value is atomic int64 of unixtime @@ -641,7 +641,7 @@ func (e *userspaceEngine) maybeReconfigWireguardLocked(discoChanged map[key.Publ } } - if !deephash.UpdateHash(&e.lastEngineSigTrim, &min, trimmedDisco, trackDisco, trackIPs) { + if !deephash.Update(&e.lastEngineSigTrim, &min, trimmedDisco, trackDisco, trackIPs) { // No changes return nil } @@ -767,8 +767,8 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, listenPort = 0 } - engineChanged := deephash.UpdateHash(&e.lastEngineSigFull, cfg) - routerChanged := deephash.UpdateHash(&e.lastRouterSig, routerCfg, dnsCfg) + engineChanged := deephash.Update(&e.lastEngineSigFull, cfg) + routerChanged := deephash.Update(&e.lastRouterSig, routerCfg, dnsCfg) if !engineChanged && !routerChanged && listenPort == e.magicConn.LocalPort() { return ErrNoChanges }