All of lore.kernel.org
 help / color / mirror / Atom feed
* [WIP PATCH] allow changing the password on remount in some cases
@ 2024-02-13  6:53 Steve French
  2024-02-16  0:52 ` Shyam Prasad N
  0 siblings, 1 reply; 8+ messages in thread
From: Steve French @ 2024-02-13  6:53 UTC (permalink / raw)
  To: CIFS
  Cc: Shyam Prasad N, Bharath S M, Meetakshi Setiya, David Howells,
	samba-technical

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

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

[-- Attachment #2: 0001-cifs-Work-in-progress-patch-to-allow-changing-passwo.patch --]
[-- Type: text/x-patch, Size: 3579 bytes --]

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [WIP PATCH] allow changing the password on remount in some cases
  2024-02-13  6:53 [WIP PATCH] allow changing the password on remount in some cases Steve French
@ 2024-02-16  0:52 ` Shyam Prasad N
  2024-02-16 14:41   ` Paulo Alcantara
  0 siblings, 1 reply; 8+ messages in thread
From: Shyam Prasad N @ 2024-02-16  0:52 UTC (permalink / raw)
  To: Steve French
  Cc: CIFS, Bharath S M, Meetakshi Setiya, David Howells, samba-technical

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.

-- 
Regards,
Shyam

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [WIP PATCH] allow changing the password on remount in some cases
  2024-02-16  0:52 ` Shyam Prasad N
@ 2024-02-16 14:41   ` Paulo Alcantara
  2024-02-16 17:06     ` Steve French
  2024-02-18 22:59     ` Steve French
  0 siblings, 2 replies; 8+ messages in thread
From: Paulo Alcantara @ 2024-02-16 14:41 UTC (permalink / raw)
  To: Shyam Prasad N, Steve French
  Cc: CIFS, Bharath S M, Meetakshi Setiya, David Howells, samba-technical

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [WIP PATCH] allow changing the password on remount in some cases
  2024-02-16 14:41   ` Paulo Alcantara
@ 2024-02-16 17:06     ` Steve French
  2024-02-18 22:59     ` Steve French
  1 sibling, 0 replies; 8+ messages in thread
From: Steve French @ 2024-02-16 17:06 UTC (permalink / raw)
  To: Paulo Alcantara
  Cc: Shyam Prasad N, CIFS, Bharath S M, Meetakshi Setiya,
	David Howells, samba-technical

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)


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [WIP PATCH] allow changing the password on remount in some cases
  2024-02-16 14:41   ` Paulo Alcantara
  2024-02-16 17:06     ` Steve French
@ 2024-02-18 22:59     ` Steve French
  2024-02-23  7:45       ` Shyam Prasad N
  1 sibling, 1 reply; 8+ messages in thread
From: Steve French @ 2024-02-18 22:59 UTC (permalink / raw)
  To: Paulo Alcantara
  Cc: Shyam Prasad N, CIFS, Bharath S M, Meetakshi Setiya,
	David Howells, samba-technical

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

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

[-- Attachment #2: 0001-cifs-allow-changing-password-during-remount.patch --]
[-- Type: text/x-patch, Size: 5093 bytes --]

From 0ab0a5fed47634cef2a49b80a35197880aa96658 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: 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 disconnected
and the user can not reconnect due to still using old password.

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/cifs_debug.c |  2 ++
 fs/smb/client/cifsglob.h   |  1 +
 fs/smb/client/fs_context.c | 23 ++++++++++++++++++-----
 fs/smb/client/smb2pdu.c    |  5 +++++
 4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 3e4209f41c18..23d2622b969f 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -488,6 +488,8 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 				ses->ses_count, ses->serverOS, ses->serverNOS,
 				ses->capabilities, ses->ses_status);
 			}
+			if (ses->expired_pwd)
+				seq_puts(m, "password no longer valid ");
 			spin_unlock(&ses->ses_lock);
 
 			seq_printf(m, "\n\tSecurity type: %s ",
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 53c75cfb33ab..ec9a26bd05a1 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1066,6 +1066,7 @@ struct cifs_ses {
 	enum securityEnum sectype; /* what security flavor was specified? */
 	bool sign;		/* is signing required? */
 	bool domainAuto:1;
+	bool expired_pwd;  /* track if access denied or expired pwd so can know if need to update */
 	unsigned int flags;
 	__u16 session_flags;
 	__u8 smb3signingkey[SMB3_SIGN_KEY_SIZE];
diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 4b2f5aa2ea0e..99702ab05f8d 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,14 @@ 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->expired_pwd)
+		need_recon = true;
+
+	rc = smb3_verify_reconfigure_ctx(fc, ctx, cifs_sb->ctx, need_recon);
 	if (rc)
 		return rc;
 
@@ -858,7 +866,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);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 608ee05491e2..a500380d1b2e 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -1536,6 +1536,11 @@ SMB2_sess_sendreceive(struct SMB2_sess_data *sess_data)
 			    &sess_data->buf0_type,
 			    CIFS_LOG_ERROR | CIFS_SESS_OP, &rsp_iov);
 	cifs_small_buf_release(sess_data->iov[0].iov_base);
+	if (rc == 0)
+		sess_data->ses->expired_pwd = false;
+	else if ((rc == -EACCES) || (rc == -EKEYEXPIRED) || (rc == -EKEYREVOKED))
+		sess_data->ses->expired_pwd = true;
+
 	memcpy(&sess_data->iov[0], &rsp_iov, sizeof(struct kvec));
 
 	return rc;
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [WIP PATCH] allow changing the password on remount in some cases
  2024-02-18 22:59     ` Steve French
@ 2024-02-23  7:45       ` Shyam Prasad N
  2024-02-23 14:08         ` Paulo Alcantara
  0 siblings, 1 reply; 8+ messages in thread
From: Shyam Prasad N @ 2024-02-23  7:45 UTC (permalink / raw)
  To: Steve French
  Cc: Paulo Alcantara, CIFS, Bharath S M, Meetakshi Setiya,
	David Howells, samba-technical

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.

-- 
Regards,
Shyam

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [WIP PATCH] allow changing the password on remount in some cases
  2024-02-23  7:45       ` Shyam Prasad N
@ 2024-02-23 14:08         ` Paulo Alcantara
  2024-02-23 18:58           ` Steve French
  0 siblings, 1 reply; 8+ messages in thread
From: Paulo Alcantara @ 2024-02-23 14:08 UTC (permalink / raw)
  To: Shyam Prasad N, Steve French
  Cc: CIFS, Bharath S M, Meetakshi Setiya, David Howells, samba-technical

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [WIP PATCH] allow changing the password on remount in some cases
  2024-02-23 14:08         ` Paulo Alcantara
@ 2024-02-23 18:58           ` Steve French
  0 siblings, 0 replies; 8+ messages in thread
From: Steve French @ 2024-02-23 18:58 UTC (permalink / raw)
  To: Paulo Alcantara
  Cc: Shyam Prasad N, CIFS, Bharath S M, Meetakshi Setiya,
	David Howells, samba-technical

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

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.



-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-allow-changing-password-during-remount.patch --]
[-- Type: text/x-patch, Size: 5294 bytes --]

From 78d5c7683f45e55e555516de80e5a17da6ede6c4 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: 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 (for the non Kerberos case, Kerberos
ticket refresh is handled differently) when the session is disconnected
and the user can not reconnect due to still using old password.

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/cifs_debug.c |  2 ++
 fs/smb/client/cifsglob.h   |  1 +
 fs/smb/client/fs_context.c | 25 ++++++++++++++++++++-----
 fs/smb/client/smb2pdu.c    |  5 +++++
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index 3e4209f41c18..23d2622b969f 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -488,6 +488,8 @@ static int cifs_debug_data_proc_show(struct seq_file *m, void *v)
 				ses->ses_count, ses->serverOS, ses->serverNOS,
 				ses->capabilities, ses->ses_status);
 			}
+			if (ses->expired_pwd)
+				seq_puts(m, "password no longer valid ");
 			spin_unlock(&ses->ses_lock);
 
 			seq_printf(m, "\n\tSecurity type: %s ",
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 53c75cfb33ab..ec9a26bd05a1 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1066,6 +1066,7 @@ struct cifs_ses {
 	enum securityEnum sectype; /* what security flavor was specified? */
 	bool sign;		/* is signing required? */
 	bool domainAuto:1;
+	bool expired_pwd;  /* track if access denied or expired pwd so can know if need to update */
 	unsigned int flags;
 	__u16 session_flags;
 	__u8 smb3signingkey[SMB3_SIGN_KEY_SIZE];
diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 4b2f5aa2ea0e..128270e7694f 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,13 @@ 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;
+		} else if (old_ctx->sectype == Kerberos)
+			cifs_errorf(fc,
+				    "can not change password for Kerberos via remount\n");
 	}
 	if (new_ctx->domainname &&
 	    (!old_ctx->domainname || strcmp(new_ctx->domainname, old_ctx->domainname))) {
@@ -843,9 +848,14 @@ 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->expired_pwd)
+		need_recon = true;
+
+	rc = smb3_verify_reconfigure_ctx(fc, ctx, cifs_sb->ctx, need_recon);
 	if (rc)
 		return rc;
 
@@ -858,7 +868,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);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 608ee05491e2..a500380d1b2e 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -1536,6 +1536,11 @@ SMB2_sess_sendreceive(struct SMB2_sess_data *sess_data)
 			    &sess_data->buf0_type,
 			    CIFS_LOG_ERROR | CIFS_SESS_OP, &rsp_iov);
 	cifs_small_buf_release(sess_data->iov[0].iov_base);
+	if (rc == 0)
+		sess_data->ses->expired_pwd = false;
+	else if ((rc == -EACCES) || (rc == -EKEYEXPIRED) || (rc == -EKEYREVOKED))
+		sess_data->ses->expired_pwd = true;
+
 	memcpy(&sess_data->iov[0], &rsp_iov, sizeof(struct kvec));
 
 	return rc;
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-02-23 18:58 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-13  6:53 [WIP PATCH] allow changing the password on remount in some cases Steve French
2024-02-16  0:52 ` Shyam Prasad N
2024-02-16 14:41   ` Paulo Alcantara
2024-02-16 17:06     ` Steve French
2024-02-18 22:59     ` Steve French
2024-02-23  7:45       ` Shyam Prasad N
2024-02-23 14:08         ` Paulo Alcantara
2024-02-23 18:58           ` Steve French

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.