[WIP] allow changing the password on remount in some cases

Commit Message

Steve French Feb. 13, 2024, 6:53 a.m. UTC

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

Comments

Shyam Prasad N Feb. 16, 2024, 12:52 a.m. UTC | #1

On Tue, Feb 13, 2024 at 12:23 PM Steve French <[e-mail]> 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.

Paulo Alcantara Feb. 16, 2024, 2:41 p.m. UTC | #2

Shyam Prasad N <[e-mail]> 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.

Steve French Feb. 16, 2024, 5:06 p.m. UTC | #3

On Fri, Feb 16, 2024 at 8:41 AM Paulo Alcantara <[e-mail]> wrote:
>
> Shyam Prasad N <[e-mail]> 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)

Steve French Feb. 18, 2024, 10:59 p.m. UTC | #4

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 <[e-mail]> wrote:
>
> Shyam Prasad N <[e-mail]> 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.

Shyam Prasad N Feb. 23, 2024, 7:45 a.m. UTC | #5

On Mon, Feb 19, 2024 at 4:29 AM Steve French <[e-mail]> 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 <[e-mail]> wrote:
> >
> > Shyam Prasad N <[e-mail]> 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.

Paulo Alcantara Feb. 23, 2024, 2:08 p.m. UTC | #6

Shyam Prasad N <[e-mail]> 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.

Steve French Feb. 23, 2024, 6:58 p.m. UTC | #7

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 <[e-mail]> wrote:
>
> Shyam Prasad N <[e-mail]> 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.

Patch

From 8632fcc917c0c35281b4bf4d8cadd5f5aaa18741 Mon Sep 17 00:00:00 2001
From: Steve French <[e-mail]>
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: [e-mail]
Signed-off-by: Steve French <[e-mail]>
---
 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