Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585963003:
PKI library Mac trust store integration (Closed)
DescriptionPKI library Mac trust store integration
BUG=690704
Review-Url: https://codereview.chromium.org/2585963003
Cr-Commit-Position: refs/heads/master@{#453731}
Committed: https://chromium.googlesource.com/chromium/src/+/ea4ed823078f8496ce4ad18975658d87137da3eb
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : test fixes #
Total comments: 23
Patch Set 4 : rebase #Patch Set 5 : review changes & cleanups #
Total comments: 76
Patch Set 6 : review changes for comment #19 #
Total comments: 24
Patch Set 7 : review changes for comment #21 (partial) #Patch Set 8 : review changes and compile fix #Patch Set 9 : moarlocks #Patch Set 10 : rebase #Patch Set 11 : compile fix? #Patch Set 12 : compile fix?? #Patch Set 13 : compile fix??? #Patch Set 14 : rebase #Patch Set 15 : updates for rebase & cl format #
Messages
Total messages: 72 (52 generated)
Patchset #2 (id:20001) has been deleted
mattm@chromium.org changed reviewers: + rsleevi@chromium.org
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The overall approach looks really nice; most of my comments were either minor design Qs or trying to answer some of the TODOs (assuming they were open questions) I wasn't clear if you were wanting to fill out more, or if you'd like a more detailed nit-picky nuanced review :) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:30: #pragma clang diagnostic ignored "-Wdeprecated-declarations" I'm happy to just jump to SecCertificateCopyData to keep this clean and blessed. Are there other deprecated functions beyond that? If you did want to use GetData for the optimization path, we should probably keep its usage localized to a helper function here (e.g. that returned a StringPiece) so that we don't accidentally introduce additional usages elsewhere in the file (keeping the ignoring scoping hyper-local via a diagnostic pop) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:68: // kSecTrustSettingsResultTrustRoot is not valid for a non-root certificate. A++ documentation - would read again. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:80: // specific to chrome?) I'm OK with saying no to this, because then it gets into the broader question of code-signing identity. As it stands, there's no explicit way to create trust settings for Chrome, because we don't expose the trust panel from the SecTrustRef (which is what allows the per-app trust settings to be created) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:95: // kSecTrustSettingsKeyUsage); I haven't actually seen where Apple surfaces this information as far as the UI goes. That is, my understanding is that this is (mostly) redundant with the policy flags. I haven't the slightest where they might use this, other than perhaps iPSec (I presume client certs use the policy string, but perhaps thats a bad assumption) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:110: // a trust dict with kSecPolicyAppleX509Basic apply to SSL verification?) AIUI, no; each policy is treated as distinct (you can see the trust panel settings in Safari when there is a certificate error, and how they populate to the database) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:295: SecItemCopyMatching(query, (CFTypeRef*)matching_items.InitializeInto()); reinterpret_cast ? Although I think it should toll-free cast, right? https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:342: // through the SecCertificate APIs. I would say if parsing fails, we're good to reject it for now, and Apple's bananas for being that lax :) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:361: // of a Name without creating a SecCertificate. Unfortunately, yeah, that's the case. I'd be hesitant to reimplement Apple's normalization routines since it's not part of the stable API (I mean, the APIs to get a normalized name are, but not how it's computed, even if it's effectively static) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... File net/cert/internal/trust_store_mac.h (right): https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:8: #include <CoreFoundation/CFString.h> AIUI, the 'Apple supported way' is CoreFoundation/CoreFoundation.h (that is, I don't believe we're supposed to go straight to CFString.h, but I could be wrong) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:17: // TrustStoreMac is an implementation of TrustStore which uses Mac OS keychain s/Mac OS/macOS/ to match Apple's naming? https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:36: TrustAnchors* out_anchors); On first read, I was surprised to see public class statics. Can/Should these be private implementation details? https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:49: TrustAnchors* matches) const override; Suggestion: Order by "interface implementation first", then internal implementation.
Description was changed from ========== WIP: PKI library Mac trust store integration BUG=TODO ========== to ========== PKI library Mac trust store integration BUG=690704 ==========
https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:30: #pragma clang diagnostic ignored "-Wdeprecated-declarations" On 2017/02/06 23:59:05, Ryan Sleevi wrote: > I'm happy to just jump to SecCertificateCopyData to keep this clean and blessed. > Are there other deprecated functions beyond that? Just that one. I've changed to SecCertificateCopyData. > If you did want to use GetData for the optimization path, we should probably > keep its usage localized to a helper function here (e.g. that returned a > StringPiece) so that we don't accidentally introduce additional usages elsewhere > in the file (keeping the ignoring scoping hyper-local via a diagnostic pop) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:80: // specific to chrome?) On 2017/02/06 23:59:04, Ryan Sleevi wrote: > I'm OK with saying no to this, because then it gets into the broader question of > code-signing identity. As it stands, there's no explicit way to create trust > settings for Chrome, because we don't expose the trust panel from the > SecTrustRef (which is what allows the per-app trust settings to be created) Acknowledged. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:95: // kSecTrustSettingsKeyUsage); On 2017/02/06 23:59:05, Ryan Sleevi wrote: > I haven't actually seen where Apple surfaces this information as far as the UI > goes. That is, my understanding is that this is (mostly) redundant with the > policy flags. I haven't the slightest where they might use this, other than > perhaps iPSec (I presume client certs use the policy string, but perhaps thats a > bad assumption) Acknowledged. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:110: // a trust dict with kSecPolicyAppleX509Basic apply to SSL verification?) On 2017/02/06 23:59:05, Ryan Sleevi wrote: > AIUI, no; each policy is treated as distinct (you can see the trust panel > settings in Safari when there is a certificate error, and how they populate to > the database) I did some manual testing. If you set a cert as trusted for basic x509 in keychain access, it actually creates two trust dicts, one with no kSecTrustSettingsPolicy entry. So the evaluating code doesn't need to do anything special to handle it, but it does end up being trusted for other uses as well. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:295: SecItemCopyMatching(query, (CFTypeRef*)matching_items.InitializeInto()); On 2017/02/06 23:59:04, Ryan Sleevi wrote: > reinterpret_cast ? Although I think it should toll-free cast, right? done. I think the reinterpret_cast is needed since it's a pointer to a pointer. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:342: // through the SecCertificate APIs. On 2017/02/06 23:59:04, Ryan Sleevi wrote: > I would say if parsing fails, we're good to reject it for now, and Apple's > bananas for being that lax :) Ack. fwiw, There were two such certs I found (on my sierra machine, haven't checked others for this). One with an invalid serial number (which we have the option for in parsing..), one with a duplicate extension (which we don't allow/have an option for.) https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.cc:361: // of a Name without creating a SecCertificate. On 2017/02/06 23:59:04, Ryan Sleevi wrote: > Unfortunately, yeah, that's the case. I'd be hesitant to reimplement Apple's > normalization routines since it's not part of the stable API (I mean, the APIs > to get a normalized name are, but not how it's computed, even if it's > effectively static) Acknowledged. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... File net/cert/internal/trust_store_mac.h (right): https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:8: #include <CoreFoundation/CFString.h> On 2017/02/06 23:59:05, Ryan Sleevi wrote: > AIUI, the 'Apple supported way' is CoreFoundation/CoreFoundation.h > > (that is, I don't believe we're supposed to go straight to CFString.h, but I > could be wrong) Done. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:17: // TrustStoreMac is an implementation of TrustStore which uses Mac OS keychain On 2017/02/06 23:59:05, Ryan Sleevi wrote: > s/Mac OS/macOS/ to match Apple's naming? Done. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:36: TrustAnchors* out_anchors); On 2017/02/06 23:59:05, Ryan Sleevi wrote: > On first read, I was surprised to see public class statics. Can/Should these be > private implementation details? Done. https://codereview.chromium.org/2585963003/diff/60001/net/cert/internal/trust... net/cert/internal/trust_store_mac.h:49: TrustAnchors* matches) const override; On 2017/02/06 23:59:05, Ryan Sleevi wrote: > Suggestion: Order by "interface implementation first", then internal > implementation. Done.
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
There's a lot of DVLOG()ing going on. I didn't really pick on them individually, but I suspect much is redundant, or at least not necessary in this phase of testing. Lots of review feedback, most of it style over substance. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:34: DVLOG(2) << "IsTrustDictionaryTrustedForPolicy?"; I'm not sure the purpose of this trace; is it purely debugging? You could use __func__ as an alternative, which has the benefit of (potential) string pooling, but that's up to you if this is still necessary. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:39: // Ignore application-specific trust settings. Comment suggestion: More detail // Trust settings may be scoped to a single application, by checking that the // code signing identity of the current application matches the serialized code // signing identity in the kSecTrustSettingsApplication key. // As this is not presently supported, skip any trust settings scoped to the // application. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:41: DVLOG(2) << "ignored trust dictionary with kSecTrustSettingsApplication"; Necessary? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:43: } newline between 43/44 https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:45: // for SSL, settings that apply to a single hostname.) // Trust settings may be scoped using policy-specific constraints. For example, // SSL trust settings might be scoped to a single hostname, or EAP settings specific // to a particular WiFi network. // As this is not presently supported, skip any policy-specific trust settings. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:51: // There doesn't seem to be a need to handle kSecTrustSettingsKeyUsage. The way this comment is worded seems more conversational than documentational. Perhaps this could be // Ignoring kSecTrustSettingsKeyUsage for now; it does not seem relevant to the // TLS case. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:53: CFTypeRef policy_typeref = It's probably worth documenting this block (namely, line 53-86) // If the trust settings are scoped to a specific policy (via kSecTrustSettingsPolicy), // ensure that the policy is the same policy as |target_policy_oid|. If there is // no kSecTrustSettingsPolicy key, it's considered a match for all policies. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:63: } If you modify //base/mac/foundation_util.h to CF_CAST_DECL(SecPolicyRef), then you could use SecPolicyRef policy_ref = base::mac::GetValueFromDictionary<SecPolicyRef>(trust_dic, kSecTrustSettingsPolicy); https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:68: // error. Is it necessary to handle this condition? The OS API guarantees that this will be populated and it will be a CFStringRef. Note: You can also do CFStringRef = base::mac::GetValueFromDictionary<CFStringRef>(policy_dict, kSecPolicyOid); https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:78: base::ScopedCFTypeRef<CFDictionaryRef> scoped_policy_dict(policy_dict); Is this right? You copy the properties on 65, which means you leak this on 71 and 76 with this being placed here https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:85: // XXX care about kSecPolicyName, kSecPolicyClient, kSecPolicyKU_<foo> ? It's unclear whether you're asking if this needs to be held to meet this CLs goals, or for the future. If the future, YAGNI. If for now, no, we don't need to handle that. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:104: // TODO: handle distrust (kSecTrustSettingsResultDeny) Do you have thoughts on how to handle this with the trust store API? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:116: // that |is_self_signed| should be treated as a trust root. s/root/anchor/ https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:121: // An empty trust settings array (that is, the trustSettings parameter returns s/trustSettings/trust_settings/ https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:122: // a valid but empty CFArray) means "always trust this certificate” with an note the trailing quote is a smart quote, not " https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:156: &trust_settings); You can instead use base::ScopedCFTypeRef<CFArrayRef> trust_settings; OSStatus err = SecTrustSettingsCopyTrustSettings(cert_handle, trust_domain, trust_settings.InitializeInto()); https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:180: : policy_oid_(base::mac::CFCast<CFStringRef>(policy_oid)) { CFStrictCast ? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:202: CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, nullptr https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:214: &alternate_keychain_search_list); Note .InitializeInto as well (although perhaps our API for TestKeychainSearchList should take a base::ScopedCFTypeRef<> explicitly if it's caller-free semantics) https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:222: // If a TestKeychainSearchList is present, it will have already set newline between 221/222 https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:228: OSStatus status = SecKeychainCopySearchList(&keychain_search_list); .InitializeInto as well https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:249: &roots_keychain); .InitializeInto :) https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:270: crypto::LogCSSMError("SecItemCopyMatching", err); |err| isn't guaranteed to be in the CSSM error space. Your line 271 is probably sufficient. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:303: options.allow_invalid_serial_numbers = true; Document? What cert(s) are this? If you have the SHA-1/SHA-256 fingerprint, we could include crt.sh links here in the documentation to explain why. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.h (right): https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:25: explicit TrustStoreMac(const void* policy_oid); Given that Line 56 stores this as a CFStringRef, is there any reason not to make the API contract a CFStringRef here as well? I'm just uncertain why the void-ing of it https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:45: TrustAnchors* out_anchors); This doesn't seem to be used in the unittest - move to .cc? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:47: // Gets the Issuer name of |cert|, as normalized by the OS. Perhaps we should expand on this documentation. // Returns the OS-normalized issuer of |cert|. // macOS internally uses a normalized form of subject/issuer names for // comparing, roughly similar to RFC3280's normalization scheme. The // normalized form is used for any database lookups and comparisons. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... File net/cert/internal/trust_store_mac_unittest.cc (right): https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:36: } Is this helper needed? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:38: ::testing::AssertionResult ReadTestCert( Perhaps document? :) https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:43: "net/data/ssl/certificates/" + file_name, "CERTIFICATE", &der); Why not use kCertificateHeader here, like you do elsewhere? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:56: std::vector<std::string> GetDerCertsFromMatchingItems( Document? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:99: } A (perhaps convoluted) way in GTest+GMock to handle this would be std::vector<std::string> SecCertificateArrayAsDER(CFArrayRef array); std::vector<std::string> ParsedCertificateListAsDER(ParsedCertificateList list); EXPECT_THAT(SecCertificateArrayAsDER(...), UnorderedElementsAreArray(ParsedCertificateListAsDER(...)); Gives you even more verbose pretty-printers. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:103: TEST(TrustStoreMacTest, MultiRootNotTrusted) { Document what this is testing :) https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:108: GetTestCertsDirectory().AppendASCII("multi-root.keychain")); The thing that makes me nervous from our API inconsistency is that here on 108, we use GetTestCertsDirectory(), but on line 43, we don't. I realize that's partly due to ReadTestDataFromPemFile not basing itself in the test certs directory, but it does create some weird API skew. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:114: SecKeychainOpen(keychain_path.MaybeAsASCII().c_str(), &keychain); .InitializeInto() https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:155: EXPECT_TRUE(MatchingItemsMatch(scoped_matching_items, {b_by_c, b_by_f})); It's not clear to me what this segment of things is testing. Is it testing that the entries are in the .keychain file? If not, could you clarify? https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:206: &find_certificate_system_roots_output)); Document a little more what these calls are doing :) why find-certificate? Why search for the "SHA-1 hash: " string vs the XML? (Note, I'm not asking you to change the usage, just document :D) https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:257: base::ScopedCFTypeRef<SecCertificateRef> scoped_cert_handle(cert_handle); Why not just force this scoper-wrapper around line 251-252?
https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:34: DVLOG(2) << "IsTrustDictionaryTrustedForPolicy?"; On 2017/02/14 19:26:16, Ryan Sleevi wrote: > I'm not sure the purpose of this trace; is it purely debugging? > > You could use __func__ as an alternative, which has the benefit of (potential) > string pooling, but that's up to you if this is still necessary. Yeah the DVLOGs were just for debugging/development. I never really know how much logging to leave in since it does sometimes come in handy later. But I went ahead and removed it. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:39: // Ignore application-specific trust settings. On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Comment suggestion: More detail > > // Trust settings may be scoped to a single application, by checking that the > // code signing identity of the current application matches the serialized code > // signing identity in the kSecTrustSettingsApplication key. > // As this is not presently supported, skip any trust settings scoped to the > // application. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:41: DVLOG(2) << "ignored trust dictionary with kSecTrustSettingsApplication"; On 2017/02/14 19:26:16, Ryan Sleevi wrote: > Necessary? Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:43: } On 2017/02/14 19:26:16, Ryan Sleevi wrote: > newline between 43/44 Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:45: // for SSL, settings that apply to a single hostname.) On 2017/02/14 19:26:16, Ryan Sleevi wrote: > // Trust settings may be scoped using policy-specific constraints. For example, > // SSL trust settings might be scoped to a single hostname, or EAP settings > specific > // to a particular WiFi network. > // As this is not presently supported, skip any policy-specific trust settings. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:51: // There doesn't seem to be a need to handle kSecTrustSettingsKeyUsage. On 2017/02/14 19:26:16, Ryan Sleevi wrote: > The way this comment is worded seems more conversational than documentational. > Perhaps this could be > > // Ignoring kSecTrustSettingsKeyUsage for now; it does not seem relevant to the > // TLS case. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:53: CFTypeRef policy_typeref = On 2017/02/14 19:26:16, Ryan Sleevi wrote: > It's probably worth documenting this block (namely, line 53-86) > > // If the trust settings are scoped to a specific policy (via > kSecTrustSettingsPolicy), > // ensure that the policy is the same policy as |target_policy_oid|. If there is > // no kSecTrustSettingsPolicy key, it's considered a match for all policies. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:63: } On 2017/02/14 19:26:16, Ryan Sleevi wrote: > If you modify //base/mac/foundation_util.h to CF_CAST_DECL(SecPolicyRef), then > you could use > > SecPolicyRef policy_ref = > base::mac::GetValueFromDictionary<SecPolicyRef>(trust_dic, > kSecTrustSettingsPolicy); Done.</noideawhatimdoing> https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:68: // error. On 2017/02/14 19:26:16, Ryan Sleevi wrote: > Is it necessary to handle this condition? The OS API guarantees that this will > be populated and it will be a CFStringRef. > > Note: You can also do > > CFStringRef = base::mac::GetValueFromDictionary<CFStringRef>(policy_dict, > kSecPolicyOid); Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:78: base::ScopedCFTypeRef<CFDictionaryRef> scoped_policy_dict(policy_dict); On 2017/02/14 19:26:16, Ryan Sleevi wrote: > Is this right? You copy the properties on 65, which means you leak this on 71 > and 76 with this being placed here ... I have no excuse. Fixed. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:85: // XXX care about kSecPolicyName, kSecPolicyClient, kSecPolicyKU_<foo> ? On 2017/02/14 19:26:16, Ryan Sleevi wrote: > It's unclear whether you're asking if this needs to be held to meet this CLs > goals, or for the future. If the future, YAGNI. If for now, no, we don't need to > handle that. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:104: // TODO: handle distrust (kSecTrustSettingsResultDeny) On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Do you have thoughts on how to handle this with the trust store API? I've thought about, but I still don't really have a solid answer.. One approach, which we discussed in the past, would be to have a separate IsCertDistrusted sort of method, that would get called in a separate step like how CRLSets are checked. The other, would be to get rid of the separation between CertIssuerSource and TrustStore, have a single interface for finding issuers which also returns a tri-state enum with each one for trusted/unspecified/distrusted. This seems simpler at first glance but I guess it could have issues if the same cert is found from multiple sources and some mark it as distrusted and others don't. Or some combination of the two where looking up issuers (whether trust anchors or not) from the keychain is one step, and then a separate step is just like GetCertTrustValue which could return trusted/unspecified/distrusted. I kinda like that since the current api seems a little off to me. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:116: // that |is_self_signed| should be treated as a trust root. On 2017/02/14 19:26:16, Ryan Sleevi wrote: > s/root/anchor/ Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:121: // An empty trust settings array (that is, the trustSettings parameter returns On 2017/02/14 19:26:16, Ryan Sleevi wrote: > s/trustSettings/trust_settings/ Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:122: // a valid but empty CFArray) means "always trust this certificate” with an On 2017/02/14 19:26:16, Ryan Sleevi wrote: > note the trailing quote is a smart quote, not " Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:156: &trust_settings); On 2017/02/14 19:26:16, Ryan Sleevi wrote: > You can instead use > > base::ScopedCFTypeRef<CFArrayRef> trust_settings; > OSStatus err = SecTrustSettingsCopyTrustSettings(cert_handle, trust_domain, > trust_settings.InitializeInto()); Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:180: : policy_oid_(base::mac::CFCast<CFStringRef>(policy_oid)) { On 2017/02/14 19:26:16, Ryan Sleevi wrote: > CFStrictCast ? Well, it does DCHECK(policy_oid_), but I guess a strict cast would distinguish between wrong type and nullptr... https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:202: CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, On 2017/02/14 19:26:15, Ryan Sleevi wrote: > nullptr Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:214: &alternate_keychain_search_list); On 2017/02/14 19:26:15, Ryan Sleevi wrote: > Note .InitializeInto as well Done. > (although perhaps our API for TestKeychainSearchList should take a > base::ScopedCFTypeRef<> explicitly if it's caller-free semantics) Yeah, it should. I can make a cl for that too. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:222: // If a TestKeychainSearchList is present, it will have already set On 2017/02/14 19:26:16, Ryan Sleevi wrote: > newline between 221/222 Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:228: OSStatus status = SecKeychainCopySearchList(&keychain_search_list); On 2017/02/14 19:26:16, Ryan Sleevi wrote: > .InitializeInto as well Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:249: &roots_keychain); On 2017/02/14 19:26:16, Ryan Sleevi wrote: > .InitializeInto :) Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:270: crypto::LogCSSMError("SecItemCopyMatching", err); On 2017/02/14 19:26:16, Ryan Sleevi wrote: > |err| isn't guaranteed to be in the CSSM error space. Your line 271 is probably > sufficient. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:303: options.allow_invalid_serial_numbers = true; On 2017/02/14 19:26:16, Ryan Sleevi wrote: > Document? What cert(s) are this? If you have the SHA-1/SHA-256 fingerprint, we > could include crt.sh links here in the documentation to explain why. done. (The other cert that fails was a com.apple.kerberos.kdc cert that has two Extended Key Usage extensions.) https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.h (right): https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:25: explicit TrustStoreMac(const void* policy_oid); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Given that Line 56 stores this as a CFStringRef, is there any reason not to make > the API contract a CFStringRef here as well? I'm just uncertain why the void-ing > of it It should be a CFStringRef, but in older versions of the SDK the constants are defined as CFTypeRefs instead. I had void* due to too-hasty fixing of compiler errors, I changed to CFTypeRef for now and added a TODO. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:45: TrustAnchors* out_anchors); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > This doesn't seem to be used in the unittest - move to .cc? Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:47: // Gets the Issuer name of |cert|, as normalized by the OS. On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Perhaps we should expand on this documentation. > > // Returns the OS-normalized issuer of |cert|. > // macOS internally uses a normalized form of subject/issuer names for > // comparing, roughly similar to RFC3280's normalization scheme. The > // normalized form is used for any database lookups and comparisons. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... File net/cert/internal/trust_store_mac_unittest.cc (right): https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:36: } On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Is this helper needed? merged it into the other function https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:38: ::testing::AssertionResult ReadTestCert( On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Perhaps document? :) Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:43: "net/data/ssl/certificates/" + file_name, "CERTIFICATE", &der); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Why not use kCertificateHeader here, like you do elsewhere? Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:56: std::vector<std::string> GetDerCertsFromMatchingItems( On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Document? Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:99: } On 2017/02/14 19:26:17, Ryan Sleevi wrote: > A (perhaps convoluted) way in GTest+GMock to handle this would be > > std::vector<std::string> SecCertificateArrayAsDER(CFArrayRef array); > std::vector<std::string> ParsedCertificateListAsDER(ParsedCertificateList list); > > EXPECT_THAT(SecCertificateArrayAsDER(...), > UnorderedElementsAreArray(ParsedCertificateListAsDER(...)); > > Gives you even more verbose pretty-printers. Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:103: TEST(TrustStoreMacTest, MultiRootNotTrusted) { On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Document what this is testing :) Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:108: GetTestCertsDirectory().AppendASCII("multi-root.keychain")); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > The thing that makes me nervous from our API inconsistency is that here on 108, > we use GetTestCertsDirectory(), but on line 43, we don't. > > I realize that's partly due to ReadTestDataFromPemFile not basing itself in the > test certs directory, but it does create some weird API skew. This is true, but I guess it doesn't worry me too much. As in, it might be nice to clean up somehow, but I'm not sure there's enough payoff to make it worth it. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:114: SecKeychainOpen(keychain_path.MaybeAsASCII().c_str(), &keychain); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > .InitializeInto() Done. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:155: EXPECT_TRUE(MatchingItemsMatch(scoped_matching_items, {b_by_c, b_by_f})); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > It's not clear to me what this segment of things is testing. Is it testing that > the entries are in the .keychain file? If not, could you clarify? It independently testing that the SecItemCopyMatching part works properly with some known data.. although the SystemCerts does test that too I think this is still useful since it can do some limited testing that doesn't depend on what's in the system root store. https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:206: &find_certificate_system_roots_output)); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Document a little more what these calls are doing :) > > why find-certificate? Why search for the "SHA-1 hash: " string vs the XML? > (Note, I'm not asking you to change the usage, just document :D) done. (It's trust-settings-export that has XML output, find-certificate is just text.) https://codereview.chromium.org/2585963003/diff/100001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:257: base::ScopedCFTypeRef<SecCertificateRef> scoped_cert_handle(cert_handle); On 2017/02/14 19:26:17, Ryan Sleevi wrote: > Why not just force this scoper-wrapper around line 251-252? Done.
https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:85: if (!CFNumberGetValue(trust_settings_result_ref, kCFNumberIntType, Perhaps fold this in to line 84 with an && https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:91: // TODO: handle distrust (kSecTrustSettingsResultDeny) It looks like you're already handling that, by way of line 96 & 99, right? https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:147: policy_oid)) To the general point of handling distrust records - I suspect that if you modified IsTrustSettingsTrustedForPolicy() to return the tristate ('yes', 'no', 'continue'), then we could appropriately handle distrust records by way of short-circuiting on yes/no and otherwise continuing to the next trust domain on 'continue' That should work, right? https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:248: CFMutableArrayRef mutable_keychain_search_list = CFArrayCreateMutableCopy( newline between 247/248 https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:273: base::AutoLock lock(crypto::GetMacSecurityServicesLock()); Line 260 and 241 would also need to be guarded w/ this lock, as they also both hit the security services locks https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:302: SecCertificateCopyNormalizedIssuerContent(cert_handle, nullptr)); This would also need the CSSM lock https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.h (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:29: explicit TrustStoreMac(const CFTypeRef policy_oid); The const is unnecessary here now, right? https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac_unittest.cc (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:274: SecCertificateCopyNormalizedSubjectContent(cert_handle, nullptr)); Locked (although in practice shouldn't matter for this test, just not sure if we should annotate it as a lock-guarded call) It's more relevant for the SecTrust calls below, but again, it shouldn't matter for a single-threaded test, so I'm torn :) https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:327: LOG(INFO) << "false_neg: " << false_neg; Thought: This will end up logging on every test run. It seems like 307 should handle sufficient logging for this, right? Should we delete these LOGs and the variables because of that?
https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:85: if (!CFNumberGetValue(trust_settings_result_ref, kCFNumberIntType, On 2017/02/16 22:27:22, Ryan Sleevi wrote: > Perhaps fold this in to line 84 with an && Done. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:91: // TODO: handle distrust (kSecTrustSettingsResultDeny) On 2017/02/16 22:27:22, Ryan Sleevi wrote: > It looks like you're already handling that, by way of line 96 & 99, right? Isn't there still the case where you have a distrust record for some non-anchor cert, so we'd want to be able to fail any chain that got built with that cert even if it chains to a valid trust anchor? https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:147: policy_oid)) On 2017/02/16 22:27:22, Ryan Sleevi wrote: > To the general point of handling distrust records - I suspect that if you > modified IsTrustSettingsTrustedForPolicy() to return the tristate ('yes', 'no', > 'continue'), then we could appropriately handle distrust records by way of > short-circuiting on yes/no and otherwise continuing to the next trust domain on > 'continue' > > That should work, right? Would work for this part, but see the question above. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:248: CFMutableArrayRef mutable_keychain_search_list = CFArrayCreateMutableCopy( On 2017/02/16 22:27:22, Ryan Sleevi wrote: > newline between 247/248 Done. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:273: base::AutoLock lock(crypto::GetMacSecurityServicesLock()); On 2017/02/16 22:27:22, Ryan Sleevi wrote: > Line 260 and 241 would also need to be guarded w/ this lock, as they also both > hit the security services locks Done. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:302: SecCertificateCopyNormalizedIssuerContent(cert_handle, nullptr)); On 2017/02/16 22:27:23, Ryan Sleevi wrote: > This would also need the CSSM lock Hm, that seems a little surprising unless you're saying that every Sec* function that touchs certs needs the lock? This function just seems to copy some bytes out of the cert structure.. (the normalization is done during parsing) What about the SecTrustSettingsCopyTrustSettings and SecPolicyCopyProperties calls? https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.h (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/16 22:27:23, Ryan Sleevi wrote: > 2017 Done. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.h:29: explicit TrustStoreMac(const CFTypeRef policy_oid); On 2017/02/16 22:27:23, Ryan Sleevi wrote: > The const is unnecessary here now, right? Done. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac_unittest.cc (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:274: SecCertificateCopyNormalizedSubjectContent(cert_handle, nullptr)); On 2017/02/16 22:27:23, Ryan Sleevi wrote: > Locked (although in practice shouldn't matter for this test, just not sure if we > should annotate it as a lock-guarded call) > > It's more relevant for the SecTrust calls below, but again, it shouldn't matter > for a single-threaded test, so I'm torn :) Well, I guess it's a good idea just for the documentation and some copy-pasta safety. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac_unittest.cc:327: LOG(INFO) << "false_neg: " << false_neg; On 2017/02/16 22:27:23, Ryan Sleevi wrote: > Thought: This will end up logging on every test run. It seems like 307 should > handle sufficient logging for this, right? Should we delete these LOGs and the > variables because of that? Done.
https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:91: // TODO: handle distrust (kSecTrustSettingsResultDeny) On 2017/02/17 00:04:18, mattm wrote: > On 2017/02/16 22:27:22, Ryan Sleevi wrote: > > It looks like you're already handling that, by way of line 96 & 99, right? > > Isn't there still the case where you have a distrust record for some non-anchor > cert, so we'd want to be able to fail any chain that got built with that cert > even if it chains to a valid trust anchor? Yup. I was thinking solely for the low-hanging fruit of CA records. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:302: SecCertificateCopyNormalizedIssuerContent(cert_handle, nullptr)); On 2017/02/17 00:04:18, mattm wrote: > On 2017/02/16 22:27:23, Ryan Sleevi wrote: > > This would also need the CSSM lock > > Hm, that seems a little surprising unless you're saying that every Sec* function > that touchs certs needs the lock? This function just seems to copy some bytes > out of the cert structure.. (the normalization is done during parsing) Yup, at least until 10.12 it seems. At the lowest level, the issue is in the ref-counting in the CSSM/CDSA code, and any properties that touch any of those objects (or cause them to be allocated/deallocated as part of the call, which isn't really something we can guarantee with the API contract) can end up tripping it up. > What about the SecTrustSettingsCopyTrustSettings and SecPolicyCopyProperties > calls? Yeah, basically. SecTrustSettings* is "safe" because it doesn't touch any of the CSSM bits, but that's not guaranteed. SecPolicy* is less-so, because it (used to) have a CSSM path in it.
LGTM (we can follow-up separately about distrust)
The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:160001) has been deleted
https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... File net/cert/internal/trust_store_mac.cc (right): https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:147: policy_oid)) On 2017/02/17 00:04:18, mattm wrote: > On 2017/02/16 22:27:22, Ryan Sleevi wrote: > > To the general point of handling distrust records - I suspect that if you > > modified IsTrustSettingsTrustedForPolicy() to return the tristate ('yes', > 'no', > > 'continue'), then we could appropriately handle distrust records by way of > > short-circuiting on yes/no and otherwise continuing to the next trust domain > on > > 'continue' > > > > That should work, right? > > Would work for this part, but see the question above. I added the tri-state. https://codereview.chromium.org/2585963003/diff/120001/net/cert/internal/trus... net/cert/internal/trust_store_mac.cc:302: SecCertificateCopyNormalizedIssuerContent(cert_handle, nullptr)); On 2017/02/17 00:54:13, Ryan Sleevi wrote: > On 2017/02/17 00:04:18, mattm wrote: > > On 2017/02/16 22:27:23, Ryan Sleevi wrote: > > > This would also need the CSSM lock > > > > Hm, that seems a little surprising unless you're saying that every Sec* > function > > that touchs certs needs the lock? This function just seems to copy some bytes > > out of the cert structure.. (the normalization is done during parsing) > > Yup, at least until 10.12 it seems. > > At the lowest level, the issue is in the ref-counting in the CSSM/CDSA code, and > any properties that touch any of those objects (or cause them to be > allocated/deallocated as part of the call, which isn't really something we can > guarantee with the API contract) can end up tripping it up. > Acknowledged. > > What about the SecTrustSettingsCopyTrustSettings and SecPolicyCopyProperties > > calls? > > Yeah, basically. SecTrustSettings* is "safe" because it doesn't touch any of the > CSSM bits, but that's not guaranteed. SecPolicy* is less-so, because it (used > to) have a CSSM path in it. Sorry, it wasn't clear to me if this should be interpeted as "yes add the locks" or "no don't need to add the locks".
On 2017/02/17 02:03:36, mattm wrote: > Sorry, it wasn't clear to me if this should be interpeted as "yes add the locks" > or "no don't need to add the locks". Ah, totally sorry about that. I was saying yes, they should be locked, and then explaining why it's more defensive than critically necessary - but I totally didn't make that clear.
On 2017/02/17 03:29:41, Ryan Sleevi wrote: > On 2017/02/17 02:03:36, mattm wrote: > > Sorry, it wasn't clear to me if this should be interpeted as "yes add the > locks" > > or "no don't need to add the locks". > > Ah, totally sorry about that. I was saying yes, they should be locked, and then > explaining why it's more defensive than critically necessary - but I totally > didn't make that clear. done.
mattm@chromium.org changed reviewers: + mark@chromium.org
+mark for base/mac OWNERS
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #10 (id:220001) has been deleted
On 2017/02/17 20:16:20, mattm wrote: > +mark for base/mac OWNERS pinging mark
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rsleevi@chromium.org changed reviewers: + thakis@chromium.org - mark@chromium.org
Switching Mark for Nico :) Nico - the one change in //base/mac to make code purty? :)
base/ lgtm
The CQ bit was checked by mattm@chromium.org
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/2585963003/#ps300001 (title: "compile fix???")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mattm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2585963003/#ps340001 (title: "updates for rebase & cl format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1488315693852820,
"parent_rev": "f5903684572a3df26e3eb91932a9de0174eb9b70", "commit_rev":
"ea4ed823078f8496ce4ad18975658d87137da3eb"}
Message was sent while issue was closed.
Description was changed from ========== PKI library Mac trust store integration BUG=690704 ========== to ========== PKI library Mac trust store integration BUG=690704 Review-Url: https://codereview.chromium.org/2585963003 Cr-Commit-Position: refs/heads/master@{#453731} Committed: https://chromium.googlesource.com/chromium/src/+/ea4ed823078f8496ce4ad1897565... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001) as https://chromium.googlesource.com/chromium/src/+/ea4ed823078f8496ce4ad1897565... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
