-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Optionally trust CA certificates in GUI for SSL client actions (e.g. updates) #3558
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
Changes from all commits
98558d2
4ecbbac
3a701ee
b60a90c
e7ecd56
e4303c1
15619ae
3c79ee7
268580a
32cd3f1
7cea2b1
b5aa0ef
2b69f26
116d5d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,7 @@ | |
| $name = $a_ca[$id]['descr']; | ||
| unset($a_ca[$id]); | ||
| write_config(); | ||
| ca_trustedcerts_generate(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| $savemsg = sprintf(gettext("Certificate Authority %s and its CRLs (if any) successfully deleted."), htmlspecialchars($name)); | ||
| pfSenseHeader("system_camanager.php"); | ||
| exit; | ||
|
|
@@ -107,6 +108,7 @@ | |
| if (!empty($a_ca[$id]['prv'])) { | ||
| $pconfig['key'] = base64_decode($a_ca[$id]['prv']); | ||
| } | ||
| $pconfig['trusted'] = isset($a_ca[$id]['trusted']); | ||
| } | ||
|
|
||
| if ($act == "new") { | ||
|
|
@@ -250,6 +252,8 @@ | |
|
|
||
| $ca['descr'] = $pconfig['descr']; | ||
|
|
||
| $ca['trusted'] = isset($pconfig['trusted']); | ||
|
|
||
| if ($act == "edit") { | ||
| $ca['descr'] = $pconfig['descr']; | ||
| $ca['refid'] = $pconfig['refid']; | ||
|
|
@@ -312,6 +316,7 @@ | |
|
|
||
| if (!$input_errors) { | ||
| write_config(); | ||
| ca_trustedcerts_generate(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure it is a good idea to trust that files named
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deleting the entire set of cert files and rewriting them provides no additional safety.
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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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".
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "inconsistently applied bloatware", for example. Uncalled for. You keep arguing in circles ignoring my explanations, so I'm done.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| pfSenseHeader("system_camanager.php"); | ||
| } | ||
| } | ||
|
|
@@ -362,6 +367,7 @@ | |
| <tr> | ||
| <th><?=gettext("Name")?></th> | ||
| <th><?=gettext("Internal")?></th> | ||
| <th><?=gettext("Trusted")?></th> | ||
| <th><?=gettext("Issuer")?></th> | ||
| <th><?=gettext("Certificates")?></th> | ||
| <th><?=gettext("Distinguished Name")?></th> | ||
|
|
@@ -405,6 +411,7 @@ | |
| <tr> | ||
| <td><?=$name?></td> | ||
| <td><i class="fa fa-<?= (!empty($ca['prv'])) ? "check" : "times" ; ?>"></i></td> | ||
| <td><i class="fa fa-<?= (isset($ca['trusted'])) ? "check" : "times" ; ?>"></i></td> | ||
| <td><i><?=$issuer_name?></i></td> | ||
| <td><?=$certcount?></td> | ||
| <td> | ||
|
|
@@ -486,6 +493,13 @@ | |
| $pconfig['descr'] | ||
| )); | ||
|
|
||
| $section->addInput(new Form_Checkbox( | ||
| 'trusted', | ||
| 'Trust', | ||
| 'Trust this CA\'s certificates', | ||
| $pconfig['trusted'] | ||
| ))->setHelp('Trust certificates generated by this CA when pfSense accesses HTTPS/FTPS servers (e.g. updates, firewall URL aliases).'); | ||
|
|
||
| if (!isset($id) || $act == "edit") { | ||
| $section->addInput(new Form_Select( | ||
| 'method', | ||
|
|
||
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.
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.
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.
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.
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.
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.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.