Trust MVC from Smart-Soft #2053

kekek2 · 2018-01-04T10:45:09Z

No description provided.

fabianfrz · 2018-01-04T17:53:44Z

src/opnsense/mvc/app/controllers/OPNsense/Trust/Api/TrustBase.php

+        if (isset($post["digest_alg"]) && !in_array($post["digest_alg"], array_keys($this->digest_algs))) {
+            $result["validations"]["{$method}.digest_alg"] = gettext("Please select a valid Digest Algorithm.");
+        }
+        return $result;


you should override performValidatoion like in #2018.

Usually the input form matches a data model. So it is convenient to validate the data entered at the model level. In the case of a Trust/Certificates, we introduce some data, and on the basis of their calculated values to be written to the model. Therefore, you need to validate the entered data, because the data for the model can simply not now contain.

fabianfrz

some quick stuff I have found:

long story short:

I don't like the model as it is now
needs XSS security
Forms: different case style

fabianfrz · 2018-01-04T17:54:50Z

src/opnsense/mvc/app/controllers/OPNsense/Trust/AuthoritiesController.php

+     */
+    public function indexAction()
+    {
+        $this->view->title = gettext('System') . ": " . gettext("Trust") . ": " . gettext("Authorities");


It may not need the title and splitted gettext calls of strings which belong together are a no go.

fabianfrz · 2018-01-04T17:58:36Z

src/opnsense/mvc/app/controllers/OPNsense/Trust/forms/existingCrl.xml

+<form>
+    <field>
+        <id>Existing.descr</id>
+        <label>Descriptive name</label>


name should start with "N" to match the other entries

fabianfrz · 2018-01-04T17:58:56Z

src/opnsense/mvc/app/controllers/OPNsense/Trust/forms/existingCA.xml

+<form>
+    <field>
+        <id>Existing.descr</id>
+        <label>Descriptive name</label>


fabianfrz · 2018-01-04T17:59:38Z

src/opnsense/mvc/app/controllers/OPNsense/Trust/forms/existingCA.xml

+    </field>
+    <field>
+        <id>Existing.serial</id>
+        <label>Serial for next certificate</label>


All upcase all first characters like the other labels

fabianfrz · 2018-01-04T17:59:55Z

src/opnsense/mvc/app/controllers/OPNsense/Trust/forms/existingCrl.xml

+    </field>
+    <field>
+        <id>Existing.text</id>
+        <label>CRL data</label>


fabianfrz · 2018-01-04T18:35:16Z

src/opnsense/mvc/app/views/OPNsense/Trust/revocation.volt

+                                "<button type=\"button\" class=\"btn btn-xs btn-default command-delete\" data-row-id=\"" + row.uuid + "\" title=\"{{ lang._('Delete CRL') }}\" alt=\"{{ lang._('Delete CRL') }}\"><span class=\"fa fa-trash-o\"></span></button>";
+                        },
+                        "Name": function (column, row) {
+                            return "<span class=\"glyphicon glyphicon-certificate __iconspacer\"></span>" + row[column.id] + "<br>" + row["Purpose"];


fabianfrz · 2018-01-04T18:42:35Z

src/www/vpn_openvpn_client.php

@@ -882,7 +887,7 @@ function addRow() {
 <?php
              if (!isset($config['cert']) || count($config['cert']) == 0) :?>
                <b><?=gettext("No Certificates defined.");?></b> <br /><?=gettext("Create one under");?>
-                <a href="system_certmanager.php"><?=gettext("System: Certificates");?></a> <?=gettext("if one is required for this connection.");?>
+                <a href="/ui/trust/certificates/"><?=gettext("System: Certificates");?></a> <?=gettext("if one is required for this connection.");?>


Isn't that "System:Trust"? Needs probably more work to do because the string needs to be a template.

fabianfrz · 2018-01-04T18:43:31Z

src/www/vpn_openvpn_server.php

                            </option>
 <?php
                          endforeach; ?>
                          </select>
 <?php
                        else :?>
                          <b><?=gettext("No Certificate Authorities defined.");?></b>
-                          <br /><?=gettext("Create one under")?> <a href="system_camanager.php"> <?=gettext("System: Certificates");?></a>.
+                          <br /><?=gettext("Create one under")?> <a href="/ui/trust/authorities/"> <?=gettext("System: Certificates");?></a>.


System: Trust

fabianfrz · 2018-01-04T18:44:08Z

src/www/vpn_openvpn_server.php

                          </option>
 <?php
                        endforeach; ?>
                        </select>
 <?php
                      else :?>
                          <b><?=gettext("No Certificates defined.");?></b>
-                          <br /><?=gettext("Create one under");?> <a href="system_certmanager.php"><?=gettext("System: Certificates");?></a>.
+                          <br /><?=gettext("Create one under");?> <a href="/ui/trust/certificates/"><?=gettext("System: Certificates");?></a>.


System: Trust

fabianfrz · 2018-01-04T18:45:08Z

src/www/wizard.php

 						continue;
 					}
-					$name = htmlspecialchars($ca['descr']);
+					$name = htmlspecialchars($ca->descr->__toString());
 					$SELECTED = "";
 					if ($value == $name) $SELECTED = " selected=\"selected\"";


selected is probably bad because uppercase is usually understand as a constant

evbevz · 2018-01-05T07:06:55Z

@fabianfrz , how do you see the model? How can it be improved?

fabianfrz · 2018-01-05T11:00:04Z

@evbevz do as many validations in the XML or in Field types - New Field Types can be created so it is possibe to add a PEM field which includes the certificate or Base64 if it is encoded. If you look at Trust.xml it does not really contain validation. But something like that exists: https://github.com/opnsense/core/pull/2053/files#diff-29bb443b0cb80ae7ef701e4f4625dcb8R76

for example $keylens and $digest_algs can be options of an OptionField and displayed in a dropdown

AdSchellevis · 2018-01-06T09:48:11Z

@kekek2 The chances that this PR will make it into core won't be very large, please coordinate in the future about planned work and the effort you require from our team for bringing features is.

We really try to review a lot of code that is send in, we try to be reasonable with extensions that are manageable, but Smart-Soft can't expect our team to fund the amount of work which is needed for these kind of changes.

Hence my earlier note in #1899 about suggestions for work items that might be worth our time to review.

The quality and stability of the core components i very important for us, more issues cost more maintenance which we try to prevent. We have walked a long way and really need to keep the code quality to our standards.

cc @evbevz

kekek2 · 2018-01-08T11:04:10Z

@fabianfrz, the model does not contain "keylens" and "digest_algs". These data are used only to generate the CA or certificate. After that they are stored in encrypted and signed form in the certificate. The open form model to store them is not necessary. So for them there is no separate field. And therefore cannot be validated by means of model.

kekek2 · 2018-01-17T11:20:43Z

Plan for a phased transfer Trust/Certificates to MVC.

Create classes to work with CA and certificates:
class Cert { public function getCrt(); public function setCrt($crt); public function getPrv(); .... } class CA { .... }
Doing these classes worked with the current config Trust/Certificates.
One by one we change in different modules for direct calls to $config["cert"] the challenges of classes Cert/CA/CRL.
Do the migration and transition to a new model of Trust/Certificates.
One by one changing the page to work with certificates on the Controller/View

AdSchellevis · 2018-01-24T17:57:54Z

@kekek2 You probably better pick something smaller to convert to the mvc framework if you want to help out, for example some of the diagnostic pages are still old styled php.
Huge projects like this needs quite some investment to coordinate, test and provide feedback.

fabianfrz reviewed Jan 4, 2018

View reviewed changes

kekek2 added 7 commits January 6, 2018 07:20

Trust MVC from Smart-Soft

42b0bf7

Remove "certs.inc"

f741bf0

Forms: different case style

b1a068d

CSS property and escape data

970feef

Rename labels and titles.

75eb982

Variable uppercase.

9ae994c

Add <MinimumValue>0</MinimumValue> to model.

b164d57

Use CertificateField

e6ed3e3

Fix OpenVPN wizard.

76fd3e6

AdSchellevis closed this Apr 19, 2018

kekek2 deleted the Trust branch August 8, 2019 05:02

fichtner mentioned this pull request Feb 15, 2024

system: move Trust section to MVC/API #7248

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trust MVC from Smart-Soft #2053

Trust MVC from Smart-Soft #2053

kekek2 commented Jan 4, 2018

fabianfrz Jan 4, 2018

kekek2 Jan 5, 2018

fabianfrz left a comment

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

fabianfrz Jan 4, 2018

evbevz commented Jan 5, 2018 •

edited

fabianfrz commented Jan 5, 2018 •

edited

AdSchellevis commented Jan 6, 2018

kekek2 commented Jan 8, 2018

kekek2 commented Jan 17, 2018

AdSchellevis commented Jan 24, 2018

Trust MVC from Smart-Soft #2053

Trust MVC from Smart-Soft #2053

Conversation

kekek2 commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evbevz commented Jan 5, 2018 • edited

fabianfrz commented Jan 5, 2018 • edited

AdSchellevis commented Jan 6, 2018

kekek2 commented Jan 8, 2018

kekek2 commented Jan 17, 2018

AdSchellevis commented Jan 24, 2018

evbevz commented Jan 5, 2018 •

edited

fabianfrz commented Jan 5, 2018 •

edited