From efe705fa56c63b5f84eac4234e03e8d5e5914347 Mon Sep 17 00:00:00 2001 From: Pablo Rodriguez Nava Date: Tue, 28 Apr 2026 10:14:01 +0200 Subject: [PATCH 1/2] OCPBUGS-83830: Apply password only if changes exist This bugfix ensures that the MCD only runs `usermod` if the password hash has actually changed and not in every update. This aligns the behavior we currently have for SSH passwords. Signed-off-by: Pablo Rodriguez Nava --- pkg/daemon/update.go | 53 +++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index 1ea0212cdd..0b0c5698f7 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -1391,29 +1391,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} @@ -2470,7 +2470,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") @@ -2495,6 +2509,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) } From b43c0b11c533ffce807e19c355c93c019e5dd0ea Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Tue, 12 May 2026 14:27:48 -0400 Subject: [PATCH 2/2] TestNoReboot should only consider password hash in /etc/shadow When usermod -P is used, the last password change field is changed. When the MachineConfig is rolled back, this value remains the same even though the actual password value has been changed back to its previous value. Consequently, the E2E test should only compare the password hashes instead of the full line. Assisted-By: Claude Opus 4.6 --- test/e2e/mcd_test.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/test/e2e/mcd_test.go b/test/e2e/mcd_test.go index a00d422b2b..683cc64626 100644 --- a/test/e2e/mcd_test.go +++ b/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.