Likewise, we should figure out what to do when there are both user policies and admin policies, but none of the user policy dictionaries apply.

	// If trust settings are present, but none of them match the policy...
	// the docs don't tell us what to do.
	//
	// "Trust settings for a given use apply if any of the dictionaries in the
	// certificate’s trust settings array satisfies the specified use." suggests
	// that it's as if there were no trust settings at all, so we should maybe
	// fallback to the admin trust settings? TODO.

@henvic out of interest, how did you distrust these certificates on your system? When I use Keychain Access to manually de-trust a root (setting "When using this certificate: Never Trust") my /Library/Security/Trust\ Settings/admin.plist looks rather different. Noticeably I see the kSecTrustSettingsPolicyName field in most of my trustSettings dicts, among a few other minor differences. If I import any of your trust settings I get similar "The Trust Settings Record was corrupted." errors, which may suggest this particular problem is a red herring due to broken trust settings.

That said there does seem to still be a mismatch between the behavior in the no-cgo and cgo implementations here. When the no-cgo implementation hits this weird parsing issue it returns errSecInvalidTrustSettings, whereas the cgo version is returning kSecTrustSettingsResultUnspecified.

Ah, I see what is happening here, cgo sslTrustSettingsResult is returning kSecTrustSettingsResultUnspecified on err != errSecSuccess || trustSettings == NULL from SecTrustSettingsCopyTrustSettings whereas no-cgo is directly returning the actual error. The cgo approach seems incorrect, I'd expect it to return kSecTrustSettingsResultInvalid perhaps? or... the error, ¯_(ツ)_/¯. Need to look at the apple source.

I've done some spelunking in the apple sources with some help from @sleevi. Various comments on the comments in the CGO version (which have mostly been copied into the direct call version):

Ignoring kSecTrustSettingsAllowedError seems somewhat dangerous. This dict key implies that there are certain errors that appear during chain validation that should be ignored, but the cert should not be wholesale trusted. Since golang doesn't understand how to apply this policy during chain building it seems prudent to skip any certs that have this in their SSL usage constraints dict. Relatedly it seems like kSecTrustSettingsKeyUsage could be applied without much pain, i.e. if we are adding certs to the root pool, and the kSecTrustSettingsResult is Trust or TrustAsRoot, it might make sense to check that the SignCert bit is set.

SecTrustServer.c is from old-macOS, the current policy is described in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/headers/SecTrustSettings.h.auto.html and this specific quote appears to be from SecLegacyAnchorSourceCopyUsageConstraints in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/trustd/SecCertificateSource.c.auto.html. The source matches the interpretation in the comment, if there are user settings they override admin settings.

There is a mismatch here between the CGO (and apple) implementations and the direct call implementation. Errors when retrieving the user or admin trust settings appear to just be thrown away, and a NULL CFArrayRef is returned (causing kSecTrustSettingsResultUnspecified for the certificate in the cgo impl and kSecTrustSettingsResultInvalid in the apple impl) rather than the error being surfaced. It seems like the direct call implementation should match this (in practice because kSecTrustSettingsResultUnspecified isn't actually implemented in the go version, because this is a root pool, not an intermediate pool, it has the same effect in the end).

This comment appears to be in the wrong place, the fallback to admin domain has already happened if the user settings are NULL, if there are neither user nor admin trust settings kSecTrustSettingsResultInvalid appears to be appropriate.

It's hard to pin down how apple treats this, but it appears to just return the first constraint that applies (see SecPVCGetTrustSettingsResult in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/trustd/SecPolicyServer.c.auto.html). My understanding is that while there are cases where there may be multiple dicts in the CFArrayRef where kSecTrustSettingsPolicy == SSL, those dicts are likely to have extra policy attached to them (i.e. kSecTrustSettingsAllowedError or kSecTrustSettingsKeyUsage or something completely different). That said it might be prudent for kSecTrustSettingsResultDeny to override the other results, since we are not sure how to interpret the extra policy.

A more high level comment: probably we should be ignoring dicts that contain keys we don't parse/don't know how to parse, since they are likely to contain policy we cannot apply in go.

Referring back, trust settings are attached to the cert independent of it's evaluation, so there doesn't appear to be any fallback from one domain to another if no settings apply to the cert. Apple source seems to imply that in the case that no trust settings apply to a cert then kSecTrustSettingsResultInvalid is returned rather than kSecTrustSettingsResultUnspecified (see SecPVCGetTrustSettingsResult again in https://opensource.apple.com/source/Security/Security-59306.41.2/trust/trustd/SecPolicyServer.c.auto.html) but again since we don't actually do anything with certs marked kSecTrustSettingsResultUnspecified it's functionally the same (for now).

Generally I think both implementations are mostly matching the apple approach, although in a few places the golang implementation feels a bit more liberal than it perhaps should be.

Sidenote: there are a handful of times in the comments that references are made to places in the apple source, but without any links (or even file names) it's very hard to track down what they are talking about. It would be extremely valuable to add explicit references to what is being talked about.

@rolandshoemaker, I distrusted them in Keychain probably the same way you did a long time ago after opening it as a regular (non-sudoer) user (at some point I was prompted for authentication with an admin account).

Hrm, interesting. I wonder if this is a case of Apple pulling the rug out from beneath themselves by changing the schema at some point and not properly migrating it or something... ¯\_(ツ)_/¯ Thanks for the info!

@henvic you might want to try to re-distrust those specific certs if you really don't want them used, it's possible apple is sticking them in the intermediate pool for chain validation, which could cause very strange chains using them to be built...

Change https://golang.org/cl/233360 mentions this issue: crypto/x509: match darwin cgo loadSystemRoots in new implementation

Kindly paging @katiehockman @rolandshoemaker @FiloSottile, about this issue. Should we perhaps move this to Go1.17?

Yep, definitely not happening for 1.16, thanks!

Thanks @rolandshoemaker for the update! Moving it to Go1.17.

Moving to backlog as it will probably be cleared by #46287.

Change https://golang.org/cl/353132 mentions this issue: crypto/x509: use platform verifier on darwin