diff mbox series

[v2] SUNRPC: Fail quickly when server does not recognize TLS

Message ID 169403069468.5590.10798268439536368989.stgit@oracle-102.nfsv4bat.org (mailing list archive)
State New, archived
Headers show
Series [v2] SUNRPC: Fail quickly when server does not recognize TLS | expand

Commit Message

Chuck Lever Sept. 6, 2023, 8:05 p.m. UTC 
From: Chuck Lever <chuck.lever@oracle.com>

rpcauth_checkverf() should return a distinct error code when a
server recognizes the AUTH_TLS probe but does not support TLS so
that the client's header decoder can respond appropriately and
quickly. No retries are necessary is in this case, since the server
has already affirmatively answered "TLS is unsupported".

Suggested-by: Trond Myklebust <trondmy@hammerspace.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/auth.c     |   11 ++++++++---
 net/sunrpc/auth_tls.c |    4 ++--
 net/sunrpc/clnt.c     |   10 +++++++++-
 3 files changed, 19 insertions(+), 6 deletions(-)

This must be applied after 'Revert "SUNRPC: Fail faster on bad verifier"'

Changes since RFC:
- Basic testing has been done
- Added an explicit check for a zero-length verifier string

Comments

Chuck Lever Sept. 6, 2023, 8:25 p.m. UTC | #1 
> On Sep 6, 2023, at 4:05 PM, Chuck Lever <cel@kernel.org> wrote:
> 
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> rpcauth_checkverf() should return a distinct error code when a
> server recognizes the AUTH_TLS probe but does not support TLS so
> that the client's header decoder can respond appropriately and
> quickly. No retries are necessary is in this case, since the server
> has already affirmatively answered "TLS is unsupported".
> 
> Suggested-by: Trond Myklebust <trondmy@hammerspace.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> net/sunrpc/auth.c     |   11 ++++++++---
> net/sunrpc/auth_tls.c |    4 ++--
> net/sunrpc/clnt.c     |   10 +++++++++-
> 3 files changed, 19 insertions(+), 6 deletions(-)
> 
> This must be applied after 'Revert "SUNRPC: Fail faster on bad verifier"'
> 
> Changes since RFC:
> - Basic testing has been done
> - Added an explicit check for a zero-length verifier string

This last bullet is stale. Instead, tls_validate() returns
PROTONOSUPPORT if the opaque token is not exactly starttls_len
bytes long.


> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 2f16f9d17966..814b0169f972 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -769,9 +769,14 @@ int rpcauth_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
>  * @task: controlling RPC task
>  * @xdr: xdr_stream containing RPC Reply header
>  *
> - * On success, @xdr is updated to point past the verifier and
> - * zero is returned. Otherwise, @xdr is in an undefined state
> - * and a negative errno is returned.
> + * Return values:
> + *   %0: Verifier is valid. @xdr now points past the verifier.
> + *   %-EIO: Verifier is corrupted or message ended early.
> + *   %-EACCES: Verifier is intact but not valid.
> + *   %-EPROTONOSUPPORT: Server does not support the requested auth type.
> + *
> + * When a negative errno is returned, @xdr is left in an undefined
> + * state.
>  */
> int
> rpcauth_checkverf(struct rpc_task *task, struct xdr_stream *xdr)
> diff --git a/net/sunrpc/auth_tls.c b/net/sunrpc/auth_tls.c
> index de7678f8a23d..87f570fd3b00 100644
> --- a/net/sunrpc/auth_tls.c
> +++ b/net/sunrpc/auth_tls.c
> @@ -129,9 +129,9 @@ static int tls_validate(struct rpc_task *task, struct xdr_stream *xdr)
> if (*p != rpc_auth_null)
> return -EIO;
> if (xdr_stream_decode_opaque_inline(xdr, &str, starttls_len) != starttls_len)
> - return -EIO;
> + return -EPROTONOSUPPORT;
> if (memcmp(str, starttls_token, starttls_len))
> - return -EIO;
> + return -EPROTONOSUPPORT;
> return 0;
> }
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 315bd59dea05..80ee97fb0bf2 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2722,7 +2722,15 @@ rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
> 
> out_verifier:
> trace_rpc_bad_verifier(task);
> - goto out_garbage;
> + switch (error) {
> + case -EPROTONOSUPPORT:
> + goto out_err;
> + case -EACCES:
> + /* Re-encode with a fresh cred */
> + fallthrough;
> + default:
> + goto out_garbage;
> + }
> 
> out_msg_denied:
> error = -EACCES;
> 
> 

--
Chuck Lever
Chuck Lever Sept. 18, 2023, 6:22 p.m. UTC | #2 
> On Sep 6, 2023, at 4:05 PM, Chuck Lever <cel@kernel.org> wrote:
> 
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> rpcauth_checkverf() should return a distinct error code when a
> server recognizes the AUTH_TLS probe but does not support TLS so
> that the client's header decoder can respond appropriately and
> quickly. No retries are necessary is in this case, since the server
> has already affirmatively answered "TLS is unsupported".
> 
> Suggested-by: Trond Myklebust <trondmy@hammerspace.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> net/sunrpc/auth.c     |   11 ++++++++---
> net/sunrpc/auth_tls.c |    4 ++--
> net/sunrpc/clnt.c     |   10 +++++++++-
> 3 files changed, 19 insertions(+), 6 deletions(-)
> 
> This must be applied after 'Revert "SUNRPC: Fail faster on bad verifier"'
> 
> Changes since RFC:
> - Basic testing has been done
> - Added an explicit check for a zero-length verifier string

Hi Anna, was this patch rejected?


> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 2f16f9d17966..814b0169f972 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -769,9 +769,14 @@ int rpcauth_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
>  * @task: controlling RPC task
>  * @xdr: xdr_stream containing RPC Reply header
>  *
> - * On success, @xdr is updated to point past the verifier and
> - * zero is returned. Otherwise, @xdr is in an undefined state
> - * and a negative errno is returned.
> + * Return values:
> + *   %0: Verifier is valid. @xdr now points past the verifier.
> + *   %-EIO: Verifier is corrupted or message ended early.
> + *   %-EACCES: Verifier is intact but not valid.
> + *   %-EPROTONOSUPPORT: Server does not support the requested auth type.
> + *
> + * When a negative errno is returned, @xdr is left in an undefined
> + * state.
>  */
> int
> rpcauth_checkverf(struct rpc_task *task, struct xdr_stream *xdr)
> diff --git a/net/sunrpc/auth_tls.c b/net/sunrpc/auth_tls.c
> index de7678f8a23d..87f570fd3b00 100644
> --- a/net/sunrpc/auth_tls.c
> +++ b/net/sunrpc/auth_tls.c
> @@ -129,9 +129,9 @@ static int tls_validate(struct rpc_task *task, struct xdr_stream *xdr)
> if (*p != rpc_auth_null)
> return -EIO;
> if (xdr_stream_decode_opaque_inline(xdr, &str, starttls_len) != starttls_len)
> - return -EIO;
> + return -EPROTONOSUPPORT;
> if (memcmp(str, starttls_token, starttls_len))
> - return -EIO;
> + return -EPROTONOSUPPORT;
> return 0;
> }
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 315bd59dea05..80ee97fb0bf2 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2722,7 +2722,15 @@ rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
> 
> out_verifier:
> trace_rpc_bad_verifier(task);
> - goto out_garbage;
> + switch (error) {
> + case -EPROTONOSUPPORT:
> + goto out_err;
> + case -EACCES:
> + /* Re-encode with a fresh cred */
> + fallthrough;
> + default:
> + goto out_garbage;
> + }
> 
> out_msg_denied:
> error = -EACCES;
> 
> 

--
Chuck Lever
Anna Schumaker Sept. 19, 2023, 6:27 p.m. UTC | #3 
On Mon, Sep 18, 2023 at 2:37 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Sep 6, 2023, at 4:05 PM, Chuck Lever <cel@kernel.org> wrote:
> >
> > From: Chuck Lever <chuck.lever@oracle.com>
> >
> > rpcauth_checkverf() should return a distinct error code when a
> > server recognizes the AUTH_TLS probe but does not support TLS so
> > that the client's header decoder can respond appropriately and
> > quickly. No retries are necessary is in this case, since the server
> > has already affirmatively answered "TLS is unsupported".
> >
> > Suggested-by: Trond Myklebust <trondmy@hammerspace.com>
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> > net/sunrpc/auth.c     |   11 ++++++++---
> > net/sunrpc/auth_tls.c |    4 ++--
> > net/sunrpc/clnt.c     |   10 +++++++++-
> > 3 files changed, 19 insertions(+), 6 deletions(-)
> >
> > This must be applied after 'Revert "SUNRPC: Fail faster on bad verifier"'
> >
> > Changes since RFC:
> > - Basic testing has been done
> > - Added an explicit check for a zero-length verifier string
>
> Hi Anna, was this patch rejected?

Nope! I was under the impression Trond would be taking it for 6.7, but
if you think it's needed earlier I can include it in the next bugfixes
pull request.

Thanks,
Anna

>
>
> > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> > index 2f16f9d17966..814b0169f972 100644
> > --- a/net/sunrpc/auth.c
> > +++ b/net/sunrpc/auth.c
> > @@ -769,9 +769,14 @@ int rpcauth_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
> >  * @task: controlling RPC task
> >  * @xdr: xdr_stream containing RPC Reply header
> >  *
> > - * On success, @xdr is updated to point past the verifier and
> > - * zero is returned. Otherwise, @xdr is in an undefined state
> > - * and a negative errno is returned.
> > + * Return values:
> > + *   %0: Verifier is valid. @xdr now points past the verifier.
> > + *   %-EIO: Verifier is corrupted or message ended early.
> > + *   %-EACCES: Verifier is intact but not valid.
> > + *   %-EPROTONOSUPPORT: Server does not support the requested auth type.
> > + *
> > + * When a negative errno is returned, @xdr is left in an undefined
> > + * state.
> >  */
> > int
> > rpcauth_checkverf(struct rpc_task *task, struct xdr_stream *xdr)
> > diff --git a/net/sunrpc/auth_tls.c b/net/sunrpc/auth_tls.c
> > index de7678f8a23d..87f570fd3b00 100644
> > --- a/net/sunrpc/auth_tls.c
> > +++ b/net/sunrpc/auth_tls.c
> > @@ -129,9 +129,9 @@ static int tls_validate(struct rpc_task *task, struct xdr_stream *xdr)
> > if (*p != rpc_auth_null)
> > return -EIO;
> > if (xdr_stream_decode_opaque_inline(xdr, &str, starttls_len) != starttls_len)
> > - return -EIO;
> > + return -EPROTONOSUPPORT;
> > if (memcmp(str, starttls_token, starttls_len))
> > - return -EIO;
> > + return -EPROTONOSUPPORT;
> > return 0;
> > }
> >
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index 315bd59dea05..80ee97fb0bf2 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -2722,7 +2722,15 @@ rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
> >
> > out_verifier:
> > trace_rpc_bad_verifier(task);
> > - goto out_garbage;
> > + switch (error) {
> > + case -EPROTONOSUPPORT:
> > + goto out_err;
> > + case -EACCES:
> > + /* Re-encode with a fresh cred */
> > + fallthrough;
> > + default:
> > + goto out_garbage;
> > + }
> >
> > out_msg_denied:
> > error = -EACCES;
> >
> >
>
> --
> Chuck Lever
>
>
Chuck Lever Sept. 19, 2023, 6:33 p.m. UTC | #4 
> On Sep 19, 2023, at 2:27 PM, Anna Schumaker <schumaker.anna@gmail.com> wrote:
> 
> On Mon, Sep 18, 2023 at 2:37 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Sep 6, 2023, at 4:05 PM, Chuck Lever <cel@kernel.org> wrote:
>>> 
>>> From: Chuck Lever <chuck.lever@oracle.com>
>>> 
>>> rpcauth_checkverf() should return a distinct error code when a
>>> server recognizes the AUTH_TLS probe but does not support TLS so
>>> that the client's header decoder can respond appropriately and
>>> quickly. No retries are necessary is in this case, since the server
>>> has already affirmatively answered "TLS is unsupported".
>>> 
>>> Suggested-by: Trond Myklebust <trondmy@hammerspace.com>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> net/sunrpc/auth.c     |   11 ++++++++---
>>> net/sunrpc/auth_tls.c |    4 ++--
>>> net/sunrpc/clnt.c     |   10 +++++++++-
>>> 3 files changed, 19 insertions(+), 6 deletions(-)
>>> 
>>> This must be applied after 'Revert "SUNRPC: Fail faster on bad verifier"'
>>> 
>>> Changes since RFC:
>>> - Basic testing has been done
>>> - Added an explicit check for a zero-length verifier string
>> 
>> Hi Anna, was this patch rejected?
> 
> Nope! I was under the impression Trond would be taking it for 6.7, but
> if you think it's needed earlier I can include it in the next bugfixes
> pull request.

Well, it goes with 'Revert "SUNRPC: Fail faster on bad verifier" '.
Otherwise that revert will cause a behavior regression for TLS
in some corner cases. If you and Trond are OK with that, it can be
left for v6.7.


> Thanks,
> Anna
> 
>> 
>> 
>>> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
>>> index 2f16f9d17966..814b0169f972 100644
>>> --- a/net/sunrpc/auth.c
>>> +++ b/net/sunrpc/auth.c
>>> @@ -769,9 +769,14 @@ int rpcauth_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
>>> * @task: controlling RPC task
>>> * @xdr: xdr_stream containing RPC Reply header
>>> *
>>> - * On success, @xdr is updated to point past the verifier and
>>> - * zero is returned. Otherwise, @xdr is in an undefined state
>>> - * and a negative errno is returned.
>>> + * Return values:
>>> + *   %0: Verifier is valid. @xdr now points past the verifier.
>>> + *   %-EIO: Verifier is corrupted or message ended early.
>>> + *   %-EACCES: Verifier is intact but not valid.
>>> + *   %-EPROTONOSUPPORT: Server does not support the requested auth type.
>>> + *
>>> + * When a negative errno is returned, @xdr is left in an undefined
>>> + * state.
>>> */
>>> int
>>> rpcauth_checkverf(struct rpc_task *task, struct xdr_stream *xdr)
>>> diff --git a/net/sunrpc/auth_tls.c b/net/sunrpc/auth_tls.c
>>> index de7678f8a23d..87f570fd3b00 100644
>>> --- a/net/sunrpc/auth_tls.c
>>> +++ b/net/sunrpc/auth_tls.c
>>> @@ -129,9 +129,9 @@ static int tls_validate(struct rpc_task *task, struct xdr_stream *xdr)
>>> if (*p != rpc_auth_null)
>>> return -EIO;
>>> if (xdr_stream_decode_opaque_inline(xdr, &str, starttls_len) != starttls_len)
>>> - return -EIO;
>>> + return -EPROTONOSUPPORT;
>>> if (memcmp(str, starttls_token, starttls_len))
>>> - return -EIO;
>>> + return -EPROTONOSUPPORT;
>>> return 0;
>>> }
>>> 
>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>> index 315bd59dea05..80ee97fb0bf2 100644
>>> --- a/net/sunrpc/clnt.c
>>> +++ b/net/sunrpc/clnt.c
>>> @@ -2722,7 +2722,15 @@ rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
>>> 
>>> out_verifier:
>>> trace_rpc_bad_verifier(task);
>>> - goto out_garbage;
>>> + switch (error) {
>>> + case -EPROTONOSUPPORT:
>>> + goto out_err;
>>> + case -EACCES:
>>> + /* Re-encode with a fresh cred */
>>> + fallthrough;
>>> + default:
>>> + goto out_garbage;
>>> + }
>>> 
>>> out_msg_denied:
>>> error = -EACCES;
>>> 
>>> 
>> 
>> --
>> Chuck Lever


--
Chuck Lever
diff mbox series

Patch

diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 2f16f9d17966..814b0169f972 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -769,9 +769,14 @@  int rpcauth_wrap_req(struct rpc_task *task, struct xdr_stream *xdr)
  * @task: controlling RPC task
  * @xdr: xdr_stream containing RPC Reply header
  *
- * On success, @xdr is updated to point past the verifier and
- * zero is returned. Otherwise, @xdr is in an undefined state
- * and a negative errno is returned.
+ * Return values:
+ *   %0: Verifier is valid. @xdr now points past the verifier.
+ *   %-EIO: Verifier is corrupted or message ended early.
+ *   %-EACCES: Verifier is intact but not valid.
+ *   %-EPROTONOSUPPORT: Server does not support the requested auth type.
+ *
+ * When a negative errno is returned, @xdr is left in an undefined
+ * state.
  */
 int
 rpcauth_checkverf(struct rpc_task *task, struct xdr_stream *xdr)
diff --git a/net/sunrpc/auth_tls.c b/net/sunrpc/auth_tls.c
index de7678f8a23d..87f570fd3b00 100644
--- a/net/sunrpc/auth_tls.c
+++ b/net/sunrpc/auth_tls.c
@@ -129,9 +129,9 @@  static int tls_validate(struct rpc_task *task, struct xdr_stream *xdr)
 	if (*p != rpc_auth_null)
 		return -EIO;
 	if (xdr_stream_decode_opaque_inline(xdr, &str, starttls_len) != starttls_len)
-		return -EIO;
+		return -EPROTONOSUPPORT;
 	if (memcmp(str, starttls_token, starttls_len))
-		return -EIO;
+		return -EPROTONOSUPPORT;
 	return 0;
 }
 
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 315bd59dea05..80ee97fb0bf2 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -2722,7 +2722,15 @@  rpc_decode_header(struct rpc_task *task, struct xdr_stream *xdr)
 
 out_verifier:
 	trace_rpc_bad_verifier(task);
-	goto out_garbage;
+	switch (error) {
+	case -EPROTONOSUPPORT:
+		goto out_err;
+	case -EACCES:
+		/* Re-encode with a fresh cred */
+		fallthrough;
+	default:
+		goto out_garbage;
+	}
 
 out_msg_denied:
 	error = -EACCES;