Skip to content

OCPBUGS-83830: Apply password only if changes exist #5889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pablintino wants to merge 2 commits into
base: main
Choose a base branch
from
+38 −15
Open

OCPBUGS-83830: Apply password only if changes exist #5889

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d98512
OCPBUGS-84332: Fix ssh and password rollbacks
pablintino Apr 27, 2026
867a618
OCPBUGS-83830: Apply password only if changes exist
pablintino Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 38 additions & 15 deletions pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,29 +1241,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 {
Copy link
Copy Markdown
Contributor

@yuqi-zhang yuqi-zhang Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thing this almost never fails since we probably never had a functional rollback

Copy link
Copy Markdown
Contributor Author

@pablintino pablintino Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on that.

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
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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}
Expand Down Expand Up @@ -2439,7 +2439,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")
Expand All @@ -2464,6 +2478,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)
Copy link
Copy Markdown
Contributor

@yuqi-zhang yuqi-zhang Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the vast majority of users would never have this set. I guess with our current logic, we'd always just be doing a usermod with an empty password hash and this is triggering the policy described in the bug?

I'm wondering if we just exit out of this function if passwordhash is unset. I guess we'd have to have special logic if the user deletes a password entry, so probably fine to keep it as is since most users wouldn't be hitting this with your conditional changes above.(just thinking if we can bypass the ondisk check)

Copy link
Copy Markdown
Contributor Author

@pablintino pablintino Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought about it, but I took the "Ansible" approach. "I'd do my best to make your state match the machine state". With an early exit or check, the user would be able to modify the node shadow and we won't never try to patch it to match the MC, that I'd say it's the sourth of true.

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)
}
Expand Down