[release-4.19] OCPBUGS-86650: Apply password only if changes exist

pkg/daemon/update.go

@@ -1408,29 +1408,29 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig, skipCertifi
 
 		defer func() {
 			if retErr != nil {
-				if err := dn.updateSSHKeys(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
+				if err := dn.updateSSHKeys(oldIgnConfig.Passwd.Users, newIgnConfig.Passwd.Users); err != nil {
 					errs := kubeErrs.NewAggregate([]error{err, retErr})
 					retErr = fmt.Errorf("error rolling back SSH keys updates: %w", errs)
 					return
 				}
 			}
 		}()
-	}
 
-	// Set password hash
-	if err := dn.SetPasswordHash(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
-		return err
-	}
+		// Set password hash
+		if err := dn.SetPasswordHash(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
+			return err
+		}
 
-	defer func() {
-		if retErr != nil {
-			if err := dn.SetPasswordHash(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
-				errs := kubeErrs.NewAggregate([]error{err, retErr})
-				retErr = fmt.Errorf("error rolling back password hash updates: %w", errs)
-				return
+		defer func() {
+			if retErr != nil {
+				if err := dn.SetPasswordHash(oldIgnConfig.Passwd.Users, newIgnConfig.Passwd.Users); err != nil {
+					errs := kubeErrs.NewAggregate([]error{err, retErr})
+					retErr = fmt.Errorf("error rolling back password hash updates: %w", errs)
+					return
+				}
 			}
-		}
-	}()
+		}()
+	}
 
 	if dn.os.IsCoreOSVariant() {
 		coreOSDaemon := CoreOSDaemon{dn}
@@ -2489,7 +2489,21 @@ func (dn *Daemon) atomicallyWriteSSHKey(authKeyPath, keys string) error {
 	return nil
 }
 
-// Set a given PasswdUser's Password Hash
+func getUserPasswordHash(user string) (string, error) {
+	shadowOut, err := exec.Command("getent", "shadow", user).CombinedOutput()
+	if err != nil {
+		return "", fmt.Errorf("Failed to check password hash for %s: %w", user, err)
+	}
+	shadowSlice := strings.SplitN(strings.TrimSpace(string(shadowOut)), ":", 3)
+	if len(shadowSlice) >= 2 {
+		return shadowSlice[1], nil
+	}
+	return "", nil
+
+}
+
+// SetPasswordHash updates the password for each user in newUsers, skipping
+// users whose password already matches the desired configuration.
 func (dn *Daemon) SetPasswordHash(newUsers, oldUsers []ign3types.PasswdUser) error {
 	// confirm that user exits
 	klog.Info("Checking if absent users need to be disconfigured")
@@ -2514,6 +2528,15 @@ func (dn *Daemon) SetPasswordHash(newUsers, oldUsers []ign3types.PasswdUser) err
 			pwhash = *u.PasswordHash
 		}
 
+		// Check if hash update is needed. Skip if not.
+		currentHash, err := getUserPasswordHash(u.Name)
+		if err != nil {
+			return err
+		}
+		if currentHash == pwhash {
+			continue
+		}
+
 		if out, err := exec.Command("usermod", "-p", pwhash, u.Name).CombinedOutput(); err != nil {
 			return fmt.Errorf("Failed to reset password for %s: %s:%w", u.Name, out, err)
 		}

test/e2e/mcd_test.go

@@ -533,8 +533,11 @@ func TestNoReboot(t *testing.T) {
 
 	currentEtcShadowContents := helpers.ExecCmdOnNode(t, cs, infraNode, "grep", "^core:", "/rootfs/etc/shadow")
 
-	if currentEtcShadowContents == initialEtcShadowContents {
-		t.Fatalf("updated password hash not found in etc/shadow, got %s", currentEtcShadowContents)
+	initialPasswordHash := extractPasswordHashFromShadowLine(initialEtcShadowContents)
+	currentPasswordHash := extractPasswordHashFromShadowLine(currentEtcShadowContents)
+
+	if currentPasswordHash == initialPasswordHash {
+		t.Fatalf("updated password hash not found in etc/shadow")
 	}
 
 	t.Logf("Node %s has Password Hash", infraNode.Name)
@@ -614,7 +617,9 @@ func TestNoReboot(t *testing.T) {
 	require.Nil(t, err)
 
 	rollbackEtcShadowContents := helpers.ExecCmdOnNode(t, cs, infraNode, "grep", "^core:", "/rootfs/etc/shadow")
-	assert.Equal(t, initialEtcShadowContents, rollbackEtcShadowContents)
+
+	assert.Equal(t, extractPasswordHashFromShadowLine(initialEtcShadowContents), extractPasswordHashFromShadowLine(rollbackEtcShadowContents),
+		"password hash should match after rollback")
 }
 
 func TestPoolDegradedOnFailToRender(t *testing.T) {
@@ -976,6 +981,17 @@ func assertExpectedPerms(t *testing.T, cs *framework.ClientSet, node corev1.Node
 	assert.Equal(t, expectedPerms, actualPerms, "expected %s to have perms %v, got: %v", path, expectedPerms, actualPerms)
 }
 
+// extractPasswordHashFromShadowLine extracts the password hash field from an /etc/shadow line.
+// The shadow file format is: username:password:lastchg:min:max:warn:inactive:expire:reserved
+// This function returns the password hash (field index 1).
+func extractPasswordHashFromShadowLine(shadowLine string) string {
+	fields := strings.Split(strings.TrimSpace(shadowLine), ":")
+	if len(fields) < 2 {
+		return ""
+	}
+	return fields[1]
+}
+
 // Tests that changes to the internal image registry pull secret has the
 // desired effect on both the ControllerConfig and the individual nodes
 // filesystems.