202337 - Make Camino use the same passwords in the Keychain as Safari

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, enhancement)

Description

[Daniel Andrews]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030416 Chimera/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030416 Chimera/0.7+

with safari implementing keychain support for passwords now, i was hoping that
camino and safari could share eachother's form passwords. unfortunately, safari
used a different format for theirs.

is there a way to have camino use safari's password format?

Reproducible: Always

Steps to Reproduce:
1.open keychain 
2.safari uses diff. format for keychain - "web form password"



Expected Results:  
i think it'd be great if safair and camino could work together, as i use both on
occasion.

Comment 1

[Simon Fraser [no longer active]]

->pinkerton

Comment 2

[Greg K.]

Some observations: It appears Safari only uses the Keychain for basic
authentication. It won't store passwords entered on a web page as Camino does.

Safari postpends the username to the Keychain "Name" value.

Camino and Safari store a different "Where" value for the same site.

Comment 3

[Adam]

>It won't store passwords entered on a web page as Camino does.
Huh? v73 saves passwords entered into the appropriate form fields on web pages.

Camino could possibly read the web form passwords stored by Safari (unless
there's some security or propriety issue), but I don't think it should start
writing out "Safari-compatible" entries. Safari provides much more control over
its Keychain entries (allowing you to selectively view/delete some/all of them)
than Camino, and if you reset Safari, it might be confusing to some if there are
copycat Safari webform passwords in the keychain that didn't get deleted (also,
not knowing how Safari determines ownership of keychain entries, it'd suck if
Safari started deleting Camino entries).

Comment 4

[Mike Pinkerton (not reading bugmail)]

i think this should be possible, i need to investigate it

Comment 5

[Jasper]

Any news Mike?

Comment 6

[David Graham]

(In reply to comment #3)
> its Keychain entries (allowing you to selectively view/delete some/all of them)
> than Camino, and if you reset Safari, it might be confusing to some if there are
> copycat Safari webform passwords in the keychain that didn't get deleted (also,
> not knowing how Safari determines ownership of keychain entries, it'd suck if
> Safari started deleting Camino entries).

I realize that this may have gotten lost in the shuffle, but I'm hoping to
revive it as it is a big deal for me, and I suspect others as well.

1. Keep in mind that the whole point of a central security system is to create a
single point of access. The access point shouldn't be thought of to belong to
one browser or the other but rather the user. The user should have control over
the access point from any supported app, and as such it should be perfectly
normal behavior to delete a keychain entry from Safari and have it affect the
other browsers on the system.
2. Safari doesn't _do_ anything on it's own -- it's the user who chooses to do
it. Some people will always make bad choices and lose data. We shouldn't
insulate the few by perpetuating a system that isn't interoperable.
3. OmniWeb already respects the Safari keychain items. It's not likely that
Safari will change to support Camino so if we want to cooperate it's gonna be on
our shoulders.
4. I want to switch over to Camino but I have about 50 or so keychain entries to
recreate in Camino. We should make it as easy as possible to use this browser if
we want to maintain momentum.

Comment 7

[Peter da Silva]

I'm voting for this bug, but I'd like you to be careful with it.

Camino doesn't seem to handle passwords well at all. If you have multiple
accounts on a site, it will only save the latest one (which is something that
going to the Safari format should help, since it allows multiple accounts), and
it also doesn't seem to handle web sites that have multiple services and
different accounts on each, and it'll do things like fill in credit-card fields
with my password if they look similar.

So I'm not sure I'm happy about the possibility of Camino offering to overwrite
Safari keychain entries until all this is fixed.

Having it IMPORT the Safari entries would be safe, though.

Comment 8

[David Greenhouse]

In addition to making Safari/Camino able to share the same set of Keychain
entries, it'd be great if they could share the same set of bookmarks, stored in
one central location on the system.

Comment 9

[Mike Pinkerton (not reading bugmail)]

totally off topic of this bug, we can't share bookmarks with safari without
losing functionality. they don't propagate attributes they don't know about when
saving.

Comment 10

[Samuel Sidler (old account; do not CC)]

I highly doubt this will make 1.0 as it's not nearly as important as fixing the
way we save passwords (multiple per site, https, etc).

Targeting for 1.1.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 315707 has been marked as a duplicate of this bug. ***

Comment 12

[Paul Borokhov (lensovet)]

just to reiterate, this bug should not even be touched until we have a way of storing multiple logins. right now i actually have some sites where i have different logins in safari and camino because it's easier to have two browsers simultaneously logged into the same site and not have to worry about logging out and then back in under a different username.

Comment 13

[Arion Lawrence]

How about a compromise where Camino has *read-only* access to the Safari keychain entries? That way, we can still get the benefits of all of the entries that have already been created, and I don't mind firing up Safari once in a while to update/add new entries as needed...

Comment 14

[Daniel A. Shockley]

I vote for at least allowing Cmaino to _read_ a keychain entry. I'm trying to switch to using Camino full time and find that it is creating new entries for every site I visit. So, I'm ending up with duplicates for every site in my keychain.

Comment 15

[François Maurit]

> just to reiterate, this bug should not even be touched until we have
> a way of storing multiple logins. right now i actually have some
> sites where i have different logins in safari and camino because it's
> easier to have two browsers simultaneously logged into the same site
> and not have to worry about logging out and then back in under a
> different username.

I can't see this as being an issue.
From Greg's comment above: "Safari postpends the username to the Keychain "Name" value."
So if Camino was to follow Safari, your two logins and their respective passwords would be saved with a different "Name", and each keychain entry would be accessible from both Safari and Camino. (So your current usage pattern could remain unchanged.)

Comment 16

[Paul Borokhov (lensovet)]

(In reply to comment #15)
> > just to reiterate, this bug should not even be touched until we have
> > ...
> I can't see this as being an issue.
> ...
ah, I see that now, but it wasn't like this back in 10.2 (which is the OS that this bug is filed under). If we've dropped 10.2 support as of 1.0, then you are correct.

Comment 17

[Samuel Sidler (old account; do not CC)]

Keychain bugs -> 1.2.

Comment 18

[Stuart Morgan]

The simple case (a user has one password each for various sites in Safari, and is trying Camino for the first time) should be trivial to fix once bug 172842 lands, and I'd prefer we get that bonus in the short term without having to create a new system to handle multiple accounts per site first.  I think there's a workable short-term compromise.

For creation, start creating with the new protocol/auth, and mark them as Camino-created.

For lookup:
1) check for a new-style protocol/auth entry created by Camino, and if found use that. If not:
2) check for an old-style protocol/auth entry, and if found upgrade it to new, mark it Camino-created, and use that.
3) check for any new-style protocol/auth entry created by anyone, and if found use that.
(There's also a possible enhancement to 2, wherein if something is found in 2, check for a match of type 3 with the same username, and if there is one and it has the same password as the type 2 match, delete the type 2. That would clean up situations where users have been forced to make the same entry in both browsers, so that any subsequent password changes would only have to be made in one place. Since it would only apply to old-style, it would be a one-time cost per entry, that shouldn't be an issue.)

For update, if the entry was created by Camino, or the account matches (only the password is changing) change the entry directly, otherwise make a new entry with the new information.

The net effect should be that the simple case will work as expected, the case where someone has one account in Safari and a different one in Camino will continue to work the way it used to, and it will continue to be possible to make a new Camino entry with a different account than what is in Safari.

Entries created by Camino won't show up in Safari, AFAICT, because it has its own internal tracking list, but there's nothing we can do about that (and it shouldn't really affect the Camino experience).

Unless anyone can find a serious problem with this plan (or doing three lookups turns out to be too expensive), I'll do this after bug 172842 lands.

Comment 19

[Stuart Morgan]

I've implemented the scheme described in comment 18; I'll post it for review once bug 172842 has landed.

Comment 20

[Paul Borokhov (lensovet)]

(In reply to comment #18)
> ...
> (There's also a possible enhancement to 2, wherein if something is found in 2,
> check for a match of type 3 with the same username, and if there is one and it
> has the same password as the type 2 match, delete the type 2. That would clean
> up situations where users have been forced to make the same entry in both
> browsers, so that any subsequent password changes would only have to be made in
> one place. Since it would only apply to old-style, it would be a one-time cost
> per entry, that shouldn't be an issue.)
> ...
is this actually possible to do? i.e. will the keychain actually allow you to compare passwords from applications other than your own? doesn't that seem kind of...not secure?

Comment 21

[Stuart Morgan]

(In reply to comment #20)
> is this actually possible to do? i.e. will the keychain actually allow you to
> compare passwords from applications other than your own? doesn't that seem kind
> of...not secure?

The keychain will allow any application to read a given keychain entry if the user authorizes it. I didn't actually implement the specific section you are quoting in this version (there are some complexities in the multi-Safari-password case, and it could easily be added later if we decide it's useful), but the premise that allows that is the same thing that allows this bug to be fixed at all. If there were no way for Camino to read entries created by Safari, this bug would have been closed three and a half years ago.

Comment 22

[Stuart Morgan]

Attachment: fix

Implements comment 18, including the merging enhancement--removing old Camino keychain entry in favor of using an existing Safari entry--in the case where the user only has one Safari entry (since that's easy and common).

It also turns out that Safari's internal list appears to be built at launch, so it actually should be able to use new-style Camino entries.

Comment 23

[Paul Borokhov (lensovet)]

(In reply to comment #22)
> Created an attachment (id=246560) [edit]
> fix
> 
> Implements comment 18, including the merging enhancement--removing old Camino
> keychain entry in favor of using an existing Safari entry--in the case where
> the user only has one Safari entry (since that's easy and common).
wait...you said in 18 that "2) check for an old-style protocol/auth entry, and if found upgrade it to new, mark it Camino-created, and use that." is there a way to delete the Safari entry instead? it'd be nice if creation dates for keychain items were retained. the way you describe it now, if the camino item was created before the safari item, you'd technically have an incorrect creation date.
am i making sense? my point is that ideally you wouldn't be deleting camino entries, but "upgrading" them, and deleting safari entries, if they are duplicates, instead. or does "upgrading" mean "create new keychain entry", so creation dates wouldn't be preserved regardless?

Comment 24

[Stuart Morgan]

(In reply to comment #23)
> the way you describe it now, if the camino item was created before the
> safari item, you'd technically have an incorrect creation date.

I don't see how if you have two existing keychain entries, one of their dates is somehow more correct than the other. And there's definitely no way I'm going to make the merging scheme more complex by comparing timestamps for the sole purpose of preserving an attribute that a vanishingly small number of users will ever see, much less care about.

> ideally you wouldn't be deleting camino entries, but "upgrading" them, and
> deleting safari entries

I know exactly what information Camino needs and uses from keychain entries, and by having found an entry I know it's compatible with Camino's needs. I do not know exactly what information Safari needs and uses from keychain entries, and I know of at least one or two ways in which is uses attributes that Camino currently does not. I see potentially serious drawbacks to deleting Safari's entries and no advantage, so I disagree that it would be ideal.

Comment 25

[Josh Aas]

Comment on attachment 246560 [details] [diff] [review]
fix

+        [[[newKeychainItems objectAtIndex:0] username] isEqualToString:[item username]])
+    {

Brace on the same line plz


+  if ([username isEqualToString:[keychainItem username]] || [keychainItem creator] == kCaminoKeychainCreatorCode)
+    [keychainItem setUsername:username password:password];
+  else {

If anything in an if statement has braces for the block they all should.

+  NSString* scheme = (port == 443) ? @"https" : @"http";

Numeric constants like that should be statics at the top of the file, confident as you may be that the port number for https isn't going to change any time soon...

Comment 26

[Stuart Morgan]

Comment on attachment 246560 [details] [diff] [review]
fix

Since those changes are minor, I'll hold off on respinning for the moment.

Comment 27

[Mike Pinkerton (not reading bugmail)]

+- (void)setHost:(NSString*)host
+{
+  const char* hostString = [host UTF8String];
+  UInt32 hostLength = hostString ? strlen(hostString) : 0;
+  if([self setAttributeType:kSecServerItemAttr toValue:(void*)hostString withLength:hostLength])
+    mHost = host;

needs to autorelease mHost otherwise you'll leak, right?

+ // If there's exactly one sharable keychain entry (so we know we'll always get
+ // that one) and it has the same account, delete this entry so we start using
+ // the sharable entry.

the problem here is what if they try 1.1a2, hate it, then go back to 1.0? all their keychain entries are gone. Isn't that kinda bad?

Comment 28

[Stuart Morgan]

(In reply to comment #27)
> +    mHost = host;
> 
> needs to autorelease mHost otherwise you'll leak, right?

Yeah, that should be
  [mHost autorelease];
  mHost = [host copy];

> + // If there's exactly one sharable keychain entry (so we know we'll always
> get
> + // that one) and it has the same account, delete this entry so we start using
> + // the sharable entry.
> 
> the problem here is what if they try 1.1a2, hate it, then go back to 1.0? all
> their keychain entries are gone. Isn't that kinda bad?

Well, all the entries for sites they've visited with 1.1a2 (or b1, or whatever), but yes. It's not just there; if they have only a Camino entry, it'll be upgraded to use different storage keys, and Camino 1.0 won't find it. This is definitely a one-way street, and we'd need to rel-note that prominently.

I could switch it to never doing anything to existing entries unless they are changed, which has some pros and cons. I'll think about that some more and then we can talk about the tradeoffs.

Comment 29

[Paul Borokhov (lensovet)]

this might be overkill, but the first time camino wants to do the upgrading/deleting, you could throw up a dialog box offering to make a backup of (all) your current (Camino) keychain items in case you want to roll back. no?

Comment 30

[Stuart Morgan]

No.

Comment 31

[Stuart Morgan]

Attachment: v2

Addresses review comments.

Comment 32

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 248055 [details] [diff] [review]
v2

+// hnd may have been created with the authentication type backward due to an

s/hnd/and

sr=pink

Comment 33

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH with typo fix.

There are bound to be some specific cases where we don't pick up a password from Safari; please file any of those as new bugs rather than as comments here.

Comment 34

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Anyone who is concerned about the ramifications of this fix, be sure to make a backup copy of your Keychains (in ~/Library/Keychains) before running a branch or trunk nightly built after 10:40 AM PST today.

We'll probably include text to this effect in the 1.1a2 relnotes, but ultimately this is a one-way street to the future at some point (a2, b1, 1.1 final)....

Comment 35

[Stuart Morgan]

The version as landed only affects passwords that are saved with the new build (either completely new entries, or changed passwords), and nothing will be deleted, so I don't see why there would be any cause for concern.

Comment 36

[Stuart Morgan]

Hm, one other thing we should probably relnote is that unlike us, Safari doesn't use the creator field to allow it to delete only its own keychain items. Now that Safari can read our new entries, it will happily remove them all when performing a "Reset Safari".

Comment 37

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Sorry, I didn't catch any comment explaining the latest version would only update entries where the login or password are changed (or completely new ones), not just accessed (which I took comment 18 to suggest) :(

Are there any other changes from the scheme outlined in comment 18 that I've missed?

Comment 38

[Stuart Morgan]

The changes from the scheme in comment 18 are:
- The part about upgrading in 2) is no longer correct
- The related enhancement to 2) therefore isn't happening either

So basically it's all the same except that we don't upgrade our old entries on read, so that the experience for someone trying 1.1a2 briefly is better.

Comment 39

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #36)
> Hm, one other thing we should probably relnote is that unlike us, Safari
> doesn't use the creator field to allow it to delete only its own keychain
> items. Now that Safari can read our new entries, it will happily remove them
> all when performing a "Reset Safari".

Did you file a bug with Apple on that?  That sounds very evil.

Comment 40

[Paul Borokhov (lensovet)]

(In reply to comment #36)
> Hm, one other thing we should probably relnote is that unlike us, Safari
> doesn't use the creator field to allow it to delete only its own keychain
> items. Now that Safari can read our new entries, it will happily remove them
> all when performing a "Reset Safari".
are you sure about this? didn't someone mention that safari keeps a separate .plist-like list of saved keychains and only touches those?

Comment 41

[Stuart Morgan]

(In reply to comment #39)
> Did you file a bug with Apple on that?  That sounds very evil.

No; it's arguably correct behavior in that the keychain is a shared resource,
and those are now parts of a shared resource that are used by Safari (the user
has to have authorized Safari to access them before it will try to delete them,
so really the release notes should say something like "Using Reset Safari will
delete any new Camino passwords that you have used in Safari"). It's not really
any different than Reset Safari deleting cookies that are shared by all WebKit-
and NSHTTPCookieStorage-based apps, which I believe it also does. One could
argue that we should do it too, but I wanted to err on the side of being
conservative.

Anyone who feels more strongly about it than I do is welcome to file a bug with
Apple.

Comment 42

[Stuart Morgan]

(In reply to comment #40)
> are you sure about this?

Yes. If I make an unqualified claim about behavior in a bug, it's a pretty good bet that I tested before making that claim. Please stop second-guessing me in bugs based on your own speculation (especially if your speculation is based on not having read the bug completely; see below). If you think I'm wrong, then test it yourself first.

> didn't someone mention that safari keeps a separate
> .plist-like list of saved keychains and only touches those?

I mentioned that it appeared to keep an internal list (neither I nor anyone else said anything about a plist), and I later (comment 22) said that it turned out to be rebuilt at launch.

Comment 43

[Samuel Sidler (old account; do not CC)]

Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.