login: 2fa: T874: fix Google authenticator issues

Move default values of TOTP configuration from a global to a per user setting.
This makes the entire code easier as no global configuration must be blended
into the per user config dict.

Also it should be possible to set the authentication window "multiple concurrent
keys" individual per user.

set system login user vyos authentication otp key 'gzkmajid7na2oltajs4kbuq7lq'
set system login user vyos authentication plaintext-password 'vyos'

5 files changed, 80 insertions, 78 deletions

diff --git a/data/templates/login/pam_otp_ga.conf.j2 b/data/templates/login/pam_otp_ga.conf.j2
index 4c1f411d1..cf51ce089 100644
--- a/data/templates/login/pam_otp_ga.conf.j2
+++ b/data/templates/login/pam_otp_ga.conf.j2
@@ -1,5 +1,5 @@
 {% if authentication.otp.key is vyos_defined %}
-{{ authentication.otp.key }}
+{{ authentication.otp.key | upper }}
 " RATE_LIMIT {{ authentication.otp.rate_limit }} {{ authentication.otp.rate_time }}
 " WINDOW_SIZE {{ authentication.otp.window_size }}
 " DISALLOW_REUSE
diff --git a/debian/vyos-1x.postinst b/debian/vyos-1x.postinst
index 9766c91b1..031e91595 100644
--- a/debian/vyos-1x.postinst
+++ b/debian/vyos-1x.postinst
@@ -23,11 +23,11 @@ fi
 
 # Add 2FA support for SSH
 sudo grep -qF -- "auth required pam_google_authenticator.so nullok" "/etc/pam.d/sshd" || \
-sudo sed -i '/^@include common-auth/a # Check OTP 2FA, if configured for the user\nauth required pam_google_authenticator.so nullok' /etc/pam.d/sshd
+sudo sed -i '/^@include common-auth/a # Check OTP 2FA, if configured for the user\nauth       required     pam_google_authenticator.so nullok' /etc/pam.d/sshd
 
 # Add 2FA support for local authentication
 sudo grep -qF -- "auth required pam_google_authenticator.so nullok" "/etc/pam.d/login" || \
-sudo sed -i '/^@include common-auth/a # Check OTP 2FA, if configured for the user\nauth required pam_google_authenticator.so nullok' /etc/pam.d/login
+sudo sed -i '/^@include common-auth/a # Check OTP 2FA, if configured for the user\nauth       required     pam_google_authenticator.so nullok' /etc/pam.d/login
 
 # Add RADIUS operator user for RADIUS authenticated users to map to
 if ! grep -q '^radius_user' /etc/passwd; then
diff --git a/interface-definitions/system-login.xml.in b/interface-definitions/system-login.xml.in
index 7dd045e6c..def42544a 100644
--- a/interface-definitions/system-login.xml.in
+++ b/interface-definitions/system-login.xml.in
@@ -8,62 +8,6 @@
           <priority>400</priority>
         </properties>
         <children>
-          <node name="authentication">
-            <properties>
-              <help>Global authentication settings</help>
-            </properties>
-            <children>
-              <node name="otp">
-                <properties>
-                  <help>2FA OTP authentication parameters</help>
-                </properties>
-                <children>
-                  <leafNode name="rate-limit">
-                    <properties>
-                      <help>Number of attempts. Limit logins to N per every M seconds</help>
-                      <valueHelp>
-                        <format>u32:1-10</format>
-                        <description>Number of attempts. Limit logins to N per every M seconds</description>
-                      </valueHelp>
-                      <constraint>
-                        <validator name="numeric" argument="--range 1-10"/>
-                      </constraint>
-                      <constraintErrorMessage>Number of login attempts must me between 1 and 10</constraintErrorMessage>
-                    </properties>
-                    <defaultValue>3</defaultValue>
-                  </leafNode>
-                  <leafNode name="rate-time">
-                    <properties>
-                      <help>Time interval. Limit logins to N per every M seconds</help>
-                      <valueHelp>
-                        <format>u32:15-600</format>
-                        <description>Time interval. Limit logins to N per every M seconds</description>
-                      </valueHelp>
-                      <constraint>
-                        <validator name="numeric" argument="--range 15-600"/>
-                      </constraint>
-                      <constraintErrorMessage>Rate limit time interval must be between 15 and 600 seconds</constraintErrorMessage>
-                    </properties>
-                    <defaultValue>30</defaultValue>
-                  </leafNode>
-                  <leafNode name="window-size">
-                    <properties>
-                      <help>Set window of concurrently valid codes</help>
-                      <valueHelp>
-                        <format>u32:1-21</format>
-                        <description>Set window of concurrently valid codes</description>
-                      </valueHelp>
-                      <constraint>
-                        <validator name="numeric" argument="--range 1-21"/>
-                      </constraint>
-                      <constraintErrorMessage>Window of concurrently valid codes must be between 1 and 21</constraintErrorMessage>
-                    </properties>
-                    <defaultValue>3</defaultValue>
-                  </leafNode>
-                </children>
-              </node>
-            </children>
-          </node>
           <tagNode name="user">
             <properties>
               <help>Local user account information</help>
@@ -75,7 +19,7 @@
             <children>
               <node name="authentication">
                 <properties>
-                  <help>Password authentication</help>
+                  <help>Authentication settings</help>
                 </properties>
                 <children>
                   <leafNode name="encrypted-password">
@@ -94,18 +38,60 @@
                   </leafNode>
                   <node name="otp">
                     <properties>
-                      <help>2FA OTP authentication parameters</help>
+                      <help>One-Time-Pad (two-factor) authentication parameters</help>
                     </properties>
                     <children>
+                      <leafNode name="rate-limit">
+                        <properties>
+                          <help>Limit number of logins (rate-limit) per rate-time</help>
+                          <valueHelp>
+                            <format>u32:1-10</format>
+                            <description>Number of attempts</description>
+                          </valueHelp>
+                          <constraint>
+                            <validator name="numeric" argument="--range 1-10"/>
+                          </constraint>
+                          <constraintErrorMessage>Number of login attempts must me between 1 and 10</constraintErrorMessage>
+                        </properties>
+                        <defaultValue>3</defaultValue>
+                      </leafNode>
+                      <leafNode name="rate-time">
+                        <properties>
+                          <help>Limit number of logins (rate-limit) per rate-time</help>
+                          <valueHelp>
+                            <format>u32:15-600</format>
+                            <description>Time interval</description>
+                          </valueHelp>
+                          <constraint>
+                            <validator name="numeric" argument="--range 15-600"/>
+                          </constraint>
+                          <constraintErrorMessage>Rate limit time interval must be between 15 and 600 seconds</constraintErrorMessage>
+                        </properties>
+                        <defaultValue>30</defaultValue>
+                      </leafNode>
+                      <leafNode name="window-size">
+                        <properties>
+                          <help>Set window of concurrently valid codes</help>
+                          <valueHelp>
+                            <format>u32:1-21</format>
+                            <description>Window size</description>
+                          </valueHelp>
+                          <constraint>
+                            <validator name="numeric" argument="--range 1-21"/>
+                          </constraint>
+                          <constraintErrorMessage>Window of concurrently valid codes must be between 1 and 21</constraintErrorMessage>
+                        </properties>
+                        <defaultValue>3</defaultValue>
+                      </leafNode>
                       <leafNode name="key">
                         <properties>
-                          <help>Token Key Secret key for the token algorithm (see RFC 4226)</help>
+                          <help>Key/secret the token algorithm (see RFC4226)</help>
                           <valueHelp>
                             <format>txt</format>
-                            <description>OTP key (base32 encoded secret)</description>
+                            <description>Base32 encoded key/token</description>
                           </valueHelp>
                           <constraint>
-                            <regex>[a-zA-Z2-7]{20,10000}</regex>
+                            <regex>[a-zA-Z2-7]{26,10000}</regex>
                           </constraint>
                           <constraintErrorMessage>Key must only include base32 characters and be at least 26 characters long</constraintErrorMessage>
                         </properties>
diff --git a/smoketest/scripts/cli/test_system_login.py b/smoketest/scripts/cli/test_system_login.py
index a99721d66..6006fe0f6 100755
--- a/smoketest/scripts/cli/test_system_login.py
+++ b/smoketest/scripts/cli/test_system_login.py
@@ -46,6 +46,14 @@ TTSb0X1zPGxPIRFy5GoGtO9Mm5h4OZk=
 """
 
 class TestSystemLogin(VyOSUnitTestSHIM.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        super(TestSystemLogin, cls).setUpClass()
+
+        # ensure we can also run this test on a live system - so lets clean
+        # out the current configuration which will break this test
+        cls.cli_delete(cls, base_path + ['radius'])
+
     def tearDown(self):
         # Delete individual users from configuration
         for user in users:
diff --git a/src/conf_mode/system-login.py b/src/conf_mode/system-login.py
index bd9cc3b89..eea6c7413 100755
--- a/src/conf_mode/system-login.py
+++ b/src/conf_mode/system-login.py
@@ -64,6 +64,18 @@ def get_config(config=None):
     login = conf.get_config_dict(base, key_mangling=('-', '_'),
                                  no_tag_node_value_mangle=True, get_first_key=True)
 
+    # We have gathered the dict representation of the CLI, but there are default
+    # options which we need to update into the dictionary retrived.
+    default_values = defaults(base)
+    # XXX: T2665: we can not safely rely on the defaults() when there are
+    # tagNodes in place, it is better to blend in the defaults manually.
+    # defaults for RADIUS and USER will be added later down this function
+    if 'radius' in default_values:
+        del default_values['radius']
+    if 'user' in default_values:
+        del default_values['user']
+    login = dict_merge(default_values, login)
+
     # users no longer existing in the running configuration need to be deleted
     local_users = get_local_users()
     cli_users = []
@@ -253,23 +265,19 @@ def apply(login):
                        user_config, permission=0o600,
                        formater=lambda _: _.replace("&quot;", '"'),
                        user=user, group='users')
-                #OTP 2FA key file generation
-                if dict_search('authentication.otp.key', user_config):
-                    user_config['authentication']['otp']['key'] = user_config['authentication']['otp']['key'].upper()
-                    user_config['authentication']['otp']['rate_limit'] = login['authentication']['otp']['rate_limit']
-                    user_config['authentication']['otp']['rate_time'] = login['authentication']['otp']['rate_time']
-                    user_config['authentication']['otp']['window_size'] = login['authentication']['otp']['window_size']
-                    render(f'{home_dir}/.google_authenticator', 'login/pam_otp_ga.conf.j2',
-                           user_config, permission=0o600,
-                           formater=lambda _: _.replace("&quot;", '"'),
-                           user=user, group='users')
-                #OTP 2FA key file deletion
-                elif os.path.exists(f'{home_dir}/.google_authenticator'):
-                    os.remove(f'{home_dir}/.google_authenticator')
-		
+
             except Exception as e:
                 raise ConfigError(f'Adding user "{user}" raised exception: "{e}"')
 
+            # Generate 2FA/MFA One-Time-Pad configuration
+            if dict_search('authentication.otp.key', user_config):
+                render(f'{home_dir}/.google_authenticator', 'login/pam_otp_ga.conf.j2',
+                       user_config, permission=0o400, user=user, group='users')
+            else:
+                # delete configuration as it's not enabled for the user
+                if os.path.exists(f'{home_dir}/.google_authenticator'):
+                    os.remove(f'{home_dir}/.google_authenticator')
+
     if 'rm_users' in login:
         for user in login['rm_users']:
             try: