Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...).
(Should have no effect, but may be safer / more future-proof.)

BUG=649953
Committed: https://crrev.com/9d33bc396faca1d6d017ed22894197e51c16d6e5
Cr-Commit-Position: refs/heads/master@{#421598}

[mattm, 2016-09-27 03:03:45]

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

[commit-bot: I haz the power, 2016-09-27 03:04:20]

Dry run: 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-09-27 03:32:25]

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

[commit-bot: I haz the power, 2016-09-27 03:32:26]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-09-27 21:48:22]

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

[mattm, 2016-09-27 21:48:23]

This seems to work, though I'm not sure what error codes to return if something
fails.

[Ryan Sleevi, 2016-09-27 22:04:42]

LGTM, all I have are nits, feel free to agree or disagree

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc
File net/cert/x509_util_mac.cc (right):

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:59: return noErr;
ERR_NOT_IMPLEMENTED / errSecNoPolicyModule

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:64: base::ScopedCFTypeRef<CFStringRef>
hostname_cfstring;
hostname_cfstring feels a little weird (naming), in that it uses hungarian type
notation.

scoped_hostname ? I suppose that feels as bad

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:68: // XXX better error code?
ERR_NOT_IMPLEMENTED / errSecNoPolicyModule

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:79: return CreatePolicy(&CSSMOID_APPLE_X509_BASIC,
NULL, 0, policy);
For safety, we can probably replace this with
https://developer.apple.com/reference/security/1397202-secpolicycreatebasicx5...

[mattm, 2016-09-27 23:35:25]

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

[commit-bot: I haz the power, 2016-09-27 23:35:45]

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

[mattm, 2016-09-27 23:38:52]

Description was changed from

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

BUG=649953
==========

to

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of
CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...). (Should have no effect, but may be
safer / more future-proof.)

BUG=649953
==========

[mattm, 2016-09-27 23:39:04]

Description was changed from

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of
CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...). (Should have no effect, but may be
safer / more future-proof.)

BUG=649953
==========

to

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of
CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...).
(Should have no effect, but may be safer / more future-proof.)

BUG=649953
==========

[mattm, 2016-09-27 23:39:23]

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc
File net/cert/x509_util_mac.cc (right):

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:59: return noErr;
On 2016/09/27 22:04:42, Ryan Sleevi (slow) wrote:
> ERR_NOT_IMPLEMENTED / errSecNoPolicyModule

Done.

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:64: base::ScopedCFTypeRef<CFStringRef>
hostname_cfstring;
On 2016/09/27 22:04:42, Ryan Sleevi (slow) wrote:
> hostname_cfstring feels a little weird (naming), in that it uses hungarian
type
> notation.
> 
> scoped_hostname ? I suppose that feels as bad

I suppose an argument could be made about variables whose only purpose is to
convert something to a different type, maybe including the type in the name is
relevant then?

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:68: // XXX better error code?
On 2016/09/27 22:04:42, Ryan Sleevi (slow) wrote:
> ERR_NOT_IMPLEMENTED / errSecNoPolicyModule

Done.

https://codereview.chromium.org/2373533003/diff/1/net/cert/x509_util_mac.cc#n...
net/cert/x509_util_mac.cc:79: return CreatePolicy(&CSSMOID_APPLE_X509_BASIC,
NULL, 0, policy);
On 2016/09/27 22:04:42, Ryan Sleevi (slow) wrote:
> For safety, we can probably replace this with
>
https://developer.apple.com/reference/security/1397202-secpolicycreatebasicx5...

Done.

[commit-bot: I haz the power, 2016-09-28 00:08:23]

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

[commit-bot: I haz the power, 2016-09-28 00:08:24]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-09-28 19:15:34]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-09-28 19:15:34]

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/2373533003/#ps20001
(title: "review changes")

[commit-bot: I haz the power, 2016-09-28 19:15:59]

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-09-28 19:21:47]

Description was changed from

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of
CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...).
(Should have no effect, but may be safer / more future-proof.)

BUG=649953
==========

to

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of
CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...).
(Should have no effect, but may be safer / more future-proof.)

BUG=649953
==========

[commit-bot: I haz the power, 2016-09-28 19:21:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-28 19:23:48]

Description was changed from

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of
CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...).
(Should have no effect, but may be safer / more future-proof.)

BUG=649953
==========

to

==========
Mac: Use SecPolicyCreateSSL instead of CreatePolicy(&CSSMOID_APPLE_TP_SSL, ...)

Fixes client cert selector not showing client certs that were not directly
issued by the specified root(s) on Sierra.

Also use SecPolicyCreateBasicX509 instead of
CreatePolicy(&CSSMOID_APPLE_X509_BASIC, ...).
(Should have no effect, but may be safer / more future-proof.)

BUG=649953

Committed: https://crrev.com/9d33bc396faca1d6d017ed22894197e51c16d6e5
Cr-Commit-Position: refs/heads/master@{#421598}
==========

[commit-bot: I haz the power, 2016-09-28 19:23:50]

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