Merge pull request #8599 from luchihoratiu/PUP-11026

(PUP-11026) Fix user password management on macOS Big Sur

Author: ciprianbadescu

acceptance/tests/resource/user/osx_10.4_should_fail_when_modify_home.rb

@@ -1,5 +1,5 @@
 test_name "should not modify the home directory of an user on OS X >= 10.14" do
-  confine :to, :platform => /osx-10.1[4-9]/
+  confine :to, :platform => /osx/
 
   tag 'audit:high',
       'audit:acceptance' # Could be done as integration tests, but would
@@ -23,7 +23,7 @@
     end
 
     step "verify the error message is correct" do
-      expected_error = /OS X version 10\.1[4-9] does not allow changing home using puppet/
+      expected_error = /OS X version [0-9\.]+ does not allow changing home using puppet/
       user_manifest = resource_manifest('user', user, ensure: 'present', home: "/opt/#{user}")
 
       apply_manifest_on(agent, user_manifest) do |result|

acceptance/tests/resource/user/osx_10.4_should_fail_when_modify_uid.rb

@@ -1,5 +1,5 @@
 test_name "should not modify the uid of an user on OS X >= 10.14" do
-  confine :to, :platform => /osx-10.1[4-9]/
+  confine :to, :platform => /osx/
 
   tag 'audit:high',
       'audit:acceptance' # Could be done as integration tests, but would
@@ -23,7 +23,7 @@
     end
 
     step "verify the error message is correct" do
-      expected_error = /OS X version 10\.1[4-9] does not allow changing uid using puppet/
+      expected_error = /OS X version [0-9\.]+ does not allow changing uid using puppet/
       user_manifest = resource_manifest('user', user, ensure: 'present', uid: rand(999999))
 
       apply_manifest_on(agent, user_manifest) do |result|

acceptance/tests/resource/user/should_allow_managed_macos_users_to_login.rb

@@ -0,0 +1,77 @@
+test_name "should allow managed macOS users to login" do
+
+  confine :to, :platform => /osx/
+
+  tag 'audit:high',
+      'audit:acceptance' # Could be done as integration tests, but would
+                         # require changing the system running the test
+                         # in ways that might require special permissions
+                         # or be harmful to the system running the test
+
+
+  # Two different test cases, with additional environment setup inbetween,
+  # were added in the same file to save on test run time (managing macOS users
+  # takes around a minute or so because when retrieving plist data from all users with
+  # `dscl readall` we also receive thousands of bytes of user avatar image)
+  agents.each do |agent|
+    teardown do
+      on(agent, puppet("resource", "user", 'testuser', "ensure=absent"))
+    end
+
+    # Checking if we can create a user with password, salt and iterations
+    step "create the test user with password and salt" do
+      # The password is 'helloworld'
+      apply_manifest_on(agent, <<-MANIFEST, :catch_failures => true)
+          user { 'testuser':
+            ensure => present,
+            password => '6ce97688468f231845d9d982f1f10832ca0c6c728a77bac51c548af99ebd9b9c62bcba15112a0c7a7e34effbb2e92635650c79c51517d72b083a4eb2a513f51ad1f8ea9556cef22456159c341d8bcd382a91708afaf253c2b727d4c6cd3d29cc26011d5d511154037330ecea0263b1be8c1c13086d029c57344291bd37952b56',
+            salt       => '377e8b60e5fdfe509cad188d5b1b9e40e78b418f8c3f0127620ea69d4c32789c',
+            iterations => 40000,
+          }
+        MANIFEST
+    end
+
+    step "verify the password was set correctly and is able to log in" do
+      on(agent, "dscl /Local/Default -authonly testuser helloworld", :acceptable_exit_codes => 0)
+    end
+
+    unless agent['platform'] =~ /osx-11/
+      skip_test "AuthenticationAuthority field fix test is not valid for macOS older than Big Sur (11.0)"
+    end
+
+    # Setting up environment to mimic situation on macOS 11 BigSur
+    # Prior to macOS BigSur, `dscl . -create` was populating more fields with
+    # default values, including AuthenticationAuthority which contains the
+    # ShadowHash type
+    # Withouth this field, login is not allowed
+    step "remove AuthenticationAuthority field from user" do
+      on(agent, "dscl /Local/Default -delete Users/testuser AuthenticationAuthority", :acceptable_exit_codes => 0)
+    end
+
+    step "expect user without AuthenticationAuthority to not be able to log in" do
+      on(agent, "dscl /Local/Default -authonly testuser helloworld", :acceptable_exit_codes => (1..255))
+    end
+
+    # Expecting Puppet to pick up the missing field and add it to
+    # make the user usable again
+    step "change password with different salt and expect AuthenticationAuthority field to be readded" do
+      # The password is still 'helloworld' but with different salt
+      apply_manifest_on(agent, <<-MANIFEST, { :catch_failures => true, :debug => true }) do |result|
+          user { 'testuser':
+            ensure => present,
+            password   => '7e8e82542a0c06595e99bfe10c6bd219d19dede137c5c2f84bf10d98d83b77302d3c9c7f7e3652d420f562613f582ab62b26a52b9b26d0d032efbd486fd865b3ba4fd8a3512137681ce87d190f8fa7848d941c6080c588528dcb682c763c040ff54992dce204c3e5dda973e7b36f7f50a774e55e99fe4c8ed6b6464614838c13',
+            salt       => '8005b8855a187086a3b59eff925a611ec61d2d66d2e786b7598fe0a0b4b8ffba',
+            iterations => 40000
+          }
+        MANIFEST
+        
+        assert_match(/User 'testuser' is missing the 'SALTED-SHA512-PBKDF2' AuthenticationAuthority key for ShadowHash/, result.stdout)
+        assert_match(/Adding 'SALTED-SHA512-PBKDF2' AuthenticationAuthority key for ShadowHash to user 'testuser'/, result.stdout)
+      end
+    end
+
+    step "verify the password was set correctly and is able to log in" do
+      on(agent, "dscl /Local/Default -authonly testuser helloworld", :acceptable_exit_codes => 0)
+    end
+  end
+end

acceptance/tests/resource/user/should_allow_password_salt_iterations_osx.rb

@@ -435,7 +435,7 @@ def salt=(value)
   ['home', 'uid', 'gid', 'comment', 'shell'].each do |setter_method|
     define_method("#{setter_method}=") do |value|
       if @property_hash[setter_method.intern]
-        if self.class.get_os_version.split('.').last.to_i >= 14 && %w(home uid).include?(setter_method)
+        if %w(home uid).include?(setter_method)
           raise Puppet::Error, "OS X version #{self.class.get_os_version} does not allow changing #{setter_method} using puppet"
         end
         begin
@@ -536,6 +536,14 @@ def write_password_to_users_plist(value)
       if (shadow_hash_data.class == Hash) && (shadow_hash_data.has_key?('SALTED-SHA512'))
         shadow_hash_data.delete('SALTED-SHA512')
       end
+
+      # Starting with macOS 11 Big Sur, the AuthenticationAuthority field
+      # could be missing entirely and without it the managed user cannot log in
+      if needs_sha512_pbkdf2_authentication_authority_to_be_added?(users_plist)
+        Puppet.debug("Adding 'SALTED-SHA512-PBKDF2' AuthenticationAuthority key for ShadowHash to user '#{@resource.name}'")
+        merge_attribute_with_dscl('Users', @resource.name, 'AuthenticationAuthority', ERB::Util.html_escape(SHA512_PBKDF2_AUTHENTICATION_AUTHORITY))
+      end
+
       set_salted_pbkdf2(users_plist, shadow_hash_data, 'entropy', value)
     end
   end
@@ -562,6 +570,17 @@ def get_shadow_hash_data(users_plist)
     end
   end
 
+  # This method will check if authentication_authority key of a user's plist
+  # needs SALTED_SHA512_PBKDF2 to be added. This is a valid case for macOS 11 (Big Sur)
+  # where users created with `dscl` started to have this field missing
+  def needs_sha512_pbkdf2_authentication_authority_to_be_added?(users_plist)
+    authority = users_plist['authentication_authority']
+    return false if Puppet::Util::Package.versioncmp(self.class.get_os_version, '11.0.0') < 0 && authority && authority.include?(SHA512_PBKDF2_AUTHENTICATION_AUTHORITY)
+
+    Puppet.debug("User '#{@resource.name}' is missing the 'SALTED-SHA512-PBKDF2' AuthenticationAuthority key for ShadowHash")
+    true
+  end
+
   # This method will embed the binary plist data comprising the user's
   # password hash (and Salt/Iterations value if the OS is 10.8 or greater)
   # into the ShadowHashData key of the user's plist.
@@ -667,4 +686,8 @@ def write_to_file(filename, value)
       raise Puppet::Error, "Could not write to file #{filename}: #{detail}", detail.backtrace
     end
   end
+
+  private
+
+  SHA512_PBKDF2_AUTHENTICATION_AUTHORITY = ';ShadowHash;HASHLIST:<SALTED-SHA512-PBKDF2,SRP-RFC5054-4096-SHA512-PBKDF2>'
 end

lib/puppet/provider/user/directoryservice.rb

@@ -925,28 +925,75 @@ module Puppet::Util::Plist
       }
     end
 
-    it 'should call set_salted_sha512 on 10.7 when given a salted-SHA512 password hash' do
-      expect(provider).to receive(:get_users_plist).and_return(sample_users_plist)
-      expect(provider).to receive(:get_shadow_hash_data).with(sample_users_plist).and_return(sha512_shadowhashdata)
-      expect(provider.class).to receive(:get_os_version).and_return('10.7')
-      expect(provider).to receive(:set_salted_sha512).with(sample_users_plist, sha512_shadowhashdata, sha512_password_hash)
-      provider.write_password_to_users_plist(sha512_password_hash)
+    before do
+      allow(provider).to receive(:merge_attribute_with_dscl).with('Users', username, 'AuthenticationAuthority', any_args)
     end
 
-    it 'should call set_salted_pbkdf2 on 10.8 when given a PBKDF2 password hash' do
-      expect(provider).to receive(:get_users_plist).and_return(sample_users_plist)
-      expect(provider).to receive(:get_shadow_hash_data).with(sample_users_plist).and_return(pbkdf2_shadowhashdata)
-      expect(provider.class).to receive(:get_os_version).and_return('10.8')
-      expect(provider).to receive(:set_salted_pbkdf2).with(sample_users_plist, pbkdf2_shadowhashdata, 'entropy', pbkdf2_password_hash)
-      provider.write_password_to_users_plist(pbkdf2_password_hash)
+    describe 'when on macOS 11 (Big Sur) or greater' do
+      before do
+        allow(provider.class).to receive(:get_os_version).and_return('11.0.0')
+      end
+
+      it 'should add salted_sha512_pbkdf2 AuthenticationAuthority key if missing' do
+        expect(provider).to receive(:get_users_plist).and_return(sample_users_plist)
+        expect(provider).to receive(:get_shadow_hash_data).with(sample_users_plist).and_return(pbkdf2_shadowhashdata)
+        expect(provider).to receive(:set_salted_pbkdf2).with(sample_users_plist, pbkdf2_shadowhashdata, 'entropy', pbkdf2_password_hash)
+        expect(provider).to receive(:needs_sha512_pbkdf2_authentication_authority_to_be_added?).and_return(true)
+
+        expect(Puppet).to receive(:debug).with("Adding 'SALTED-SHA512-PBKDF2' AuthenticationAuthority key for ShadowHash to user 'nonexistent_user'")
+        provider.write_password_to_users_plist(pbkdf2_password_hash)
+      end
+
+      it 'should not add salted_sha512_pbkdf2 AuthenticationAuthority key if not missing' do
+        expect(provider).to receive(:get_users_plist).and_return(sample_users_plist)
+        expect(provider).to receive(:get_shadow_hash_data).with(sample_users_plist).and_return(pbkdf2_shadowhashdata)
+        expect(provider).to receive(:set_salted_pbkdf2).with(sample_users_plist, pbkdf2_shadowhashdata, 'entropy', pbkdf2_password_hash)
+        expect(provider).to receive(:needs_sha512_pbkdf2_authentication_authority_to_be_added?).and_return(false)
+
+        expect(Puppet).not_to receive(:debug).with("Adding 'SALTED-SHA512-PBKDF2' AuthenticationAuthority key for ShadowHash to user 'nonexistent_user'")
+        provider.write_password_to_users_plist(pbkdf2_password_hash)
+      end
     end
 
-    it "should delete the SALTED-SHA512 key in the shadow_hash_data hash if it exists on a 10.8 system and write_password_to_users_plist has been called to set the user's password" do
-      expect(provider).to receive(:get_users_plist).and_return('users_plist')
-      expect(provider).to receive(:get_shadow_hash_data).with('users_plist').and_return(sha512_shadowhashdata)
-      expect(provider.class).to receive(:get_os_version).and_return('10.8')
-      expect(provider).to receive(:set_salted_pbkdf2).with('users_plist', {}, 'entropy', pbkdf2_password_hash)
-      provider.write_password_to_users_plist(pbkdf2_password_hash)
+    describe 'when on macOS version lower than 11' do
+      before do
+        allow(provider.class).to receive(:get_os_version)
+        allow(provider).to receive(:needs_sha512_pbkdf2_authentication_authority_to_be_added?).and_return(false)
+      end
+
+      it 'should not add salted_sha512_pbkdf2 AuthenticationAuthority' do
+        expect(provider).to receive(:get_users_plist).and_return(sample_users_plist)
+        expect(provider).to receive(:get_shadow_hash_data).with(sample_users_plist).and_return(pbkdf2_shadowhashdata)
+        expect(provider).to receive(:set_salted_pbkdf2).with(sample_users_plist, pbkdf2_shadowhashdata, 'entropy', pbkdf2_password_hash)
+        expect(provider).to receive(:needs_sha512_pbkdf2_authentication_authority_to_be_added?).and_return(false)
+
+        expect(Puppet).not_to receive(:debug).with("Adding 'SALTED-SHA512-PBKDF2' AuthenticationAuthority key for ShadowHash to user 'nonexistent_user'")
+        provider.write_password_to_users_plist(pbkdf2_password_hash)
+      end
+
+      it 'should call set_salted_sha512 on 10.7 when given a salted-SHA512 password hash' do
+        expect(provider).to receive(:get_users_plist).and_return(sample_users_plist)
+        expect(provider).to receive(:get_shadow_hash_data).with(sample_users_plist).and_return(sha512_shadowhashdata)
+        expect(provider.class).to receive(:get_os_version).and_return('10.7')
+        expect(provider).to receive(:set_salted_sha512).with(sample_users_plist, sha512_shadowhashdata, sha512_password_hash)
+        provider.write_password_to_users_plist(sha512_password_hash)
+      end
+
+      it 'should call set_salted_pbkdf2 on 10.8 when given a PBKDF2 password hash' do
+        expect(provider).to receive(:get_users_plist).and_return(sample_users_plist)
+        expect(provider).to receive(:get_shadow_hash_data).with(sample_users_plist).and_return(pbkdf2_shadowhashdata)
+        expect(provider.class).to receive(:get_os_version).and_return('10.8')
+        expect(provider).to receive(:set_salted_pbkdf2).with(sample_users_plist, pbkdf2_shadowhashdata, 'entropy', pbkdf2_password_hash)
+        provider.write_password_to_users_plist(pbkdf2_password_hash)
+      end
+
+      it "should delete the SALTED-SHA512 key in the shadow_hash_data hash if it exists on a 10.8 system and write_password_to_users_plist has been called to set the user's password" do
+        expect(provider).to receive(:get_users_plist).and_return('users_plist')
+        expect(provider).to receive(:get_shadow_hash_data).with('users_plist').and_return(sha512_shadowhashdata)
+        expect(provider.class).to receive(:get_os_version).and_return('10.8')
+        expect(provider).to receive(:set_salted_pbkdf2).with('users_plist', {}, 'entropy', pbkdf2_password_hash)
+        provider.write_password_to_users_plist(pbkdf2_password_hash)
+      end
     end
   end
 
@@ -1203,6 +1250,7 @@ module Puppet::Util::Plist
     before :each do
       allow(provider.class).to receive(:get_all_users).and_return(all_users_hash)
       allow(provider.class).to receive(:get_list_of_groups).and_return(group_plist_hash_guid)
+      allow(provider).to receive(:merge_attribute_with_dscl).with('Users', username, 'AuthenticationAuthority', any_args)
       provider.class.prefetch({})
     end