diff mbox series

[2/2] cifs: support mounting with alternate password to allow password rotation

Message ID 20241030142829.234828-2-meetakshisetiyaoss@gmail.com
State New
Headers show
Series [1/2] cifs: during remount, make sure passwords are in sync | expand

Commit Message

Meetakshi Setiya Oct. 30, 2024, 2:27 p.m. UTC 
From: Meetakshi Setiya <msetiya@microsoft.com>

This patch introduces the following changes to support password rotation on
mount:

1. If an existing session is not found and the new session setup results in
EACCES, EKEYEXPIRED or EKEYREVOKED, swap password and password2 (if
available), and retry the mount.

2. To match the new mount with an existing session, add conditions to check
if a) password and password2 of the new mount and the existing session are
the same, or b) password of the new mount is the same as the password2 of
the existing session, and password2 of the new mount is the same as the
password of the existing session.

3. If an existing session is found, but needs reconnect, retry the session
setup after swapping password and password2 (if available), in case the
previous attempt results in EACCES, EKEYEXPIRED or EKEYREVOKED.

Signed-off-by: Meetakshi Setiya <msetiya@microsoft.com>
---
 fs/smb/client/connect.c | 57 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

Comments

Paulo Alcantara Nov. 7, 2024, 7:05 p.m. UTC | #1 
meetakshisetiyaoss@gmail.com writes:

> @@ -2245,6 +2269,7 @@ struct cifs_ses *
>  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  {
>  	int rc = 0;
> +	int retries = 0;
>  	unsigned int xid;
>  	struct cifs_ses *ses;
>  	struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
> @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  			cifs_dbg(FYI, "Session needs reconnect\n");
>  
>  			mutex_lock(&ses->session_mutex);
> +
> +retry_old_session:
>  			rc = cifs_negotiate_protocol(xid, ses, server);
>  			if (rc) {
>  				mutex_unlock(&ses->session_mutex);
> @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  			rc = cifs_setup_session(xid, ses, server,
>  						ctx->local_nls);
>  			if (rc) {
> +				if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> +					(rc == -EKEYREVOKED)) && !retries && ses->password2) {
> +					retries++;
> +					cifs_info("Session reconnect failed, retrying with alternate password\n");

Please don't add more noisy messages over reconnect.  Remember that if
SMB session doesn't get re-established, there will be flood enough on
dmesg with "Send error in SessSetup = ..." messages on every 2s that
already pisses off users and customers.

> +					swap(ses->password, ses->password2);
> +					goto retry_old_session;
> +				}
>  				mutex_unlock(&ses->session_mutex);
>  				/* problem -- put our reference */
>  				cifs_put_smb_ses(ses);
> @@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  	ses->chans_need_reconnect = 1;
>  	spin_unlock(&ses->chan_lock);
>  
> +retry_new_session:
>  	mutex_lock(&ses->session_mutex);
>  	rc = cifs_negotiate_protocol(xid, ses, server);
>  	if (!rc)
> @@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>  	       sizeof(ses->smb3signingkey));
>  	spin_unlock(&ses->chan_lock);
>  
> -	if (rc)
> -		goto get_ses_fail;
> +	if (rc) {
> +		if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> +			(rc == -EKEYREVOKED)) && !retries && ses->password2) {
> +			retries++;
> +			cifs_info("Session setup failed, retrying with alternate password\n");

Ditto.

> +			swap(ses->password, ses->password2);
> +			goto retry_new_session;
> +		} else
> +			goto get_ses_fail;
> +	}
>  
>  	/*
>  	 * success, put it on the list and add it as first channel
> -- 
> 2.46.0.46.g406f326d27
Shyam Prasad N Nov. 8, 2024, 12:13 p.m. UTC | #2 
On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> meetakshisetiyaoss@gmail.com writes:
>
> > @@ -2245,6 +2269,7 @@ struct cifs_ses *
> >  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >  {
> >       int rc = 0;
> > +     int retries = 0;
> >       unsigned int xid;
> >       struct cifs_ses *ses;
> >       struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >                       cifs_dbg(FYI, "Session needs reconnect\n");
> >
> >                       mutex_lock(&ses->session_mutex);
> > +
> > +retry_old_session:
> >                       rc = cifs_negotiate_protocol(xid, ses, server);
> >                       if (rc) {
> >                               mutex_unlock(&ses->session_mutex);
> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >                       rc = cifs_setup_session(xid, ses, server,
> >                                               ctx->local_nls);
> >                       if (rc) {
> > +                             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> > +                                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
> > +                                     retries++;
> > +                                     cifs_info("Session reconnect failed, retrying with alternate password\n");
>
> Please don't add more noisy messages over reconnect.  Remember that if
> SMB session doesn't get re-established, there will be flood enough on
> dmesg with "Send error in SessSetup = ..." messages on every 2s that
> already pisses off users and customers.
>
Perhaps we could do a cifs_dbg instead of cifs_info.
But Paulo, the problem here is that we retry every 2s. I think we
should address that instead.
One way is to do an exponential backoff every time we retry.

I'd also want to understand why we need the reconnect work? Why not
always do smb2_reconnect when someone does filesystem calls on the
mount point?
That way, we avoid unnecessary retries altogether. @Steve French your opinions?

> > +                                     swap(ses->password, ses->password2);
> > +                                     goto retry_old_session;
> > +                             }
> >                               mutex_unlock(&ses->session_mutex);
> >                               /* problem -- put our reference */
> >                               cifs_put_smb_ses(ses);
> > @@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >       ses->chans_need_reconnect = 1;
> >       spin_unlock(&ses->chan_lock);
> >
> > +retry_new_session:
> >       mutex_lock(&ses->session_mutex);
> >       rc = cifs_negotiate_protocol(xid, ses, server);
> >       if (!rc)
> > @@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >              sizeof(ses->smb3signingkey));
> >       spin_unlock(&ses->chan_lock);
> >
> > -     if (rc)
> > -             goto get_ses_fail;
> > +     if (rc) {
> > +             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> > +                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
> > +                     retries++;
> > +                     cifs_info("Session setup failed, retrying with alternate password\n");
>
> Ditto.
>
> > +                     swap(ses->password, ses->password2);
> > +                     goto retry_new_session;
> > +             } else
> > +                     goto get_ses_fail;
> > +     }
> >
> >       /*
> >        * success, put it on the list and add it as first channel
> > --
> > 2.46.0.46.g406f326d27
Paulo Alcantara Nov. 8, 2024, 3:20 p.m. UTC | #3 
Shyam Prasad N <nspmangalore@gmail.com> writes:

> On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote:
>>
>> meetakshisetiyaoss@gmail.com writes:
>>
>> > @@ -2245,6 +2269,7 @@ struct cifs_ses *
>> >  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>> >  {
>> >       int rc = 0;
>> > +     int retries = 0;
>> >       unsigned int xid;
>> >       struct cifs_ses *ses;
>> >       struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
>> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>> >                       cifs_dbg(FYI, "Session needs reconnect\n");
>> >
>> >                       mutex_lock(&ses->session_mutex);
>> > +
>> > +retry_old_session:
>> >                       rc = cifs_negotiate_protocol(xid, ses, server);
>> >                       if (rc) {
>> >                               mutex_unlock(&ses->session_mutex);
>> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
>> >                       rc = cifs_setup_session(xid, ses, server,
>> >                                               ctx->local_nls);
>> >                       if (rc) {
>> > +                             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
>> > +                                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
>> > +                                     retries++;
>> > +                                     cifs_info("Session reconnect failed, retrying with alternate password\n");
>>
>> Please don't add more noisy messages over reconnect.  Remember that if
>> SMB session doesn't get re-established, there will be flood enough on
>> dmesg with "Send error in SessSetup = ..." messages on every 2s that
>> already pisses off users and customers.
>>
> Perhaps we could do a cifs_dbg instead of cifs_info.

Yep, with FYI.

> But Paulo, the problem here is that we retry every 2s. I think we
> should address that instead.
> One way is to do an exponential backoff every time we retry.

Agreed, but that doesn't mean we should add more noisy messages.

> I'd also want to understand why we need the reconnect work?

I see it as an optimisation to allow next IOs to not take longer because
it needs to reconnect SMB session.  If there was no prior filesystem
activity, why don't allow the client itself to reconnect the session?

Besides, SMB2_IOCTL currently doesn't call smb2_reconnect(), so
reconnect worker would be required to reconnect the session and then
allow SMB2_IOCTL to work.  We'd need to change that because with recent
special file support, we need to avoid failures when creating or parsing
reparse points because the SMB session isn't re-established yet.

> Why not always do smb2_reconnect when someone does filesystem calls on
> the mount point?

We already do for most operations.  SMB2_IOCTL and SMB2_TREE_CONNECT,
for instance, can't call smb2_reconnect() as they would deadlock.
Meetakshi Setiya Nov. 13, 2024, 11:43 a.m. UTC | #4 
Thanks for the review Paulo, here is the updated patch that uses
cifs_dbg (FYI) instead of cifs_info.

Best
Meetakshi

On Fri, Nov 8, 2024 at 8:50 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
>
> > On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote:
> >>
> >> meetakshisetiyaoss@gmail.com writes:
> >>
> >> > @@ -2245,6 +2269,7 @@ struct cifs_ses *
> >> >  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> >  {
> >> >       int rc = 0;
> >> > +     int retries = 0;
> >> >       unsigned int xid;
> >> >       struct cifs_ses *ses;
> >> >       struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
> >> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> >                       cifs_dbg(FYI, "Session needs reconnect\n");
> >> >
> >> >                       mutex_lock(&ses->session_mutex);
> >> > +
> >> > +retry_old_session:
> >> >                       rc = cifs_negotiate_protocol(xid, ses, server);
> >> >                       if (rc) {
> >> >                               mutex_unlock(&ses->session_mutex);
> >> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
> >> >                       rc = cifs_setup_session(xid, ses, server,
> >> >                                               ctx->local_nls);
> >> >                       if (rc) {
> >> > +                             if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
> >> > +                                     (rc == -EKEYREVOKED)) && !retries && ses->password2) {
> >> > +                                     retries++;
> >> > +                                     cifs_info("Session reconnect failed, retrying with alternate password\n");
> >>
> >> Please don't add more noisy messages over reconnect.  Remember that if
> >> SMB session doesn't get re-established, there will be flood enough on
> >> dmesg with "Send error in SessSetup = ..." messages on every 2s that
> >> already pisses off users and customers.
> >>
> > Perhaps we could do a cifs_dbg instead of cifs_info.
>
> Yep, with FYI.
>
> > But Paulo, the problem here is that we retry every 2s. I think we
> > should address that instead.
> > One way is to do an exponential backoff every time we retry.
>
> Agreed, but that doesn't mean we should add more noisy messages.
>
> > I'd also want to understand why we need the reconnect work?
>
> I see it as an optimisation to allow next IOs to not take longer because
> it needs to reconnect SMB session.  If there was no prior filesystem
> activity, why don't allow the client itself to reconnect the session?
>
> Besides, SMB2_IOCTL currently doesn't call smb2_reconnect(), so
> reconnect worker would be required to reconnect the session and then
> allow SMB2_IOCTL to work.  We'd need to change that because with recent
> special file support, we need to avoid failures when creating or parsing
> reparse points because the SMB session isn't re-established yet.
>
> > Why not always do smb2_reconnect when someone does filesystem calls on
> > the mount point?
>
> We already do for most operations.  SMB2_IOCTL and SMB2_TREE_CONNECT,
> for instance, can't call smb2_reconnect() as they would deadlock.
diff mbox series

Patch

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 15d94ac4095e..71b305230e14 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1898,11 +1898,35 @@  static int match_session(struct cifs_ses *ses,
 			    CIFS_MAX_USERNAME_LEN))
 			return 0;
 		if ((ctx->username && strlen(ctx->username) != 0) &&
-		    ses->password != NULL &&
-		    strncmp(ses->password,
-			    ctx->password ? ctx->password : "",
-			    CIFS_MAX_PASSWORD_LEN))
-			return 0;
+		    ses->password != NULL) {
+
+			/* New mount can only share sessions with an existing mount if:
+			 * 1. Both password and password2 match, or
+			 * 2. password2 of the old mount matches password of the new mount
+			 *    and password of the old mount matches password2 of the new
+			 *	  mount
+			 */
+			if (ses->password2 != NULL && ctx->password2 != NULL) {
+				if (!((strncmp(ses->password, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0 &&
+					strncmp(ses->password2, ctx->password2,
+					CIFS_MAX_PASSWORD_LEN) == 0) ||
+					(strncmp(ses->password, ctx->password2,
+					CIFS_MAX_PASSWORD_LEN) == 0 &&
+					strncmp(ses->password2, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0)))
+					return 0;
+
+			} else if ((ses->password2 == NULL && ctx->password2 != NULL) ||
+				(ses->password2 != NULL && ctx->password2 == NULL)) {
+				return 0;
+
+			} else {
+				if (strncmp(ses->password, ctx->password ?
+					ctx->password : "", CIFS_MAX_PASSWORD_LEN))
+					return 0;
+			}
+		}
 	}
 
 	if (strcmp(ctx->local_nls->charset, ses->local_nls->charset))
@@ -2245,6 +2269,7 @@  struct cifs_ses *
 cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 {
 	int rc = 0;
+	int retries = 0;
 	unsigned int xid;
 	struct cifs_ses *ses;
 	struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
@@ -2263,6 +2288,8 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 			cifs_dbg(FYI, "Session needs reconnect\n");
 
 			mutex_lock(&ses->session_mutex);
+
+retry_old_session:
 			rc = cifs_negotiate_protocol(xid, ses, server);
 			if (rc) {
 				mutex_unlock(&ses->session_mutex);
@@ -2275,6 +2302,13 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 			rc = cifs_setup_session(xid, ses, server,
 						ctx->local_nls);
 			if (rc) {
+				if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
+					(rc == -EKEYREVOKED)) && !retries && ses->password2) {
+					retries++;
+					cifs_info("Session reconnect failed, retrying with alternate password\n");
+					swap(ses->password, ses->password2);
+					goto retry_old_session;
+				}
 				mutex_unlock(&ses->session_mutex);
 				/* problem -- put our reference */
 				cifs_put_smb_ses(ses);
@@ -2350,6 +2384,7 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	ses->chans_need_reconnect = 1;
 	spin_unlock(&ses->chan_lock);
 
+retry_new_session:
 	mutex_lock(&ses->session_mutex);
 	rc = cifs_negotiate_protocol(xid, ses, server);
 	if (!rc)
@@ -2362,8 +2397,16 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	       sizeof(ses->smb3signingkey));
 	spin_unlock(&ses->chan_lock);
 
-	if (rc)
-		goto get_ses_fail;
+	if (rc) {
+		if (((rc == -EACCES) || (rc == -EKEYEXPIRED) ||
+			(rc == -EKEYREVOKED)) && !retries && ses->password2) {
+			retries++;
+			cifs_info("Session setup failed, retrying with alternate password\n");
+			swap(ses->password, ses->password2);
+			goto retry_new_session;
+		} else
+			goto get_ses_fail;
+	}
 
 	/*
 	 * success, put it on the list and add it as first channel