Date:   Tue, 19 Apr 2022 16:00:40 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Trond Myklebust <trondmy@...merspace.com>
CC:     Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-cifs@...r.kernel.org" <linux-cifs@...r.kernel.org>,
        "borisp@...dia.com" <borisp@...dia.com>,
        "simo@...hat.com" <simo@...hat.com>,
        "ak@...pesta-tech.com" <ak@...pesta-tech.com>
Subject: Re: [PATCH RFC 00/15] Prototype implementation of RPC-with-TLS

Hi Trond-

Thanks for the early review!


> On Apr 18, 2022, at 11:31 PM, Trond Myklebust <trondmy@...merspace.com> wrote:
> 
> On Mon, 2022-04-18 at 12:51 -0400, Chuck Lever wrote:
>> This series implements RPC-with-TLS in the Linux kernel:
>> 
>> https://datatracker.ietf.org/doc/draft-ietf-nfsv4-rpc-tls/
>> 
>> This prototype is based on the previously posted mechanism for
>> providing a TLS handshake facility to in-kernel TLS consumers.
>> 
>> For the purpose of demonstration, the Linux NFS client is modified
>> to add a new mount option: xprtsec = [ none|auto|tls ] . Updates
>> to the nfs(5) man page are being developed separately.
>> 
> 
> I'm fine with having a userspace level 'auto' option if that's a
> requirement for someone, however I see no reason why we would need to
> implement that in the kernel.
> 
> Let's just have a robust mechanism for immediately returning an error
> if the user supplies a 'tls' option on the client that the server
> doesn't support, and let the negotiation policy be worked out in
> userspace by the 'mount.nfs' utility. Otherwise we'll rathole into
> another twisty maze of policy decisions that generate kernel level CVEs
> instead of a set of more gentle fixes.

Noted.

However, one of Rick's preferences is that "auto" not use
transport-layer security unless the server requires it via
a SECINFO/MNT pseudoflavor, which only the kernel would be
privy to. I'll have to think about whether we want to make
that happen.


>> The new mount option enables client administrators to require in-
>> transit encryption for their NFS traffic, protecting the weak
>> security of AUTH_SYS. An x.509 certificate is not required on the
>> client for this protection.
> 
> That doesn't really do much to 'protect the weak security of AUTH_SYS'.

My description doesn't really explain the whole plan, it
introduces only what's in the current prototype. Eventually
I'd like to do this:

  xprtsec= none | auto | tls | mtls | psk | ...

where
  none: transport-layer security is explicitly disabled
  auto: pick one based on what authentication material is available
  tls: encryption-only TLSv1.3 (no client cert needed)
  mtls: encryption and mutual authentication (client cert required)
  psk: pre-shared key
  ...: we could require wiregard, EAP, or IPSEC if someone cares
       to implement one or more of them


> It just means that nobody can tamper with our AUTH_SYS credential while
> in flight. It is still quite possible for the client to spoof both its
> IP address and user/group credentials.

True enough. But some folks are interested only in encryption.
They trust their clients, but not the network.


> A better recommendation would be to have users select sys=krb5 when
> they have the ability to do so. Doing so ensures that both the client
> and server are authenticating to one another, while also guaranteeing
> RPC message integrity and privacy.

With xprtsec=mtls (see above), the server and client mutually
authenticate, which provides a higher degree of security as you
describe here.

I agree that xprtsec=tls + sec=krb5 is probably the ultimate
combination of security with the least performance compromise.
The prototype posted here should support this combination right
now.


>> This prototype has been tested against prototype TLS-capable NFS
>> servers. The Linux NFS server itself does not yet have support for
>> RPC-with-TLS, but it is planned.
>> 
>> At a later time, the Linux NFS client will also get support for
>> x.509 authentication (for which a certificate will be required on
>> the client) and PSK. For this demonstration, only authentication-
>> less TLS (encryption-only) is supported.
>> 
>> ---
>> 
>> Chuck Lever (15):
>>       SUNRPC: Replace dprintk() call site in xs_data_ready
>>       SUNRPC: Ignore data_ready callbacks during TLS handshakes
>>       SUNRPC: Capture cmsg metadata on client-side receive
>>       SUNRPC: Fail faster on bad verifier
>>       SUNRPC: Widen rpc_task::tk_flags
>>       SUNRPC: Add RPC client support for the RPC_AUTH_TLS
>> authentication flavor
>>       SUNRPC: Refactor rpc_call_null_helper()
>>       SUNRPC: Add RPC_TASK_CORK flag
>>       SUNRPC: Add a cl_xprtsec_policy field
>>       SUNRPC: Expose TLS policy via the rpc_create() API
>>       SUNRPC: Add infrastructure for async RPC_AUTH_TLS probe
>>       SUNRPC: Add FSM machinery to handle RPC_AUTH_TLS on reconnect
>>       NFS: Replace fs_context-related dprintk() call sites with
>> tracepoints
>>       NFS: Have struct nfs_client carry a TLS policy field
>>       NFS: Add an "xprtsec=" NFS mount option
>> 
>> 
>>  fs/nfs/client.c                 |  22 ++++
>>  fs/nfs/fs_context.c             |  70 ++++++++--
>>  fs/nfs/internal.h               |   2 +
>>  fs/nfs/nfs3client.c             |   1 +
>>  fs/nfs/nfs4client.c             |  16 ++-
>>  fs/nfs/nfstrace.h               |  77 +++++++++++
>>  fs/nfs/super.c                  |  10 ++
>>  include/linux/nfs_fs_sb.h       |   7 +-
>>  include/linux/sunrpc/auth.h     |   1 +
>>  include/linux/sunrpc/clnt.h     |  14 +-
>>  include/linux/sunrpc/sched.h    |  36 +++---
>>  include/linux/sunrpc/xprt.h     |  14 ++
>>  include/linux/sunrpc/xprtsock.h |   2 +
>>  include/net/tls.h               |   2 +
>>  include/trace/events/sunrpc.h   | 157 ++++++++++++++++++++--
>>  net/sunrpc/Makefile             |   2 +-
>>  net/sunrpc/auth.c               |   2 +
>>  net/sunrpc/auth_tls.c           | 117 +++++++++++++++++
>>  net/sunrpc/clnt.c               | 222 +++++++++++++++++++++++++++++-
>> --
>>  net/sunrpc/debugfs.c            |   2 +-
>>  net/sunrpc/xprt.c               |   3 +
>>  net/sunrpc/xprtsock.c           | 211 +++++++++++++++++++++++++++++-
>>  22 files changed, 920 insertions(+), 70 deletions(-)
>>  create mode 100644 net/sunrpc/auth_tls.c
>> 
>> --
>> Chuck Lever
>> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@...merspace.com

--
Chuck Lever

Powered by blists - more mailing lists