Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2910893002:
Improved support for loading client certificates on smart cards on macOS
DescriptionImproved support for loading client certificates on smart cards on macOS
macOS 10.12 changed the existing APIs that Chromium uses to find client
certificates in the keychain in a way that causes certificates located
on smart cards to no longer be found. This change causes Chromium to
look for certificates using both the existing API, as well as a second
API which will find smart card based certificates.
BUG=
Patch Set 1 #
Total comments: 17
Patch Set 2 : Improved support for loading client certificates on smart cards on macOS #
Total comments: 14
Patch Set 3 : Improved support for loading client certificates on smart cards on macOS #
Total comments: 7
Patch Set 4 : Improved support for loading client certificates on smart cards on macOS #
Total comments: 2
Patch Set 5 : Improved support for loading client certificates on smart cards on macOS #Messages
Total messages: 23 (7 generated)
The CQ bit was checked by agaynor@mozilla.com
The CQ bit was unchecked by agaynor@mozilla.com
Description was changed from ========== Improved support for loading client certificates on smart cards on macOS macOS 10.12 changed the existing APIs that Chromium uses to find client certificates in the keychain in a way that causes certificates located on smart cards to no longer be found. This change causes Chromium to look for certificates using both the existing API, as well as a second API which will find smart card based certificates. BUG= ========== to ========== Improved support for loading client certificates on smart cards on macOS macOS 10.12 changed the existing APIs that Chromium uses to find client certificates in the keychain in a way that causes certificates located on smart cards to no longer be found. This change causes Chromium to look for certificates using both the existing API, as well as a second API which will find smart card based certificates. BUG= ==========
agaynor@mozilla.com changed reviewers: + ajwong@chromium.org, rsleevi@chromium.org
rsleevi@chromium.org changed reviewers: + mattm@chromium.org
Adding mattm@ as a reviewer, but there's a big question in all the style comments below that may be the root cause :) https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:298: // smart card based certificates, and this way misses some soft certificates. We try to avoid pronouns in comments (I/we) and try to explain/document the root cause (e.g. via discussion from opensource.apple.com - see line 251 for an example) More complete documentation may be: // macOS provides two ways to search for identities. // SecIdentitySearchCreate() is deprecated, as it relies on CSSM_KEYUSE_SIGN (part of the deprecated CDSM/CSSA implementation), but // is necessary to return some certificates that would otherwise not // be returned by SecItemCopyMatching(), which is the non-deprecated // way. // However, SecIdentitySearchCreate() will not return all items, particularly smart-card based identities, so it's necessary to // call both functions. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:299: const void *keys[] = { You can use "git cl format" to ensure this is properly formatted, but Chrome groups the * to the type rather than the identifier ("void* keys" vs "void *keys") https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expre... permits both, but https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md specifies for Chromium :) https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:299: const void *keys[] = { So, I _believe_ for correctness, that kSecAttrCanSign should be kCFBooleanTrue (if RSA) or kSecAttrCanDerive should be kCFBooleanTrue (if ECDSA/ECDH) Could you describe what type of key you have? I'd be willing to bet it's an ECDSA/ECDH key, which is why the CSSM_KEYUSE_SIGN isn't sufficient. I'd be willing to bet that if we also passed CSSM_KEYUSE_DERIVE, your cert would be returned. Context is that https://opensource.apple.com/source/Security/Security-57740.51.3/OSX/libsecur... shows that SecItemCopyMatching punts to SecItemCopyMatching_osx which then maps the kSecAttrCan* flags into CSSM keyusage fields, and then just calls SecIdentitySearchCreate(). The difference in your invocation here and the existing invocation is you're not supplying a kSecAttrCan* parameter, and so you're getting all keyUsages, by virtue of _CssmKeyUsageFromQuery https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:309: CFDictionaryRef query = CFDictionaryCreate( You can see we used the ScopedCFTypeRef C++ helpers here to ensure RAII https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:318: err = SecItemCopyMatching(query, (CFTypeRef *)&result); Note: Chromium explicitly uses C++ casts rather than C casts - see https://google.github.io/styleguide/cppguide.html#Casting If you're using ScopedCFTypeRef, you can use .InitializeInto() to return an acceptable pointer for this :) https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:319: if (!err) { In general, we try to handle the error case first, to avoid too much nesting. You can see this in line 273 or 284, for example. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:340: SecCertificateRef cert_handle; .InitializeInto :) https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... File net/ssl/client_cert_store_mac.h (right): https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.h:52: // |preferred_cert| if it it shte |preferred_identity. typo: if it is the |preferred_identity| https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.h:52: // |preferred_cert| if it it shte |preferred_identity. Could you expand on what you're trying to document here? What's the distinction between |preferred_identity| and |identity| here as the parameters? Some further documentation seems warranted here in the header, although complete documentation of the method in the header would be undesirable.
I believe I got all the issues you pointed out, thanks. As I indicated, even with the change you expected to "break" this (kSecAttrCanSign) this continues to work for me. Let me know if there's any additional debugging you'd like. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:298: // smart card based certificates, and this way misses some soft certificates. On 2017/05/30 18:00:01, Ryan Sleevi wrote: > We try to avoid pronouns in comments (I/we) and try to explain/document the root > cause (e.g. via discussion from http://opensource.apple.com - see line 251 for an > example) > > More complete documentation may be: > > // macOS provides two ways to search for identities. > // SecIdentitySearchCreate() is deprecated, as it relies on CSSM_KEYUSE_SIGN > (part of the deprecated CDSM/CSSA implementation), but > // is necessary to return some certificates that would otherwise not > // be returned by SecItemCopyMatching(), which is the non-deprecated > // way. > // However, SecIdentitySearchCreate() will not return all items, particularly > smart-card based identities, so it's necessary to > // call both functions. Done. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:299: const void *keys[] = { On 2017/05/30 18:00:01, Ryan Sleevi wrote: > You can use "git cl format" to ensure this is properly formatted, but Chrome > groups the * to the type rather than the identifier ("void* keys" vs "void > *keys") > > https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expre... > permits both, but > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md > specifies for Chromium :) Done. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:299: const void *keys[] = { It's a boring old RSA key. I've added |kSecAttrCanSign=kCFBooleanTrue| and this patch continues to fix this issue for me. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:309: CFDictionaryRef query = CFDictionaryCreate( On 2017/05/30 18:00:01, Ryan Sleevi wrote: > You can see we used the ScopedCFTypeRef C++ helpers here to ensure RAII Done. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:318: err = SecItemCopyMatching(query, (CFTypeRef *)&result); On 2017/05/30 18:00:00, Ryan Sleevi wrote: > Note: Chromium explicitly uses C++ casts rather than C casts - see > https://google.github.io/styleguide/cppguide.html#Casting > > If you're using ScopedCFTypeRef, you can use .InitializeInto() to return an > acceptable pointer for this :) Done. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:340: SecCertificateRef cert_handle; On 2017/05/30 18:00:01, Ryan Sleevi wrote: > .InitializeInto :) Done. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... File net/ssl/client_cert_store_mac.h (right): https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.h:52: // |preferred_cert| if it it shte |preferred_identity. On 2017/05/30 18:00:01, Ryan Sleevi wrote: > typo: if it is the |preferred_identity| Done.
Sorry about the delay. https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/1/net/ssl/client_cert_store_m... net/ssl/client_cert_store_mac.cc:299: const void *keys[] = { On 2017/05/30 18:00:01, Ryan Sleevi wrote: > So, I _believe_ for correctness, that kSecAttrCanSign should be kCFBooleanTrue > (if RSA) or kSecAttrCanDerive should be kCFBooleanTrue (if ECDSA/ECDH) > > Could you describe what type of key you have? I'd be willing to bet it's an > ECDSA/ECDH key, which is why the CSSM_KEYUSE_SIGN isn't sufficient. I'd be > willing to bet that if we also passed CSSM_KEYUSE_DERIVE, your cert would be > returned. > > Context is that > https://opensource.apple.com/source/Security/Security-57740.51.3/OSX/libsecur... > shows that SecItemCopyMatching punts to SecItemCopyMatching_osx which then maps > the kSecAttrCan* flags into CSSM keyusage fields, and then just calls > SecIdentitySearchCreate(). The difference in your invocation here and the > existing invocation is you're not supplying a kSecAttrCan* parameter, and so > you're getting all keyUsages, by virtue of _CssmKeyUsageFromQuery Looking at that code a bit, it seems that SecItemCategorizeQuery is used to decide whether to do the SecItemCopyMatching_ios and/or SecItemCopyMatching_osx, and the default is to do both. It looks like this query doesn't have any of the params that would exclude either type, so that can explain how this produces different results. (I guess that also means that if we were confident about which versions of OSX that translation code is fully implemented on, we could get rid of the SecIdentitySearch... code from here entirely. I looked at the 10.9 version, it appears to have similar stuff, though with some differences. (https://opensource.apple.com/source/Security/Security-55471/libsecurity_keych...) I didn't dig in enough to really be confident about it though. Just to be safe keeping both seems fine.) https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:311: kCFAllocatorDefault, keys, values, sizeof(values) / sizeof(values[0]), arraysize(values) https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:314: err = SecItemCopyMatching( put this in a base::AutoLock lock(crypto::GetMacSecurityServicesLock()); block (eg, like line 281) https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:320: (SecIdentityRef)item); C style casts aren't allowed, should use reinterpret_cast https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:330: void ClientCertStoreMac::AddIdentity( It appears this could be an anonymous function instead of a private method. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:332: scoped_refptr<X509Certificate>& preferred_cert, non-const reference parameters aren't allowed, these should be passed as pointers instead. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:334: SecIdentityRef identity) { Input parameters should come before output params.
I think I got all the points. I'd love to delete the first code path, but I don't know how to be 100% confident that the new one covers all the same cases. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:311: kCFAllocatorDefault, keys, values, sizeof(values) / sizeof(values[0]), On 2017/06/05 23:34:28, mattm wrote: > arraysize(values) Done. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:314: err = SecItemCopyMatching( On 2017/06/05 23:34:27, mattm wrote: > put this in a base::AutoLock lock(crypto::GetMacSecurityServicesLock()); block > (eg, like line 281) Done. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:320: (SecIdentityRef)item); On 2017/06/05 23:34:27, mattm wrote: > C style casts aren't allowed, should use reinterpret_cast Done. Although I had to also use a const_cast, which doesn't seem like it should be necessary. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:330: void ClientCertStoreMac::AddIdentity( On 2017/06/05 23:34:28, mattm wrote: > It appears this could be an anonymous function instead of a private method. Done. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:332: scoped_refptr<X509Certificate>& preferred_cert, On 2017/06/05 23:34:27, mattm wrote: > non-const reference parameters aren't allowed, these should be passed as > pointers instead. Done. https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:334: SecIdentityRef identity) { On 2017/06/05 23:34:27, mattm wrote: > Input parameters should come before output params. Done.
https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:330: void ClientCertStoreMac::AddIdentity( On 2017/06/06 22:38:49, agaynor wrote: > On 2017/06/05 23:34:28, mattm wrote: > > It appears this could be an anonymous function instead of a private method. > > Done. Also move it into the "namespace {}" block above.
couple of drive-by nits... sleeiv and mattm have primary review. https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:306: // macOS provides two ways to search for identities. SecIdentitySearchCreate() macOS -> MacOS codesearch shows it's about 8x more prevalent to capitalize the first letter there. https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:313: const void* keys[] = { Should these really be stack allocated? Make them static? Otherwise, you're gonna incur a O(n) copy per call. https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:363: DCHECK(!preferred_cert->get()); If 2 match in production, and this just uses the last one, what happens?
https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:306: // macOS provides two ways to search for identities. SecIdentitySearchCreate() On 2017/06/07 00:35:24, awong wrote: > macOS -> MacOS > > codesearch shows it's about 8x more prevalent to capitalize the first letter > there. Well, Apple did change the official name to "macOS" some time ago. If you limit it to comments only, macOS seems to be more common than MacOS. (I'd agree with capitalizing the M when it's used in method names.) https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:363: DCHECK(!preferred_cert->get()); On 2017/06/07 00:35:24, awong wrote: > If 2 match in production, and this just uses the last one, what happens? It's which one will be listed first in the client cert selector dialog (assuming it passes the filter), so if 2 matched, the 2nd one would win and get listed first, and the 1st would be forgotten. However, if they are both equal to preferred_identity, that means they would be duplicates anyway, so ignoring one would be fine.
https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:306: // macOS provides two ways to search for identities. SecIdentitySearchCreate() On 2017/06/07 01:07:56, mattm wrote: > On 2017/06/07 00:35:24, awong wrote: > > macOS -> MacOS > > > > codesearch shows it's about 8x more prevalent to capitalize the first letter > > there. > > Well, Apple did change the official name to "macOS" some time ago. > > If you limit it to comments only, macOS seems to be more common than MacOS. (I'd > agree with capitalizing the M when it's used in method names.) garagh. okay, ignore my comment then.
https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/20001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:330: void ClientCertStoreMac::AddIdentity( On 2017/06/07 00:24:07, mattm wrote: > On 2017/06/06 22:38:49, agaynor wrote: > > On 2017/06/05 23:34:28, mattm wrote: > > > It appears this could be an anonymous function instead of a private method. > > > > Done. > > Also move it into the "namespace {}" block above. Done. https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:313: const void* keys[] = { On 2017/06/07 00:35:24, awong wrote: > Should these really be stack allocated? Make them static? > > Otherwise, you're gonna incur a O(n) copy per call. Done.
https://codereview.chromium.org/2910893002/diff/60001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/60001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:336: static const void* keys[] = { Oh yeah...and if it's a constant, call it kKeys and kValues
https://codereview.chromium.org/2910893002/diff/60001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_mac.cc (right): https://codereview.chromium.org/2910893002/diff/60001/net/ssl/client_cert_sto... net/ssl/client_cert_store_mac.cc:336: static const void* keys[] = { On 2017/06/07 21:21:08, awong wrote: > Oh yeah...and if it's a constant, call it kKeys and kValues Done.
lgtm, thanks!
Sorry, I forgot about CLA stuff. It looks like Mozilla has a corporate CLA with Google, but your email address isn't showing up as part of it. You'll need to get added to that group, then add an AUTHORS entry in this CL too.
Apologies, had unsaved drafts but Matt already got them. LGTM and throwing to CQ.
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
The author agaynor@mozilla.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA. |
