-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add new darwin certificate trust settings table #8715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new darwin certificate trust settings table #8715
Conversation
|
@Micah-Kolide can you please rebase/merge for macOS CI fixes? |
Will do! |
52cbcea to
ee1c446
Compare
zwass
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looking good though I have requested some changes.
|
@Micah-Kolide on my system, trust_policy_name | trust_policy_data | trust_allowed_error | trust_key_usage | trust_result are all empty for all the rows. Is that expected? Do you know a way I can test these with nonempty values? |
Hey @zwass, does running the below commands generate any output? Mine looks like this (minus the *'s): If there are any trust settings configured, then removing the I edited the trust settings in my keychain on the Zscaler Root CA cert for these to show up, but I've also tested this by using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I installed Charles Proxy and was able see the trust settings.
When I got this to start returning data, it brought up a number of questions for me.
I'm pretty confident that we need to be emitting one row for each of the trust settings entries (see comment below).
Also,
Should we emit a row when there are no trust settings configured? I think maybe not? But I'm not sure exactly how the table would be used. Certificates with no configured trust settings do already show up in the certificates table.
On my machine I see several entries with no common name. This gives me some concern. Any idea what that's about?
+-------------+----------------------------------+--------------+-------------------+-------------------+---------------------+-----------------+--------------+
| common_name | serial | trust_domain | trust_policy_name | trust_policy_data | trust_allowed_error | trust_key_usage | trust_result |
+-------------+----------------------------------+--------------+-------------------+-------------------+---------------------+-----------------+--------------+
| | 15C8BD65475CAFB897005EE406D2BC9D | system | | | | | |
| | 5D938D306736C8061D1AC754846907 | system | | | | | |
| | 00 | system | | | | | |
| | 00 | system | | | | | |
| | 00 | system | | | | | |
| | 110034B64EC6362D36 | system | | | | | |
| | 200605167002 | system | | | | | |
+-------------+----------------------------------+--------------+-------------------+-------------------+---------------------+-----------------+--------------+
I also never saw any values for trust_policy_data and trust_key_usage even though it does seem like you used them according to the documentation.
You are very right about returning one result per trust setting, and I feel bad for missing that. I have updated the table results to have one per trust setting, and omit certificates without trust settings. I unfortunately also saw the entires without a common name and I'm not sure what those are about, but they are also omitted with the latest changes. I also never saw any values for |
zwass
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvements! My last request is to return the error as a string. This code seems to work and correspond with what the CLI command returns:
// Log the trust_allowed_error as a string (convert with
// SecCopyErrorMessageString)
uint32_t trust_allowed_error_value;
CFNumberGetValue((CFNumberRef)trust_allowed_error,
kCFNumberSInt32Type,
&trust_allowed_error_value);
CFStringRef error =
SecCopyErrorMessageString(trust_allowed_error_value, nil);
LOG(ERROR) << "Trust allowed error: " << stringFromCFString(error)
<< " name:: " << r["common_name"]
<< " policy:: " << r["trust_policy_name"];
CFRelease(error);
Thank you! Here are the results with the |
zwass
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully last changes 🤞
zwass
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you for the persistence!
Of course! Thank you for the very helpful and patient reviews. |
This PR adds a new table for darwin devices, the
certificate_trust_settingstable to acquire the configured cert trust settings of each certificate domain for increased visibility into the status of installed certificates. This data is similar to executingsecurity dump-trust-settings -d(for the 'admin' domain), orsecurity dump-trust-settings -s(for the 'system' domain).Something that
securityfails to do, is correctly report the configured trust settings of MDM distributed certificates. At least in some cases, it will report that 0 settings are configured regardless of which settings are set up.For this table I'm implementing Apple's Security framework to copy the certificate trust settings for a cert domain, and enumerate the returned data.
When I was building the table, I could see that the array returned from
SecTrustSettingsCopyTrustSettingsincluded the trust setting policy name:kSecTrustSettingsPolicyName, but when building the table it would fail for the use of an undeclared identifier. This is why I am manually defining this key identifier.Here is some redacted output: