| Message ID | CAH2r5mtUnLDtwbW7or=Uc+AXkzLpHsJoPuoLE7yyjPVYjvZCow@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [WIP] allow changing the password on remount in some cases | expand |
On Tue, Feb 13, 2024 at 12:23 PM Steve French <smfrench@gmail.com> wrote: > > cifs: Work-in-progress patch to allow changing password > during remount > > There are cases where a session is disconnected but we can > not reconnect successfully since the user's password has changed > on the server (or expired) and this case currently can not be fixed > without unmount and mounting again which is not always realistic to do. > This patch allows remount to change the password when the session > is disconnected. > > This patch needs to be tested for cases where you have multiuser mounts > and to make sure that there are no cases where we are changing > passwords for a different user than the one for the master tcon's > session (cifs_sb->tcon->ses->username) > > Future patches should also allow us to setup the keyring (cifscreds) > to have an "alternate password" so we would be able to change > the password before the session drops (without the risk of races > between when the password changes and the disconnect occurs - > ie cases where the old password is still needed because the new > password has not fully rolled out to all servers yet). > > See attached patch > > > -- > Thanks, > > Steve need_recon would also be true in other cases, for example when the network is temporarily disconnected. This patch will allow changing of password even then. We could setup a special flag when the server returns a STATUS_LOGON_FAILURE for SessionSetup. We can make the check for that flag and then allow password change on remount. Another option is to extend the multiuser keyring mechanism for single user use case as well, and use that for password update. Ideally, we should be able to setup multiple passwords in that keyring and iterate through them once to see if SessionSetup goes through. It'll be a bigger change than this though.
Shyam Prasad N <nspmangalore@gmail.com> writes: > need_recon would also be true in other cases, for example when the > network is temporarily disconnected. This patch will allow changing of > password even then. > We could setup a special flag when the server returns a > STATUS_LOGON_FAILURE for SessionSetup. We can make the check for that > flag and then allow password change on remount. Yes. Allowing password change over remount simply because network is disconnected is not a good idea. The user could mistype the password when performing a remount and then everything would stop working. Not to mention that this patch is only handling a specfic case where a mount would have a single SMB session, which isn't true for a DFS mount. > Another option is to extend the multiuser keyring mechanism for single > user use case as well, and use that for password update. > Ideally, we should be able to setup multiple passwords in that keyring > and iterate through them once to see if SessionSetup goes through. Yes, sounds like the best approach so far. It would allow users to update their passwords in keyring and sysadmins could drop existing SMB sessions from server side and then the client would reconnect by using new password from keyring. This wouldn't even require a remount. Besides, marking this for -stable makes no sense.
On Fri, Feb 16, 2024 at 8:41 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > need_recon would also be true in other cases, for example when the > > network is temporarily disconnected. This patch will allow changing of > > password even then. > > We could setup a special flag when the server returns a > > STATUS_LOGON_FAILURE for SessionSetup. We can make the check for that > > flag and then allow password change on remount. > > Yes. Allowing password change over remount simply because network is > disconnected is not a good idea. The user could mistype the password > when performing a remount and then everything would stop working. I agree - will change patch to do that. > Not to mention that this patch is only handling a specfic case where a > mount would have a single SMB session, which isn't true for a DFS mount. We should do a patch for that too. Agreed. > > Another option is to extend the multiuser keyring mechanism for single > > user use case as well, and use that for password update. > > Ideally, we should be able to setup multiple passwords in that keyring > > and iterate through them once to see if SessionSetup goes through. > > Yes, sounds like the best approach so far. It would allow users to > update their passwords in keyring and sysadmins could drop existing SMB > sessions from server side and then the client would reconnect by using > new password from keyring. This wouldn't even require a remount. Yes - I was discussing this with David Howells, and having a backup password in keyring is helpful in long run (and better solution for some) but we also need remount because that is what user's would intuitively try first. > Besides, marking this for -stable makes no sense. Problem we have is that it can be (and has sometimes been) a big problem for user when password keys rotate and no way to fix it other than unmount - so we will need the "easy and low risk" solution available for distros since keyring won't work for some use cases (although helpful for others)
Updated the patch to allow remount to only change the password if disconnected and the session fails to reconnect due to continued access denied or password expired errors. Any thoughts on whether I should add another patch to throttle repeated session setups after access denied or key expired (currently looks like repeated every few seconds) maybe double the time repeatedly (e.g. until a max of 10 or 20 or 30 seconds? between reconnect attempts) instead of every two seconds. On Fri, Feb 16, 2024 at 8:41 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > need_recon would also be true in other cases, for example when the > > network is temporarily disconnected. This patch will allow changing of > > password even then. > > We could setup a special flag when the server returns a > > STATUS_LOGON_FAILURE for SessionSetup. We can make the check for that > > flag and then allow password change on remount. > > Yes. Allowing password change over remount simply because network is > disconnected is not a good idea. The user could mistype the password > when performing a remount and then everything would stop working. > > Not to mention that this patch is only handling a specfic case where a > mount would have a single SMB session, which isn't true for a DFS mount. > > > Another option is to extend the multiuser keyring mechanism for single > > user use case as well, and use that for password update. > > Ideally, we should be able to setup multiple passwords in that keyring > > and iterate through them once to see if SessionSetup goes through. > > Yes, sounds like the best approach so far. It would allow users to > update their passwords in keyring and sysadmins could drop existing SMB > sessions from server side and then the client would reconnect by using > new password from keyring. This wouldn't even require a remount. > > Besides, marking this for -stable makes no sense.
On Mon, Feb 19, 2024 at 4:29 AM Steve French <smfrench@gmail.com> wrote: > > Updated the patch to allow remount to only change the password if > disconnected and the session fails to reconnect due to continued > access denied or password expired errors. > > Any thoughts on whether I should add another patch to throttle > repeated session setups after access denied or key expired (currently > looks like repeated every few seconds) maybe double the time > repeatedly (e.g. until a max of 10 or 20 or 30 seconds? between > reconnect attempts) instead of every two seconds. > > On Fri, Feb 16, 2024 at 8:41 AM Paulo Alcantara <pc@manguebit.com> wrote: > > > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > > > need_recon would also be true in other cases, for example when the > > > network is temporarily disconnected. This patch will allow changing of > > > password even then. > > > We could setup a special flag when the server returns a > > > STATUS_LOGON_FAILURE for SessionSetup. We can make the check for that > > > flag and then allow password change on remount. > > > > Yes. Allowing password change over remount simply because network is > > disconnected is not a good idea. The user could mistype the password > > when performing a remount and then everything would stop working. > > > > Not to mention that this patch is only handling a specfic case where a > > mount would have a single SMB session, which isn't true for a DFS mount. > > > > > Another option is to extend the multiuser keyring mechanism for single > > > user use case as well, and use that for password update. > > > Ideally, we should be able to setup multiple passwords in that keyring > > > and iterate through them once to see if SessionSetup goes through. > > > > Yes, sounds like the best approach so far. It would allow users to > > update their passwords in keyring and sysadmins could drop existing SMB > > sessions from server side and then the client would reconnect by using > > new password from keyring. This wouldn't even require a remount. > > > > Besides, marking this for -stable makes no sense. > > > > -- > Thanks, > > Steve No major objections for this patch. While it may not cover all cases like DFS, multiuser etc., it's still a starting point to allowing users to change password on existing mounts without unmounting. Steve: We may want to check additionally that sec_type is not Kerberos for this remount. I also think that we should have a future patch to update passwords using cifscreds utility even for single user mounts.
Shyam Prasad N <nspmangalore@gmail.com> writes: > No major objections for this patch. While it may not cover all cases > like DFS, multiuser etc., it's still a starting point to allowing > users to change password on existing mounts without unmounting. As long as it doesn't go through -stable and is accompanied with at least a new option like 'forcenewcreds', should be fine. Then you have the next merge window to handle the missing cases and fix any problems.
Here is an updated patch which handles the point about preventing remount from changing it for the sec=krb5 case. This does look like something important for stable (users have cases where they can't unmount due to a running app but need to update an expired password) - are you preferring that we only allow this when user also specifies a new mount option in addition to "remount" ie "forcenewcreds" (or probably better would be to call it "newpassword=" mount option)? On Fri, Feb 23, 2024 at 8:08 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > No major objections for this patch. While it may not cover all cases > > like DFS, multiuser etc., it's still a starting point to allowing > > users to change password on existing mounts without unmounting. > > As long as it doesn't go through -stable and is accompanied with at > least a new option like 'forcenewcreds', should be fine. Then you have > the next merge window to handle the missing cases and fix any problems.
From 8632fcc917c0c35281b4bf4d8cadd5f5aaa18741 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Tue, 13 Feb 2024 00:40:01 -0600 Subject: [PATCH] cifs: Work-in-progress patch to allow changing password during remount There are cases where a session is disconnected and password has changed on the server (or expired) for this user and this currently can not be fixed without unmount and mounting again. This patch allows remount to change the password when the session is disconnect. It needs to be tested for cases where you have multiuser mounts and to make sure that there are no cases where we are changing passwords for a different user than the one for the master tcon's session (cifs_sb->tcon->ses->username) Future patches should also allow us to setup the keyring (cifscreds) to have an "alternate password" so we would be able to change the password before the session drops (without the risk of races between when the password changes and the disconnect occurs - ie cases where the old password is still needed because the new password has not fully rolled out to all servers yet). Cc: stable@vger.kernel.org Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/smb/client/fs_context.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c index aec8dbd1f9db..c7a0b2bd7a15 100644 --- a/fs/smb/client/fs_context.c +++ b/fs/smb/client/fs_context.c @@ -772,7 +772,7 @@ static void smb3_fs_context_free(struct fs_context *fc) */ static int smb3_verify_reconfigure_ctx(struct fs_context *fc, struct smb3_fs_context *new_ctx, - struct smb3_fs_context *old_ctx) + struct smb3_fs_context *old_ctx, bool need_recon) { if (new_ctx->posix_paths != old_ctx->posix_paths) { cifs_errorf(fc, "can not change posixpaths during remount\n"); @@ -798,8 +798,11 @@ static int smb3_verify_reconfigure_ctx(struct fs_context *fc, } if (new_ctx->password && (!old_ctx->password || strcmp(new_ctx->password, old_ctx->password))) { - cifs_errorf(fc, "can not change password during remount\n"); - return -EINVAL; + if (need_recon == false) { + cifs_errorf(fc, + "can not change password of active session during remount\n"); + return -EINVAL; + } } if (new_ctx->domainname && (!old_ctx->domainname || strcmp(new_ctx->domainname, old_ctx->domainname))) { @@ -843,9 +846,15 @@ static int smb3_reconfigure(struct fs_context *fc) struct smb3_fs_context *ctx = smb3_fc2context(fc); struct dentry *root = fc->root; struct cifs_sb_info *cifs_sb = CIFS_SB(root->d_sb); + struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses; + bool need_recon = false; int rc; - rc = smb3_verify_reconfigure_ctx(fc, ctx, cifs_sb->ctx); + if ((ses->ses_status == SES_NEED_RECON) || + (ses->ses_status == SES_IN_SETUP)) + need_recon = true; + + rc = smb3_verify_reconfigure_ctx(fc, ctx, cifs_sb->ctx, need_recon); if (rc) return rc; @@ -858,7 +867,12 @@ static int smb3_reconfigure(struct fs_context *fc) STEAL_STRING(cifs_sb, ctx, UNC); STEAL_STRING(cifs_sb, ctx, source); STEAL_STRING(cifs_sb, ctx, username); - STEAL_STRING_SENSITIVE(cifs_sb, ctx, password); + if (need_recon == false) + STEAL_STRING_SENSITIVE(cifs_sb, ctx, password); + else { + kfree_sensitive(ses->password); + ses->password = kstrdup(ctx->password, GFP_KERNEL); + } STEAL_STRING(cifs_sb, ctx, domainname); STEAL_STRING(cifs_sb, ctx, nodename); STEAL_STRING(cifs_sb, ctx, iocharset); -- 2.40.1