Do not use a shell command for this, we have functions to make directories.

Will do.

Do not use a shell command for this, use a function similar to this: https://github.com/pfsense/FreeBSD-ports/blob/devel/net-mgmt/pfSense-pkg-net-snmp/files/usr/local/pkg/net-snmp.inc#L223
If you want to exclude something, then write your files with a unique prefix and then glob using that prefix,

I agree with this and the previous comment that I should be using pfSense library functions over shell commands. The mkdir code is easy to change; I had aped other code in certs.inc that did the same thing, so my next pull request may be a few patches to clean up that file.

I will refactor to eliminate the call to /usr/bin/find and use is_link and unlink instead. There's no way to use prefixes in /etc/ssl/certs, unfortunately, because OpenSSL requires that the files be named {cert_hash}.0, and OpenSSL doesn't support certificates in subdirectories of cert_path, so I can't do that either. Symlinks seemed a reasonable compromise to not completely obliterate the ability of power users and developers to manually place certificates in the directory.

No need to make this variable for a single use, combine the two lines openssl_x509_parse(base64_decode($crt));

Why do trusted certs need to be re/generated during boot up? /etc/ssl/certs is a static dir is it not? The certs should still be there across reboots. Only reason I think of is restoring/swapping a config. But the trusted certs re/generation for that scenario should and could be done by other means. Such as part of the config restore process. Or a trusted certs regeneration button in ca manager for those times when manually swapping a config.

It's safer to do it at boot time to ensure it's consistent. It's not like it's a huge amount of work or writes.

And completely unnecessary. It's not like people are restoring or swapping configs in prod every other time they reboot. Also not friendly to write cycle sensitive media.

Unless you can check to be sure the state is consistent and update as needed, it is necessary. You don't know what someone might have done, or what might have happened on upgrade, etc. It's not like it's happening every few minutes, the number of writes overall is minimal. Better safe than sorry.

Unless you can check to be sure the state is consistent and update as needed, it is necessary.

Which is how it should be done. Verify what is there against what config says should be there and add/remove accordingly. Also include a "synchronize" button on the CA manager page for user to be able to manually initiated the same thing.

Would rather trusted certs be generated/removed on a per cert add/edit basis instead of regenerating all of them every time a change is made. Even changes that do not make changes to trusted certs settings.

Would rather trusted certs be generated/removed on a per cert add/edit basis instead of regenerating all of them every time a change is made. Even changes that do not make changes to trusted certs settings.

I'm not sure it is a good idea to trust that files named *hashvalue*.0 have the correct certificate as their contents. I could certainly read in the existing file and check if it contains the correct certificate, but that seems like a lot of overhead compared to rewriting the certfiles during change/reboot. Thoughts on security implications of blindly trusting the file contents?

There are some more comments on this up higher. I'd rather see them rewritten each boot to be absolutely sure. I don't see saving 2-3 writes per reboot as being worth all the extra complexity. You could read the files in and compare, or compute and compare hashes without reading directly, but it's a lot of overhead for minimal benefit.

Deleting the entire set of cert files and rewriting them provides no additional safety.

1. If the contained cert does not match the hash name then it will fail for the site. The certs here are found for the site by their hash name. They are not all read until a match is found. That's why they are required to be hash named.

2. Symlinks are not being destroyed. So anyone who wishes to tamper can do so simply by creating a link to a cert located anywhere on the system they want.

3. Anyone wishing to tamper can simply put a cert of their choosing in the CA bundle (/usr/local/share/certs/ca-root-nss.crt). It is not purged and reloaded at boot either.

This argument of safety and trust is nonsensical.

No need to read the files in and compare. Just create an array of file names config says should be there and add/remove accordingly. Not much work at all really, and far less work than writing files that don't need to be written. Deleting and rewriting the files is much more work for no benefit.

// Get list of trust files for CA certs from config.
foreach($config['ca'] as $ca) {
	$ca_hash = cert_get_hash($ca['crt']);
	$ca_certfiles[] = "{$g['etc_path']}/ssl/certs/{$ca_hash}.0";
}

// Remove cert trust files that have no corresponding CA cert in config.
foreach (glob("{$g['etc_path']}/ssl/certs/*") as $cert_file) {
	if (!in_array($cert_file, $ca_certfiles)) {
		unlink_if_exists($cert_file);
	}
}

// Add missing cert trust files for CA certs in config.  Remove cert trust files of CA certs in config that are not trusted or are revoked.
foreach ($a_ca as $id => $value) {
	ca_trustedcert_add($id);
}

Wanting to be sure the firewall has correct contents in the files is hardly "nonsensical". When I say "safe" I mean "Ensure the contents of the files are correct and that all are present", not necessarily "safe" from a security standpoint (which is also important).

If a file is named properly but contains the wrong contents (0-byte file, truncated, overwritten, etc) it would be completely missed unless the contents of the files are checked/verified. There would be no way to correct that unless the file is overwritten or removed and rewritten. The use of the CA would fail but there's no way to correct that, and it wouldn't be obvious to the user that they might have to trigger a forced rewrite by edit/save of the CA if it was fixed to force an update then.

You're still jumping through a lot of hoops to avoid something that isn't even a sizable problem. A few writes per reboot isn't going to kill a disk. We need to be sure the files are in a consistent state with correct contents, making no assumptions and taking no shortcuts.

Just because it's the way you what pfSense to behave does not make it good or even beneficial. Microsoft does things because that's they way they want it to be too. Doesn't necessarily make it better. Doing that way just because that's the way we want it to be is not sound design. Any good engineer will tell you that the more you "touch" something increases the probability of introducing an error/failure/etc. The file is there, it's good. Leave it alone and let it be. Just the like CA bundle file.

If customers are willing to accept your inconsistently applied bloatware method of file integrity assurance. That is between them and you. But yes I prefer it be left alone once there. And that's how it will be done on my systems.

If it really is that critically important to you/pfSense then the rewriting on boot up is still not the "right"/"best" way of ensuring the content is correct, and has much more overhead than doing it in a well engineered fashion that could also be applied to any file deemed to be critical.

You disagree, I get it. Let it go. No need for insults or name-calling. Your way is not better, it's riskier. You don't know the file is good until you check it to know it's good. You want to allow for the possibility that it's not right -- that's on you, but that isn't how we operate. Overwrite or read/verify checksum, either way is fine since you end up with a file in a known-good state.

Leaving unverified data in place because the filename happens to be correct isn't "well engineered".

I agree. No need for insults or name-calling.

Where did I insult you? Where did I call you a name?

You certainly hurled some insults at me though.

My way is no more riskier than how the CA bundle is currently being handled. And like I stated. Rewriting files at boot up is still not the "right" way to ensure file integrity. There are better, more reliable methods with far less overhead.

Leaving unverified data in place because the filename happens to be correct isn't "well engineered".

But you and pfSense are okay with that as long is it's other just as critical files. Yes. That logic is well reasoned out. Not.

I have never said they shouldn't be verified. But if they are going to be then do it "right". With a well engineer method and consistently apply it to critical files.

"inconsistently applied bloatware", for example. Uncalled for. You keep arguing in circles ignoring my explanations, so I'm done.

Talking down to someone with statements like; let me boil it down to basic, so you can get it and such. Yes those kinds of statements that you made are insults. I wasn't born yesterday. I know what those statements are about. They are intended to belittle.

I'm not the one arguing in circles and ignoring your explanations. I'm debunking them and the reasoning that's being given for them.

Think this is what you really want to do here: $pconfig['trusted'] = isset($a_ca[$id]['trusted']);

Shouldn't need this as it will not be set by default.

This should be all that it needed here: $ca['trusted'] = isset($pconfig['trusted']);

Not sure about this one and subsequent comment that suggests deleting && $ca['trusted']. isset() will return TRUE even if the variable is set FALSE, right? Is it completely safe to assume that a boolean FALSE never appears in global $config?

Option set/not set rather that true/false is the way it is done throughout pfSense. Yes using isset instead of true/false works, is cleaner/clearer and is the consistent boolean option technique in pfSense config.

Don't need the && $ca['trusted'] here.

And along with the other issets this would be an isset test also. if (isset($ca['trusted']))

Believe array_filter syntax is incorrect. The callback function should be the second parameter.
Even so in my testing is_file was true for symlinks too. So don't think this will do what is expected.

Was tired and mimicked array_map() syntax. I'll roll this into my next push. Speaking of, regarding your previous comment about squashing, won't a project maintainer be merging all this activity as a squash commit, anyway?

The merge should have them all squashed together. But I know that on mine when I look at a file history it shows all the individual commits.