Adium

Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#4463 closed defect (fixed)

can't use multiple keychains?

Reported by: adium Owned by: evands
Milestone: Adium X 1.0.2 Component: Adium Core
Version: Severity: regression
Keywords: keychain Cc: wd@…
Patch Status:

Description

With v0.89.1 I was able to move my Adium saved password into a second/alternate keychain; Under 1.0b2, it no longer seems to see the passwords in the alternate keychain. If I move them back to the login chain, it works, so the entries are still valid; it just can't see them in the alternate chain. For completeness, I did try removing them, re-creating them in the login chain, and moving them to the alternate chain (but that didn't change anything).

I couldn't find any authoritative docs on multiple keychains under OSX; the original reference I found was:

http://www.osxfaq.com/DailyTips/09-2004/09-06.ws

While they describe a "more secure" alternate chain, in my case it's a "non-time-locked" alternate chain. That is, my login chain times out and locks itself, and I want my adium passwords in a chain that doesn't timeout and lock itself, so that if my connection is dropped while I'm away, it can reconnect, rather than prompting and waiting for a password..

Thanks.

(I didn't see a field on the webpage for my email address? It's "wd@…")

Change History (13)

comment:1 Changed 14 years ago by Juan Manuel Palacios

Cc: wd@… added
Component: NoneAdium Core
Milestone: Adium 1.0 - After beta 1
priority: normallow
Severity: normalRegression

This is a regression from 0.8.1, but a low priority one for now, given the amount of work still pending to complete the beta stage. Adjusting values accordingly.

-jmpp

comment:2 Changed 14 years ago by adium

Note: typo in cc field -- should end in ".net" rather than ".ne". Thanks.

comment:3 Changed 14 years ago by Evan Schoenberg

Cc: wd@… added; wd@… removed

comment:4 Changed 14 years ago by Peter Hosey

Owner: changed from nobody to Peter Hosey
Status: newassigned

We only use the default keychain currently. A fix for this would be to iterate all keychains and show the ask-for-password dialog when all are exhausted.

I'll look into pulling this off sometime this week.

comment:5 Changed 14 years ago by Chris Forsythe

Milestone: Adium 1.0 - After beta 1Adium X 1.0

We're not going to block any betas on this, and probably not 1.0. Putting to 1.0 to give Boredzo time, but if it's not done by release time push it to sometime after 1.0

comment:6 Changed 14 years ago by Chris Forsythe

Milestone: Adium X 1.0Adium X 1.0.1

comment:7 Changed 13 years ago by Chris Forsythe

Milestone: Adium X 1.0.1Sometime after 1.0

comment:8 Changed 13 years ago by Rob Speed

I just stumbled across this bug again after forgetting about it for some time. Can we get a more specific milestone target? I can see this as being a somewhat important issue for some security-minded people.

comment:9 Changed 13 years ago by poisonousinsect

This bug has been bothering me too.

I looked at AIKeychain, and needless (ie. the source of this bug) complexity has been introduced. Client code apparently used to get passwords by using a class method (+[AIKeychain getPasswordFromKeychainForService:account:]) that searched all the keychains with SecKeychainFindInternetPassword(NULL,…), but now clients get a specific keychain (+[AIKeychain defaultKeychain_error:]) and search that (-[AIKeychain internetPasswordForServer:…]).

I don't know if there was something wrong with the old version that provoked the change, but otherwise the whole class should be reverted, because Adium should never deal with individual keychains.

If reverting is too extreme, or other code depends on it (although I didn't see any), have +[AIKeychain defaultKeychain_error:] return a keychain object with keychainRef set to NULL. Or, don't change AIKeychain, but have AdiumPasswords.m and AISystemNetworkDefaults.m use the old class methods which do the proper keychain searching. All of these changes would ultimately call SecKeychainFindInternetPassword(NULL,…), which I believe is the correct solution.

It's worth pointing out that Adium should definitely not enumerate the keychains itself—use the system that's already there.

I hope this helps, boredzo. I only briefly glanced over the code, so apologies if I'm way off track. I should also mention that I'm looking at slightly old SVN code. I can look at the latest code sometime, if necessary.

comment:10 Changed 13 years ago by Evan Schoenberg

Milestone: Good idea for "later"Adium X 1.0.2

Indeed, it looks like in every keychain related function, passing NULL asks it to use the default keychain, except for in SecKeychainFindInternetPassword() in which prophesy states:

keychainOrArray
A reference to an array of keychains to search, a single keychain or NULL to search the user’s default keychain search list.

So NULL is good. But what if there's no keychain at all?

Regarding SecKeychainCreate(), Apple writes:

This function creates an empty keychain. The keychain, password, and initialAccess parameters are optional. If user interaction to create a keychain is posted, the newly-created keychain is automatically unlocked after creation. The system ensures that a default keychain is created for the user at login, thus, in most cases, you do not need to call this function yourself. Users can create additional keychains, or change the default, by using the Keychain Access application. However, a missing default keychain is not recreated automatically, and you may receive an errSecNoDefaultKeychain error from other functions if a default keychain does not exist. In that case, you can use this function followed

by SecKeychainSetDefault (page 10), to create a new default keychain.

So checking against the availability of a default keychain -- and creating one if necessary -- is a good thing. Continuing to use that returned default keychain rather than letting the search Just Happen isn't.

comment:11 Changed 13 years ago by Evan Schoenberg

Owner: changed from Peter Hosey to Evan Schoenberg
Status: assignednew

comment:12 Changed 13 years ago by Evan Schoenberg

Resolution: fixed
Status: newclosed

(In [19088]) This should make the default AIKeychain behave as expected, searching the keychain list appropriately and otherwise using default functionality of SecKeychain to talk to the default one. Calling functions which explicitly require a keychain to be passed (don't accept NULL) will obtain the default SecKeychain temporarily and use it if one hasn't been specified, which can still be done via the non-'default' creation methods. Fixes #4463

comment:13 Changed 13 years ago by Evan Schoenberg

(In [19089]) Merged [19088]: This should make the default AIKeychain behave as expected, searching the keychain list appropriately and otherwise using default functionality of SecKeychain to talk to the default one. Calling functions which explicitly require a keychain to be passed (don't accept NULL) will obtain the default SecKeychain temporarily and use it if one hasn't been specified, which can still be done via the non-'default' creation methods. Fixes #4463

Note: See TracTickets for help on using tickets.