| Message ID | 1378659519-18924-1-git-send-email-Trond.Myklebust@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sep 8, 2013, at 12:58 PM, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > Commit 11ba3b1e01b67b7d19f26fba94fabdb60878e809 (Add a default flavor > to an export's e_secinfo list) breaks the ordering of security flavours > in the secinfo list, by reordering 'sec=sys' to always be the first > secinfo flavour if one fails to set a default 'sec' setting. Setting a default security flavor should occur only if no sec= option is specified. In the below case, clearly there is a sec= setting. Why was the default security flavor logic triggered anyway? > An export of the form: > > /export -sync,no_subtree_check,mp \ > 192.168.1.0/24(sec=krb5p:krb5i:krb5,rw,sec=sys,ro) > > ends up getting translated by exportfs into the following entry in > /var/lib/nfs/etab: > > /export 192.168.1.0/24(ro,sync,wdelay,hide,nocrossmnt,\ > secure,root_squash,no_all_squash,\ > no_subtree_check,secure_locks,acl,\ > mountpoint,anonuid=65534,anongid=65534,\ > sec=sys,ro,root_squash,no_all_squash,\ > sec=krb5p:krb5i:krb5,rw,root_squash,no_all_squash) > > Note how the 'sec=sys' is now listed first… > The fix is to defer adding the default flavour until the call to > secinfo_show, when we can see if it is even needed at all. > With the patch, the above export is now correctly entered in > /var/lib/nfs/etab as: > > /export 192.168.1.0/24(ro,sync,wdelay,hide,nocrossmnt,\ > secure,root_squash,no_all_squash,\ > no_subtree_check,secure_locks,acl,\ > mountpoint,anonuid=65534,anongid=65534,\ > sec=krb5p:krb5i:krb5,rw,root_squash,no_all_squash,\ > sec=sys,ro,root_squash,no_all_squash) > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > Cc: Chuck Lever <chuck.lever@oracle.com> The key is whether the derived pseudo-root security flavor setting is still correct after your fix. Did you confirm the test case in 11ba3b1's description is still addressed? > --- > support/nfs/exports.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/support/nfs/exports.c b/support/nfs/exports.c > index dea040f..3e99de6 100644 > --- a/support/nfs/exports.c > +++ b/support/nfs/exports.c > @@ -63,6 +63,7 @@ static int parsesquash(char *list, int **idp, int *lenp, char **ep); > static int parsenum(char **cpp); > static void freesquash(void); > static void syntaxerr(char *msg); > +static struct flav_info *find_flavor(char *name); > > void > setexportent(char *fname, char *type) > @@ -201,6 +202,8 @@ void secinfo_show(FILE *fp, struct exportent *ep) > struct sec_entry *p1, *p2; > int flags; > > + if (ep->e_secinfo[0].flav == NULL) > + secinfo_addflavor(find_flavor("sys"), ep); > for (p1=ep->e_secinfo; p1->flav; p1=p2) { > > fprintf(fp, ",sec=%s", p1->flav->flavour); > @@ -643,8 +646,6 @@ bad_option: > cp++; > } > > - if (ep->e_secinfo[0].flav == NULL) > - secinfo_addflavor(find_flavor("sys"), ep); > fix_pseudoflavor_flags(ep); > ep->e_squids = squids; > ep->e_sqgids = sqgids; > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2013-09-08 at 15:58 -0400, Chuck Lever wrote: > On Sep 8, 2013, at 12:58 PM, Trond Myklebust <Trond.Myklebust@netapp.com> wrote: > > > Commit 11ba3b1e01b67b7d19f26fba94fabdb60878e809 (Add a default flavor > > to an export's e_secinfo list) breaks the ordering of security flavours > > in the secinfo list, by reordering 'sec=sys' to always be the first > > secinfo flavour if one fails to set a default 'sec' setting. > > Setting a default security flavor should occur only if no sec= option is specified. In the below case, clearly there is a sec= setting. Why was the default security flavor logic triggered anyway? parseopts() is called multiple times in, for instance, getexportent(). The first time is for the default options. Then it gets called for the hostname(s). IOW: it is too early to do that kind of check in parseopts. We don't expect the security flavours to have been resolved until the entire line has been parsed. > > An export of the form: > > > > /export -sync,no_subtree_check,mp \ > > 192.168.1.0/24(sec=krb5p:krb5i:krb5,rw,sec=sys,ro) > > > > ends up getting translated by exportfs into the following entry in > > /var/lib/nfs/etab: > > > > /export 192.168.1.0/24(ro,sync,wdelay,hide,nocrossmnt,\ > > secure,root_squash,no_all_squash,\ > > no_subtree_check,secure_locks,acl,\ > > mountpoint,anonuid=65534,anongid=65534,\ > > sec=sys,ro,root_squash,no_all_squash,\ > > sec=krb5p:krb5i:krb5,rw,root_squash,no_all_squash) > > > > Note how the 'sec=sys' is now listed first… > > > The fix is to defer adding the default flavour until the call to > > secinfo_show, when we can see if it is even needed at all. > > With the patch, the above export is now correctly entered in > > /var/lib/nfs/etab as: > > > > /export 192.168.1.0/24(ro,sync,wdelay,hide,nocrossmnt,\ > > secure,root_squash,no_all_squash,\ > > no_subtree_check,secure_locks,acl,\ > > mountpoint,anonuid=65534,anongid=65534,\ > > sec=krb5p:krb5i:krb5,rw,root_squash,no_all_squash,\ > > sec=sys,ro,root_squash,no_all_squash) > > > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > > Cc: Chuck Lever <chuck.lever@oracle.com> > > The key is whether the derived pseudo-root security flavor setting is still correct after your fix. Did you confirm the test case in 11ba3b1's description is still addressed? Yes, your test case passes with flying colours. [root@dragvoll ~]# ./exportfs -v /mnt <world>(ro,wdelay,root_squash,no_subtree_check,mountpoint,sec=sys,ro,root_squash,no_all_squash) /export <world>(ro,wdelay,root_squash,no_subtree_check,mountpoint,sec=krb5i:krb5,rw,root_squash,no_all_squash,sec=sys,ro,root_squash,no_all_squash) [root@dragvoll ~]# cat /proc/net/rpc/nfsd.export/content #path domain(flags) / *(ro,root_squash,sync,no_wdelay,no_subtree_check,v4root,fsid=0,uuid=0000fd00:00000000:00000000:00000000,sec=1:390004:390003) and [root@dragvoll ~]# ./exportfs -v /export <world>(ro,wdelay,root_squash,no_subtree_check,mountpoint,sec=krb5i:krb5,rw,root_squash,no_all_squash,sec=sys,ro,root_squash,no_all_squash) /mnt <world>(ro,wdelay,root_squash,no_subtree_check,mountpoint,sec=sys,ro,root_squash,no_all_squash) [root@dragvoll ~]# cat /proc/net/rpc/nfsd.export/content #path domain(flags) / *(ro,root_squash,sync,no_wdelay,no_subtree_check,v4root,fsid=0,uuid=0000fd00:00000000:00000000:00000000,sec=390004:390003:1) Unfortunately, the general case is still broken: [root@dragvoll ~]# ./exportfs -v /export 192.168.1.0/24(ro,wdelay,root_squash,no_subtree_check,mountpoint,sec=krb5i:krb5,rw,root_squash,no_all_squash,sec=sys,ro,root_squash,no_all_squash) /mnt <world>(ro,wdelay,root_squash,no_subtree_check,mountpoint,sec=sys,ro,root_squash,no_all_squash) [root@dragvoll ~]# cat /proc/net/rpc/nfsd.export/content #path domain(flags) / *,192.168.1.0/24(ro,root_squash,sync,no_wdelay,no_subtree_check,v4root,fsid=0,uuid=0000fd00:00000000:00000000:00000000,sec=1) IOW: when the code combines 2 different exports such as the above export rules involving one export for world + one export for a subnet, it screws up and just sets the sec=sys default... -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
diff --git a/support/nfs/exports.c b/support/nfs/exports.c index dea040f..3e99de6 100644 --- a/support/nfs/exports.c +++ b/support/nfs/exports.c @@ -63,6 +63,7 @@ static int parsesquash(char *list, int **idp, int *lenp, char **ep); static int parsenum(char **cpp); static void freesquash(void); static void syntaxerr(char *msg); +static struct flav_info *find_flavor(char *name); void setexportent(char *fname, char *type) @@ -201,6 +202,8 @@ void secinfo_show(FILE *fp, struct exportent *ep) struct sec_entry *p1, *p2; int flags; + if (ep->e_secinfo[0].flav == NULL) + secinfo_addflavor(find_flavor("sys"), ep); for (p1=ep->e_secinfo; p1->flav; p1=p2) { fprintf(fp, ",sec=%s", p1->flav->flavour); @@ -643,8 +646,6 @@ bad_option: cp++; } - if (ep->e_secinfo[0].flav == NULL) - secinfo_addflavor(find_flavor("sys"), ep); fix_pseudoflavor_flags(ep); ep->e_squids = squids; ep->e_sqgids = sqgids;
Commit 11ba3b1e01b67b7d19f26fba94fabdb60878e809 (Add a default flavor to an export's e_secinfo list) breaks the ordering of security flavours in the secinfo list, by reordering 'sec=sys' to always be the first secinfo flavour if one fails to set a default 'sec' setting. An export of the form: /export -sync,no_subtree_check,mp \ 192.168.1.0/24(sec=krb5p:krb5i:krb5,rw,sec=sys,ro) ends up getting translated by exportfs into the following entry in /var/lib/nfs/etab: /export 192.168.1.0/24(ro,sync,wdelay,hide,nocrossmnt,\ secure,root_squash,no_all_squash,\ no_subtree_check,secure_locks,acl,\ mountpoint,anonuid=65534,anongid=65534,\ sec=sys,ro,root_squash,no_all_squash,\ sec=krb5p:krb5i:krb5,rw,root_squash,no_all_squash) Note how the 'sec=sys' is now listed first... The fix is to defer adding the default flavour until the call to secinfo_show, when we can see if it is even needed at all. With the patch, the above export is now correctly entered in /var/lib/nfs/etab as: /export 192.168.1.0/24(ro,sync,wdelay,hide,nocrossmnt,\ secure,root_squash,no_all_squash,\ no_subtree_check,secure_locks,acl,\ mountpoint,anonuid=65534,anongid=65534,\ sec=krb5p:krb5i:krb5,rw,root_squash,no_all_squash,\ sec=sys,ro,root_squash,no_all_squash) Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> Cc: Chuck Lever <chuck.lever@oracle.com> --- support/nfs/exports.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)