Support hmac_secret rotation in the otp feature

This will allow OTP authentication if the OTP key was created
with hmac_old_secret.  However, since it cannot update the OTP
secret on the device, it calls the otp_valid_code_for_old_secret
configuration method, and the user can then record whatever
information is needed, and use it to inform the user that they
need to rotate their OTP.

This explicitly does not handle creating a new OTP key if the
hmac_secret has changed between when the setup form was displayed
and when it was submitted.  The OTP would then need to be rotated,
and it's best to avoid that.  The user will get an invalid key
error and can submit again to use the new hmac_secret.

CHANGELOG

@@ -1,18 +1,20 @@
 === master
 
-* Support hmac_secret rotation in email_base feature (jeremyevans) (#365)
+* Support hmac_secret rotation in the otp feature (jeremyevans) (#365)
 
-* Support hmac_secret rotation in webauthn feature (jeremyevans) (#365)
+* Support hmac_secret rotation in the email_base feature (jeremyevans) (#365)
 
-* Support hmac_secret rotation in jwt_refresh feature (jeremyevans) (#365)
+* Support hmac_secret rotation in the webauthn feature (jeremyevans) (#365)
 
-* Support hmac_secret rotation in single_session feature (jeremyevans) (#365)
+* Support hmac_secret rotation in the jwt_refresh feature (jeremyevans) (#365)
 
-* Support hmac_secret rotation in remember feature (jeremyevans) (#365)
+* Support hmac_secret rotation in the single_session feature (jeremyevans) (#365)
 
-* Support hmac_secret rotation via hmac_old_secret configuration method in active_sessions feature (jeremyevans) (#365)
+* Support hmac_secret rotation in the remember feature (jeremyevans) (#365)
 
-* Support argon2 secret rotation via argon2_old_secret configuration method and update_password_hash feature (jeremyevans) (#365)
+* Support hmac_secret rotation via hmac_old_secret configuration method in the active_sessions feature (jeremyevans) (#365)
+
+* Support argon2 secret rotation via argon2_old_secret configuration method and the update_password_hash feature (jeremyevans) (#365)
 
 * Support jwt secret rotation via jwt_old_secret configuration method, if using jwt 2.4+ (jeremyevans) (#365)
 

doc/otp.rdoc

@@ -86,5 +86,6 @@ otp_remove_auth_failures :: Removes OTP authentication failures for the current
 otp_setup_view :: The HTML to use for the form to setup OTP authentication.
 otp_tmp_key(secret) :: Set the secret to use for the temporary OTP key, during OTP setup.
 otp_update_last_use :: Update the last time OTP authentication was successful for the account.  Return true if the authentication should be allowed, or false if it should not be allowed because the last authentication was too recent and indicates the possible reuse of a TOTP authentication code.
+otp_valid_code_for_old_secret :: Called when valid OTP authentication is performed using hmac_old_secret.  This indicates the OTP needs to be rotated before support for the previous hmac secret value is removed.  You can use this to track users who need their OTP rotated, and take appropriate action.
 otp_valid_code?(auth_code) :: Whether the given code is the currently valid OTP auth code for the account.
 otp_valid_key?(secret) :: Whether the given secret is a valid OTP secret.

lib/rodauth/features/otp.rb

@@ -94,7 +94,8 @@ module Rodauth
 
     auth_private_methods(
       :otp_add_key,
-      :otp_tmp_key
+      :otp_tmp_key,
+      :otp_valid_code_for_old_secret
     )
 
     internal_request_method :otp_setup_params
@@ -248,6 +249,17 @@ def otp_exists?
     end
 
     def otp_valid_code?(ot_pass)
+      if _otp_valid_code?(ot_pass, otp)
+        true
+      elsif hmac_secret_rotation? && _otp_valid_code?(ot_pass, _otp_for_key(otp_hmac_old_secret(otp_key)))
+        _otp_valid_code_for_old_secret
+        true
+      else
+        false
+      end
+    end
+
+    def _otp_valid_code?(ot_pass, otp)
       return false unless otp_exists?
       ot_pass = ot_pass.gsub(/\s+/, '')
       if drift = otp_drift
@@ -368,9 +380,17 @@ def otp_hmac_secret(key)
       base32_encode(compute_raw_hmac(ROTP::Base32.decode(key)), key.bytesize)
     end
 
+    def otp_hmac_old_secret(key)
+      base32_encode(compute_raw_hmac_with_secret(ROTP::Base32.decode(key), hmac_old_secret), key.bytesize)
+    end
+
     def otp_valid_key?(secret)
       return false unless secret =~ /\A([a-z2-7]{16}|[a-z2-7]{32})\z/
       if otp_keys_use_hmac?
+        # Purposely do not allow creating new OTPs with old secrets,
+        # since OTP rotation is difficult.  The user will get shown
+        # the same page with an updated secret, which they can submit
+        # to setup OTP.
         timing_safe_eql?(otp_hmac_secret(param(otp_setup_raw_param)), secret)
       else
         true
@@ -400,6 +420,10 @@ def _otp_tmp_key(secret)
       @otp_key = secret
     end
 
+    # Called for valid OTP codes for old secrets
+    def _otp_valid_code_for_old_secret
+    end
+
     def _otp_add_key(secret)
       # Uniqueness errors can't be handled here, as we can't be sure the secret provided
       # is the same as the current secret.
@@ -411,8 +435,12 @@ def _otp_key
       otp_key_ds.get(otp_keys_column)
     end
 
+    def _otp_for_key(key)
+      otp_class.new(key, :issuer=>otp_issuer, :digits=>otp_digits, :interval=>otp_interval)
+    end
+
     def _otp
-      otp_class.new(otp_user_key, :issuer=>otp_issuer, :digits=>otp_digits, :interval=>otp_interval)
+      _otp_for_key(otp_user_key)
     end
 
     def otp_key_ds

spec/two_factor_spec.rb

@@ -12,11 +12,21 @@ def reset_otp_last_use
   it "should allow two factor authentication setup, login, recovery, removal" do
     sms_phone = sms_message = nil
     hmac_secret = '123'
+    hmac_old_secret = nil
+    old_secret_used = false
     rodauth do
       enable :login, :logout, :otp, :recovery_codes, :sms_codes
       hmac_secret do
         hmac_secret
       end
+      hmac_old_secret do
+        hmac_old_secret
+      end
+      otp_valid_code_for_old_secret do
+        raise if old_secret_used
+        old_secret_used = true
+      end
+
       sms_send do |phone, msg|
         proc{super(phone, msg)}.must_raise NotImplementedError
         sms_phone = phone
@@ -115,7 +125,31 @@ def reset_otp_last_use
     page.html.must_include 'Invalid authentication code'
     reset_otp_last_use
 
+    hmac_secret = '124'
+    hmac_old_secret = '125'
+    fill_in 'Authentication Code', :with=>"#{totp.now[0..2]} #{totp.now[3..-1]}"
+    click_button 'Authenticate Using TOTP'
+    page.find('#error_flash').text.must_equal 'Error logging in via TOTP authentication'
+    page.html.must_include 'Invalid authentication code'
+    reset_otp_last_use
+
+    otp_auth_path = page.current_path
+    visit otp_auth_path
+    old_secret_used.must_equal false
+    hmac_secret = '333'
+    hmac_old_secret = '321'
+    fill_in 'Authentication Code', :with=>"#{totp.now[0..2]} #{totp.now[3..-1]}"
+    click_button 'Authenticate Using TOTP'
+    page.find('#notice_flash').text.must_equal 'You have been multifactor authenticated'
+    page.html.must_include 'With 2FA'
+    old_secret_used.must_equal true
+    reset_otp_last_use
+
+    logout
+    login
+    visit otp_auth_path
     hmac_secret = '321'
+    hmac_old_secret = nil
     fill_in 'Authentication Code', :with=>"#{totp.now[0..2]} #{totp.now[3..-1]}"
     click_button 'Authenticate Using TOTP'
     page.find('#notice_flash').text.must_equal 'You have been multifactor authenticated'