-
Notifications
You must be signed in to change notification settings - Fork 243
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.
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
Set emergency login for core user with random password #3755
Conversation
This superseed #3689 and incorporate the suggestion in that PR. |
Wouldn't this 'rotate' away ? better leave it in a dedicated file or part of the instance crc.json |
pkg/crc/machine/start.go
Outdated
for i := range b { | ||
b[i] = charset[rand.Intn(len(charset))] //nolint | ||
} | ||
logging.Infof("Emergency login password for core user is: %s", b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will disappear from the log file when it is gets 'cleaned'/rotated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbraad updated.
938e8bd
to
0c5331c
Compare
for i := range b { | ||
b[i] = charset[rand.Intn(len(charset))] //nolint | ||
} | ||
if err := os.WriteFile(constants.PasswdFilePath, b, 0600); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove on stop and when the setting has been disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbraad On the stop we shouldnt' remove it otherwise we need to also remove core
user password on stop, IMO we just need to delete it during crc delete
not crc stop
action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it get a new password on restart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reiterated and now when stop => start
happen and user unset enable-emergency-login
setting then core
user password is locked and from shell not able to login.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't regenerate the ssh key on start/stop/start, I don't think the user password needs to behave differently, so as long as enable-emergency-login
is set, we can keep the same password until the VM is deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't think this should happen, but it is the behaviour that existed in the PR: "wouldn't it get a new password on restart?" was not something I implied being happy with, but rather an observation of the current suggestion.
pkg/crc/machine/start.go
Outdated
} | ||
|
||
func lockCoreUserPasswd(sshRunner *crcssh.Runner) error { | ||
_, _, err := sshRunner.RunPrivileged("delete core user password", "passwd", "--lock", "core") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd name this disableEmergencyLogin
for symmetry with enableEmergencyLogin
, and the RunPrivileged
string could be disable core user password
pkg/crc/machine/start.go
Outdated
if err := lockCoreUserPasswd(sshRunner); err != nil { | ||
return nil, errors.Wrap(err, "Error deleting the password for core user") | ||
} | ||
// VM started and SSH available, so we can enable the emergency login |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment is needed.
pkg/crc/machine/start.go
Outdated
@@ -417,6 +418,16 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig) | |||
} | |||
logging.Info("CRC VM is running") | |||
|
|||
if err := lockCoreUserPasswd(sshRunner); err != nil { | |||
return nil, errors.Wrap(err, "Error deleting the password for core user") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be moved to an else
in if startConfig.EmergencyLogin {
as both operations exclusive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from da66c4e#r1277594992 this looks good to me.
This PR allow user to set the emergency login for core user with a random generated password. It is helpful when user lost the ssh access and use VMM to debug the issue which require password for `core` user. Generated password is stored in `~/.crc/machine/crc/passwd` file which user can consume in case of debugging. When user unset the `enable-emergency-login` setting then password is locked for `core` user. ``` $ crc config set enable-emergency-login true $ crc config view - enable-emergency-login : true [...] $ crc start [...] INFO CRC VM is running INFO Emergency login password for core user is stored to /home/prkumar/.crc/machines/crc/passwd INFO Updating authorized keys... [...] ```
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@praveenkumar: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
This PR allow user to set the emergency login for core user with a random generated password. It is helpful when user lost the ssh access and use VMM to debug the issue which require password for
core
user.Generated password is printed on stdout if user use cli to start the VM and also part of
~/.crc/crc.log
in case want to access it later. In case of VM is started by api endpoint then password would be part of~/.crc.crcd.log
.