172842 - Implement Keychain Services API

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, enhancement, P3)

Description

[Thomas F. O'Connell]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20020909
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.1) Gecko/20020909

now that bug 152485 is FIXED, it's time to update to the latest Apple API.

Reproducible: Always

Steps to Reproduce:





see
http://developer.apple.com/techpubs/macosx/CoreTechnologies/securityservices/keychainservices/keychainservices.html
(also listed in bug 152485) for more.

Comment 1

[Stephane Moureau]

Since several versions the Keychain is implemented.
Can be changed to Fixed/Solved.

Comment 2

[Thomas F. O'Connell]

actually, this bug refers to a change Apple made in the API since Chimera began
using the Keychain. i believe the bug is still valid, although low priority, in
my opinion, since a workable solution is in place.

Comment 3

[Jasper]

Please update.

Comment 4

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

This only service I can see that's not been implemented is multiple passwords,
which is covered by bug 178607. As such, I'm closing this bug. If I'm wrong,
feel free to reopen.

Comment 5

[L. H.]

Reopening. It uses old Keychain Manager instead of new Keychain Services.

Comment 6

[Jasper]

Keep it open, Josh said he would work on moving the our keychain support to the
new api's maybe this is something that can be achieved during that change.

Comment 7

[Chris Lawson (gone)]

CC'ing Josh per comment 6, and Håkan, since he was looking into this at one point.

Simon, I presume you're not still working on this, so I've reassigned back to default.

Comment 8

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

Keychain bugs -> 1.2.

Comment 9

[Stuart Morgan]

Taking. I was going to fix this before I left, so it seems like a good place to start up again.

Comment 10

[Stuart Morgan]

(Once I can actually start up again, of course)

Comment 11

[Stuart Morgan]

Attachment: Use KS (trunk version)

Switches from KeychainManager to KeychainServices.  Also does on-the-fly fixup of any backwards items it finds.

Notes:
- This won't compile on branch, because there's a type-conflict-inducing file that has been removed on the trunk but needs patching on branch.  I'll make a branch version once a trunk version has passed muster.
- This requires project changes: Security.framework needs to be linked, and the KeychainItem files need to be added.
- The new KeychainItem files are added to src/browser, because that's where the KeychainService files are.  However, I'd suggest that now that there's a src/formfill folder, unless anyone strongly objects to the cvs history disconnect we should put all of the Keychain* files in src/formfill (and the corresponding folder in the Xcode UI).
- There's lots of whitespace cleanup, but unfortunately it's mostly the kind -w wouldn't help with.

Comment 12

[Josh Aas]

Comment on attachment 239800 [details] [diff] [review]
Use KS (trunk version)

A diff -w version of this patch would be great given the number of whitespace changes!

Comment 13

[Paul Borokhov (lensovet)]

he said «- There's lots of whitespace cleanup, but unfortunately it's mostly the kind -w would NOT help with.»

Comment 14

[Josh Aas]

Comment on attachment 239800 [details] [diff] [review]
Use KS (trunk version)

+      *user = [[entry username] createNewUnicodeBuffer];

This will probably work fine even if username is nil but it makes me uneasy looking at it.

I assume we were importing the header files for functions like "kcfindinternetpassword" through <Carbon/Carbon.h>? I don't see you removing any more specific header files for those functions. Just checking.

Otherwise this looks good.

Comment 15

[Stuart Morgan]

Attachment: whitespace changes manually removed

Same as above, but with all the whitespace changes manually ripped out (or silently applied where they were inline with real changes). It's not a valid patch by any stretch of the imagination, but it's probably easier to read.

Comment 16

[Stuart Morgan]

Attachment: patch v2 (trunk version)

-[KeychainItem username] can't return nil (it's set to @"" if KS craps out for some reason), so the buffer thing is safe.

The old stuff was indeed coming through Carbon/CoreServices, but come to think of it it doesn't look like anything else in KeychainService.mm uses them, so this patch removes them both.

Anyone else want to take a look?

Comment 17

[Josh Aas]

Comment on attachment 239814 [details] [diff] [review]
patch v2 (trunk version)

Maybe things are different the the Camino world these days, but usually if you don't actually request a review from someone it won't get done. And I just know Pinkerton wants to review this.

Comment 18

[Stuart Morgan]

Comment on attachment 239814 [details] [diff] [review]
patch v2 (trunk version)

I assume pink will want to sr it, but one of the other Camino people may want to review it before then.

Let's try this instead.

Comment 19

[Stuart Morgan]

Comment on attachment 239814 [details] [diff] [review]
patch v2 (trunk version)

One more time. Sorry for the spam.

Comment 20

[Mike Pinkerton (not reading bugmail)]

does this migrate keychain entries?

Comment 21

[Stuart Morgan]

(In reply to comment #20)
> does this migrate keychain entries?

In general it doesn't need to, since this is a format-compatible move to KS. Using more accurate protocol/auth types, sharing with Safari, etc. will all be done in follow-up bugs.

The only modifications it makes are fixing any backwards items as they are used (and the only difference in storage is that KS won't make things backwards, because the KS headers were, unlike the KM headers, made Intel-friendly).  I considered doing an all-at-once migration, but that will be messy for anyone going back and forth between versions, so I thought we'd do this for now, then later on do a one-time migration and rip out the double-checking.

Comment 22

[Nick Kreeger]

Comment on attachment 239814 [details] [diff] [review]
patch v2 (trunk version)

I'll take a look tonight

Comment 23

[Simon Fraser [no longer active]]

A first-glance question. Should you pass the protocol into the saving and getting methods, in case we want to save passwords for ftp, or https at some point?

Comment 24

[Stuart Morgan]

It'll need to be done at some point if for no other reason than to fix bug 197670.  I was going to change that as part of later work, but there's no reason I couldn't do it now if you'd prefer.

Comment 25

[Simon Fraser [no longer active]]

Might as well do it now.

Comment 26

[Stuart Morgan]

Comment on attachment 239814 [details] [diff] [review]
patch v2 (trunk version)

Clearing review flags while I cook up some more related functionality that the later patches will need.

Comment 27

[Stuart Morgan]

Attachment: more complete KeychainItem version

This pushes the protocol up out of KeychainItem (to the same level where the auth type is currently decided), and also adds KeychainItem protocol and auth type accessors that I'll need to fix some of the other bugs.

Comment 28

[Stuart Morgan]

Comment on attachment 240742 [details] [diff] [review]
more complete KeychainItem version

Kicking over to pink now that he's back.  Feel free to steal it back if you still want it, Simon.

Comment 29

[Simon Fraser [no longer active]]

Sorry, I got swamped this week.

Comment 30

[Stuart Morgan]

Attachment: conceptual version of v3 diff

Sorry, I should have done this before.  This is an updated hacked up patch file with the whitespace/style changes all ripped out.  I also rearranged some chunks of the diff to align new code with the code it actually replaces, in the areas where the changes were too extensive for diff to figure out correctly.

Pink, this should be much more palatable for review as a way to see what the real changes are (you should also be able to split the review roughly in half by reviewing KeychainItem first, then the KeychainServices.mm changes as a later chunk).

Simon, if you happen to feel like jumping in here I know Pink wouldn't mind at all.

Comment 31

[Stuart Morgan]

I'll need to respin this to apply cleanly once bug 355033 lands.

Comment 32

[Mike Pinkerton (not reading bugmail)]

+@interface KeychainItem : NSObject
+{
+  SecKeychainItemRef mKeychainItemRef;

make the members @private

+  NSString* mUsername;
+  NSString* mPassword;

mention if these are strong or weak references

+- (void)setUsername:(NSString*)username andPassword:(NSString*)password;

prepositions are ok (with, for, etc), but i don't think we need conjunctions in param names. "and" is obvious and redundant and just makes it longer needlessly. There are a bunch of these, just omit the "and" in all cases.

+  unsigned int i;
+  for (i = 0; i < attrList->count; i++) {

declare |i| in the for loop

+  const char* usernameString = [username UTF8String];
+  user.data = (void*)usernameString;
+  user.length = strlen(user.data);

i've seen cases from time to time where UTF8String returns NULL. Should we worry about those? calling strlen() on NULL will crash.

+    NSLog(@"Couldn't update keychain item user and password");

if you're going to log, why not log the account name?

sr=pink with those changes.

Comment 33

[Stuart Morgan]

(In reply to comment #32)
> +  unsigned int i;
> +  for (i = 0; i < attrList->count; i++) {
> 
> declare |i| in the for loop

This is a .m file, which means to do that and still compile we either have to change the C dialect on that specific file (which seems really ugly) or for the whole project (which I wasn't sure we wanted). Which way do you want to go?

Comment 34

[Stuart Morgan]

I'll need somebody to make a project patch for both trunk and branch once we have an answer to the above which (in addition to possibly doing one of the above) adds KeychainItem.{h,m} in the same places KeychainServices.{h,mm} are now, and adds /System/Library/Frameworks/Security.framework to the linked framework section.

Comment 35

[Stuart Morgan]

Attachment: branch type hack

And the last piece is that on branch we need something like this to prevent a type collision. I'm not sure if there's a better way for us to conditionalize this (or how much we care since this file is evidently regarded as a bad move, and has been removed entirely on the trunk), or who all needs to sign off on it to get it on the branch.

Comment 36

[Mike Pinkerton (not reading bugmail)]

isn't c99 the default for .m files? that's legal in c99.

Comment 37

[Håkan Waara]

(In reply to comment #36)
> isn't c99 the default for .m files? that's legal in c99.

I don't think it is (default). I always get that in my projects...

Comment 38

[Stuart Morgan]

(In reply to comment #36)
> isn't c99 the default for .m files?

Nope.

Comment 39

[Mike Pinkerton (not reading bugmail)]

then we should make it the default or make it a .mm file. gah. what a lame language.

Comment 40

[Mike Pinkerton (not reading bugmail)]

can we file a separate bug for that nspr wrapper? we'd have to get wan-teh to sign off on it.

Comment 41

[Stuart Morgan]

Per IRC discussion, the project change should also move the keychain files to the formfill project directory, and change the references to point to the formfill directory on disk.

Comment 42

[Stuart Morgan]

Comment on attachment 245362 [details] [diff] [review]
branch type hack

Moved the type issue to bug 360583

Comment 43

[Stuart Morgan]

(In reply to comment #41)
> Per IRC discussion, the project change should also move the keychain files to
> the formfill project directory, and change the references to point to the
> formfill directory on disk.

You can ignore this. I realized that the move should be a separate checkin with unchanged files to prevent all my changes from being impossible to see in the cvs history. I checked that in on trunk and branch.

Comment 44

[Stuart Morgan]

Attachment: sr=pink version

Once we have the project patch adding KeychainItem.* and Security.framework, we're in business on the trunk.

Comment 45

[Stuart Morgan]

Smokey discovered that the nspr thing is still in trunk too; my tree just wasn't cleaned up full from when I first wrote the keychain stuff. So this is fully blocked on bug 360583.

Comment 46

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

Attachment: Trunk project patch (untested)

Here's the trunk project patch, just so I won't be the holdup if the nspr thing gets movement next week.

It's untested in that my build with it was unsuccessful due to bug 360583 :/

Comment 47

[Stuart Morgan]

We now know how to work around this; compiling KeychainService.mm with -DNO_NSPR_10_SUPPORT.  Once Smokey has a chance to make a new project patch, I'll land this on the trunk.

Comment 48

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

Attachment: Trunk project patch v2

Trunk project patch with -DNO_NSPR_10_SUPPORT for KeychainService.mm

Let loose the chickens!

Comment 49

[Stuart Morgan]

Attachment: trunk project patch v3

I had to tweak this slightly; the flags are per-target (something that's not at all clear in Xcode 1.x, IIRC), so this wouldn't have worked for static builds.

Comment 50

[Stuart Morgan]

Landed on trunk. Thanks for all the help, Smokey!

Comment 51

[Stuart Morgan]

Once this has simmered on the trunk for a little while, we can land this on branch. I should point out that if you are on an intel-based machine, the passwords of any sites you visit with this change will no longer be visible to version of Camino without the change.

Comment 52

[Stuart Morgan]

Attachment: trunk project patch v4

This time without the typo :P

Comment 53

[Stuart Morgan]

I haven't heard any screaming, and there aren't really any subtleties here. Any objections to landing this on branch now?

Comment 54

[Stuart Morgan]

Landed on MOZILLA_1_8_BRANCH.

Comment 55

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

We might need to take this for 1.0.4 for bug 361463.

Comment 56

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

(In reply to comment #55)
> We might need to take this for 1.0.4 for bug 361463.

Stuart, can we even take this given that 1.0.x still has 10.2 support?

Comment 57

[Stuart Morgan]

Offhand, I don't think there's anything 10.3 until we get into constants we use for Safari compatibility, which is all in the other bug. Someone would need to test compiling against 10.2.8 to make sure though.

Comment 58

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

Attachment: 1.8.0 patch, phase 1 (move, c99)

Here's the first step in getting KS as a whole ready for a try-out on the 1.8.0 branch; this is 

1) the "cvs moves" of KeychainService.{m,h} from src/browser to src/formfill
2) the project change to reflect the move
3) the project change to move KeychainService.m into the Form Fill group
4) the project change to a default C dialect of c99

Note that, unfortunately, even these simple changes don't build:

/Users/smokey/Camino/dev/180branch/Camino10xBranch/camino/src/browser/BrowserWrapper.mm: In function `objc_object* -[BrowserWrapper initWithFrame:inWindow:](BrowserWrapper*, objc_selector*, _NSRect, NSWindow*)':
/Users/smokey/Camino/dev/180branch/Camino10xBranch/camino/src/browser/BrowserWrapper.mm:128: error: `KeychainService' undeclared (first use this function)
/Users/smokey/Camino/dev/180branch/Camino10xBranch/camino/src/browser/BrowserWrapper.mm:128: error: (Each undeclared identifier is reported only once for each function it appears in.)

On Ian's suggestion, I did a distclean + rebuild, but that doesn't fix it, either.

Comment 59

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

Comment on attachment 249481 [details] [diff] [review]
1.8.0 patch, phase 1 (move, c99)

This is actually fine if you cvs remove the files :P  I'd just zeroed their contents as a "dummy" step in this diff (for easier reversion to a clean tree), and even though the project file knows the right location of the files, the 0-byte ones in src/browser/ were being picked up instead of/before the ones in src/formfill :P

Comment 60

[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.

Comment 61

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

Clearing nomination; 1.8.0 fix will be in bug 379438.