Implement Keychain reauthorization

Implement Keychain reauthorization.

This implements chrome::browser::mac::KeychainReauthorize, which will rewrite
all Keychain items accessible to Chrome having an old requirement string
showing up in any ACL, transitioning them to the new requirement string, which
is now used when signing the application. Rewriting is handled by deleting the
old Keychain item and storing a new one in its place.

The transition code is not yet live, but the requirement string for signed
applications is.

BUG=108238
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137235

[Mark Mentovai, 2012-05-08 15:29:44]

This code isn’t live yet, but it’s the meat of the part that will be live.

[Nico, 2012-05-08 18:46:25]

This looks all fine as far as I can tell. (Disclaimer: I haven't used the
keychain apis before.)

http://codereview.chromium.org/10344009/diff/8002/chrome/browser/mac/security...
File chrome/browser/mac/security_wrappers.cc (right):

http://codereview.chromium.org/10344009/diff/8002/chrome/browser/mac/security...
chrome/browser/mac/security_wrappers.cc:65: : item_(item),
nit: item_(CFRetain(item)) might be a bit more readable (also elsewhere)

http://codereview.chromium.org/10344009/diff/8002/chrome/browser/mac/security...
chrome/browser/mac/security_wrappers.cc:354: item_id =
attributes_and_data.item_class();
I thought this looked suspicious, but apple's keychain_utilities.c does it too.

[Mark Mentovai, 2012-05-08 20:34:49]

I’ll add Stuart as someone who has Keychain experience. If you’re comfortable
LGTMing, I’ll still address any of Stuart’s comments post-commit.

http://codereview.chromium.org/10344009/diff/8002/chrome/browser/mac/security...
File chrome/browser/mac/security_wrappers.cc (right):

http://codereview.chromium.org/10344009/diff/8002/chrome/browser/mac/security...
chrome/browser/mac/security_wrappers.cc:65: : item_(item),
Nico wrote:
> nit: item_(CFRetain(item)) might be a bit more readable (also elsewhere)

CFRetain bypasses the type system and thus requires casts, which are probably a
net readability minus.

/chrome/trunk/src/chrome/browser/mac/security_wrappers.cc:65:7: error: no
matching constructor for initialization of
'base::mac::ScopedCFTypeRef<SecKeychainItemRef>'
    : item_(CFRetain(item)),
      ^     ~~~~~~~~~~~~~~
../base/mac/scoped_cftyperef.h:32:12: note: candidate constructor not viable:
cannot convert argument of incomplete type 'CFTypeRef' (aka 'const void *') to
'OpaqueSecKeychainItemRef *'
  explicit ScopedCFTypeRef(CFT object = NULL)
           ^
../base/mac/scoped_cftyperef.h:81:28: note: candidate constructor not viable:
cannot convert argument of incomplete type 'CFTypeRef' (aka 'const void *') to
'const base::mac::ScopedCFTypeRef<OpaqueSecKeychainItemRef *>'
  DISALLOW_COPY_AND_ASSIGN(ScopedCFTypeRef);
                           ^
../base/basictypes.h:85:3: note: expanded from macro 'DISALLOW_COPY_AND_ASSIGN'
  TypeName(const TypeName&);               \
  ^

http://codereview.chromium.org/10344009/diff/8002/chrome/browser/mac/security...
chrome/browser/mac/security_wrappers.cc:354: item_id =
attributes_and_data.item_class();
Nico wrote:
> I thought this looked suspicious, but apple's keychain_utilities.c does it
too.

You’re right, it is suspicious, and it deserves a comment.

[stuartmorgan, 2012-05-10 06:50:54]

LGTM; all nits

That said, I found this *incredibly* hard to read, mostly because it's one huge
function containing all the layers of looping, control flow, etc. as well as
every fiddly detail of the keychain APIs. Anything that could be done to add a
bit of layering so that control flow and fiddly details aren't all one big ball
of wax would be very helpful. I've suggested a couple of possibilities that may
work, but almost anything that's separable in that function would be a win.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
File chrome/browser/mac/keychain_reauthorize.cc (right):

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:64: // below was restored afterwards
as an interim fix to the Keychain
"the one below" is ambiguous, especially since it could easily be the same thing
as "this certificate" given the length of the comment.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:65: // authorization problem.
Worth a bug ref? I'm guessing there are about three of us who know what this is
referring to.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:134: }
Chromium style discourages single-line braces; it would be nice if all this code
followed that since it's not in a file that already has the other style.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:147: for (CFIndex acl_index = 0;
acl_index < acl_count; ++acl_index) {
The layers of if statements in a double for-loop is pretty opaque. I think this
block would benefit a lot from an introductory comment explaining roughly what
it's doing.

In fact, could you reasonably extract most or all of this loop into a well-named
helper function that returns a bool for having modified or something similar? If
it's feasible (it's hard to see how many arguments such a function would need),
that could help readability of the function quite a bit.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:192:
this_application.reset(CrSTrustedApplicationCreateFromPath(NULL));
Is it really expensive enough to do this (and unlikely enough to happen at some
point) that it's worth doing lazily instead of just up-front? It's a small
thing, but it's extra complexity in an already-complex function.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:195: if (!added_this_application) {
I find
  if (!foo)
  else
to generally be less readable than the reverse.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:242: // a new attribute list devoid
of zero-length attributes.
Could this whole step be a helper function?

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
File chrome/browser/mac/keychain_reauthorize.h (right):

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.h:13: void KeychainReauthorize();
I can haz method level comment?

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
File chrome/browser/mac/security_wrappers.cc (right):

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
chrome/browser/mac/security_wrappers.cc:45: OSSTATUS_LOG(ERROR, status) <<
"SecKeychainGetUserInteractionAllowed";
I thought having static strings on non-debug LOG statements was strongly
discouraged, since they don't provide any information that the line doesn't and
make the binary larger.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
chrome/browser/mac/security_wrappers.cc:67: CFRetain(item_);
Can you at least comment this pattern? I had to go back and re-read the header
to see that you weren't leaking everything, because nothing here suggests that
these are scoped objects.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
File chrome/browser/mac/security_wrappers.h (right):

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
chrome/browser/mac/security_wrappers.h:39: explicit
ScopedSecKeychainSetUserInteractionAllowed(Boolean allowed);
It's already C++ code; why not bool?

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
chrome/browser/mac/security_wrappers.h:43: Boolean old_allowed_;
Same.

[Mark Mentovai, 2012-05-14 21:28:45]

I broke things up substantially here. I haven’t tested this yet but will do that
in parallel with you taking a second look. Thanks for the review.

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
File chrome/browser/mac/keychain_reauthorize.cc (right):

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/keychai...
chrome/browser/mac/keychain_reauthorize.cc:195: if (!added_this_application) {
stuartmorgan wrote:
> I find
>   if (!foo)
>   else
> to generally be less readable than the reverse.

In this case, !added_this_application always happens before
added_this_application, and I think it makes sense to structure it according to
“the thing that happens first come first in the file.”

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
File chrome/browser/mac/security_wrappers.h (right):

http://codereview.chromium.org/10344009/diff/17001/chrome/browser/mac/securit...
chrome/browser/mac/security_wrappers.h:39: explicit
ScopedSecKeychainSetUserInteractionAllowed(Boolean allowed);
stuartmorgan wrote:
> It's already C++ code; why not bool?

Because of pointers. This doesn’t work without ugly casts that aren’t even
strictly correct:

  bool old_allowed_;
  OSStatus status = SecKeychainGetUserInteractionAllowed(&old_allowed_);

/chrome/trunk/src/chrome/browser/mac/security_wrappers.cc:43:21: error: no
matching function for call to 'SecKeychainGetUserInteractionAllowed'
  OSStatus status = SecKeychainGetUserInteractionAllowed(&old_allowed_);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Xcode3.2/SDKs/MacOSX10.5.sdk/System/Library/Frameworks/Security.framework/Headers/SecKeychain.h:598:10:
note: candidate function not viable: no known conversion from 'bool *' to
'Boolean *' (aka 'unsigned char *') for 1st argument; 
OSStatus SecKeychainGetUserInteractionAllowed(Boolean *state);
         ^
1 error generated.

Once you have to have one Boolean for the field, it’s stupid to make the
constructor argument not match.

[stuartmorgan, 2012-05-15 12:03:30]

LGTM, thanks for the rework! Much easier to hold each part in my head as I was
reading; this is now probably about as readable as keychain code gets.

[commit-bot: I haz the power, 2012-05-15 18:36:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mark@chromium.org/10344009/31001

[Mark Mentovai, 2012-05-15 18:43:06]

I completed my testing with this and only needed to make a couple of errors
tolerable without logging anything. That difference is in patch set 5, and this
is now going in. The next change will be a small one to enable the transition
code.