| Message ID | 1364924947-16985-3-git-send-email-simo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > owner@vger.kernel.org] On Behalf Of Simo Sorce > Sent: Tuesday, April 02, 2013 1:49 PM > To: Linux NFS Mailing list > Subject: [PATCH 2/3] Avoid reverse resolution for server name > > A NFS client should be able to work properly even if the DNS Reverse record > for the server is not set. There is no excuse to forcefully prevent that from > working when it can. Note that rpc.statd has the same limitation. > This patch adds a new -N option that takes an 'on' or 'off' argument and > controls whether forced PTR resolution is avoided (on) or performed (off) '-N' already has a very different meaning on rpc.mountd and rpc.nfsd. It might therefore be better to choose a different name to avoid confusion. Also, why do a single option with an 'on/off' argument instead of just choosing 2 options ? Trond -- 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 Tue, 2013-04-02 at 17:58 +0000, Myklebust, Trond wrote: > > -----Original Message----- > > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > > owner@vger.kernel.org] On Behalf Of Simo Sorce > > Sent: Tuesday, April 02, 2013 1:49 PM > > To: Linux NFS Mailing list > > Subject: [PATCH 2/3] Avoid reverse resolution for server name > > > > A NFS client should be able to work properly even if the DNS Reverse record > > for the server is not set. There is no excuse to forcefully prevent that from > > working when it can. > > Note that rpc.statd has the same limitation. > > > This patch adds a new -N option that takes an 'on' or 'off' argument and > > controls whether forced PTR resolution is avoided (on) or performed (off) > > '-N' already has a very different meaning on rpc.mountd and rpc.nfsd. It might therefore be better to choose a different name to avoid confusion. > Also, why do a single option with an 'on/off' argument instead of just choosing 2 options ? > Well Jeff seemed to suggest that way, I do not have any preference, can you please give me 2 letters to use ? Meanwhile I'll check rpc.statd as well. Simo.
On Tue, 2013-04-02 at 17:58 +0000, Myklebust, Trond wrote: > > -----Original Message----- > > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > > owner@vger.kernel.org] On Behalf Of Simo Sorce > > Sent: Tuesday, April 02, 2013 1:49 PM > > To: Linux NFS Mailing list > > Subject: [PATCH 2/3] Avoid reverse resolution for server name > > > > A NFS client should be able to work properly even if the DNS Reverse record > > for the server is not set. There is no excuse to forcefully prevent that from > > working when it can. > > Note that rpc.statd has the same limitation. I checked rpc.statd with a quick "git grep getnameinfo", and as far as I can see PTR lookups are done only if the 'name' is actually an IP address, but skipped if not. So I think rpc.statd is ok. I see rpc.statd uses getaddrinfo with hints set to AI_NUMERICHOST, if that's preferred over using inet_pton I can change my patches to do the same. Simo.
On 02/04/13 14:21, Simo Sorce wrote: > On Tue, 2013-04-02 at 17:58 +0000, Myklebust, Trond wrote: >>> -----Original Message----- >>> From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- >>> owner@vger.kernel.org] On Behalf Of Simo Sorce >>> Sent: Tuesday, April 02, 2013 1:49 PM >>> To: Linux NFS Mailing list >>> Subject: [PATCH 2/3] Avoid reverse resolution for server name >>> >>> A NFS client should be able to work properly even if the DNS Reverse record >>> for the server is not set. There is no excuse to forcefully prevent that from >>> working when it can. >> >> Note that rpc.statd has the same limitation. > > I checked rpc.statd with a quick "git grep getnameinfo", and as far as I > can see PTR lookups are done only if the 'name' is actually an IP > address, but skipped if not. So I think rpc.statd is ok. > > I see rpc.statd uses getaddrinfo with hints set to AI_NUMERICHOST, if > that's preferred over using inet_pton I can change my patches to do the > same. Please do.. let try to keep it consistent.... thanks! steved. -- 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 Tue, 2013-04-02 at 14:25 -0400, Steve Dickson wrote: > > On 02/04/13 14:21, Simo Sorce wrote: > > On Tue, 2013-04-02 at 17:58 +0000, Myklebust, Trond wrote: > >>> -----Original Message----- > >>> From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > >>> owner@vger.kernel.org] On Behalf Of Simo Sorce > >>> Sent: Tuesday, April 02, 2013 1:49 PM > >>> To: Linux NFS Mailing list > >>> Subject: [PATCH 2/3] Avoid reverse resolution for server name > >>> > >>> A NFS client should be able to work properly even if the DNS Reverse record > >>> for the server is not set. There is no excuse to forcefully prevent that from > >>> working when it can. > >> > >> Note that rpc.statd has the same limitation. > > > > I checked rpc.statd with a quick "git grep getnameinfo", and as far as I > > can see PTR lookups are done only if the 'name' is actually an IP > > address, but skipped if not. So I think rpc.statd is ok. > > > > I see rpc.statd uses getaddrinfo with hints set to AI_NUMERICHOST, if > > that's preferred over using inet_pton I can change my patches to do the > > same. > Please do.. let try to keep it consistent.... thanks! Ok I looked at doing it, and looked at a helper function to reuse to avoid duplication, however the functions in statd/hostname.c are static, then I looked ad host_pton in support/export/hostname.c and realized 2 things: - the comments there say inet_pton is actually better for what I need to do, in fact inet_pton is used as a filter before calling getaddrinfo. - the function can't be used in gssd_proc.c as the file is not linked there. So given the above I would prefer to not change the use of inet_pton in the patches and leave them as is. Simo.
On Tue, 02 Apr 2013 14:08:56 -0400 Simo Sorce <simo@redhat.com> wrote: > On Tue, 2013-04-02 at 17:58 +0000, Myklebust, Trond wrote: > > > -----Original Message----- > > > From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- > > > owner@vger.kernel.org] On Behalf Of Simo Sorce > > > Sent: Tuesday, April 02, 2013 1:49 PM > > > To: Linux NFS Mailing list > > > Subject: [PATCH 2/3] Avoid reverse resolution for server name > > > > > > A NFS client should be able to work properly even if the DNS Reverse record > > > for the server is not set. There is no excuse to forcefully prevent that from > > > working when it can. > > > > Note that rpc.statd has the same limitation. > > > > > This patch adds a new -N option that takes an 'on' or 'off' argument and > > > controls whether forced PTR resolution is avoided (on) or performed (off) > > > > '-N' already has a very different meaning on rpc.mountd and rpc.nfsd. It might therefore be better to choose a different name to avoid confusion. > > Also, why do a single option with an 'on/off' argument instead of just choosing 2 options ? > > > Well Jeff seemed to suggest that way, I do not have any preference, can > you please give me 2 letters to use ? > > Meanwhile I'll check rpc.statd as well. > > Simo. > FWIW, doesn't much matter to me... You just want some clear way to designate what behavior you want, and some way to tell whether it was specified at all (so you can disable the warning if it was).
On 02/04/13 13:58, Myklebust, Trond wrote: >> -----Original Message----- >> From: linux-nfs-owner@vger.kernel.org [mailto:linux-nfs- >> owner@vger.kernel.org] On Behalf Of Simo Sorce >> Sent: Tuesday, April 02, 2013 1:49 PM >> To: Linux NFS Mailing list >> Subject: [PATCH 2/3] Avoid reverse resolution for server name >> >> A NFS client should be able to work properly even if the DNS Reverse record >> for the server is not set. There is no excuse to forcefully prevent that from >> working when it can. > > Note that rpc.statd has the same limitation. > >> This patch adds a new -N option that takes an 'on' or 'off' argument and >> controls whether forced PTR resolution is avoided (on) or performed (off) > > '-N' already has a very different meaning on rpc.mountd and rpc.nfsd. It might therefore be better to choose a different name to avoid confusion. > Also, why do a single option with an 'on/off' argument instead of just choosing 2 options ? Do we really what two options? why not just have something like -Z that will restore today default? steved. > > Trond > -- > 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 > -- 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
This one does the same thing as the previous but uses -z/-Z options. Pick the one you like most, but this one is less code, so I guess it is porefereable. Simo Sorce (2): Avoid reverse resolution for server name Document new -z/-Z options utils/gssd/gss_util.h | 2 ++ utils/gssd/gssd.c | 10 ++++++++-- utils/gssd/gssd.man | 13 ++++++++++++- utils/gssd/gssd_proc.c | 25 +++++++++++++++++++++---- 4 files changed, 43 insertions(+), 7 deletions(-) -- 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
diff --git a/utils/gssd/gss_util.h b/utils/gssd/gss_util.h index aa9f77806075f9ab67a7763a75a010369ba2d1b9..663fb0998bede6144118f890b9311ee8687176e3 100644 --- a/utils/gssd/gss_util.h +++ b/utils/gssd/gss_util.h @@ -52,4 +52,6 @@ int gssd_check_mechs(void); gss_krb5_set_allowable_enctypes(min, cred, num, types) #endif +extern int avoid_ptr; + #endif /* _GSS_UTIL_H_ */ diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c index 07b1e52e6b84e9bcba96e7a63b0505ca7823482a..cc326dc2a729c1191c9f72ffd32203a58452b793 100644 --- a/utils/gssd/gssd.c +++ b/utils/gssd/gssd.c @@ -82,10 +82,19 @@ sig_hup(int signal) return; } +static int get_bool_arg(const char *arg) +{ + if (strcmp(arg, "on") == 0) + return 1; + if (strcmp(arg, "off") == 0) + return 0; + return -1; +} + static void usage(char *progname) { - fprintf(stderr, "usage: %s [-f] [-l] [-M] [-n] [-v] [-r] [-p pipefsdir] [-k keytab] [-d ccachedir] [-t timeout] [-R preferred realm]\n", + fprintf(stderr, "usage: %s [-f] [-l] [-M] [-N on|off] [-n] [-v] [-r] [-p pipefsdir] [-k keytab] [-d ccachedir] [-t timeout] [-R preferred realm]\n", progname); exit(1); } @@ -102,7 +111,7 @@ main(int argc, char *argv[]) char *progname; memset(ccachesearch, 0, sizeof(ccachesearch)); - while ((opt = getopt(argc, argv, "fvrlmnMp:k:d:t:R:")) != -1) { + while ((opt = getopt(argc, argv, "fvrlmnMp:k:d:t:R:N:")) != -1) { switch (opt) { case 'f': fg = 1; @@ -150,6 +159,11 @@ main(int argc, char *argv[]) errx(1, "Encryption type limits not supported by Kerberos libraries."); #endif break; + case 'N': + avoid_ptr = get_bool_arg(optarg); + if (avoid_ptr == -1) + errx(1, "Invalid argument for -N option"); + break; default: usage(argv[0]); break; diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c index ea01e92e4565670b97dea1a936d2f0dbdc7c4610..21d4e1d78eb54d177626cb0a19b9de4e93e0a20d 100644 --- a/utils/gssd/gssd_proc.c +++ b/utils/gssd/gssd_proc.c @@ -67,6 +67,7 @@ #include <errno.h> #include <gssapi/gssapi.h> #include <netdb.h> +#include <ctype.h> #include "gssd.h" #include "err_util.h" @@ -107,6 +108,8 @@ struct pollfd * pollarray; unsigned long pollsize; /* the size of pollaray (in pollfd's) */ +int avoid_ptr = 0; + /* * convert a presentation address string to a sockaddr_storage struct. Returns * true on success or false on failure. @@ -165,12 +168,26 @@ addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port) * convert a sockaddr to a hostname */ static char * -sockaddr_to_hostname(const struct sockaddr *sa, const char *addr) +get_servername(const char *name, const struct sockaddr *sa, const char *addr) { socklen_t addrlen; int err; char *hostname; char hbuf[NI_MAXHOST]; + unsigned char buf[sizeof(struct in6_addr)]; + int do_ptr_lookup = 0; + + if (avoid_ptr) { + /* try to determine if this is a name, or an IP address. + * If it is an IP fallback to a PTR lookup */ + if (strchr(name, '.') && inet_pton(AF_INET, name, buf) == 1) + do_ptr_lookup = 1; /* IPv4 */ + else if (strchr(name, ':') && inet_pton(AF_INET6, name, buf) == 1) + do_ptr_lookup = 1; /* or IPv6 */ + if (!do_ptr_lookup) { + return strdup(name); + } + } switch (sa->sa_family) { case AF_INET: @@ -208,7 +225,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, struct sockaddr *addr) { #define INFOBUFLEN 256 char buf[INFOBUFLEN + 1]; - static char dummy[128]; + static char server[128]; int nbytes; static char service[128]; static char address[128]; @@ -236,7 +253,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, "service: %127s %15s version %15s\n" "address: %127s\n" "protocol: %15s\n", - dummy, + server, service, program, version, address, protoname); @@ -258,7 +275,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, if (!addrstr_to_sockaddr(addr, address, port)) goto fail; - *servername = sockaddr_to_hostname(addr, address); + *servername = get_servername(server, addr, address); if (*servername == NULL) goto fail;
A NFS client should be able to work properly even if the DNS Reverse record for the server is not set. There is no excuse to forcefully prevent that from working when it can. This patch adds a new -N option that takes an 'on' or 'off' argument and controls whether forced PTR resolution is avoided (on) or performed (off) To avoid breaking current behavior the option defaults to off by default, ideally we will turn this on by default after a transition period. Signed-off-by: Simo Sorce <simo@redhat.com> --- utils/gssd/gss_util.h | 2 ++ utils/gssd/gssd.c | 18 ++++++++++++++++-- utils/gssd/gssd_proc.c | 25 +++++++++++++++++++++---- 3 files changed, 39 insertions(+), 6 deletions(-)