Use SecKeyCreateSignature on macOS 10.12 and later.

Use SecKeyCreateSignature on macOS 10.12 and later.

macOS 10.12 breaks the CSSM code for smartcards. Switch to the new APIs.
In the process, refactor the Android unit tests to be common on all
platforms and unit test this stuff. SecKeychainCreate finally works, so
we can unit test the entire path from certificate to key and compare
signatures against OpenSSL.

That second part probably bears repeating. Eight years into the project's
lifetime, we FINALLY have unit tests for this code!

BUG=666796, 673058
Committed: https://crrev.com/ebe0803de7a569dee0c7b0ab6b4685dc749b8f29
Cr-Commit-Position: refs/heads/master@{#438392}

[davidben, 2016-12-13 21:19:43]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 21:20:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-12-13 21:38:57]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2016-12-13 21:38:57]

I feel like I am morally obligated to send this CL to you since you thought the
test couldn't be done. :-)

[commit-bot: I haz the power, 2016-12-13 22:09:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 22:09:04]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-12-13 22:21:08]

Since most of these are nits or requests for further clarification, I'll go
ahead and LGTM

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
File net/ssl/ssl_platform_key_mac.cc (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.cc:127: return;                                    
                      \
Is it really fatal if these can't be looked up?

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.cc:153: SecKeyCreateSignatureFunc
SecKeyCreateSignature = NULL;
nullptr?

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.cc:307: kHashes + arraysize(kHashes));
pedantry nit: Can you not std::begin(kHashes), std::end(kHashes) ?

Alternatively, can you not
return std::vector<SSLPrivateKey::Hash>{
  SSLPrivateKey::Hash::SHA512, SSLPrivateKey::Hash::SHA384, ... };

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_mac.h
File net/ssl/ssl_platform_key_mac.h (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:19: // public key and returns an SSLPrivateKey
backed by the playform key. If
// Returns an SSLPrivateKey backed by the platform private key in |keychain|
// that corresponds to |certificate|'s public key

?

(It's pedantry, but it seems to be more direct/active)

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:20: // |keychain| is nullptr, the user's default
search list is used instead.
s/the user's/the process's/ ?

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:21: NET_EXPORT_PRIVATE
scoped_refptr<SSLPrivateKey>
Any documentation about threading?

e.g.

"Note: This function may block on external resources, and should generally only
be called on worker threads."

(Or something to the effect)

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:22:
FetchClientCertPrivateKeyFromKeychain(X509Certificate* certificate,
nit: const?

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
File net/ssl/ssl_platform_key_mac_unittest.cc (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac_unittest.cc:75:
SecKeychainCreate(keychain_path.value().c_str(), 0, "", FALSE,
From an Obj-C standpoint, does this need to be treated as a .mm file for the use
of Boolean? If bool <-> Boolean is toll-free, why not use false (bool) rather
than FALSE (#defined) ?

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac_unittest.cc:108: bssl::UniquePtr<uint8_t>
free_encoded(encoded);
nit: scoped_encoded ? The naming threw me off a little

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
File net/ssl/ssl_private_key_test_util.cc (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
net/ssl/ssl_private_key_test_util.cc:199: for (auto hash : hashes) {
const auto&

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
File net/ssl/ssl_private_key_test_util.h (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
net/ssl/ssl_private_key_test_util.h:14: const char*
SSLPrivateKeyTypeToString(SSLPrivateKey::Type type);
const char* const ?

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
net/ssl/ssl_private_key_test_util.h:17: // properties and advertised signature
algorithms. The caller is responsible for
From reading only this header, I'm not sure what "on all properties" means?

[davidben, 2016-12-14 00:59:35]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[davidben, 2016-12-14 00:59:43]

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
File net/ssl/ssl_platform_key_mac.cc (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.cc:127: return;                                    
                      \
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> Is it really fatal if these can't be looked up?

If it happens, we got the version check wrong. I think that's fine for
NOTREACHED() that is checked at runtime.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.cc:153: SecKeyCreateSignatureFunc
SecKeyCreateSignature = NULL;
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> nullptr?

Done.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.cc:307: kHashes + arraysize(kHashes));
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> pedantry nit: Can you not std::begin(kHashes), std::end(kHashes) ?
> 
> Alternatively, can you not
> return std::vector<SSLPrivateKey::Hash>{
>   SSLPrivateKey::Hash::SHA512, SSLPrivateKey::Hash::SHA384, ... };

Done. (This was copy-pasted from code that predated us being able to use that.
Fixed in both.)

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_mac.h
File net/ssl/ssl_platform_key_mac.h (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:19: // public key and returns an SSLPrivateKey
backed by the playform key. If
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> // Returns an SSLPrivateKey backed by the platform private key in |keychain|
> // that corresponds to |certificate|'s public key
> 
> ?
> 
> (It's pedantry, but it seems to be more direct/active)

Done. Also updated the ssl_platform_key.h copy.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:20: // |keychain| is nullptr, the user's default
search list is used instead.
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> s/the user's/the process's/ ?

Done.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:21: NET_EXPORT_PRIVATE
scoped_refptr<SSLPrivateKey>
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> Any documentation about threading?
> 
> e.g.
> 
> "Note: This function may block on external resources, and should generally
only
> be called on worker threads."
> 
> (Or something to the effect)

Unfortunately, this is currently called on the IO thread. :-/

To fix that, we need to do the thing where ClientCertStore vends out
"identities" which are certs + a handle which can be cashed in for one of these,
at which point I expect this will no longer be exposed and it'd be a funny
ClientCertStoreMac constructor or something.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac.h:22:
FetchClientCertPrivateKeyFromKeychain(X509Certificate* certificate,
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> nit: const? 

Done.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
File net/ssl/ssl_platform_key_mac_unittest.cc (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac_unittest.cc:75:
SecKeychainCreate(keychain_path.value().c_str(), 0, "", FALSE,
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> From an Obj-C standpoint, does this need to be treated as a .mm file for the
use
> of Boolean? If bool <-> Boolean is toll-free, why not use false (bool) rather
> than FALSE (#defined) ?

FALSE is #defined in CoreFoundation/CFBase.h, which is a C header, not Obj-C.
false works fine too, but I feel like using the #defines rather than false is
marginally better? Boolean is typedef'd to unsigned char (also in CFBase.h)
which behaves slightly differently from bool. (Doesn't implicitly turn values
outside {0, 1} to 1. Not that that's relevant here.)

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_platform_key_ma...
net/ssl/ssl_platform_key_mac_unittest.cc:108: bssl::UniquePtr<uint8_t>
free_encoded(encoded);
On 2016/12/13 22:21:07, Ryan Sleevi wrote:
> nit: scoped_encoded ? The naming threw me off a little

Done.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
File net/ssl/ssl_private_key_test_util.cc (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
net/ssl/ssl_private_key_test_util.cc:199: for (auto hash : hashes) {
On 2016/12/13 22:21:08, Ryan Sleevi wrote:
> const auto&

It's an enum. :-P Switched to a concrete type per
https://chromium-cpp.appspot.com/.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
File net/ssl/ssl_private_key_test_util.h (right):

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
net/ssl/ssl_private_key_test_util.h:14: const char*
SSLPrivateKeyTypeToString(SSLPrivateKey::Type type);
On 2016/12/13 22:21:08, Ryan Sleevi wrote:
> const char* const ?

Returning const out of a function doesn't actually do anything useful. (In fact,
I believe I've seen MSVC explode if you do it.) The first const says something
about what the pointer is pointing to. The second says you can't modify the
value of the pointer... but the pointer value is an rvalue anyway, so that
doesn't really mean anything.

https://codereview.chromium.org/2566273008/diff/1/net/ssl/ssl_private_key_tes...
net/ssl/ssl_private_key_test_util.h:17: // properties and advertised signature
algorithms. The caller is responsible for
On 2016/12/13 22:21:08, Ryan Sleevi wrote:
> From reading only this header, I'm not sure what "on all properties" means?

Rephrased to be more explicit.

[commit-bot: I haz the power, 2016-12-14 01:00:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-12-14 01:52:05]

The CQ bit was unchecked by davidben@chromium.org

[davidben, 2016-12-14 01:52:09]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-12-14 01:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2566273008/#ps20001
(title: "rsleevi comments")

[commit-bot: I haz the power, 2016-12-14 01:52:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 02:22:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481680329233400,
"parent_rev": "43369a946eed32bf5694c4554a2ea1ce524ec23a", "commit_rev":
"4290ea2b7d1b58c8da8b09880dbe166d078c3c63"}

[commit-bot: I haz the power, 2016-12-14 02:23:24]

Description was changed from

==========
Use SecKeyCreateSignature on macOS 10.12 and later.

macOS 10.12 breaks the CSSM code for smartcards. Switch to the new APIs.
In the process, refactor the Android unit tests to be common on all
platforms and unit test this stuff. SecKeychainCreate finally works, so
we can unit test the entire path from certificate to key and compare
signatures against OpenSSL.

That second part probably bears repeating. Eight years into the project's
lifetime, we FINALLY have unit tests for this code!

BUG=666796,673058
==========

to

==========
Use SecKeyCreateSignature on macOS 10.12 and later.

macOS 10.12 breaks the CSSM code for smartcards. Switch to the new APIs.
In the process, refactor the Android unit tests to be common on all
platforms and unit test this stuff. SecKeychainCreate finally works, so
we can unit test the entire path from certificate to key and compare
signatures against OpenSSL.

That second part probably bears repeating. Eight years into the project's
lifetime, we FINALLY have unit tests for this code!

BUG=666796,673058

Review-Url: https://codereview.chromium.org/2566273008
==========

[commit-bot: I haz the power, 2016-12-14 02:23:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-14 02:27:24]

Description was changed from

==========
Use SecKeyCreateSignature on macOS 10.12 and later.

macOS 10.12 breaks the CSSM code for smartcards. Switch to the new APIs.
In the process, refactor the Android unit tests to be common on all
platforms and unit test this stuff. SecKeychainCreate finally works, so
we can unit test the entire path from certificate to key and compare
signatures against OpenSSL.

That second part probably bears repeating. Eight years into the project's
lifetime, we FINALLY have unit tests for this code!

BUG=666796,673058

Review-Url: https://codereview.chromium.org/2566273008
==========

to

==========
Use SecKeyCreateSignature on macOS 10.12 and later.

macOS 10.12 breaks the CSSM code for smartcards. Switch to the new APIs.
In the process, refactor the Android unit tests to be common on all
platforms and unit test this stuff. SecKeychainCreate finally works, so
we can unit test the entire path from certificate to key and compare
signatures against OpenSSL.

That second part probably bears repeating. Eight years into the project's
lifetime, we FINALLY have unit tests for this code!

BUG=666796,673058

Committed: https://crrev.com/ebe0803de7a569dee0c7b0ab6b4685dc749b8f29
Cr-Commit-Position: refs/heads/master@{#438392}
==========

[commit-bot: I haz the power, 2016-12-14 02:27:24]

Patchset 2 (id:??) landed as
https://crrev.com/ebe0803de7a569dee0c7b0ab6b4685dc749b8f29
Cr-Commit-Position: refs/heads/master@{#438392}

[Alexander Yashkin, 2016-12-14 12:29:11]

On 2016/12/14 at 02:27:24, commit-bot wrote:
> Patchset 2 (id:??) landed as
https://crrev.com/ebe0803de7a569dee0c7b0ab6b4685dc749b8f29
> Cr-Commit-Position: refs/heads/master@{#438392}

Unfortunately this change breaks Chromium compilation on macos 10.11.6 (building
at ab9d0bcb41c77781cf482f4937686cc604b41112).


../../net/ssl/ssl_platform_key_mac.cc:148:51: error: 'SecKeyAlgorithm' is
partial: introduced in macOS 10.12 [-Werror,-Wunguarded-availability]
                                                  SecKeyAlgorithm algorithm,
                                                  ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Security.framework/Headers/SecKey.h:992:21:
note: 'SecKeyAlgorithm' has been explicitly marked partial here
typedef CFStringRef SecKeyAlgorithm CF_STRING_ENUM
                    ^
../../net/ssl/ssl_platform_key_mac.cc:148:51: note: explicitly redeclare
'SecKeyAlgorithm' to silence this warning
                                                  SecKeyAlgorithm algorithm,

[davidben, 2016-12-14 12:45:19]

On 2016/12/14 12:29:11, Alexander Yashkin wrote:
> On 2016/12/14 at 02:27:24, commit-bot wrote:
> > Patchset 2 (id:??) landed as
> https://crrev.com/ebe0803de7a569dee0c7b0ab6b4685dc749b8f29
> > Cr-Commit-Position: refs/heads/master@{#438392}
> 
> Unfortunately this change breaks Chromium compilation on macos 10.11.6
(building
> at ab9d0bcb41c77781cf482f4937686cc604b41112).
> 
> 
> ../../net/ssl/ssl_platform_key_mac.cc:148:51: error: 'SecKeyAlgorithm' is
> partial: introduced in macOS 10.12 [-Werror,-Wunguarded-availability]
>                                                   SecKeyAlgorithm algorithm,
>                                                   ^
>
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Security.framework/Headers/SecKey.h:992:21:
> note: 'SecKeyAlgorithm' has been explicitly marked partial here
> typedef CFStringRef SecKeyAlgorithm CF_STRING_ENUM
>                     ^
> ../../net/ssl/ssl_platform_key_mac.cc:148:51: note: explicitly redeclare
> 'SecKeyAlgorithm' to silence this warning
>                                                   SecKeyAlgorithm algorithm,

It looks like you're building against the 10.12 SDK, but we're not doing that
yet. Are you not building using the usual Chromium setup?

Still that does suggest something other than that #ifdef is needed to manage
this. I'll go poke at that.

[davidben, 2016-12-14 13:06:31]

On 2016/12/14 12:45:19, davidben wrote:
> On 2016/12/14 12:29:11, Alexander Yashkin wrote:
> > On 2016/12/14 at 02:27:24, commit-bot wrote:
> > > Patchset 2 (id:??) landed as
> > https://crrev.com/ebe0803de7a569dee0c7b0ab6b4685dc749b8f29
> > > Cr-Commit-Position: refs/heads/master@{#438392}
> > 
> > Unfortunately this change breaks Chromium compilation on macos 10.11.6
> (building
> > at ab9d0bcb41c77781cf482f4937686cc604b41112).
> > 
> > 
> > ../../net/ssl/ssl_platform_key_mac.cc:148:51: error: 'SecKeyAlgorithm' is
> > partial: introduced in macOS 10.12 [-Werror,-Wunguarded-availability]
> >                                                   SecKeyAlgorithm algorithm,
> >                                                   ^
> >
>
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Security.framework/Headers/SecKey.h:992:21:
> > note: 'SecKeyAlgorithm' has been explicitly marked partial here
> > typedef CFStringRef SecKeyAlgorithm CF_STRING_ENUM
> >                     ^
> > ../../net/ssl/ssl_platform_key_mac.cc:148:51: note: explicitly redeclare
> > 'SecKeyAlgorithm' to silence this warning
> >                                                   SecKeyAlgorithm algorithm,
> 
> It looks like you're building against the 10.12 SDK, but we're not doing that
> yet. Are you not building using the usual Chromium setup?
> 
> Still that does suggest something other than that #ifdef is needed to manage
> this. I'll go poke at that.

This resolve things? https://codereview.chromium.org/2572113002/

[Alexander Yashkin, 2016-12-16 05:46:12]

On 2016/12/14 at 13:06:31, davidben wrote:
> On 2016/12/14 12:45:19, davidben wrote:
> > On 2016/12/14 12:29:11, Alexander Yashkin wrote:
> > > On 2016/12/14 at 02:27:24, commit-bot wrote:
> > > > Patchset 2 (id:??) landed as
> > > https://crrev.com/ebe0803de7a569dee0c7b0ab6b4685dc749b8f29
> > > > Cr-Commit-Position: refs/heads/master@{#438392}
> > > 
> > > Unfortunately this change breaks Chromium compilation on macos 10.11.6
> > (building
> > > at ab9d0bcb41c77781cf482f4937686cc604b41112).
> > > 
> > > 
> > > ../../net/ssl/ssl_platform_key_mac.cc:148:51: error: 'SecKeyAlgorithm' is
> > > partial: introduced in macOS 10.12 [-Werror,-Wunguarded-availability]
> > >                                                   SecKeyAlgorithm
algorithm,
> > >                                                   ^
> > >
> >
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Security.framework/Headers/SecKey.h:992:21:
> > > note: 'SecKeyAlgorithm' has been explicitly marked partial here
> > > typedef CFStringRef SecKeyAlgorithm CF_STRING_ENUM
> > >                     ^
> > > ../../net/ssl/ssl_platform_key_mac.cc:148:51: note: explicitly redeclare
> > > 'SecKeyAlgorithm' to silence this warning
> > >                                                   SecKeyAlgorithm
algorithm,
> > 
> > It looks like you're building against the 10.12 SDK, but we're not doing
that
> > yet. Are you not building using the usual Chromium setup?
> > 
> > Still that does suggest something other than that #ifdef is needed to manage
> > this. I'll go poke at that.
> 
> This resolve things? https://codereview.chromium.org/2572113002/

Yes, thanks, it helps.