do OTP remember me cookie per IP

test plan:
 * configure MFA for a user
 * log in to canvas, and remember me on the MFA verification page
 * log out and log back in; you should not need to enter your MFA
 * log out, and switch internet connections to a different public
   IP (using tethering would be great for this)
 * log in, and you should have to enter your MFA
 * log out and log back in; you should not need to enter your MFA
 * log out and switch back to your original internet connection
 * log in, and you should not have to enter your MFA

Change-Id: Id682ecdabcae8c520ff43f6c90387f78382f5755
Reviewed-on: https://gerrit.instructure.com/25889
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Reviewed-by: Jacob Fugal <jacob@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

app/controllers/pseudonym_sessions_controller.rb

@@ -524,8 +524,10 @@ class PseudonymSessionsController < ApplicationController
 
       if params[:otp_login][:remember_me] == '1'
         now = Time.now.utc
+        old_cookie = cookies['canvas_otp_remember_me']
+        old_cookie = nil unless @current_user.validate_otp_secret_key_remember_me_cookie(old_cookie)
         cookies['canvas_otp_remember_me'] = {
-              :value => @current_user.otp_secret_key_remember_me_cookie(now),
+              :value => @current_user.otp_secret_key_remember_me_cookie(now, old_cookie, request.remote_ip),
               :expires => now + 30.days,
               :domain => otp_remember_me_cookie_domain,
               :httponly => true,
@@ -566,8 +568,7 @@ class PseudonymSessionsController < ApplicationController
     @current_pseudonym = pseudonym
     Auditors::Authentication.record(@current_pseudonym, 'login')
 
-    otp_passed ||= cookies['canvas_otp_remember_me'] &&
-        @current_user.validate_otp_secret_key_remember_me_cookie(cookies['canvas_otp_remember_me'])
+    otp_passed ||= @current_user.validate_otp_secret_key_remember_me_cookie(cookies['canvas_otp_remember_me'], request.remote_ip)
     if !otp_passed
       mfa_settings = @current_user.mfa_settings
       if (@current_user.otp_secret_key && mfa_settings == :optional) ||

app/models/user.rb

@@ -2323,14 +2323,24 @@ class User < ActiveRecord::Base
     orig_profile(force_reload) || build_profile
   end
 
-  def otp_secret_key_remember_me_cookie(time)
-    "#{time.to_i}.#{Canvas::Security.hmac_sha1("#{time.to_i}.#{self.otp_secret_key}")}"
+  def parse_otp_remember_me_cookie(cookie)
+    return 0, [], nil unless cookie
+    time, *ips, hmac = cookie.split('-')
+    [time, ips, hmac]
   end
 
-  def validate_otp_secret_key_remember_me_cookie(value)
-    value =~ /^(\d+)\.[0-9a-f]+/ &&
-        $1.to_i >= (Time.now.utc - 30.days).to_i &&
-        value == otp_secret_key_remember_me_cookie($1)
+  def otp_secret_key_remember_me_cookie(time, current_cookie, remote_ip = nil)
+    _, ips, _ = parse_otp_remember_me_cookie(current_cookie)
+    cookie = [time.to_i, *[*ips, remote_ip].compact.sort].join('-')
+
+    "#{cookie}-#{Canvas::Security.hmac_sha1("#{cookie}.#{self.otp_secret_key}")}"
+  end
+
+  def validate_otp_secret_key_remember_me_cookie(value, remote_ip = nil)
+    time, ips, hmac = parse_otp_remember_me_cookie(value)
+    time.to_i >= (Time.now.utc - 30.days).to_i &&
+        (remote_ip.nil? || ips.include?(remote_ip)) &&
+        value == otp_secret_key_remember_me_cookie(time, value)
   end
 
   def otp_secret_key

spec/controllers/pseudonym_sessions_controller_spec.rb

@@ -762,10 +762,12 @@ describe PseudonymSessionsController do
       user_with_pseudonym(:active_all => 1, :password => 'qwerty')
       @user.otp_secret_key = ROTP::Base32.random_base32
       @user.save!
+
+      ActionController::TestRequest.any_instance.stubs(:remote_ip).returns('myip')
     end
 
     it "should skip otp verification for a valid cookie" do
-      cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(Time.now.utc)
+      cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(Time.now.utc, nil, 'myip')
       post 'create', :pseudonym_session => { :unique_id => @pseudonym.unique_id, :password => 'qwerty' }
       response.should redirect_to dashboard_url(:login_success => 1)
     end
@@ -777,13 +779,13 @@ describe PseudonymSessionsController do
     end
 
     it "should ignore an expired cookie" do
-      cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(6.months.ago)
+      cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(6.months.ago, nil, 'myip')
       post 'create', :pseudonym_session => { :unique_id => @pseudonym.unique_id, :password => 'qwerty' }
       response.should render_template('otp_login')
     end
 
     it "should ignore a cookie from an old secret_key" do
-      cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(6.months.ago)
+      cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(6.months.ago, nil, 'myip')
 
       @user.otp_secret_key = ROTP::Base32.random_base32
       @user.save!
@@ -791,6 +793,12 @@ describe PseudonymSessionsController do
       post 'create', :pseudonym_session => { :unique_id => @pseudonym.unique_id, :password => 'qwerty' }
       response.should render_template('otp_login')
     end
+
+    it "should ignore a cookie for a different IP" do
+      cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(Time.now.utc, nil, 'otherip')
+      post 'create', :pseudonym_session => { :unique_id => @pseudonym.unique_id, :password => 'qwerty' }
+      response.should render_template('otp_login')
+    end
   end
 
   describe 'create' do
@@ -938,6 +946,16 @@ describe PseudonymSessionsController do
         cookies['canvas_otp_remember_me'].should_not be_nil
       end
 
+      it "should add the current ip to existing ips" do
+        cookies['canvas_otp_remember_me'] = @user.otp_secret_key_remember_me_cookie(Time.now.utc, nil, 'ip1')
+        ActionController::Request.any_instance.stubs(:remote_ip).returns('ip2')
+        post 'otp_login', :otp_login => { :verification_code => ROTP::TOTP.new(@user.otp_secret_key).now, :remember_me => '1' }
+        response.should redirect_to dashboard_url(:login_success => 1)
+        cookies['canvas_otp_remember_me'].should_not be_nil
+        _, ips, _ = @user.parse_otp_remember_me_cookie(cookies['canvas_otp_remember_me'])
+        ips.sort.should == ['ip1', 'ip2']
+      end
+
       it "should fail for an incorrect token" do
         post 'otp_login', :otp_login => { :verification_code => '123456' }
         response.should render_template('otp_login')

spec/models/user_spec.rb

@@ -2521,4 +2521,20 @@ describe User do
       test_student.reload.enrollments.each { |e| e.should be_deleted }
     end
   end
+
+  describe "otp remember me cookie" do
+    before do
+      @user = User.new
+      @user.otp_secret_key = ROTP::Base32.random_base32
+    end
+
+    it "should add an ip to an existing cookie" do
+      cookie1 = @user.otp_secret_key_remember_me_cookie(Time.now.utc, nil, 'ip1')
+      cookie2 = @user.otp_secret_key_remember_me_cookie(Time.now.utc, cookie1, 'ip2')
+      @user.validate_otp_secret_key_remember_me_cookie(cookie1, 'ip1').should be_true
+      @user.validate_otp_secret_key_remember_me_cookie(cookie1, 'ip2').should be_false
+      @user.validate_otp_secret_key_remember_me_cookie(cookie2, 'ip1').should be_true
+      @user.validate_otp_secret_key_remember_me_cookie(cookie2, 'ip2').should be_true
+    end
+  end
 end