Skip to content

Commit 5868949

Browse files
authored
fix: add cross-process lock around cache.New to prevent macOS keychain race (#744)
* fix: add cross-process lock around cache.New to prevent macOS keychain race (#740) When multiple kubelogin processes start concurrently, the upstream azidentity/cache package's storage test uses a non-atomic check-then-add pattern on the macOS keychain, causing 'The specified item already exists in the keychain' (-25299) errors. This adds a file lock (syscall.Flock on Unix, no-op on Windows) around cache.New(nil) calls to serialize the storage test across processes. The lock is best-effort: if it cannot be acquired, execution proceeds without it to avoid breaking existing behavior. * fix: address golangci-lint errcheck and gocritic findings * refactor: use x/sys/unix and os.UserCacheDir() for lock file Address PR review feedback: - Switch from syscall to golang.org/x/sys/unix for consistency with project conventions (already used in pkg/internal/pop/cache/linux.go) - Use os.UserCacheDir() instead of os.TempDir() for lock file path to avoid unnecessary cross-user contention (macOS keychain is per-user) - Add os.MkdirAll to ensure the cache directory exists before creating the lock file, with fallback to os.TempDir() on any error * Use user-scoped subdirectory for lock file path Place the lock file under os.UserCacheDir()/kubelogin/ instead of directly in the cache directory. In the os.TempDir() fallback, incorporate the UID (kubelogin-<uid>) so the lock remains per-user even in a world-writable directory. This addresses review feedback about cross-user contention and symlink risks when the lock file falls back to /tmp.
1 parent f2dcbd9 commit 5868949

11 files changed

+194
-14
lines changed

pkg/internal/token/azurepipelinescredential.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"github.com/Azure/azure-sdk-for-go/sdk/azcore"
99
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
1010
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
11-
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
1211
"k8s.io/klog/v2"
1312

1413
"github.com/Azure/kubelogin/pkg/internal/env"
@@ -31,7 +30,7 @@ func newAzurePipelinesCredential(opts *Options) (CredentialProvider, error) {
3130
err error
3231
)
3332
if opts.UsePersistentCache {
34-
c, err = cache.New(nil)
33+
c, err = newPersistentCache()
3534
if err != nil {
3635
klog.V(5).Infof("failed to create cache: %v", err)
3736
}

pkg/internal/token/clientcertcredential.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
"github.com/Azure/azure-sdk-for-go/sdk/azcore"
1313
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
1414
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
15-
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
1615
"golang.org/x/crypto/pkcs12"
1716
"k8s.io/klog/v2"
1817
)
@@ -38,7 +37,7 @@ func newClientCertificateCredential(opts *Options) (CredentialProvider, error) {
3837
err error
3938
)
4039
if opts.UsePersistentCache {
41-
c, err = cache.New(nil)
40+
c, err = newPersistentCache()
4241
if err != nil {
4342
klog.V(5).Infof("failed to create cache: %v", err)
4443
}

pkg/internal/token/clientsecretcredential.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"github.com/Azure/azure-sdk-for-go/sdk/azcore"
88
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
99
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
10-
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
1110
"k8s.io/klog/v2"
1211
)
1312

@@ -32,7 +31,7 @@ func newClientSecretCredential(opts *Options) (CredentialProvider, error) {
3231
err error
3332
)
3433
if opts.UsePersistentCache {
35-
c, err = cache.New(nil)
34+
c, err = newPersistentCache()
3635
if err != nil {
3736
klog.V(5).Infof("failed to create cache: %v", err)
3837
}

pkg/internal/token/devicecodecredential.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"github.com/Azure/azure-sdk-for-go/sdk/azcore"
99
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
1010
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
11-
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
1211
"k8s.io/klog/v2"
1312
)
1413

@@ -30,7 +29,7 @@ func newDeviceCodeCredential(opts *Options, record azidentity.AuthenticationReco
3029
err error
3130
)
3231
if opts.UsePersistentCache {
33-
c, err = cache.New(nil)
32+
c, err = newPersistentCache()
3433
if err != nil {
3534
klog.V(5).Infof("failed to create cache: %v", err)
3635
}

pkg/internal/token/interactivebrowsercredential.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"github.com/Azure/azure-sdk-for-go/sdk/azcore"
88
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
99
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
10-
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
1110
"k8s.io/klog/v2"
1211
)
1312

@@ -29,7 +28,7 @@ func newInteractiveBrowserCredential(opts *Options, record azidentity.Authentica
2928
err error
3029
)
3130
if opts.UsePersistentCache {
32-
c, err = cache.New(nil)
31+
c, err = newPersistentCache()
3332
if err != nil {
3433
klog.V(5).Infof("failed to create cache: %v", err)
3534
}
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
package token
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"path/filepath"
7+
8+
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
9+
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
10+
)
11+
12+
// cacheNewFunc is the function used to create a new persistent cache.
13+
// It is a variable to allow overriding in tests.
14+
var cacheNewFunc = cache.New
15+
16+
// newPersistentCache creates a persistent token cache with cross-process
17+
// synchronization to prevent a race condition when multiple kubelogin
18+
// processes start concurrently. The upstream azidentity/cache package
19+
// tests storage availability using a non-atomic check-then-add pattern
20+
// on macOS keychain, which fails with "The specified item already exists
21+
// in the keychain" (-25299) when two processes race.
22+
//
23+
// This function uses a file lock to serialize the storage test across
24+
// processes. If the lock cannot be acquired, it proceeds without locking
25+
// (best-effort) to avoid breaking existing behavior.
26+
//
27+
// See https://github.com/Azure/kubelogin/issues/740
28+
func newPersistentCache() (azidentity.Cache, error) {
29+
lockDir := lockFileDir()
30+
lockPath := filepath.Join(lockDir, "cache-test.lock")
31+
unlock := acquireProcessLock(lockPath)
32+
defer unlock()
33+
return cacheNewFunc(nil)
34+
}
35+
36+
// lockFileDir returns a user-scoped directory for the lock file.
37+
// It prefers os.UserCacheDir()/kubelogin and falls back to
38+
// os.TempDir()/kubelogin-<uid> so that the lock file is never
39+
// placed directly in a shared, world-writable directory.
40+
func lockFileDir() string {
41+
if cacheDir, err := os.UserCacheDir(); err == nil {
42+
dir := filepath.Join(cacheDir, "kubelogin")
43+
if err := os.MkdirAll(dir, 0700); err == nil {
44+
return dir
45+
}
46+
}
47+
// Fallback: use a UID-scoped subdirectory under the temp dir
48+
// so the lock file is not directly in a world-writable location.
49+
dir := filepath.Join(os.TempDir(), fmt.Sprintf("kubelogin-%d", os.Getuid()))
50+
if err := os.MkdirAll(dir, 0700); err != nil {
51+
// Last resort: use temp dir directly. The lock is best-effort,
52+
// so this is acceptable.
53+
return os.TempDir()
54+
}
55+
return dir
56+
}
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
package token
2+
3+
import (
4+
"fmt"
5+
"path/filepath"
6+
"sync"
7+
"testing"
8+
9+
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
10+
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
func TestNewPersistentCache_Success(t *testing.T) {
16+
original := cacheNewFunc
17+
defer func() { cacheNewFunc = original }()
18+
19+
called := false
20+
cacheNewFunc = func(opts *cache.Options) (azidentity.Cache, error) {
21+
called = true
22+
assert.Nil(t, opts)
23+
return azidentity.Cache{}, nil
24+
}
25+
26+
c, err := newPersistentCache()
27+
assert.NoError(t, err)
28+
assert.Equal(t, azidentity.Cache{}, c)
29+
assert.True(t, called)
30+
}
31+
32+
func TestNewPersistentCache_Error(t *testing.T) {
33+
original := cacheNewFunc
34+
defer func() { cacheNewFunc = original }()
35+
36+
expectedErr := fmt.Errorf("test error")
37+
cacheNewFunc = func(opts *cache.Options) (azidentity.Cache, error) {
38+
return azidentity.Cache{}, expectedErr
39+
}
40+
41+
c, err := newPersistentCache()
42+
assert.ErrorIs(t, err, expectedErr)
43+
assert.Equal(t, azidentity.Cache{}, c)
44+
}
45+
46+
func TestNewPersistentCache_ConcurrentAccess(t *testing.T) {
47+
original := cacheNewFunc
48+
defer func() { cacheNewFunc = original }()
49+
50+
var mu sync.Mutex
51+
callCount := 0
52+
cacheNewFunc = func(opts *cache.Options) (azidentity.Cache, error) {
53+
mu.Lock()
54+
callCount++
55+
mu.Unlock()
56+
return azidentity.Cache{}, nil
57+
}
58+
59+
var wg sync.WaitGroup
60+
const goroutines = 10
61+
for i := 0; i < goroutines; i++ {
62+
wg.Add(1)
63+
go func() {
64+
defer wg.Done()
65+
_, err := newPersistentCache()
66+
assert.NoError(t, err)
67+
}()
68+
}
69+
wg.Wait()
70+
assert.Equal(t, goroutines, callCount)
71+
}
72+
73+
func TestAcquireProcessLock_ReturnsUnlockFunc(t *testing.T) {
74+
lockPath := filepath.Join(t.TempDir(), "test.lock")
75+
unlock := acquireProcessLock(lockPath)
76+
require.NotNil(t, unlock)
77+
78+
// Release the lock — should not panic
79+
unlock()
80+
}
81+
82+
func TestAcquireProcessLock_InvalidPath(t *testing.T) {
83+
// Use a path in a non-existent directory
84+
lockPath := filepath.Join(t.TempDir(), "nonexistent", "subdir", "test.lock")
85+
unlock := acquireProcessLock(lockPath)
86+
require.NotNil(t, unlock)
87+
88+
// Should be a no-op function, calling it should not panic
89+
unlock()
90+
}
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
//go:build unix
2+
3+
package token
4+
5+
import (
6+
"os"
7+
8+
"golang.org/x/sys/unix"
9+
"k8s.io/klog/v2"
10+
)
11+
12+
// acquireProcessLock attempts to acquire an exclusive file lock at the given path.
13+
// It returns a function that releases the lock. If the lock cannot be acquired,
14+
// it returns a no-op function so callers always get a valid unlock function.
15+
func acquireProcessLock(path string) func() {
16+
f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0600)
17+
if err != nil {
18+
klog.V(5).Infof("failed to open lock file %s: %v", path, err)
19+
return func() {}
20+
}
21+
if err := unix.Flock(int(f.Fd()), unix.LOCK_EX); err != nil {
22+
klog.V(5).Infof("failed to acquire lock on %s: %v", path, err)
23+
f.Close()
24+
return func() {}
25+
}
26+
return func() {
27+
if err := unix.Flock(int(f.Fd()), unix.LOCK_UN); err != nil {
28+
klog.V(5).Infof("failed to release lock on %s: %v", path, err)
29+
}
30+
f.Close()
31+
}
32+
}
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
//go:build windows
2+
3+
package token
4+
5+
// acquireProcessLock is a no-op on Windows because the macOS keychain
6+
// race condition (issue #740) does not apply. Returns a no-op unlock function.
7+
func acquireProcessLock(_ string) func() {
8+
return func() {}
9+
}

pkg/internal/token/usernamepasswordcredential.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"github.com/Azure/azure-sdk-for-go/sdk/azcore"
88
"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
99
"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
10-
"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
1110
"k8s.io/klog/v2"
1211
)
1312

@@ -35,7 +34,7 @@ func newUsernamePasswordCredential(opts *Options, record azidentity.Authenticati
3534
err error
3635
)
3736
if opts.UsePersistentCache {
38-
c, err = cache.New(nil)
37+
c, err = newPersistentCache()
3938
if err != nil {
4039
klog.V(5).Infof("failed to create cache: %v", err)
4140
}

0 commit comments

Comments
 (0)