sstp: T2110: use uniform RADIUS CLI syntax

- migrate RADIUS configuration to a more uniform syntax accross the system
  - authentication radius-server x.x.x.x to authentication radius server x.x.x.x
  - authentication radius-settings to authentication radius

3 files changed, 150 insertions, 105 deletions

diff --git a/interface-definitions/vpn-sstp.xml.in b/interface-definitions/vpn-sstp.xml.in
index e2d6aa75e..1508c3313 100644
--- a/interface-definitions/vpn-sstp.xml.in
+++ b/interface-definitions/vpn-sstp.xml.in
@@ -113,37 +113,23 @@
                     <multi />
                 </properties>
               </leafNode>
-              <tagNode name="radius-server">
-                <properties>
-                  <help>IP address of RADIUS server</help>
-                  <valueHelp>
-                    <format>ipv4</format>
-                      <description>IP address of RADIUS server</description>
-                  </valueHelp>
-                </properties>
-                <children>
-                  <leafNode name="secret">
-                    <properties>
-                      <help>Key for accessing the specified server</help>
-                    </properties>
-                  </leafNode>
-                  <leafNode name="req-limit">
-                    <properties>
-                      <help>Maximum number of simultaneous requests to server (default: unlimited)</help>
-                    </properties>
-                  </leafNode>
-                  <leafNode name="fail-time">
-                    <properties>
-                      <help>If server does not responds mark it as unavailable for this time (seconds)</help>
-                    </properties>
-                  </leafNode>
-                </children>
-              </tagNode>
-              <node name="radius-settings">
-                <properties>
-                  <help>RADIUS settings</help>
-                </properties>
+              #include <include/radius-server.xml.i>
+              <node name="radius">
                 <children>
+                  <tagNode name="server">
+                    <children>
+                      <leafNode name="req-limit">
+                        <properties>
+                          <help>Maximum number of simultaneous requests to server (default: unlimited)</help>
+                        </properties>
+                      </leafNode>
+                      <leafNode name="fail-time">
+                        <properties>
+                          <help>If server does not responds mark it as unavailable for this time (seconds)</help>
+                        </properties>
+                      </leafNode>
+                    </children>
+                  </tagNode>
                   <leafNode name="timeout">
                     <properties>
                       <help>Timeout to wait response from server (seconds)</help>
@@ -151,22 +137,22 @@
                   </leafNode>
                   <leafNode name="acct-timeout">
                     <properties>
-                      <help>Timeout to wait reply for Interim-Update packets. (default 3 seconds)</help>
+                      <help>Timeout for Interim-Update packets (default 3 seconds)</help>
                     </properties>
                   </leafNode>
                   <leafNode name="max-try">
                     <properties>
-                      <help>Maximum number of tries to send Access-Request/Accounting-Request queries</help>
+                      <help>Maximum tries for Access-Request/Accounting-Request queries</help>
                     </properties>
                   </leafNode>
                   <leafNode name="nas-identifier">
                     <properties>
-                      <help>Value to send to RADIUS server in NAS-Identifier attribute and to be matched in DM/CoA requests.</help>
+                      <help>NAS-Identifier attribute sent to RADIUS</help>
                     </properties>
                   </leafNode>
                   <leafNode name="nas-ip-address">
                     <properties>
-                      <help>Value to send to RADIUS server in NAS-IP-Address attribute and to be matched in DM/CoA requests. Also DM/CoA server will bind to that address.</help>
+                      <help>NAS-IP-Address attribute sent to RADIUS</help>
                        <constraint>
                         <validator name="ipv4-address"/>
                       </constraint>
@@ -175,14 +161,14 @@
                         <format>ipv4</format>
                         <description>NAS-IP-Address Attribute Value</description>
                       </valueHelp>
-                    </properties>
-                  </leafNode>
-                  <node name="dae-server">
+                     </properties>
+                   </leafNode>
+                  <node name="dynamic-author">
                     <properties>
-                      <help>IPv4 address and port to bind Dynamic Authorization Extension server (DM/CoA)</help>
+                      <help>Dynamic Authorization Extension/Change of Authorization server</help>
                     </properties>
                     <children>
-                      <leafNode name="ip-address">
+                      <leafNode name="server">
                         <properties>
                           <help>IP address for Dynamic Authorization Extension server (DM/CoA)</help>
                           <constraint>
@@ -207,7 +193,7 @@
                           </constraint>
                         </properties>
                       </leafNode>
-                      <leafNode name="secret">
+                      <leafNode name="key">
                         <properties>
                           <help>Secret for Dynamic Authorization Extension server (DM/CoA)</help>
                         </properties>
@@ -221,17 +207,17 @@
                     <children>
                       <leafNode name="attribute">
                         <properties>
-                          <help>Specifies which radius attribute contains rate information. (default is Filter-Id)</help>
+                          <help>Specifies RADIUS attribute containing rate information (default 'Filter-Id')</help>
                         </properties>
                       </leafNode>
                       <leafNode name="vendor">
                         <properties>
-                          <help>Specifies the vendor dictionary. (dictionary needs to be in /usr/share/accel-ppp/radius)</help>
+                          <help>Specifies vendor dictionary (needs to be in /usr/share/accel-ppp/radius)</help>
                         </properties>
                       </leafNode>
                       <leafNode name="enable">
                         <properties>
-                          <help>Enables Bandwidth shaping via RADIUS</help>
+                          <help>Enable RADIUS bandwidth shaping</help>
                           <valueless />
                         </properties>
                       </leafNode>
diff --git a/src/conf_mode/vpn_sstp.py b/src/conf_mode/vpn_sstp.py
index 362eeddbb..e8c5155dd 100755
--- a/src/conf_mode/vpn_sstp.py
+++ b/src/conf_mode/vpn_sstp.py
@@ -100,27 +100,26 @@ chap-secrets=/etc/accel-ppp/sstp/chap-secrets
 [radius]
 verbose=1
 {% for r in radius_server %}
-server={{ r.server }},{{ r.secret }},req-limit={{ r.req_limit }},fail-time={{ r.fail_time }}
+server={{ r.server }},{{ r.key }},auth-port={{ r.port }},req-limit={{ r.req_limit }},fail-time={{ r.fail_time }}
 {% endfor -%}
 
-{% if radius_acct_tmo  %}
 acct-timeout={{ radius_acct_tmo }}
-{% endif -%}
-{% if radius_timeout %}
 timeout={{ radius_timeout }}
-{% endif -%}
-{% if rad_max_try %}
-max-try={{ rad_max_try }}
-{% endif -%}
+max-try={{ radius_max_try }}
+
 {% if radius_nas_id %}
 nas-identifier={{ radius_nas_id }}
 {% endif -%}
 {% if radius_nas_ip %}
 nas-ip-address={{ radius_nas_ip }}
 {% endif -%}
+{% if radius_source_address %}
+bind={{ radius_source_address }}
+{% endif -%}
+
 
-{% if radius_dae %}
-dae-server={{ radius_dae.server }}:{{ radius_dae.port }},{{ radius_dae.secret }}
+{% if radius_dynamic_author %}
+dae-server={{ radius_dynamic_author.server }}:{{ radius_dynamic_author.port }},{{ radius_dynamic_author.key }}
 {% endif -%}
 {% endif %}
 
@@ -207,14 +206,15 @@ default_config_data = {
     'auth_mode' : 'local',
     'auth_proto' : [],
     'radius_server' : [],
-    'radius_acct_tmo' : '',
-    'radius_max_try' : '',
-    'radius_timeout' : '',
+    'radius_acct_tmo' : '3',
+    'radius_max_try' : '3',
+    'radius_timeout' : '3',
     'radius_nas_id' : '',
     'radius_nas_ip' : '',
+    'radius_source_address' : '',
     'radius_shaper_attr' : '',
     'radius_shaper_vendor': '',
-    'radius_dae' : {},
+    'radius_dynamic_author' : '',
     'ssl_ca' : '',
     'ssl_cert' : '',
     'ssl_key' : '',
@@ -279,76 +279,84 @@ def get_config():
 
     #
     # RADIUS auth and settings
-    conf.set_level(base_path)
-    if conf.exists(['authentication', 'radius-server']):
-        for server in conf.list_nodes(['authentication', 'radius-server']):
+    conf.set_level(base_path + ['authentication', 'radius'])
+    if conf.exists(['server']):
+        for server in conf.list_nodes(['server']):
             radius = {
                 'server' : server,
-                'secret' : '',
+                'key' : '',
                 'fail_time' : 0,
+                'port' : '1812',
                 'req_limit' : 0
             }
 
-            conf.set_level(base_path + ['authentication', 'radius-server', server])
-
-            if conf.exists(['secret']):
-                radius['secret'] = conf.return_value(['secret'])
+            conf.set_level(base_path + ['authentication', 'radius', 'server', server])
 
             if conf.exists(['fail-time']):
                 radius['fail-time'] = conf.return_value(['fail-time'])
 
+            if conf.exists(['port']):
+                radius['port'] = conf.return_value(['port'])
+
             if conf.exists(['req-limit']):
                 radius['req_limit'] = conf.return_value(['req-limit'])
 
-            sstp['radius_server'].append(radius)
+            if conf.exists(['key']):
+                radius['key'] = conf.return_value(['key'])
+
+            if not conf.exists(['disable']):
+                sstp['radius_server'].append(radius)
 
+        #
         # advanced radius-setting
-        conf.set_level(base_path + ['authentication', 'radius-settings'])
-        if conf.exists([]):
-            if conf.exists(['acct-timeout']):
-                sstp['radius_acct_tmo'] = conf.return_value(['acct-timeout'])
+        conf.set_level(base_path + ['authentication', 'radius'])
 
-            if conf.exists(['max-try']):
-                sstp['radius_max_try'] = conf.return_value(['max-try'])
+        if conf.exists(['acct-timeout']):
+            sstp['radius_acct_tmo'] = conf.return_value(['acct-timeout'])
 
-            if conf.exists(['timeout']):
-                sstp['radius_timeout'] = conf.return_value(['timeout'])
+        if conf.exists(['max-try']):
+            sstp['radius_max_try'] = conf.return_value(['max-try'])
 
-            if conf.exists(['nas-identifier']):
-                sstp['radius_nas_id'] = conf.return_value(['nas-identifier'])
+        if conf.exists(['timeout']):
+            sstp['radius_timeout'] = conf.return_value(['timeout'])
 
-            if conf.exists(['nas-ip-address']):
-                sstp['radius_nas_ip'] = conf.return_value(['nas-ip-address'])
+        if conf.exists(['nas-identifier']):
+            sstp['radius_nas_id'] = conf.return_value(['nas-identifier'])
 
-            # Dynamic Authorization Extensions (DOA)/
-            # Change Of Authentication (COA)
-            if conf.exists(['dae-server']):
-                dae = {
-                    'port' : '',
-                    'server' : '',
-                    'secret' : ''
-                }
+        if conf.exists(['nas-ip-address']):
+            sstp['radius_nas_ip'] = conf.return_value(['nas-ip-address'])
 
-                if conf.exists(['ip-address']):
-                    dae['server'] = conf.return_value(['ip-address'])
+        if conf.exists(['source-address']):
+            sstp['radius_source_address'] = conf.return_value(['source-address'])
+
+        # Dynamic Authorization Extensions (DOA)/Change Of Authentication (COA)
+        if conf.exists(['dynamic-author']):
+            dae = {
+                'port' : '',
+                'server' : '',
+                'key' : ''
+            }
 
-                if conf.exists(['port']):
-                    dae['port'] = conf.return_value(['port'])
+            if conf.exists(['dynamic-author', 'server']):
+                dae['server'] = conf.return_value(['dynamic-author', 'server'])
 
-                if conf.exists(['secret']):
-                    dae['secret'] = conf.return_value(['secret'])
+            if conf.exists(['dynamic-author', 'port']):
+                dae['port'] = conf.return_value(['dynamic-author', 'port'])
 
-                sstp['radius_dae'] = dae
+            if conf.exists(['dynamic-author', 'key']):
+                dae['key'] = conf.return_value(['dynamic-author', 'key'])
 
-            if conf.exists(['rate-limit', 'enable']):
-                sstp['radius_shaper_attr'] = 'Filter-Id'
-                c_attr = ['rate-limit', 'enable', 'attribute']
-                if conf.exists(c_attr):
-                    sstp['radius_shaper_attr'] = conf.return_value(c_attr)
+            sstp['radius_dynamic_author'] = dae
 
-                c_vendor = ['rate-limit', 'enable', 'vendor']
-                if conf.exists(c_vendor):
-                    sstp['radius_shaper_vendor'] = conf.return_value(c_vendor)
+        if conf.exists(['rate-limit', 'enable']):
+            sstp['radius_shaper_attr'] = 'Filter-Id'
+            c_attr = ['rate-limit', 'enable', 'attribute']
+            if conf.exists(c_attr):
+                sstp['radius_shaper_attr'] = conf.return_value(c_attr)
+
+            c_vendor = ['rate-limit', 'enable', 'vendor']
+            if conf.exists(c_vendor):
+                sstp['radius_shaper_vendor'] = conf.return_value(c_vendor)
 
     #
     # authentication protocols
@@ -466,8 +474,8 @@ def verify(sstp):
             raise ConfigError('RADIUS authentication requires at least one server')
 
         for radius in sstp['radius_server']:
-            if not radius['secret']:
-                raise ConfigError(f"Missing RADIUS secret for server {{ radius['server'] }}")
+            if not radius['key']:
+                raise ConfigError(f"Missing RADIUS secret for server {{ radius['key'] }}")
 
 def generate(sstp):
     if sstp is None:
@@ -486,6 +494,9 @@ def generate(sstp):
             f.write(config_text)
 
         os.chmod(chap_secrets, S_IRUSR | S_IWUSR | S_IRGRP )
+    else:
+        if os.path.exists(chap_secrets):
+             os.unlink(chap_secrets)
 
     return sstp
 
@@ -526,6 +537,7 @@ def apply(sstp):
     else:
         accel_cmd('restart')
 
+
 if __name__ == '__main__':
     try:
         c = get_config()
diff --git a/src/migration-scripts/sstp/0-to-1 b/src/migration-scripts/sstp/0-to-1
index 88d3b4fb4..652a2662f 100755
--- a/src/migration-scripts/sstp/0-to-1
+++ b/src/migration-scripts/sstp/0-to-1
@@ -14,7 +14,12 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+
 # - migrate from "service sstp-server" to "vpn sstp"
+# - remove primary/secondary identifier from nameserver
+# - migrate RADIUS configuration to a more uniform syntax accross the system
+#   - authentication radius-server x.x.x.x to authentication radius server x.x.x.x
+#   - authentication radius-settings to authentication radius
 
 import os
 import sys
@@ -58,8 +63,50 @@ else:
 
         config.delete(dns_base)
 
-    print(config.to_string())
-    sys.exit(1)
+
+    # migrate radius options - copy subtree
+    # thus must happen before migration of the individual RADIUS servers
+    old_options = new_base + ['authentication', 'radius-settings']
+    new_options = new_base + ['authentication', 'radius']
+    config.copy(old_options, new_options)
+    config.delete(old_options)
+
+
+    # migrate radius dynamic author / change of authorisation server
+    dae_old = new_base + ['authentication', 'radius', 'dae-server']
+    if config.exists(dae_old):
+        config.rename(dae_old, 'dynamic-author')
+        dae_new = new_base + ['authentication', 'radius', 'dynamic-author']
+
+        if config.exists(dae_new + ['ip-address']):
+            config.rename(dae_new + ['ip-address'], 'server')
+
+        if config.exists(dae_new + ['secret']):
+            config.rename(dae_new + ['secret'], 'key')
+
+
+    # migrate radius server
+    radius_server = new_base + ['authentication', 'radius-server']
+    if config.exists(radius_server):
+        for server in config.list_nodes(radius_server):
+            base = radius_server + [server]
+            new = new_base + ['authentication', 'radius', 'server', server]
+
+            # convert secret to key
+            if config.exists(base + ['secret']):
+                tmp = config.return_value(base + ['secret'])
+                config.set(new + ['key'], value=tmp)
+
+            if config.exists(base + ['fail-time']):
+                tmp = config.return_value(base + ['fail-time'])
+                config.set(new + ['fail-time'], value=tmp)
+
+            if config.exists(base + ['req-limit']):
+                tmp = config.return_value(base + ['req-limit'])
+                config.set(new + ['req-limit'], value=tmp)
+
+        config.set_tag(new_base + ['authentication', 'radius', 'server'])
+        config.delete(radius_server)
 
     try:
         with open(file_name, 'w') as f: