Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

persistent storage isn't available due to error "The specified item already exists in the keychain." #23588

Open
mateuszlitwin opened this issue Oct 14, 2024 · 2 comments · May be fixed by #23619
Open

persistent storage isn't available due to error "The specified item already exists in the keychain." #23588

mateuszlitwin opened this issue Oct 14, 2024 · 2 comments · May be fixed by #23619
Assignees
@chlowell
Labels
Azure.Identity bug This issue requires a change to an existing behavior in the product in order to be resolved. customer-reported Issues that are reported by GitHub users external to the Azure organization.
Milestone
2024-11

Comments

@mateuszlitwin
Copy link

mateuszlitwin commented Oct 14, 2024 •
edited
Loading

Bug Report

  • output of go version
	github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.0
	github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache v0.3.0
  • What happened?

Executing multiple instances of a program on macOS with keychain token caching ("github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache") fails with:

persistent storage isn't available due to error "The specified item already exists in the keychain. (-25299)"

This is because the tryStorage function is executed many times when many instances of a program are executed concurrently: https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azidentity/cache/cache.go#L71 The sync.Once is obviously not enough to prevent cache from "trying" storage simultaneously from many programs at once.

  • What did you expect or want to happen?

Cache creation not failing.

  • How can we reproduce it?

Execute program with token caching many times in parallel. I think just many cache.New(nil) in parallel is needed.

package main

import (
	"fmt"

	"github.com/Azure/azure-sdk-for-go/sdk/azidentity/cache"
)

func main() {
	_, err := cache.New(nil)
	if err != nil {
		fmt.Printf("failed to create cache: %s", err)
	}
}


for i in {1..10}; do ./main &; done; wait
  • Anything we should know about your environment.

I suggest giving an option to disable the tryStorage check in cache.Options. I think the change is trivial and seems to work in my environment.

Additionally, the tryStorage takes ~30ms on M2 MAX so it would be nice to have an option to shave that time from the Go cli.

General question: Is the cache safe for concurrent use from multiple programs? e.g. requesting and caching tokens from multiple programs simultaneously on macOS? can keychain be modified concurrently?
@github-actions github-actions bot added Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Service Attention Workflow: This issue is responsible by Azure service team. Storage Storage Service (Queues, Blobs, Files) labels Oct 14, 2024
@github-actions GitHub Actions
Copy link

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage.

@jhendrixMSFT jhendrixMSFT added Azure.Identity and removed question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Storage Storage Service (Queues, Blobs, Files) Service Attention Workflow: This issue is responsible by Azure service team. Client This issue points to a problem in the data-plane of the library. labels Oct 14, 2024
@github-actions github-actions bot added the needs-team-triage Workflow: This issue needs the team to triage. label Oct 14, 2024
@chlowell chlowell added this to the 2024-11 milestone Oct 16, 2024
@chlowell chlowell added bug This issue requires a change to an existing behavior in the product in order to be resolved. and removed needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team needs-team-triage Workflow: This issue needs the team to triage. labels Oct 16, 2024
@chlowell
Copy link
Member

chlowell commented Oct 19, 2024 •
edited
Loading

Thanks for opening this issue! You're quite right that cache.New does nothing to prevent conflict between multiple processes, and it should. I think we don't want to synchronize processes with e.g. a kernel mutex but we can prevent contention by simply adding some randomness to the test cache name. That should unblock all the common scenarios, although on at least some platforms it would still be possible for processes to fail the storage test if the degree of concurrency were very high. How many simultaneous processes do you need?

As for making the storage test optional, I agree that's trivial and would also solve this problem. However, I think it would make the API more difficult to understand and the broader feature harder to use:

// before
c, err := cache.New(nil)
if err != nil {
    // Persistence isn't possible. Fall back to in-memory caching or
    // abandon the operation. Falling back is simple because you
    // haven't created a credential or client yet.
}

// after
c, err := cache.New(&cache.Options{
        // how do I decide whether to set this?
        SkipStorageTest: true,
    }
)
if err != nil {
    // actually err is always nil, and that doesn't indicate the cache is useable
}
// later, somewhere else in the app...
err := client.Foo()
if err != nil {
    // need a new type to distinguish persistent storage errors
    // from HTTP errors, authNZ errors, server errors, etc.
    var e *cache.StorageError
    if errors.As(err, &e) {
        // if you want to fall back to in-memory caching, create
        // a new credential and client, then call Foo again
    }
}

@chlowell chlowell linked a pull request Oct 21, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chlowell chlowell

Labels
Azure.Identity bug This issue requires a change to an existing behavior in the product in order to be resolved. customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
Azure Identity SDK Improvements
Status: In Progress
Milestone
2024-11
Development

Successfully merging a pull request may close this issue.

Enable concurrent process to construct persistent caches chlowell/azure-sdk-for-go
3 participants
@mateuszlitwin @jhendrixMSFT @chlowell