diff --git a/go.mod b/go.mod index 657f938..669fbe6 100644 --- a/go.mod +++ b/go.mod @@ -2,12 +2,15 @@ module github.com/energyworldnet/az-devops-token go 1.23 -require github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2 +require ( + github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2 + github.com/gofrs/flock v0.12.1 +) require ( github.com/golang-jwt/jwt/v5 v5.2.2 // indirect github.com/google/uuid v1.3.0 // indirect github.com/kylelemons/godebug v1.1.0 // indirect github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect - golang.org/x/sys v0.5.0 // indirect + golang.org/x/sys v0.22.0 // indirect ) diff --git a/go.sum b/go.sum index 4dac208..82f695d 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,9 @@ github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2 h1:RHK7bS+HQMslb1sZpAokUt+zTVmue0hKSs2C791hhzU= github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2/go.mod h1:HKpQxkWaGLJ+D/5H8QRpyQXA1eKjxkFlOMwck5+33Jk= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/gofrs/flock v0.12.1 h1:MTLVXXHf8ekldpJk3AKicLij9MdwOWkZ+a/jHHZby9E= +github.com/gofrs/flock v0.12.1/go.mod h1:9zxTsyu5xtJ9DK+1tFZyibEV7y3uwDxPPfbxeeHCoD0= github.com/golang-jwt/jwt/v5 v5.2.2 h1:Rl4B7itRWVtYIHFrSNd7vhTiz9UpLdi6gZhZ3wEeDy8= github.com/golang-jwt/jwt/v5 v5.2.2/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= @@ -8,6 +12,12 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= +github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= golang.org/x/sys v0.0.0-20210616045830-e2b7044e8c71/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI= +golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/token/token.go b/internal/token/token.go index 2877f64..d92aa7c 100644 --- a/internal/token/token.go +++ b/internal/token/token.go @@ -18,6 +18,7 @@ import ( "github.com/AzureAD/microsoft-authentication-library-for-go/apps/cache" "github.com/AzureAD/microsoft-authentication-library-for-go/apps/public" + "github.com/gofrs/flock" ) // adoResource is the public, global Azure DevOps resource ID (identical for @@ -32,6 +33,11 @@ const adoResource = "499b84ac-1321-427f-aa17-267ca6975798" // feed tokens live ~1h, so this leaves ample refresh headroom. const MinLifetime = 20 * time.Minute +// lockRetryDelay is how long WithLock sleeps between attempts to acquire a +// contended cross-process lock. Operations under the lock are sub-second, so a +// short poll keeps a waiter's latency low without busy-spinning. +const lockRetryDelay = 50 * time.Millisecond + // Scopes returns the delegated scopes requested for a feed token. func Scopes() []string { return []string{adoResource + "/vso.packaging"} } @@ -81,6 +87,48 @@ func (c *FileCache) Export(_ context.Context, m cache.Marshaler, _ cache.ExportH return os.Rename(tmp, c.Path) } +// WithLock runs fn while holding an exclusive cross-process advisory lock on +// lockPath, then releases it. It serializes the whole reload → refresh → +// export sequence across concurrent invocations so a later writer's Export +// cannot clobber a newer refresh-token/cache update a peer just wrote. The +// lock is an OS advisory lock, so it is released automatically if the process +// dies mid-operation — a crash leaves no stale lock. +// +// It blocks (polling every lockRetryDelay) until the lock is acquired or ctx is +// done. When ctx expires first it returns (false, nil) without running fn, so a +// caller on a latency-sensitive path can degrade rather than hang. A genuine +// lock error (e.g. the lock directory cannot be created) is returned as-is. +func WithLock(ctx context.Context, lockPath string, fn func()) (bool, error) { + /* The lock is acquired before Export creates the cache directory, so + ensure the parent exists first (mirrors Export). Otherwise the first run + on a machine with a not-yet-created cache dir couldn't create the + lockfile. */ + if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { + return false, err + } + fl := flock.New(lockPath) + locked, err := fl.TryLockContext(ctx, lockRetryDelay) + if err != nil { + /* A context error means we simply ran out of time waiting for a peer to + release — report it as a clean give-up (false, nil) so the caller can + degrade, not as a failure. */ + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return false, nil + } + return false, err + } + if !locked { + return false, nil + } + /* Release is best-effort: an Unlock failure is not actionable here — this is + a short-lived CLI about to exit, and the OS releases the advisory lock and + closes the descriptor on exit regardless. Folding it into the return would + also make a rare release hiccup discard an already-acquired token. */ + defer func() { _ = fl.Unlock() }() + fn() + return true, nil +} + // StripAccessTokens removes the access-token entries from a serialized MSAL // cache, leaving the refresh token and everything else intact. MSAL's silent // path then finds no cached access token and redeems the refresh token for a diff --git a/internal/token/token_test.go b/internal/token/token_test.go index 479cf41..372ea8e 100644 --- a/internal/token/token_test.go +++ b/internal/token/token_test.go @@ -1,7 +1,14 @@ package token_test import ( + "bufio" + "context" "encoding/json" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" "testing" "time" @@ -42,6 +49,152 @@ func TestStripAccessTokensRejectsInvalidJSON(t *testing.T) { } } +func TestWithLockRunsFnAndReleases(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "cache.lock") + + ran := false + locked, err := token.WithLock(context.Background(), lockPath, func() { ran = true }) + if err != nil { + t.Fatalf("WithLock: %v", err) + } + if !locked { + t.Fatal("expected to acquire an uncontended lock") + } + if !ran { + t.Error("fn should have run while the lock was held") + } + + // The lock must be released on return: a second acquisition succeeds. + locked, err = token.WithLock(context.Background(), lockPath, func() {}) + if err != nil { + t.Fatalf("WithLock (second): %v", err) + } + if !locked { + t.Error("lock should be released after WithLock returns") + } +} + +func TestWithLockCreatesParentDir(t *testing.T) { + // A not-yet-created cache dir (the pre-login state) must not stop the lock. + lockPath := filepath.Join(t.TempDir(), "nested", "cache.lock") + locked, err := token.WithLock(context.Background(), lockPath, func() {}) + if err != nil { + t.Fatalf("WithLock: %v", err) + } + if !locked { + t.Error("expected to acquire the lock after creating the parent dir") + } +} + +// lockHolderEnv names the env var that switches TestWithLockCrossProcess into +// its child (lock-holder) role. The child holds the lock on the named path +// until the parent closes its stdin. +const lockHolderEnv = "AZ_DEVOPS_TOKEN_TEST_LOCK_PATH" + +// lockHeldSignal is written to stdout by the child once the lock is held, so +// the parent only starts contending after acquisition. +const lockHeldSignal = "LOCK_HELD" + +func TestWithLockCrossProcess(t *testing.T) { + if lockPath := os.Getenv(lockHolderEnv); lockPath != "" { + // Child role: hold the lock until the parent closes our stdin (EOF), + // signalling readiness first. + _, _ = token.WithLock(context.Background(), lockPath, func() { + fmt.Println(lockHeldSignal) + _, _ = io.Copy(io.Discard, os.Stdin) + }) + return + } + + lockPath := filepath.Join(t.TempDir(), "cache.lock") + + // Re-exec this test binary as a separate process that holds the lock, to + // exercise the genuinely cross-process advisory lock rather than same-process + // scheduling. The context stops the helper if the test exits early. + holderCtx, cancelHolder := context.WithCancel(context.Background()) + cmd := exec.CommandContext(holderCtx, os.Args[0], "-test.run=^TestWithLockCrossProcess$") //nolint:gosec // re-execs this test binary itself, not external input + cmd.Env = append(os.Environ(), lockHolderEnv+"="+lockPath) + cmd.Stderr = os.Stderr + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatalf("StdinPipe: %v", err) + } + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatalf("StdoutPipe: %v", err) + } + if err := cmd.Start(); err != nil { + t.Fatalf("start lock holder: %v", err) + } + // Safety net: if an assertion below fails early, don't leave the child + // blocked (and the lockfile undeletable). + t.Cleanup(func() { + cancelHolder() + _ = cmd.Wait() + }) + + // Wait until the child reports the lock is held, bounding the wait so a + // stalled child (never signals) or one that exits before signalling (EOF) + // fails fast with a clear diagnostic instead of hanging or falling through + // to a misleading contended-lock assertion. + ready := make(chan error, 1) + go func() { + scanner := bufio.NewScanner(stdout) + for scanner.Scan() { + if scanner.Text() == lockHeldSignal { + ready <- nil + return + } + } + if err := scanner.Err(); err != nil { + ready <- fmt.Errorf("reading lock holder output: %w", err) + return + } + ready <- fmt.Errorf("lock holder exited before signalling it held the lock") + }() + select { + case err := <-ready: + if err != nil { + t.Fatalf("waiting for lock holder: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for lock holder to acquire the lock") + } + + // The lock is held by another process: acquisition must give up within the + // deadline without running fn, so a latency-sensitive caller can degrade. + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + ran := false + locked, err := token.WithLock(ctx, lockPath, func() { ran = true }) + if err != nil { + t.Fatalf("contended WithLock: %v", err) + } + if locked { + t.Error("should not acquire a lock another process holds") + } + if ran { + t.Error("fn must not run when the lock is not acquired") + } + + // Release the holder and wait for it to exit, so the lock is fully released. + if err := stdin.Close(); err != nil { + t.Fatalf("close holder stdin: %v", err) + } + if err := cmd.Wait(); err != nil { + t.Fatalf("lock holder exited with error: %v", err) + } + + // The lock is acquirable again now that the holder is gone. + locked, err = token.WithLock(context.Background(), lockPath, func() {}) + if err != nil { + t.Fatalf("post-release WithLock: %v", err) + } + if !locked { + t.Error("lock should be acquirable after the holder exits") + } +} + func TestNeedsForcedRefresh(t *testing.T) { now := time.Unix(1_700_000_000, 0) cases := []struct { diff --git a/main.go b/main.go index 832cc18..760812c 100644 --- a/main.go +++ b/main.go @@ -97,29 +97,77 @@ func main() { return } - // A cached/refreshable token satisfies both paths: it's what `print` emits, - // and it makes `login` idempotent (no prompt when already signed in). - silentCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - tok, ok := token.Silent(silentCtx, app, fc) - cancel() - if ok { - if login { - fmt.Fprintln(os.Stderr, "already signed in") + // Serialize the whole acquire across processes so a peer's reload → refresh → + // export can't interleave with ours and clobber a newer cache write. The lock + // is held from the silent attempt through any interactive sign-in, so two + // logins can't both prompt and race their writes; it also dedupes concurrent + // refreshes, since a waiter finds the fresh token already cached on release. + lockPath := cachePath() + ".lock" + + // Bound the wait for the lock. `print` shares mise's tolerance for a slow + // config load; `login` may queue behind an in-progress interactive sign-in, + // which is user-paced and can take a while. + acquireTimeout := 30 * time.Second + if login { + acquireTimeout = 2 * time.Minute + } + lockCtx, cancelLock := context.WithTimeout(context.Background(), acquireTimeout) + defer cancelLock() + + var ( + tok string + haveToken bool + loginErr error + ) + locked, lockErr := token.WithLock(lockCtx, lockPath, func() { + // A cached/refreshable token satisfies both paths: it's what `print` emits, + // and it makes `login` idempotent (no prompt when already signed in). + silentCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + tok, haveToken = token.Silent(silentCtx, app, fc) + cancel() + if haveToken || !login { return } - fmt.Print(tok) + // No cached token on the login path: sign in interactively, still under the + // lock. context.Background() (not lockCtx) bounds this — the browser round + // trip must not be cut short by the acquire timeout. + loginErr = interactiveLogin(context.Background(), app) + }) + if lockErr != nil { + fmt.Fprintf(os.Stderr, "cache lock: %v\n", lockErr) + if login { + os.Exit(1) + } + return + } + if !locked { + // A peer held the lock past our budget. On `print`, stay silent (empty + // output, exit 0) so mise's config load falls through to a clean 401; on + // `login`, surface it. + if login { + fmt.Fprintln(os.Stderr, "timed out waiting for another sign-in to finish") + os.Exit(1) + } return } if !login { - // No usable token — degrade to empty output so mise's config load and the - // consuming package manager fall through to a clean 401 rather than erroring. - fmt.Fprintln(os.Stderr, "no token — run `az-devops-token login`") + if !haveToken { + // No usable token — degrade to empty output so mise's config load and the + // consuming package manager fall through to a clean 401 rather than erroring. + fmt.Fprintln(os.Stderr, "no token — run `az-devops-token login`") + return + } + fmt.Print(tok) return } - if err := interactiveLogin(context.Background(), app); err != nil { - fmt.Fprintf(os.Stderr, "login failed: %v\n", err) + if haveToken { + fmt.Fprintln(os.Stderr, "already signed in") + return + } + if loginErr != nil { + fmt.Fprintf(os.Stderr, "login failed: %v\n", loginErr) os.Exit(1) } fmt.Fprintln(os.Stderr, "signed in")