refactor(keyretriever): reduce duplicate master-key WARN logs (#589)

Author: moonD4rk

browser/chromium/chromium.go

@@ -3,6 +3,7 @@ package chromium
 import (
 	"os"
 	"path/filepath"
+	"sync"
 	"time"
 
 	"github.com/moond4rk/hackbrowserdata/crypto/keyretriever"
@@ -51,17 +52,7 @@ func NewBrowsers(cfg types.BrowserConfig) ([]*Browser, error) {
 	return browsers, nil
 }
 
-// SetKeyRetrievers wires the per-tier master-key retrievers used by Extract. Each slot
-// (V10 / V11 / V20) is populated only on platforms where that cipher tier is used:
-//
-//   - Windows: V10 (DPAPI) + V20 (ABE). V11 nil — Chromium does not emit v11 prefix on Windows.
-//   - Linux:   V10 ("peanuts" kV10Key) + V11 (D-Bus Secret Service kV11Key). V20 nil.
-//   - macOS:   V10 (Keychain chain). V11 and V20 nil.
-//
-// Slots are independent — a failure or absence in one tier does not affect others. A single
-// Chromium profile can carry mixed cipher-prefix ciphertexts (the motivation for issue #578), so
-// every configured retriever runs at extract time and decryptValue picks the matching key per
-// ciphertext.
+// SetKeyRetrievers wires the per-tier master-key retrievers (V10/V11/V20) used by Extract; unused tiers stay nil.
 func (b *Browser) SetKeyRetrievers(r keyretriever.Retrievers) {
 	b.retrievers = r
 }
@@ -178,12 +169,11 @@ func (b *Browser) acquireFiles(session *filemanager.Session, categories []types.
 	return tempPaths
 }
 
-// getMasterKeys retrieves the Chromium master keys for every configured tier. Chrome mixes
-// cipher tiers on the same profile — v20 for new cookies alongside v10 passwords on Windows; v10
-// (peanuts) alongside v11 (keyring) on Linux after session-mode changes — so every retriever in
-// b.retrievers runs independently and keyretriever.NewMasterKeys assembles the results. Any tier
-// key may be nil if its retriever failed or is not configured for this platform; decryptValue
-// treats a missing tier key as "that tier cannot decrypt" so partial success is still reported.
+// warnedMasterKeyFailure dedupes "master key retrieval" WARN per installation (BrowserName + UserDataDir);
+// profiles share one Safe Storage entry, but glob-expanded configs may yield multiple installations of the same browser.
+var warnedMasterKeyFailure sync.Map
+
+// getMasterKeys retrieves master keys for all configured cipher tiers.
 func (b *Browser) getMasterKeys(session *filemanager.Session) keyretriever.MasterKeys {
 	label := b.BrowserName() + "/" + b.ProfileName()
 
@@ -207,7 +197,12 @@ func (b *Browser) getMasterKeys(session *filemanager.Session) keyretriever.Maste
 
 	keys, err := keyretriever.NewMasterKeys(b.retrievers, b.cfg.Storage, localStateDst)
 	if err != nil {
-		log.Warnf("%s: master key retrieval: %v", label, err)
+		installKey := b.BrowserName() + "|" + b.cfg.UserDataDir
+		if _, already := warnedMasterKeyFailure.LoadOrStore(installKey, struct{}{}); !already {
+			log.Warnf("%s: master key retrieval: %v", b.BrowserName(), err)
+		} else {
+			log.Debugf("%s: master key retrieval: %v", label, err)
+		}
 	}
 	return keys
 }

crypto/keyretriever/keyretriever.go

@@ -48,7 +48,7 @@ func (c *ChainRetriever) RetrieveKey(storage, localStatePath string) ([]byte, er
 			return key, nil
 		}
 		if err != nil {
-			log.Warnf("keyretriever %T failed: %v", r, err)
+			log.Debugf("keyretriever %T failed: %v", r, err)
 			errs = append(errs, fmt.Errorf("%T: %w", r, err))
 		}
 	}

crypto/keyretriever/keyretriever_darwin.go

@@ -14,6 +14,8 @@ import (
 	"time"
 
 	"github.com/moond4rk/keychainbreaker"
+
+	"github.com/moond4rk/hackbrowserdata/log"
 )
 
 // https://source.chromium.org/chromium/chromium/src/+/master:components/os_crypt/os_crypt_mac.mm;l=157
@@ -37,18 +39,25 @@ type GcoredumpRetriever struct {
 	err     error
 }
 
+// RetrieveKey logs internal failures at Debug and returns (nil, nil) so ChainRetriever falls
+// through to the next retriever silently. The most common failure ("requires root privileges")
+// is documented expected behavior, not a warning-worthy condition; surfacing it on every profile
+// would drown out genuine warnings. The same pattern is used by ABERetriever (see abe_windows.go).
 func (r *GcoredumpRetriever) RetrieveKey(storage, _ string) ([]byte, error) {
 	r.once.Do(func() {
 		r.records, r.err = DecryptKeychainRecords()
-		if r.err != nil {
-			r.err = fmt.Errorf("gcoredump: %w", r.err)
-		}
 	})
 	if r.err != nil {
-		return nil, r.err
+		log.Debugf("gcoredump: %v", r.err)
+		return nil, nil //nolint:nilerr // intentional silent fallthrough
 	}
 
-	return findStorageKey(r.records, storage)
+	key, err := findStorageKey(r.records, storage)
+	if err != nil {
+		log.Debugf("gcoredump: %v", err)
+		return nil, nil //nolint:nilerr // intentional silent fallthrough
+	}
+	return key, nil
 }
 
 // loadKeychainRecords opens login.keychain-db and unlocks it with the given
@@ -141,7 +150,14 @@ func (r *SecurityCmdRetriever) retrieveKeyOnce(storage string) ([]byte, error) {
 		if errors.Is(ctx.Err(), context.DeadlineExceeded) {
 			return nil, fmt.Errorf("security command timed out after %s", securityCmdTimeout)
 		}
-		return nil, fmt.Errorf("security command: %w (%s)", err, strings.TrimSpace(stderr.String()))
+		// `security find-generic-password` exits non-zero with empty stderr when the user denies
+		// the keychain access prompt or enters the wrong password. Surface that explicitly so the
+		// error message is actionable instead of the cryptic "exit status 128 ()".
+		stderrStr := strings.TrimSpace(stderr.String())
+		if stderrStr == "" {
+			return nil, fmt.Errorf("security command: %w (likely keychain access denied or wrong password)", err)
+		}
+		return nil, fmt.Errorf("security command: %w (%s)", err, stderrStr)
 	}
 	if stderr.Len() > 0 {
 		return nil, fmt.Errorf("keychain: %s", strings.TrimSpace(stderr.String()))