Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5463b973-7395-8150-f1f7-fe26c3d86443@redhat.com>
Date:   Thu, 23 Mar 2023 13:53:48 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v1 2/4] exports: Add an xprtsec= export option
To:     Jeff Layton <jlayton@kernel.org>,
        Chuck Lever III <chuck.lever@oracle.com>
Cc:     Chuck Lever <cel@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
References: <167932279970.3437.7130911928591001093.stgit@manet.1015granger.net>
 <167932293857.3437.10836642078898996996.stgit@manet.1015granger.net>
 <b1da1fdbb190f50409f9fcd18b466defdfc04353.camel@kernel.org>
 <07F10068-A3E6-4C45-BB1E-F67FF4378155@oracle.com>
 <01df993e636b200dbd7636946761208bb183d5c7.camel@kernel.org>
Content-Language: en-US
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <01df993e636b200dbd7636946761208bb183d5c7.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


Sorry for the delayed response....

On 3/21/23 2:58 PM, Jeff Layton wrote:
> On Tue, 2023-03-21 at 18:08 +0000, Chuck Lever III wrote:
>>
>>> On Mar 21, 2023, at 7:55 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>>
>>> On Mon, 2023-03-20 at 10:35 -0400, Chuck Lever wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>
>>>> The overall goal is to enable administrators to require the use of
>>>> transport layer security when clients access particular exports.
>>>>
>>>> This patch adds support to exportfs to parse and display a new
>>>> xprtsec= export option. The setting is not yet passed to the kernel.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>> support/include/nfs/export.h |    6 +++
>>>> support/include/nfslib.h     |   14 +++++++
>>>> support/nfs/exports.c        |   85 ++++++++++++++++++++++++++++++++++++++++++
>>>> utils/exportfs/exportfs.c    |    1
>>>> 4 files changed, 106 insertions(+)
>>>>
>>>> diff --git a/support/include/nfs/export.h b/support/include/nfs/export.h
>>>> index 0eca828ee3ad..b29c6fa4f554 100644
>>>> --- a/support/include/nfs/export.h
>>>> +++ b/support/include/nfs/export.h
>>>> @@ -40,4 +40,10 @@
>>>> #define NFSEXP_OLD_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
>>>> 					| NFSEXP_ALLSQUASH)
>>>>
>>>> +enum {
>>>> +	NFSEXP_XPRTSEC_NONE = 1,
>>>> +	NFSEXP_XPRTSEC_TLS = 2,
>>>> +	NFSEXP_XPRTSEC_MTLS = 3,
>>>> +};
>>>> +
>>>
>>> Can we put these into a uapi header somewhere and then just have
>>> nfs-utils use those if they're defined?
>>
>> I moved these to include/uapi/linux/nfsd/export.h in the
>> kernel patches, and adjust the nfs-utils patches to use the
>> same numeric values in exportfs as the kernel.
>>
>> But it's not clear how a uAPI header would become visible
>> during, say, an RPM build of nfs-utils. Does anyone know
>> how that works? The kernel docs I've read suggest uAPI is
>> for user space tools that actually live in the kernel source
>> tree.
>>
> 
> Unfortunately, you need to build on a box that has kernel headers from
> an updated kernel.
I would not like to add this dependency to nfs-utils or sub-packages.

> 
> The usual way to deal with this is to have a copy in the userland
> sources but only define them if one of the relevant constants isn't
> already defined.
> 
> So you'll probably want to keep something like this in the userland
> tree:
> 
> #ifndef NFSEXP_XPRTSEC_NONE
> # define NFSEXP_XPRTSEC_NONE 1
> # define NFSEXP_XPRTSEC_TLS  2
> # define NFSEXP_XPRTSEC_MTLS 3
> #endif
> 
> There may be some way to do that and keep it as an enum too. I'm not
> sure.
Is there a reason these could not be in export.h?

steved.

> 
>> I think the cases where only user space or only the kernel
>> support xprtsec should work OK: the kernel has a default
>> transport layer security policy of "all ok" and old kernels
>> ignore export options from user space they don't recognize.
>>
> 
> Great!
> 
>>>> #endif /* _NSF_EXPORT_H */
>>>> diff --git a/support/include/nfslib.h b/support/include/nfslib.h
>>>> index 6faba71bf0cd..9a188fb84790 100644
>>>> --- a/support/include/nfslib.h
>>>> +++ b/support/include/nfslib.h
>>>> @@ -62,6 +62,18 @@ struct sec_entry {
>>>> 	int flags;
>>>> };
>>>>
>>>> +#define XPRTSECMODE_COUNT 4
>>>> +
>>>> +struct xprtsec_info {
>>>> +	const char		*name;
>>>> +	int			number;
>>>> +};
>>>> +
>>>> +struct xprtsec_entry {
>>>> +	const struct xprtsec_info *info;
>>>> +	int			flags;
>>>> +};
>>>> +
>>>> /*
>>>>   * Data related to a single exports entry as returned by getexportent.
>>>>   * FIXME: export options should probably be parsed at a later time to
>>>> @@ -83,6 +95,7 @@ struct exportent {
>>>> 	char *          e_fslocdata;
>>>> 	char *		e_uuid;
>>>> 	struct sec_entry e_secinfo[SECFLAVOR_COUNT+1];
>>>> +	struct xprtsec_entry e_xprtsec[XPRTSECMODE_COUNT + 1];
>>>> 	unsigned int	e_ttl;
>>>> 	char *		e_realpath;
>>>> };
>>>> @@ -99,6 +112,7 @@ struct rmtabent {
>>>> void			setexportent(char *fname, char *type);
>>>> struct exportent *	getexportent(int,int);
>>>> void 			secinfo_show(FILE *fp, struct exportent *ep);
>>>> +void			xprtsecinfo_show(FILE *fp, struct exportent *ep);
>>>> void			putexportent(struct exportent *xep);
>>>> void			endexportent(void);
>>>> struct exportent *	mkexportent(char *hname, char *path, char *opts);
>>>> diff --git a/support/nfs/exports.c b/support/nfs/exports.c
>>>> index 7f12383981c3..da8ace3a65fd 100644
>>>> --- a/support/nfs/exports.c
>>>> +++ b/support/nfs/exports.c
>>>> @@ -99,6 +99,7 @@ static void init_exportent (struct exportent *ee, int fromkernel)
>>>> 	ee->e_fslocmethod = FSLOC_NONE;
>>>> 	ee->e_fslocdata = NULL;
>>>> 	ee->e_secinfo[0].flav = NULL;
>>>> +	ee->e_xprtsec[0].info = NULL;
>>>> 	ee->e_nsquids = 0;
>>>> 	ee->e_nsqgids = 0;
>>>> 	ee->e_uuid = NULL;
>>>> @@ -248,6 +249,17 @@ void secinfo_show(FILE *fp, struct exportent *ep)
>>>> 	}
>>>> }
>>>>
>>>> +void xprtsecinfo_show(FILE *fp, struct exportent *ep)
>>>> +{
>>>> +	struct xprtsec_entry *p1, *p2;
>>>> +
>>>> +	for (p1 = ep->e_xprtsec; p1->info; p1 = p2) {
>>>> +		fprintf(fp, ",xprtsec=%s", p1->info->name);
>>>> +		for (p2 = p1 + 1; p2->info && (p1->flags == p2->flags); p2++)
>>>> +			fprintf(fp, ":%s", p2->info->name);
>>>> +	}
>>>> +}
>>>> +
>>>> static void
>>>> fprintpath(FILE *fp, const char *path)
>>>> {
>>>> @@ -344,6 +356,7 @@ putexportent(struct exportent *ep)
>>>> 	}
>>>> 	fprintf(fp, "anonuid=%d,anongid=%d", ep->e_anonuid, ep->e_anongid);
>>>> 	secinfo_show(fp, ep);
>>>> +	xprtsecinfo_show(fp, ep);
>>>> 	fprintf(fp, ")\n");
>>>> }
>>>>
>>>> @@ -482,6 +495,75 @@ static unsigned int parse_flavors(char *str, struct exportent *ep)
>>>> 	return out;
>>>> }
>>>>
>>>> +static const struct xprtsec_info xprtsec_name2info[] = {
>>>> +	{ "none",	NFSEXP_XPRTSEC_NONE },
>>>> +	{ "tls",	NFSEXP_XPRTSEC_TLS },
>>>> +	{ "mtls",	NFSEXP_XPRTSEC_MTLS },
>>>> +	{ NULL,		0 }
>>>> +};
>>>> +
>>>> +static const struct xprtsec_info *find_xprtsec_info(const char *name)
>>>> +{
>>>> +	const struct xprtsec_info *info;
>>>> +
>>>> +	for (info = xprtsec_name2info; info->name; info++)
>>>> +		if (strcmp(info->name, name) == 0)
>>>> +			return info;
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Append the given xprtsec mode to the exportent's e_xprtsec array,
>>>> + * or do nothing if it's already there. Returns the index of flavor in
>>>> + * the resulting array in any case.
>>>> + */
>>>> +static int xprtsec_addmode(const struct xprtsec_info *info, struct exportent *ep)
>>>> +{
>>>> +	struct xprtsec_entry *p;
>>>> +
>>>> +	for (p = ep->e_xprtsec; p->info; p++)
>>>> +		if (p->info == info || p->info->number == info->number)
>>>> +			return p - ep->e_xprtsec;
>>>> +
>>>> +	if (p - ep->e_xprtsec >= XPRTSECMODE_COUNT) {
>>>> +		xlog(L_ERROR, "more than %d xprtsec modes on an export\n",
>>>> +			XPRTSECMODE_COUNT);
>>>> +		return -1;
>>>> +	}
>>>> +	p->info = info;
>>>> +	p->flags = ep->e_flags;
>>>> +	(p + 1)->info = NULL;
>>>> +	return p - ep->e_xprtsec;
>>>> +}
>>>> +
>>>> +/*
>>>> + * @str is a colon seperated list of transport layer security modes.
>>>> + * Their order is recorded in @ep, and a bitmap corresponding to the
>>>> + * list is returned.
>>>> + *
>>>> + * A zero return indicates an error.
>>>> + */
>>>> +static unsigned int parse_xprtsec(char *str, struct exportent *ep)
>>>> +{
>>>> +	unsigned int out = 0;
>>>> +	char *name;
>>>> +
>>>> +	while ((name = strsep(&str, ":"))) {
>>>> +		const struct xprtsec_info *info = find_xprtsec_info(name);
>>>> +		int bit;
>>>> +
>>>> +		if (!info) {
>>>> +			xlog(L_ERROR, "unknown xprtsec mode %s\n", name);
>>>> +			return 0;
>>>> +		}
>>>> +		bit = xprtsec_addmode(info, ep);
>>>> +		if (bit < 0)
>>>> +			return 0;
>>>> +		out |= 1 << bit;
>>>> +	}
>>>> +	return out;
>>>> +}
>>>> +
>>>> /* Sets the bits in @mask for the appropriate security flavor flags. */
>>>> static void setflags(int mask, unsigned int active, struct exportent *ep)
>>>> {
>>>> @@ -687,6 +769,9 @@ bad_option:
>>>> 			active = parse_flavors(opt+4, ep);
>>>> 			if (!active)
>>>> 				goto bad_option;
>>>> +		} else if (strncmp(opt, "xprtsec=", 8) == 0) {
>>>> +			if (!parse_xprtsec(opt + 8, ep))
>>>> +				goto bad_option;
>>>> 		} else {
>>>> 			xlog(L_ERROR, "%s:%d: unknown keyword \"%s\"\n",
>>>> 					flname, flline, opt);
>>>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
>>>> index 6d79a5b3480d..37b9e4b3612d 100644
>>>> --- a/utils/exportfs/exportfs.c
>>>> +++ b/utils/exportfs/exportfs.c
>>>> @@ -743,6 +743,7 @@ dump(int verbose, int export_format)
>>>> #endif
>>>> 			}
>>>> 			secinfo_show(stdout, ep);
>>>> +			xprtsecinfo_show(stdout, ep);
>>>> 			printf("%c\n", (c != '(')? ')' : ' ');
>>>> 		}
>>>> 	}
>>>>
>>>>
>>>
>>> -- 
>>> Jeff Layton <jlayton@kernel.org>
>>
>> --
>> Chuck Lever
>>
>>
>