Add password complexity enforcement via zxcvbn

Gemfile

@@ -78,6 +78,7 @@ gem "sprockets",                        "~>3.7.2",           :require => false
 gem "sync",                             "~>0.5",             :require => false
 gem "terminal",                                              :require => false
 gem "wim_parser",                       "~>1.0",             :require => false
+gem "zxcvbn-ruby",                      "~>2.0",             :require => false
 
 # gems to resolve security issues
 gem "cgi",  "~> 0.5"

app/models/user.rb

@@ -49,11 +49,16 @@ class User < ApplicationRecord
                                 :allow_nil => true, :message => "must be a valid email address"},
                     :length => {:maximum => 255}
   validates :current_group, :inclusion => {:in => proc { |u| u.miq_groups }, :allow_nil => true, :if => :current_group_id_changed?}
+  validate  :password_complexity, :if => :password
 
   # use authenticate_bcrypt rather than .authenticate to avoid confusion
   # with the class method of the same name (User.authenticate)
   alias_method :authenticate_bcrypt, :authenticate
 
+  # Allows callers (e.g. seed) to bypass password complexity checks for a
+  # single save by setting this transient flag before saving.
+  attr_writer :skip_complexity_check
+
   serialize     :settings, :type => Hash   # Implement settings column as a hash
   default_value_for(:settings) { {} }
 
@@ -178,6 +183,7 @@ def nil_email_field_if_blank
   def dummy_password_for_external_auth
     if password.blank? && password_digest.blank? &&
        !self.class.authenticator(userid).uses_stored_password?
+      self.skip_complexity_check = true
       self.password = "dummy"
     end
   end
@@ -456,7 +462,7 @@ def self.seed
       group = MiqGroup.in_my_region.find_by(:description => group_description)
 
       _log.info("Creating #{user_id} user...")
-      user = create(user_attributes)
+      user = create(user_attributes.merge(:skip_complexity_check => true))
       user.miq_groups = [group] if group
       user.save
       _log.info("Creating #{user_id} user... Complete")
@@ -473,8 +479,28 @@ def self.seed_data
   end
   private_class_method :seed_data
 
+  MANAGEIQ_DICTIONARY_WORDS = %w[manageiq miq vmdb].freeze
+
   private
 
+  def password_complexity
+    return if @skip_complexity_check
+
+    rules = ::Settings.authentication.password_rules
+    return if rules.complexity_min_score == 0
+
+    require "zxcvbn"
+    tester = Zxcvbn.tester_builder
+                   .add_word_list("manageiq_terms", MANAGEIQ_DICTIONARY_WORDS)
+                   .add_word_list("extra_terms", Array(rules.complexity_extra_words))
+                   .build
+    result = tester.test(password)
+    return unless result.score < rules.complexity_min_score
+
+    details = [result.feedback.warning, *result.feedback.suggestions].compact_blank
+    errors.add(:base, [_("Password is not strong enough"), *details].join("\n"))
+  end
+
   def unlock_queue
     MiqQueue.create_with(:deliver_on => Time.now.utc + ::Settings.authentication.locked_account_timeout.to_i)
             .put_unless_exists(

config/settings.yml

@@ -11,6 +11,9 @@
   :locked_account_timeout: 2.minutes
   :amazon_key:
   :amazon_secret:
+  :password_rules:
+    :complexity_min_score: 3
+    :complexity_extra_words: []
 :binary_blob:
   :keep_orphaned: 1.month
   :purge_window_size: 100

config/settings/development.yml

@@ -1,4 +1,7 @@
 ---
+:authentication:
+  :password_rules:
+    :complexity_min_score: 0
 :log:
   :level_rails: debug
 :webservices:

spec/factories/user.rb

@@ -1,3 +1,7 @@
+require 'bcrypt'
+
+FACTORY_USER_PASSWORD_DIGEST = BCrypt::Password.create("dummy").freeze
+
 FactoryBot.define do
   factory :user do
     transient do
@@ -11,8 +15,7 @@
     sequence(:userid) { |s| "user#{s}" }
     sequence(:name)   { |s| "Test User #{s}" }
 
-    # encrypted password for "dummy"
-    password_digest { "$2a$10$FTbGT/y/PQ1HvoOoc1FcyuuTtHzfop/uG/mcEAJLYpzmsUIJcGT7W" }
+    password_digest { FACTORY_USER_PASSWORD_DIGEST }
 
     after :build do |u, e|
       if e.miq_groups.blank? && (e.role || e.features)

spec/models/authenticator/database_spec.rb

@@ -1,7 +1,7 @@
 RSpec.describe Authenticator::Database do
   subject { Authenticator::Database.new({}) }
-  let!(:alice) { FactoryBot.create(:user, :userid => 'alice', :password => 'secret') }
-  let!(:vincent) { FactoryBot.create(:user, :userid => 'Vincent', :password => 'secret') }
+  let!(:alice) { FactoryBot.create(:user, :userid => 'alice') }
+  let!(:vincent) { FactoryBot.create(:user, :userid => 'Vincent') }
 
   describe '#uses_stored_password?' do
     it "is true" do
@@ -31,7 +31,7 @@ def authenticate
     end
 
     let(:username) { 'alice' }
-    let(:password) { 'secret' }
+    let(:password) { 'dummy' }
 
     context "with correct password" do
       it "succeeds" do

spec/models/user_password_spec.rb

@@ -1,39 +1,161 @@
 require 'bcrypt'
 
-RSpec.describe "User Password" do
-  context "With admin user" do
+RSpec.describe User, "password" do
+  let(:strong_password)  { "Tr0ub4dor&3" } # Score 4 — zxcvbn rates this very strong
+  let(:weak_password)    { "password" }    # Score 0 — top-10 common password
+  let(:smartvm_password) { "smartvm" }     # Score 1 — low entropy, the default admin seed password
+
+  # Build a user record without persisting; password= triggers the virtual attr
+  def build_user(password)
+    FactoryBot.build(:user, :password => password)
+  end
+
+  context "with admin user" do
     before do
       EvmSpecHelper.local_miq_server
 
-      @old = 'smartvm'
+      @old = smartvm_password
       @admin = FactoryBot.create(:user, :userid          => 'admin',
-                                         :password_digest => BCrypt::Password.create(@old))
+                                        :password_digest => BCrypt::Password.create(@old))
     end
 
-    it "should have set password" do
+    it "has set password" do
       expect(@admin.authenticate_bcrypt(@old)).to eq(@admin)
     end
 
     context "call change_password" do
       before do
-        @new = 'Zug-drep5s'
-        @admin.change_password(@old, @new)
+        @admin.change_password(@old, strong_password)
       end
 
-      it "should change password" do
-        expect(@admin.authenticate_bcrypt(@new)).to eq(@admin)
+      it "changes password" do
+        expect(@admin.authenticate_bcrypt(strong_password)).to eq(@admin)
       end
     end
 
     context "call password=" do
       before do
-        @new = 'Zug-drep5s'
-        @admin.password = @new
+        @admin.password = strong_password
         @admin.save!
       end
 
-      it "should change password" do
-        expect(@admin.authenticate_bcrypt(@new)).to eq(@admin)
+      it "changes password" do
+        expect(@admin.authenticate_bcrypt(strong_password)).to eq(@admin)
+      end
+    end
+  end
+
+  context "complexity" do
+    context "with default settings (min_score 3)" do
+      it "accepts a strong password (score 4)" do
+        expect(build_user(strong_password)).to be_valid
+      end
+
+      it "rejects a weak password (score 0)" do
+        user = build_user(weak_password)
+        expect(user).not_to be_valid
+        expect(user.errors[:base].first).to include("Password is not strong enough")
+      end
+
+      it "includes zxcvbn feedback in the base error message" do
+        user = build_user("P@ssw0rd")
+        user.valid?
+        message = user.errors[:base].first
+        expect(message).to include("Password is not strong enough")
+        expect(message).to include("This is similar to a commonly used password")
+        expect(message).to include("Predictable substitutions like '@' instead of 'a' don't help very much")
+      end
+
+      it "rejects a hardcoded ManageIQ term (manageiq)" do
+        expect(build_user("manageiq")).not_to be_valid
+      end
+
+      it "rejects a hardcoded ManageIQ term (miq)" do
+        expect(build_user("miq12345")).not_to be_valid
+      end
+
+      it "rejects a hardcoded ManageIQ term (vmdb)" do
+        expect(build_user("vmdb1234")).not_to be_valid
+      end
+
+      it "rejects smartvm as weak (score 1, below default min_score 3)" do
+        expect(build_user(smartvm_password)).not_to be_valid
+      end
+
+      it "rejects a common substitution pattern (P@ssw0rd, score 0)" do
+        expect(build_user("P@ssw0rd")).not_to be_valid
+      end
+
+      context "with skip_complexity_check set" do
+        it "allows a weak password" do
+          user = build_user(smartvm_password)
+          user.skip_complexity_check = true
+          expect(user).to be_valid
+        end
+      end
+
+      context "when no password is being set" do
+        it "does not run the complexity check" do
+          # password_digest set directly; the password virtual attr is never touched
+          user = FactoryBot.build(:user, :password_digest => "$2a$10$FTbGT/y/PQ1HvoOoc1FcyuuTtHzfop/uG/mcEAJLYpzmsUIJcGT7W")
+          expect(user).to be_valid
+        end
+      end
+    end
+
+    context "when min_score is 0 (enforcement disabled)" do
+      before { stub_settings_merge(:authentication => {:password_rules => {:complexity_min_score => 0}}) }
+
+      it "accepts a weak password" do
+        expect(build_user(weak_password)).to be_valid
+      end
+
+      it "accepts smartvm" do
+        expect(build_user(smartvm_password)).to be_valid
+      end
+    end
+
+    context "with min_score lowered to 1" do
+      before { stub_settings_merge(:authentication => {:password_rules => {:complexity_min_score => 1}}) }
+
+      it "accepts smartvm (score 1) when the bar is low enough" do
+        expect(build_user(smartvm_password)).to be_valid
+      end
+
+      it "still rejects a score-0 password" do
+        expect(build_user(weak_password)).not_to be_valid
+      end
+    end
+
+    context "with operator-defined extra words" do
+      before do
+        stub_settings_merge(:authentication => {:password_rules => {:complexity_extra_words => %w[acme corp]}})
+      end
+
+      it "rejects a password based on an operator-defined term" do
+        expect(build_user("acme1234")).not_to be_valid
+      end
+
+      it "still accepts a strong password unrelated to the extra words" do
+        expect(build_user(strong_password)).to be_valid
+      end
+    end
+
+    context "User.seed" do
+      # User.seed sets skip_complexity_check so the weak default password is
+      # accepted during first boot without disabling enforcement globally.
+      it "seeds the admin user successfully" do
+        User.seed
+        expect(User.find_by(:userid => "admin")).to be_present
+      end
+    end
+
+    context "operator-generated admin password" do
+      # The manageiq-operator generates passwords as RawURLEncoding(randomBytes(12))
+      # - 16 URL-safe base64 chars from the alphabet [A-Za-z0-9_-]. The wide character
+      # set ensures all outputs score 4 on zxcvbn regardless of the random content.
+      it "accepts any generated password" do
+        expect(build_user("QFzUfycaksV9Ctg_")).to be_valid
       end
     end
   end

spec/models/user_spec.rb

@@ -51,13 +51,13 @@
   end
 
   describe "#change_password" do
-    let(:user) { FactoryBot.create(:user, :password => "smartvm") }
+    let(:user) { FactoryBot.create(:user) }
 
     it "should change user password" do
-      password    = user.password
-      newpassword = "newpassword"
-      user.change_password(password, newpassword)
-      expect(user.password).to eq(newpassword)
+      password     = "dummy"
+      new_password = "Tr0ub4dor&3"
+      user.change_password(password, new_password)
+      expect(user.authenticate_bcrypt(new_password)).to eq(user)
     end
 
     it "should raise an error when asked to change user password" do
@@ -175,7 +175,7 @@
   end
 
   describe '#fail_login!' do
-    let(:user) { FactoryBot.create(:user, :password => "smartvm", :failed_login_attempts => 0) }
+    let(:user) { FactoryBot.create(:user, :failed_login_attempts => 0) }
 
     it 'increases the number of failed login attempts' do
       user.fail_login!
@@ -365,19 +365,20 @@
   end
 
   context ".authenticate_with_http_basic" do
-    let(:user) { FactoryBot.create(:user, :password => "dummy") }
+    let(:user) { FactoryBot.create(:user) }
+    let(:plaintext_password) { "dummy" }
 
     before do
       EvmSpecHelper.local_miq_server
     end
 
     it "should login with good username/password" do
-      expect(User.authenticate_with_http_basic(user.userid, user.password)).to eq([true, user.userid])
+      expect(User.authenticate_with_http_basic(user.userid, plaintext_password)).to eq([true, user.userid])
     end
 
     it "should fail with bad username" do
       bad_userid = "bad_userid"
-      expect(User.authenticate_with_http_basic(bad_userid, user.password)).to eq([false, bad_userid])
+      expect(User.authenticate_with_http_basic(bad_userid, plaintext_password)).to eq([false, bad_userid])
     end
 
     it "should fail with bad password" do