diff --git a/pkg/credhelper/cmd_login.go b/pkg/credhelper/cmd_login.go index d54560b..e6f2d11 100644 --- a/pkg/credhelper/cmd_login.go +++ b/pkg/credhelper/cmd_login.go @@ -29,9 +29,11 @@ func runLogin(cmd *cobra.Command, args []string) error { appViewURL := buildAppViewURL(serverURL) - sc, err := loadConfig() + // Login always saves, so a config we could not parse must stop us here: + // continuing would write the empty fallback config over the user's accounts. + sc, err := loadConfigForWrite() if err != nil { - fmt.Fprintf(os.Stderr, "Warning: config load error: %v\n", err) + return err } // Check if already logged in diff --git a/pkg/credhelper/cmd_logout.go b/pkg/credhelper/cmd_logout.go index b994903..e81befc 100644 --- a/pkg/credhelper/cmd_logout.go +++ b/pkg/credhelper/cmd_logout.go @@ -27,9 +27,9 @@ func runLogout(cmd *cobra.Command, args []string) error { appViewURL := buildAppViewURL(serverURL) - sc, err := loadConfig() + sc, err := loadConfigForWrite() if err != nil { - return fmt.Errorf("loading config: %w", err) + return err } reg := sc.findRegistry(appViewURL) diff --git a/pkg/credhelper/cmd_switch.go b/pkg/credhelper/cmd_switch.go index 71da231..397f2b0 100644 --- a/pkg/credhelper/cmd_switch.go +++ b/pkg/credhelper/cmd_switch.go @@ -27,9 +27,9 @@ func runSwitch(cmd *cobra.Command, args []string) error { appViewURL := buildAppViewURL(serverURL) - sc, err := loadConfig() + sc, err := loadConfigForWrite() if err != nil { - return fmt.Errorf("loading config: %w", err) + return err } reg := sc.findRegistry(appViewURL) diff --git a/pkg/credhelper/config.go b/pkg/credhelper/config.go index 1bd0c80..d4cf2eb 100644 --- a/pkg/credhelper/config.go +++ b/pkg/credhelper/config.go @@ -2,11 +2,22 @@ package credhelper import ( "encoding/json" + "errors" "fmt" "os" + "path/filepath" + "sort" "time" ) +// errConfigUnusable marks a config file that exists on disk but could not be +// understood — a newer on-disk version, malformed JSON, a truncated file, or an +// unreadable one. Callers that intend to write MUST refuse when they see it +// (see [loadConfigForWrite]): loadConfig hands back an empty config so read +// paths can still print a useful message, and saving that empty config over a +// real one would destroy every stored account. +var errConfigUnusable = errors.New("credential config is unusable") + // configVersion is the current on-disk format version. v3 keys the accounts // map (and Active) by DID, the stable identity; v2 keyed them by handle, which // broke on handle renames. The struct shape is identical between v2 and v3 — @@ -63,22 +74,31 @@ func loadConfig() (*StoredConfig, error) { if os.IsNotExist(err) { return newStoredConfig(), nil } - return newStoredConfig(), err + return newStoredConfig(), fmt.Errorf("%w: %v", errConfigUnusable, err) } // v3 (current) and v2 share the same struct shape — v2 keyed accounts by // handle, v3 by DID. Unmarshal once, then branch on the version. var sc StoredConfig if err := json.Unmarshal(data, &sc); err == nil && sc.Registries != nil { - switch sc.Version { - case configVersion: + switch { + case sc.Version == configVersion: return &sc, nil - case 2: + case sc.Version == 2: migrated := migrateV2toV3(&sc) if err := migrated.save(); err != nil { return migrated, fmt.Errorf("saving migrated config: %w", err) } return migrated, nil + case sc.Version > configVersion: + // A newer build wrote this file. Stop here rather than falling + // through to the legacy probes: they would all miss, loadConfig + // would return an empty config, and the next save would overwrite + // every account. This is the downgrade/reinstall path, so it has to + // be non-destructive. + return newStoredConfig(), fmt.Errorf( + "%w: on-disk config is version %d but this build of %s understands version %d; upgrade %s to use it", + errConfigUnusable, sc.Version, cfg.BinaryName, configVersion, cfg.BinaryName) } } @@ -139,7 +159,31 @@ func loadConfig() (*StoredConfig, error) { return migrated, nil } - return newStoredConfig(), fmt.Errorf("unrecognized config format") + return newStoredConfig(), fmt.Errorf("%w: unrecognized config format", errConfigUnusable) +} + +// loadConfigForWrite loads the config for a caller that intends to save it back. +// It refuses only on errConfigUnusable — the paths where loadConfig could not +// understand the file and therefore handed back an *empty* config, which a save +// would write over the user's real accounts. +// +// Other errors carry a fully populated config and must not block the caller. The +// v2→v3 migration in particular returns its migrated config alongside a "saving +// migrated config" error when the config directory is not writable (read-only +// $HOME in a container, ENOSPC, restrictive mode). Every pre-v3 user passes +// through that path on their next invocation, so failing it would hard-break +// `docker pull` and `docker login` for exactly the population that is migrating. +// The in-memory config is correct; the caller's own save will surface any +// persistence failure on its own terms. +func loadConfigForWrite() (*StoredConfig, error) { + sc, err := loadConfig() + if err != nil && errors.Is(err, errConfigUnusable) { + return nil, fmt.Errorf("refusing to modify stored credentials: %w", err) + } + if err != nil { + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) + } + return sc, nil } func newStoredConfig() *StoredConfig { @@ -149,20 +193,60 @@ func newStoredConfig() *StoredConfig { } } -// save writes the config to disk. +// save writes the config to disk atomically. +// +// This file is the user's only copy of their device secrets, and since v3 it is +// written during ordinary `docker pull`/`docker login` traffic (migration and +// DID backfill), not just on explicit login. A truncate-then-write would leave a +// zero-length or half-written file if the helper is interrupted mid-write — +// Ctrl-C, Docker reaping the process, a laptop suspending — and the user cannot +// recover from that because they no longer know which accounts they had. func (c *StoredConfig) save() error { - path := getConfigPath() data, err := json.MarshalIndent(c, "", " ") if err != nil { return err } - return os.WriteFile(path, data, 0600) + return writeFileAtomic(getConfigPath(), data) +} + +// writeFileAtomic writes data to path via a temp file in the same directory +// followed by a rename, so path always holds either the previous contents or the +// complete new contents. The rename also replaces the destination's mode with +// the temp file's 0600, which repairs a config that was left group- or +// world-readable by a restore from backup. +func writeFileAtomic(path string, data []byte) error { + f, err := os.CreateTemp(filepath.Dir(path), ".device-*.json.tmp") + if err != nil { + return err + } + tmp := f.Name() + // No-op once the rename below succeeds; on any earlier return it removes + // the partial file rather than leaving litter next to the real config. + defer os.Remove(tmp) //nolint:errcheck + + if _, err := f.Write(data); err != nil { + f.Close() //nolint:errcheck + return err + } + // Flush before renaming. On ext4/XFS a rename can become durable while the + // new file's data blocks have not, which would resurrect the very + // empty-file case this function exists to prevent. + if err := f.Sync(); err != nil { + f.Close() //nolint:errcheck + return err + } + if err := f.Close(); err != nil { + return err + } + return os.Rename(tmp, path) } // getOrCreateRegistry returns (or creates) a RegistryConfig for the given URL. +// A present-but-null entry (possible in a hand-edited or corrupted file) is +// replaced rather than returned, so callers never dereference nil. func (c *StoredConfig) getOrCreateRegistry(registryURL string) *RegistryConfig { reg, ok := c.Registries[registryURL] - if !ok { + if !ok || reg == nil { reg = &RegistryConfig{ Accounts: make(map[string]*Account), } @@ -176,28 +260,53 @@ func (c *StoredConfig) findRegistry(registryURL string) *RegistryConfig { return c.Registries[registryURL] } +// scanFor returns the matching account with the lexically smallest map key, +// along with that key. Go randomizes map iteration order, so any scan that can +// match more than one entry has to impose its own order — otherwise the helper +// hands Docker a different secret from one invocation to the next. Nil entries +// (hand-edited or corrupted files) are skipped. +func (r *RegistryConfig) scanFor(match func(*Account) bool) (*Account, string) { + var best *Account + var bestKey string + for k, acct := range r.Accounts { + if acct == nil || !match(acct) { + continue + } + if best == nil || k < bestKey { + best, bestKey = acct, k + } + } + return best, bestKey +} + // find returns the account matching identity, which may be a DID or a handle. -// It checks the map key first (DID or provisional handle key) then scans by -// DID/Handle, so an image ref by handle resolves even when the map is +// It checks the map key first (DID or provisional handle key), then scans by +// DID, then by handle, so an image ref by handle resolves even when the map is // DID-keyed. Returns nil if no account matches. +// +// DID matches are tried before handle matches, and each scan is deterministic, +// because two entries can legitimately share a handle: renaming a handle and +// logging in again leaves the old DID-keyed entry behind with the same label. func (r *RegistryConfig) find(identity string) *Account { if identity == "" { return nil } - if acct, ok := r.Accounts[identity]; ok { + if acct, ok := r.Accounts[identity]; ok && acct != nil { return acct } - for _, acct := range r.Accounts { - if acct.DID == identity || acct.Handle == identity { - return acct - } + if acct, _ := r.scanFor(func(a *Account) bool { return a.DID == identity }); acct != nil { + return acct } - return nil + acct, _ := r.scanFor(func(a *Account) bool { return a.Handle == identity }) + return acct } // activeAccount returns the active account, falling back to the sole account // when Active is unset or dangling and exactly one account exists. func (r *RegistryConfig) activeAccount() *Account { + if r == nil { + return nil + } if acct := r.find(r.Active); acct != nil { return acct } @@ -228,18 +337,29 @@ func (r *RegistryConfig) upsert(acct *Account) *Account { var existing *Account var existingKey string + + // 1. Exact DID key. if acct.DID != "" { - if e, ok := r.Accounts[acct.DID]; ok { + if e, ok := r.Accounts[acct.DID]; ok && e != nil { existing, existingKey = e, acct.DID } } + // 2. Same DID stored under some other key (a provisional handle key that + // has since been backfilled). + if existing == nil && acct.DID != "" { + existing, existingKey = r.scanFor(func(e *Account) bool { return e.DID == acct.DID }) + } + // 3. Handle match — but never one that already belongs to a *different* + // DID. Two entries can share a handle after a rename, and matching one + // of those would let the DID assignment below overwrite another + // identity's DID, destroying that account's credentials. if existing == nil && acct.Handle != "" { - for k, e := range r.Accounts { - if e.Handle == acct.Handle || (acct.DID != "" && e.DID == acct.DID) { - existing, existingKey = e, k - break + existing, existingKey = r.scanFor(func(e *Account) bool { + if e.Handle != acct.Handle { + return false } - } + return acct.DID == "" || e.DID == "" || e.DID == acct.DID + }) } if existing == nil { @@ -373,29 +493,100 @@ func (c *StoredConfig) removeAccount(registryURL, identity string) { // migrateV2toV3 re-keys a v2 (handle-keyed) config to v3 (DID-keyed) in place. // Accounts with a stored DID move under their DID key; DID-less accounts keep // the handle as a provisional key and self-heal on the next successful get. -// No account is dropped. +// +// Two v2 entries can collapse onto one v3 key: renaming a handle and logging in +// again leaves the old entry behind under the same DID, which is precisely the +// case v3 exists to fix. That is a merge, not a drop — see [v2EntryBeats] for +// the tie-break, and note the loser still donates its secret if the winner has +// none. Both the merge and the Active re-point iterate in sorted key order, so +// the outcome never depends on Go's randomized map iteration. func migrateV2toV3(sc *StoredConfig) *StoredConfig { for _, reg := range sc.Registries { + if reg == nil { + continue + } oldActive := reg.Active newAccounts := make(map[string]*Account, len(reg.Accounts)) - var activeAcct *Account - for oldKey, acct := range reg.Accounts { + wonWith := make(map[string]string, len(reg.Accounts)) // new key -> winning v2 key + + for _, oldKey := range sortedKeys(reg.Accounts) { + acct := reg.Accounts[oldKey] + if acct == nil { + continue // tolerate a hand-edited or corrupted file + } if acct.Handle == "" { acct.Handle = oldKey // v2 map key was the handle } - newAccounts[acct.key()] = acct - if oldKey == oldActive || acct.Handle == oldActive { - activeAcct = acct + + newKey := acct.key() + incumbent, collision := newAccounts[newKey] + if !collision { + newAccounts[newKey], wonWith[newKey] = acct, oldKey + continue + } + if v2EntryBeats(acct, oldKey, incumbent, wonWith[newKey], oldActive) { + if acct.DeviceSecret == "" { + acct.DeviceSecret = incumbent.DeviceSecret + } + newAccounts[newKey], wonWith[newKey] = acct, oldKey + } else if incumbent.DeviceSecret == "" { + incumbent.DeviceSecret = acct.DeviceSecret } } + reg.Accounts = newAccounts reg.Active = "" - reg.setActive(activeAcct) + // Prefer the entry whose winning v2 key was the active one; only fall + // back to a handle match if none did. Two entries can share a handle + // under different DIDs, and a handle match alone would then activate an + // arbitrary one of them. + for _, newKey := range sortedKeys(newAccounts) { + if wonWith[newKey] == oldActive { + reg.setActive(newAccounts[newKey]) + break + } + } + if reg.Active == "" { + for _, newKey := range sortedKeys(newAccounts) { + if newAccounts[newKey].Handle == oldActive { + reg.setActive(newAccounts[newKey]) + break + } + } + } } sc.Version = configVersion return sc } +// v2EntryBeats reports whether candidate should displace incumbent when both v2 +// entries re-key onto the same v3 key. Tie-breaks, in order: the entry v2 had +// marked active, then the entry that actually carries a device secret, then the +// lexically smaller v2 key. Every tier is a total order over the inputs, so the +// winner is the same on every run. +func v2EntryBeats(cand *Account, candKey string, inc *Account, incKey, oldActive string) bool { + candActive := oldActive != "" && (candKey == oldActive || cand.Handle == oldActive) + incActive := oldActive != "" && (incKey == oldActive || inc.Handle == oldActive) + if candActive != incActive { + return candActive + } + if candHasSecret, incHasSecret := cand.DeviceSecret != "", inc.DeviceSecret != ""; candHasSecret != incHasSecret { + return candHasSecret + } + return candKey < incKey +} + +// sortedKeys returns m's keys in lexical order, for iteration that must be +// reproducible across runs. +func sortedKeys(m map[string]*Account) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // getUpdateCheckCachePath returns the path to the update check cache file func getUpdateCheckCachePath() string { homeDir, err := os.UserHomeDir() diff --git a/pkg/credhelper/config_safety_test.go b/pkg/credhelper/config_safety_test.go new file mode 100644 index 0000000..531abe4 --- /dev/null +++ b/pkg/credhelper/config_safety_test.go @@ -0,0 +1,407 @@ +package credhelper + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// Regression tests for the credential-destroying paths in the v3 config format. +// Each of these could silently empty a user's device.json, which is unrecoverable +// — they cannot know which accounts they had. + +// readConfigFile returns the raw bytes currently on disk at the config path. +func readConfigFile(t *testing.T, dir string) []byte { + t.Helper() + data, err := os.ReadFile(filepath.Join(dir, "device.json")) + if err != nil { + t.Fatalf("reading config: %v", err) + } + return data +} + +// TestSaveIsAtomic verifies save() actually writes out of place: the destination +// inode must be REPLACED, not modified. That is the property that makes an +// interrupted write harmless, and it is what distinguishes this from +// os.WriteFile's truncate-then-write (which would keep the same inode and expose +// a zero-length window). +func TestSaveIsAtomic(t *testing.T) { + dir := setupConfigDir(t) + path := filepath.Join(dir, "device.json") + writeConfigFile(t, dir, `{"version":3,"registries":{}}`) + + before, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + + sc := newStoredConfig() + sc.addAccount("https://atcr.io", &Account{ + Handle: "evan.jarrett.net", DID: "did:plc:evan", DeviceSecret: "atcr_device_A", + }) + if err := sc.save(); err != nil { + t.Fatalf("save: %v", err) + } + + after, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if os.SameFile(before, after) { + t.Error("save() wrote in place (same file); an interrupted write could truncate the config") + } + + // The rename must not leave the temp file behind. + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".device-") { + t.Errorf("temp file %q left behind after save", e.Name()) + } + } +} + +// TestWriteFileAtomicPreservesOnFailure verifies a failed write leaves the +// previous contents completely intact and drops no temp file. Failure is forced +// by pointing at a path whose parent is not a directory, so os.CreateTemp fails. +func TestWriteFileAtomicPreservesOnFailure(t *testing.T) { + dir := setupConfigDir(t) + path := filepath.Join(dir, "device.json") + const original = `{"version":3,"registries":{"https://atcr.io":{"active":"did:plc:evan","accounts":{"did:plc:evan":{"handle":"evan.jarrett.net","did":"did:plc:evan","device_secret":"atcr_device_A"}}}}}` + writeConfigFile(t, dir, original) + + // A temp file cannot be created inside a regular file. + if err := writeFileAtomic(filepath.Join(path, "nested.json"), []byte("x")); err == nil { + t.Fatal("writeFileAtomic succeeded against an invalid directory") + } + + if got := string(readConfigFile(t, dir)); got != original { + t.Errorf("config changed after a failed write:\n got %q\nwant %q", got, original) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), ".device-") { + t.Errorf("temp file %q left behind after a failed write", e.Name()) + } + } +} + +// TestMigrationSaveFailureStillYieldsUsableConfig covers the population that +// matters most right now: a pre-v3 user whose config directory is not writable. +// The migration cannot persist, but the in-memory config is complete, so the +// helper must keep working rather than refusing every operation. +func TestMigrationSaveFailureStillYieldsUsableConfig(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("running as root; mode bits do not restrict writes") + } + dir := setupConfigDir(t) + writeConfigFile(t, dir, `{ + "version": 2, + "registries": { + "https://atcr.io": { + "active": "evan.jarrett.net", + "accounts": {"evan.jarrett.net": {"handle": "evan.jarrett.net", "did": "did:plc:evan", "device_secret": "atcr_device_A"}} + } + } + }`) + // Make the directory read-only so the migration's save fails. + if err := os.Chmod(dir, 0500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0700) }) + + sc, err := loadConfigForWrite() + if err != nil { + t.Fatalf("loadConfigForWrite refused a config it could read: %v", err) + } + if sc == nil { + t.Fatal("loadConfigForWrite returned nil config") + } + acct := sc.Registries["https://atcr.io"].Accounts["did:plc:evan"] + if acct == nil || acct.DeviceSecret != "atcr_device_A" { + t.Fatalf("migrated account missing or wrong: %+v", acct) + } +} + +// TestSaveTightensLoosePermissions covers a device.json restored from a tarball +// or backup with a wide mode. os.WriteFile would have kept 0644 (its perm +// argument only applies at creation); the atomic rename replaces it. +func TestSaveTightensLoosePermissions(t *testing.T) { + dir := setupConfigDir(t) + path := filepath.Join(dir, "device.json") + if err := os.WriteFile(path, []byte(`{"version":3,"registries":{}}`), 0644); err != nil { + t.Fatal(err) + } + + sc := newStoredConfig() + if err := sc.save(); err != nil { + t.Fatalf("save: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0600 { + t.Errorf("config mode = %o, want 0600 after rewrite", got) + } +} + +// TestNewerVersionIsNotWiped is the downgrade path: an older build cannot parse +// a newer on-disk version. It must refuse to write rather than saving an empty +// config over the user's accounts. +func TestNewerVersionIsNotWiped(t *testing.T) { + dir := setupConfigDir(t) + original := `{ + "version": 99, + "registries": { + "https://atcr.io": { + "active": "did:plc:evan", + "accounts": { + "did:plc:evan": {"handle": "evan.jarrett.net", "did": "did:plc:evan", "device_secret": "atcr_device_A"}, + "did:plc:bob": {"handle": "bob.example", "did": "did:plc:bob", "device_secret": "atcr_device_B"} + } + } + } + }` + writeConfigFile(t, dir, original) + + if _, err := loadConfigForWrite(); err == nil { + t.Fatal("loadConfigForWrite accepted a newer-version config; it must refuse so callers don't overwrite it") + } + + // loadConfig itself still yields a usable empty config for read-only + // callers, but reports the error that makes writers back off. + sc, err := loadConfig() + if err == nil { + t.Fatal("loadConfig should report an error for an unsupported version") + } + if sc == nil { + t.Fatal("loadConfig must still return a usable config for read paths") + } + // The sentinel is what loadConfigForWrite gates on, so it must be present. + if !errors.Is(err, errConfigUnusable) { + t.Errorf("error does not wrap errConfigUnusable: %v", err) + } + // The dedicated version branch must produce an actionable message rather + // than falling through to the generic "unrecognized config format". + if !strings.Contains(err.Error(), "version 99") { + t.Errorf("error should name the on-disk version so the user knows to upgrade, got: %v", err) + } + + // Nothing may have touched the file. + var before, after any + if err := json.Unmarshal([]byte(original), &before); err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(readConfigFile(t, dir), &after); err != nil { + t.Fatalf("config on disk is no longer valid JSON: %v", err) + } + if len(readConfigFile(t, dir)) == 0 { + t.Fatal("config file was emptied") + } +} + +// TestMalformedConfigIsNotWiped covers truncated or corrupt JSON — the state a +// non-atomic write used to be able to produce. +func TestMalformedConfigIsNotWiped(t *testing.T) { + for name, contents := range map[string]string{ + "truncated": `{"version":3,"registries":{"https://atcr.io":{"acti`, + "empty": ``, + "notJSON": `this is not json`, + } { + t.Run(name, func(t *testing.T) { + dir := setupConfigDir(t) + writeConfigFile(t, dir, contents) + + _, err := loadConfigForWrite() + if err == nil { + t.Fatal("loadConfigForWrite accepted an unparseable config; writers would overwrite it") + } + // Gating is by sentinel, not by "any error" — see + // TestMigrationSaveFailureStillYieldsUsableConfig for the case that + // must NOT be refused. + if !errors.Is(err, errConfigUnusable) { + t.Errorf("error does not wrap errConfigUnusable, so the guard would let it through: %v", err) + } + if got := string(readConfigFile(t, dir)); got != contents { + t.Errorf("config was modified: got %q, want %q", got, contents) + } + }) + } +} + +// TestMigrateV2toV3_DIDCollisionIsDeterministic covers two v2 handle-keyed +// entries that collapse onto one v3 DID key — the handle-rename case v3 exists +// to fix. The winner must be the same on every run (Go randomizes map +// iteration), and the surviving account must keep a usable secret. +func TestMigrateV2toV3_DIDCollisionIsDeterministic(t *testing.T) { + const v2 = `{ + "version": 2, + "registries": { + "https://atcr.io": { + "active": "alice.com", + "accounts": { + "alice.bsky.social": {"handle": "alice.bsky.social", "did": "did:plc:alice", "device_secret": "atcr_device_STALE"}, + "alice.com": {"handle": "alice.com", "did": "did:plc:alice", "device_secret": "atcr_device_CURRENT"} + } + } + } + }` + + // Repeat: a single pass can pass by luck when the bug is map-order dependent. + for i := 0; i < 50; i++ { + dir := setupConfigDir(t) + writeConfigFile(t, dir, v2) + + sc, err := loadConfig() + if err != nil { + t.Fatalf("loadConfig: %v", err) + } + reg := sc.Registries["https://atcr.io"] + if reg == nil { + t.Fatal("registry missing after migration") + } + if len(reg.Accounts) != 1 { + t.Fatalf("accounts = %d, want 1 (both entries share a DID)", len(reg.Accounts)) + } + + acct := reg.Accounts["did:plc:alice"] + if acct == nil { + t.Fatal("merged account not keyed by DID") + } + // The v2 active entry wins, so the current secret survives, not the stale one. + if acct.DeviceSecret != "atcr_device_CURRENT" { + t.Fatalf("iteration %d: secret = %q, want atcr_device_CURRENT (active entry must win)", i, acct.DeviceSecret) + } + if acct.Handle != "alice.com" { + t.Fatalf("iteration %d: handle = %q, want alice.com", i, acct.Handle) + } + if reg.Active != "did:plc:alice" { + t.Fatalf("iteration %d: active = %q, want did:plc:alice", i, reg.Active) + } + } +} + +// TestMigrateV2toV3_CollisionDonatesSecret verifies the losing entry still +// donates its secret when the winner has none, so a merge never produces an +// account with no credential. +func TestMigrateV2toV3_CollisionDonatesSecret(t *testing.T) { + // Looped: with map-order-dependent code this is a coin flip, so a single + // pass can pass by luck. + for i := 0; i < 50; i++ { + testCollisionDonatesSecret(t, i) + } +} + +func testCollisionDonatesSecret(t *testing.T, iteration int) { + t.Helper() + dir := setupConfigDir(t) + // The active entry wins but carries no secret; the loser's must be adopted. + writeConfigFile(t, dir, `{ + "version": 2, + "registries": { + "https://atcr.io": { + "active": "alice.com", + "accounts": { + "alice.bsky.social": {"handle": "alice.bsky.social", "did": "did:plc:alice", "device_secret": "atcr_device_ONLY"}, + "alice.com": {"handle": "alice.com", "did": "did:plc:alice", "device_secret": ""} + } + } + } + }`) + + sc, err := loadConfig() + if err != nil { + t.Fatalf("loadConfig: %v", err) + } + acct := sc.Registries["https://atcr.io"].Accounts["did:plc:alice"] + if acct == nil { + t.Fatal("merged account missing") + } + if acct.DeviceSecret != "atcr_device_ONLY" { + t.Fatalf("iteration %d: secret = %q, want atcr_device_ONLY donated from the losing entry", iteration, acct.DeviceSecret) + } +} + +// TestMigrateV2toV3_ToleratesNullEntries verifies a hand-edited or corrupted +// file with null entries doesn't panic the helper on Docker's credential path. +func TestMigrateV2toV3_ToleratesNullEntries(t *testing.T) { + dir := setupConfigDir(t) + writeConfigFile(t, dir, `{ + "version": 2, + "registries": { + "https://atcr.io": {"active": "a.com", "accounts": {"a.com": null, "b.com": {"handle":"b.com","did":"did:plc:b","device_secret":"atcr_device_B"}}}, + "https://other.io": null + } + }`) + + sc, err := loadConfig() + if err != nil { + t.Fatalf("loadConfig: %v", err) + } + reg := sc.Registries["https://atcr.io"] + if reg == nil { + t.Fatal("registry missing") + } + if a := reg.Accounts["did:plc:b"]; a == nil || a.DeviceSecret != "atcr_device_B" { + t.Errorf("valid account did not survive alongside a null entry: %+v", a) + } +} + +// TestUpsertDoesNotStealAnotherIdentitysDID covers two accounts sharing a handle +// (a rename leaves the old entry behind). Upserting one must not rewrite the +// other's DID, which would destroy that account's credentials. +func TestUpsertDoesNotStealAnotherIdentitysDID(t *testing.T) { + reg := &RegistryConfig{Accounts: map[string]*Account{ + "did:plc:alice": {Handle: "shared.com", DID: "did:plc:alice", DeviceSecret: "atcr_device_ALICE"}, + }} + + reg.upsert(&Account{Handle: "shared.com", DID: "did:plc:bob", DeviceSecret: "atcr_device_BOB"}) + + if len(reg.Accounts) != 2 { + t.Fatalf("accounts = %d, want 2 (alice must not be overwritten)", len(reg.Accounts)) + } + alice := reg.Accounts["did:plc:alice"] + if alice == nil || alice.DeviceSecret != "atcr_device_ALICE" { + t.Errorf("alice was clobbered: %+v", alice) + } + bob := reg.Accounts["did:plc:bob"] + if bob == nil || bob.DeviceSecret != "atcr_device_BOB" { + t.Errorf("bob not stored: %+v", bob) + } +} + +// TestFindIsDeterministic covers duplicate handles: find must return the same +// account every call, or Docker gets a different secret run to run. +func TestFindIsDeterministic(t *testing.T) { + newReg := func() *RegistryConfig { + return &RegistryConfig{Accounts: map[string]*Account{ + "did:plc:aaa": {Handle: "shared.com", DID: "did:plc:aaa", DeviceSecret: "atcr_device_A"}, + "did:plc:zzz": {Handle: "shared.com", DID: "did:plc:zzz", DeviceSecret: "atcr_device_Z"}, + }} + } + + want := newReg().find("shared.com") + if want == nil { + t.Fatal("find returned nil for a known handle") + } + for i := 0; i < 50; i++ { + if got := newReg().find("shared.com"); got.DID != want.DID { + t.Fatalf("iteration %d: find returned %s, want %s consistently", i, got.DID, want.DID) + } + } + + // An exact DID lookup must still win over the handle scan. + if got := newReg().find("did:plc:zzz"); got == nil || got.DID != "did:plc:zzz" { + t.Errorf("exact DID lookup returned %+v", got) + } +} diff --git a/pkg/credhelper/protocol.go b/pkg/credhelper/protocol.go index e9ca38f..c25ec71 100644 --- a/pkg/credhelper/protocol.go +++ b/pkg/credhelper/protocol.go @@ -72,9 +72,13 @@ func runGet(cmd *cobra.Command, args []string) error { appViewURL := buildAppViewURL(serverURL) - sc, err := loadConfig() + // `get` can rewrite the config (DID backfill, or removing an account whose + // credentials no longer validate), so it must not proceed on a config it + // failed to parse — the empty fallback would be saved over the real one. + // Returned, not printed: Run's top-level handler already prints it. + sc, err := loadConfigForWrite() if err != nil { - fmt.Fprintf(os.Stderr, "Warning: config load error: %v\n", err) + return err } acct, err := sc.resolveAccount(appViewURL, serverURL) @@ -144,9 +148,14 @@ func runStore(cmd *cobra.Command, args []string) error { appViewURL := buildAppViewURL(creds.ServerURL) - sc, err := loadConfig() + // Refuse to write when the existing config couldn't be read. Warning and + // continuing would save the empty fallback config, replacing every stored + // account with just this one. That is the downgrade path: an older build + // cannot parse a v3 file, so the first `docker login` would wipe it. + // Returned, not printed: Run's top-level handler already prints it. + sc, err := loadConfigForWrite() if err != nil { - fmt.Fprintf(os.Stderr, "Warning: config load error: %v\n", err) + return err } // Docker's store protocol only supplies {username, secret} — no DID. Resolve @@ -170,9 +179,13 @@ func runErase(cmd *cobra.Command, args []string) error { appViewURL := buildAppViewURL(serverURL) - sc, err := loadConfig() + // Erase writes, so an unreadable config means we do nothing rather than + // save the empty fallback over it. Reported as success: Docker surfaces a + // failing `logout` as a hard error, and there is nothing the user can act on. + sc, err := loadConfigForWrite() if err != nil { - return nil // No config, nothing to erase + fmt.Fprintf(os.Stderr, "Warning: %v\n", err) + return nil } reg := sc.findRegistry(appViewURL) diff --git a/pkg/credhelper/resolve.go b/pkg/credhelper/resolve.go index d18c34b..8e237fd 100644 --- a/pkg/credhelper/resolve.go +++ b/pkg/credhelper/resolve.go @@ -1,6 +1,7 @@ package credhelper import ( + "context" "fmt" "io" "net" @@ -8,6 +9,15 @@ import ( "time" ) +// dnsResolveTimeout bounds the _atproto TXT lookup. Handle resolution runs on +// Docker's synchronous credential path (every `store`, and every `get` for an +// account that has no DID yet), and the default resolver applies no deadline of +// its own. Without this, a blackholing DNS setup — captive portal, split-horizon +// VPN — stalls `docker login` and `docker pull` for the resolver's full retry +// budget on every invocation. Resolution is best-effort, so giving up early just +// leaves the account on its provisional handle key. +const dnsResolveTimeout = 2 * time.Second + // resolveHandleDID resolves an AT-proto handle to its DID using the two // standard resolution methods, no server or auth required: // @@ -55,7 +65,10 @@ func looksLikeDID(s string) bool { // resolveHandleDNS looks up the _atproto. TXT record and extracts the // DID. Returns "" on any failure. func resolveHandleDNS(handle string) string { - records, err := net.LookupTXT("_atproto." + handle) + ctx, cancel := context.WithTimeout(context.Background(), dnsResolveTimeout) + defer cancel() + + records, err := net.DefaultResolver.LookupTXT(ctx, "_atproto."+handle) if err != nil { return "" }