Adium

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#7773 closed defect (fixed)

Keychain name entries are not very informational.

Reported by: rcfa Owned by: nobody
Milestone: Adium 1.3 Component: Adium Core
Version: Severity: normal
Keywords: starter Cc: rcfa+adiumx.com@…, patches@…
Patch Status: Accepted

Description

The keychain is designed to avoid redundancy. As such e.g. a pop password is stored under a name like "pop://pop.somemailserver.com/", with an account that corresponds to the username, and finally the password.

Translated to instant messaging, this should mean a password should be stored with a name like "aim://login.oscar.aol.com:5190", an account that corresponds to the AIM account name, and finally the password.

Storing passwords this way causes no trouble if keychains are synced over multiple systems on which only some accounts are used in Adium, or where the accounts have been added in a different sequence. It further allows IM accounts and passwords to be stored in an application agnostic way. Nothing is more obnoxious than having the same password stored multiple times in the keychain, like e.g. if IE, Safari, FireFox, Camino, Opera all decided to store web passwords differently.

Instead currently passwords are stored in a utterly meaningless way: The account is something like Mac.6 which has no meaning, particularly since I can reorder the accounts within Adium, and there's no indication which account is Mac.6, nor will it help me to display the password, if I forget an account password, because I will have to display all Mac.* passwords, and try them all out, because there's no clear way to associate one with a specific account. The location is stored as something like AdIM://Adium.Mac.6 which is neither a proper URL, and is further utterly redundant, because we have Mac.6 already in the account! Instead I still don't know what protocol, host, port is used for the connection.

In short, the way passwords are stored in the keychain is barely better than just storing them in a proprietary preferences file and bypassing the keychain entirely.

Attachments (1)

7773-tomgr.diff (9.8 KB) - added by Thomas Gibson-Robinson 12 years ago.
Updated version

Download all attachments as: .zip

Change History (21)

comment:1 Changed 12 years ago by Chris Forsythe

Ever thought to look in ~/Library/Application Support/Adium 2.0/Users/Default/Accounts.plist ?

Adium is designed so that if you copied ~/Library/Application Support/Adium 2.0 to another machine, and copy the keychain, then it'll all work. Some people have considered revealing account id's in the keychain entries to be bad in case of a compromised system, I believe this was the justification years ago for the system we have.

comment:2 Changed 12 years ago by Evan Schoenberg

That would be 'security through obscurity', Tick - revealing the account ID in the keychain entry is no different from having it be coded by a number which can, as you point out, be very easily looked up in a plist on the same system.

The current system is just wrong, the result of a poorly thought out coding decision. I'd be happy to see a patch which makes the storage not suck as it currently does, with the caveat that it must also include an upgrade path for changing the current entries into the new format.

comment:3 Changed 12 years ago by Ronald C.F. Antony

I agree, the idea that account data should be "protected" by obscuring it, is not going to hold up. You just need to launch Adium or look at the plist files to reveal that information anyway. What should be secret isn't the account name, but the password.

As for security: First, the keychain is encrypted, second, particularly sensitive data belongs into an appropriate separate keychain, not into the login keychain, and thus should have a separate password. Third, if the system is compromised, you'll have all sorts of other problems, the IM account names are the least of your worries.

If that line of reasoning is taken, you just have to abandon using keychain altogether, which for the paranoid could be an option (ask for account and password each time you launch Adium), but for a regular user, neither that sort of paranoia nor the current way of storing the info makes sense.

Of course, migration of the current way to a better way is ideal, but I'd rather deal with the one-time hassle of re-entering my accounts than living for the future with the current situation, if that's what the choice comes down to.

comment:4 in reply to:  2 ; Changed 12 years ago by Chris Forsythe

Replying to evands:

That would be 'security through obscurity', Tick - revealing the account ID in the keychain entry is no different from having it be coded by a number which can, as you point out, be very easily looked up in a plist on the same system.

Right, I'm not saying it's any good, just mentioning what I think was the reasoning.

Is there an easy way/good way to convert the current keychain entries though? I don't want users to have to re-enter passwords they probably already forgot after typing them, and I'm sure nobody else wants that either.

comment:5 Changed 12 years ago by Chris Forsythe

Summary: Password storage in keychain suboptimal, to say the least...Keychain name entries are not very informational.

Rewording the summary to be more accurate.

comment:6 in reply to:  4 ; Changed 12 years ago by Evan Schoenberg

Replying to tick:

Replying to evands:

That would be 'security through obscurity', Tick - revealing the account ID in the keychain entry is no different from having it be coded by a number which can, as you point out, be very easily looked up in a plist on the same system.

Right, I'm not saying it's any good, just mentioning what I think was the reasoning.

Nah, the reasoning was that you can have multiple accounts with the same service + account name within Adium, so they needed to be separate. This is patently dumb, though, because service + account name uniquely has a single password.

I can say it's dumb since it was my reasoning, 3 years ago or so... :P

Is there an easy way/good way to convert the current keychain entries though? I don't want users to have to re-enter passwords they probably already forgot after typing them, and I'm sure nobody else wants that either.

Sure. A block of upgrade code that reads the keychain entry using service + internalObjectID, writes it using service + account name, with whatever else we're now storing, then deletes the old keychain entry. Someone just has to write it ;)

comment:7 in reply to:  6 Changed 12 years ago by Chris Forsythe

Replying to evands:

Replying to tick:

Replying to evands:

That would be 'security through obscurity', Tick - revealing the account ID in the keychain entry is no different from having it be coded by a number which can, as you point out, be very easily looked up in a plist on the same system.

Right, I'm not saying it's any good, just mentioning what I think was the reasoning.

Nah, the reasoning was that you can have multiple accounts with the same service + account name within Adium, so they needed to be separate. This is patently dumb, though, because service + account name uniquely has a single password.

I can say it's dumb since it was my reasoning, 3 years ago or so... :P

Maybe I'm thinking of something else then. *shrug*

I can say the only time I would need multiples of the same account would be with testing, but I really haven't done that in a long time. What if we just did a aim.accountname.1 or something similar?

comment:8 Changed 12 years ago by Evan Schoenberg

Number isn't needed at all and just creates unexpected breakages. It should just be aim.[accountname compactedString]

comment:9 in reply to:  8 Changed 12 years ago by Ronald C.F. Antony

Replying to evands:

Number isn't needed at all and just creates unexpected breakages. It should just be aim.[accountname compactedString]

Are there standardized URLs sort of the equivalent of the mailto: URLs for IM services? If so, ideally it would seem, such a URL should be the name.

comment:10 Changed 12 years ago by Jordan

Milestone: Good idea for "later"

Setting to good idea for later simply because nobody is working on it at the moment, bring it forward if you decide to work on this.

As was mentioned before, make sure you include:

  1. Adding a new account creates the keychain name as mentioned above
  2. Upgrading from a current version of Adium (with the current style of keychain naming) will create new names in the keychain for each account, copying the usernames and passwords from the old entries, and deleting the old entries (perhaps only after testing connectivity to confirm the new entries are successful).

comment:11 Changed 12 years ago by Colin Barrett

Keywords: goodfirstbug added

This would be a good patch for someone to cut their teeth on. Pretty isolated. Ultra super duper hyper bonus points if it comes with unit tests (O M G!)

comment:12 Changed 12 years ago by Colin Barrett

Keywords: starter added; goodfirstbug removed

I suck at remembering keywords.

comment:13 Changed 12 years ago by Colin Barrett

Patch Status: NoneNeeds Dev Review

comment:14 Changed 12 years ago by Colin Barrett

Cc: patches@… added

Nit: I don't like "obscured" for the name. It's not obscured, it was so you could rename accounts (which was a little silly to allow, granted).

How about oldStyle instead?

Evan can you review this?

Changed 12 years ago by Thomas Gibson-Robinson

Attachment: 7773-tomgr.diff added

Updated version

comment:15 Changed 12 years ago by Thomas Gibson-Robinson

I've uploaded an altered version where I changed obscured to oldStyle - a much better name (thanks cbarrett)!

comment:16 Changed 12 years ago by Evan Schoenberg

Now that I've fixed the patches report, I'm going through the tickets with patches. Sorry for the stagnation of this ticket in the meantime, tomgr.

The only remaining problem I have with this change is that we're iterating through all accounts' passwords every time Adium launches. This should be a one-time upgrade rather than an every-time upgrade for launch performance reasons. I'll go ahead and make that change as I apply it.

comment:17 Changed 12 years ago by Evan Schoenberg

Milestone: Good idea for "later"Adium X 1.3

comment:18 Changed 12 years ago by Evan Schoenberg

Resolution: fixed
Status: newclosed

(In [22401]) Patch from tomgr which makes Adium store account passwords in a more useful form, as ServiceID.Screenname instead of AdiumProfile.ServiceID.AccountNumber. An account's password will be the same wherever you go, and there's no reason to obscure the Keychain Access output.

The upgrade runs once, when Adium 1.3 is first launched. Reverting to Adium 1.2.x will subsequently show no passwords stored, though they can of course be typed in again and therefore saved in both formats.

Thanks, Tom! Closes #7773.

comment:19 Changed 12 years ago by Thomas Gibson-Robinson

That's ok - I can certainly see you have been busy elsewhere!

comment:20 Changed 12 years ago by Robert

Patch Status: Needs Dev ReviewAccepted
Note: See TracTickets for help on using tickets.