You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Chris Monson <ch...@orangatango.net> on 2003/03/04 18:26:22 UTC
[PATCH] IPLookups (revisited)
Jeff Trawick wrote:
> Chris Monson wrote:
>
> (patch)
>
> A couple of comments:
>
> Why not just store whatever flags should be passed in to
> apr_sockaddr_info_get()
I have changed it to store the flag itself, rather than an enumeration
of possible settings.
> In the code below, we already have an IPv4 address and we're getting
> apr_sockaddr_info_get() to build a representation of it. I don't
> think the IP lookup flags should be specified here.
You are correct. I have removed the offending code. The new patch
against Apache 2.0.44 is included (do I need to send a patch against the
CVS head?).
I still want to know whether it makes sense to change the entries in
proxy_util.c, and that requires a little discussion as to whether a
request_rec pointer should be required in the ap_get_iplookup_flags
function or not. Currently it is required so that we can allow for
per-directory configuration of IPLookups (which may seem like overkill,
but I am thinking of mod_rewrite and friends that may want to use it to
do things on a per-directory basis).
C
Re: [PATCH] IPLookups (revisited)
Posted by Bill Stoddard <bi...@wstoddard.com>.
Bill Stoddard wrote:
> This function is definitely useful but I'm not too keen on the directive
> name (but maybe its okay). Maybe something like:
>
> EnableIP all|ipv4|ipv6
>
> I'm also wondering if there are any needs/opportunities to control other
> ipv4 vs ipv6 behaviour of the server?
>
> Bill
>
Jeff and I spent some time talking and I think the directive name in the
patch is okay.
Bill
Re: [PATCH] IPLookups (revisited)
Posted by Bill Stoddard <bi...@wstoddard.com>.
This function is definitely useful but I'm not too keen on the directive
name (but maybe its okay). Maybe something like:
EnableIP all|ipv4|ipv6
I'm also wondering if there are any needs/opportunities to control other
ipv4 vs ipv6 behaviour of the server?
Bill
Re: [PATCH] IPLookups (revisited)
Posted by Jeff Trawick <tr...@attglobal.net>.
I'll get off my ass and pore over it soon.
Chris Monson wrote:
> So, any votes? Any thoughts?
>
> Chris Monson wrote:
>
> [PATCH]
>
>
Re: [PATCH] IPLookups (revisited)
Posted by Chris Monson <ch...@orangatango.net>.
So, any votes? Any thoughts?
Chris Monson wrote:
[PATCH]
Re: [PATCH] IPLookups (revisited)
Posted by Jeff Trawick <tr...@attglobal.net>.
Chris Monson wrote:
> Here is a (gentle) reminder that this patch is still in limbo land.
yes :(
> I am curious as to what the thoughts are on its inclusion in the
> development version (since it generates binary incompatibility, it can't
> be included in the current non-dev version).
the binary incompatibility is simply from adding new fields to the
middle of some structures, right? that can be fixed
Re: [PATCH] IPLookups (revisited)
Posted by Chris Monson <ch...@orangatango.net>.
Here is a (gentle) reminder that this patch is still in limbo land. I
have found it extremely useful in the work that I do with Apache, but I
can't really vote for myself. :-)
I am curious as to what the thoughts are on its inclusion in the
development version (since it generates binary incompatibility, it can't
be included in the current non-dev version).
C
Chris Monson wrote:
> Jeff Trawick wrote:
>
>> Chris Monson wrote:
>>
>> (patch)
>>
>> A couple of comments:
>>
>> Why not just store whatever flags should be passed in to
>> apr_sockaddr_info_get()
>
>
> I have changed it to store the flag itself, rather than an enumeration
> of possible settings.
>
>> In the code below, we already have an IPv4 address and we're getting
>> apr_sockaddr_info_get() to build a representation of it. I don't
>> think the IP lookup flags should be specified here.
>
>
> You are correct. I have removed the offending code. The new patch
> against Apache 2.0.44 is included (do I need to send a patch against
> the CVS head?).
>
> I still want to know whether it makes sense to change the entries in
> proxy_util.c, and that requires a little discussion as to whether a
> request_rec pointer should be required in the ap_get_iplookup_flags
> function or not. Currently it is required so that we can allow for
> per-directory configuration of IPLookups (which may seem like
> overkill, but I am thinking of mod_rewrite and friends that may want
> to use it to do things on a per-directory basis).
>
> C
>
>------------------------------------------------------------------------
>
>*** server/core.c Tue Mar 4 09:15:54 2003
>--- new/server/core.c Tue Mar 4 09:41:53 2003
>***************
>*** 144,149 ****
>--- 144,150 ----
> conf->use_canonical_name = USE_CANONICAL_NAME_UNSET;
>
> conf->hostname_lookups = HOSTNAME_LOOKUP_UNSET;
>+ conf->ip_lookup_flags = 0; /* the default flag is to attempt all query types */
> conf->do_rfc1413 = DEFAULT_RFC1413 | 2; /* set bit 1 to indicate default */
> conf->satisfy = SATISFY_NOSPEC;
>
>***************
>*** 620,625 ****
>--- 621,634 ----
> * here...
> */
>
>+ AP_DECLARE(apr_int32_t) ap_get_iplookup_flags(request_rec *r)
>+ {
>+ core_dir_config *conf;
>+ conf = (core_dir_config *)ap_get_module_config(r->per_dir_config,
>+ &core_module);
>+ return conf->ip_lookup_flags;
>+ }
>+
> AP_DECLARE(int) ap_allow_options(request_rec *r)
> {
> core_dir_config *conf =
>***************
>*** 2113,2118 ****
>--- 2122,2153 ----
> return NULL;
> }
>
>+ static const char *set_ip_lookups(cmd_parms *cmd, void *d_,
>+ const char *arg)
>+ {
>+ core_dir_config *d = d_;
>+ const char *err = ap_check_cmd_context(cmd, NOT_IN_LIMIT);
>+
>+ if (err != NULL) {
>+ return err;
>+ }
>+
>+ if (!strcasecmp(arg, "all")) {
>+ d->ip_lookup_flags = 0;
>+ }
>+ else if (!strcasecmp(arg, "ipv4okay")) {
>+ d->ip_lookup_flags = APR_IPV4_ADDR_OK;
>+ }
>+ else if (!strcasecmp(arg, "ipv6okay")) {
>+ d->ip_lookup_flags = APR_IPV6_ADDR_OK;
>+ }
>+ else {
>+ return "parameter must be 'all', 'ipv4okay', or 'ipv6okay'";
>+ }
>+
>+ return NULL;
>+ }
>+
> static const char *set_serverpath(cmd_parms *cmd, void *dummy,
> const char *arg)
> {
>***************
>*** 2982,2987 ****
>--- 3017,3024 ----
> ACCESS_CONF|RSRC_CONF,
> "\"on\" to enable, \"off\" to disable reverse DNS lookups, or \"double\" to "
> "enable double-reverse DNS lookups"),
>+ AP_INIT_TAKE1("IPLookups", set_ip_lookups, NULL, ACCESS_CONF | RSRC_CONF,
>+ "Set the IP lookup order (All|IPv4Okay|IPv6Okay)"),
> AP_INIT_TAKE1("ServerAdmin", set_server_string_slot,
> (void *)APR_OFFSETOF(server_rec, server_admin), RSRC_CONF,
> "The email address of the server administrator"),
>*** include/http_core.h Tue Mar 4 09:16:25 2003
>--- new/include/http_core.h Tue Mar 4 09:37:47 2003
>***************
>*** 135,140 ****
>--- 135,148 ----
> #define AP_MIN_BYTES_TO_WRITE 8000
>
> /**
>+ * Retrieve the value of IPLookups as flags for apr_sockaddr_info_get
>+ * @param r The current request
>+ * @return the flags bitmask for apr_sockaddr_info_get
>+ * @deffunc apr_int32_t ap_get_iplookup_flags(request_rec *r)
>+ */
>+ AP_DECLARE(apr_int32_t) ap_get_iplookup_flags(request_rec *r);
>+
>+ /**
> * Retrieve the value of Options for this request
> * @param r The current request
> * @return the Options bitmask
>***************
>*** 464,469 ****
>--- 472,482 ----
> #define HOSTNAME_LOOKUP_UNSET 3
> unsigned int hostname_lookups : 4;
>
>+ /* The lookup flags provide hints to any module that is interested
>+ * in doing a DNS query via apr_sockaddr_info_get. Config is IPLookups.
>+ */
>+ apr_int32_t ip_lookup_flags;
>+
> signed int do_rfc1413 : 2; /* See if client is advertising a username? */
>
> signed int content_md5 : 2; /* calculate Content-MD5? */
>*** modules/proxy/proxy_http.c Tue Mar 4 09:17:42 2003
>--- new/modules/proxy/proxy_http.c Tue Mar 4 09:20:12 2003
>***************
>*** 204,209 ****
>--- 204,210 ----
> int server_port;
> apr_status_t err;
> apr_sockaddr_t *uri_addr;
>+ apr_int32_t lookup_flags;
> /*
> * Break up the URL to determine the host to connect to
> */
>***************
>*** 222,231 ****
> "proxy: HTTP connecting %s to %s:%d", *url, uri->hostname,
> uri->port);
>
> /* do a DNS lookup for the destination host */
> /* see memory note above */
> err = apr_sockaddr_info_get(&uri_addr, apr_pstrdup(c->pool, uri->hostname),
>! APR_UNSPEC, uri->port, 0, c->pool);
>
> /* allocate these out of the connection pool - the check on
> * r->connection->id makes sure that this string does not get accessed
>--- 223,234 ----
> "proxy: HTTP connecting %s to %s:%d", *url, uri->hostname,
> uri->port);
>
>+ lookup_flags = ap_get_iplookup_flags(r);
>+
> /* do a DNS lookup for the destination host */
> /* see memory note above */
> err = apr_sockaddr_info_get(&uri_addr, apr_pstrdup(c->pool, uri->hostname),
>! APR_UNSPEC, uri->port, lookup_flags, c->pool);
>
> /* allocate these out of the connection pool - the check on
> * r->connection->id makes sure that this string does not get accessed
>***************
>*** 236,242 ****
> p_conn->port = proxyport;
> /* see memory note above */
> err = apr_sockaddr_info_get(&p_conn->addr, p_conn->name, APR_UNSPEC,
>! p_conn->port, 0, c->pool);
> } else {
> p_conn->name = apr_pstrdup(c->pool, uri->hostname);
> p_conn->port = uri->port;
>--- 239,245 ----
> p_conn->port = proxyport;
> /* see memory note above */
> err = apr_sockaddr_info_get(&p_conn->addr, p_conn->name, APR_UNSPEC,
>! p_conn->port, lookup_flags, c->pool);
> } else {
> p_conn->name = apr_pstrdup(c->pool, uri->hostname);
> p_conn->port = uri->port;
>*** modules/proxy/proxy_ftp.c Tue Mar 4 09:17:50 2003
>--- new/modules/proxy/proxy_ftp.c Tue Mar 4 09:41:00 2003
>***************
>*** 801,806 ****
>--- 801,807 ----
> #if defined(USE_MDTM) && (defined(HAVE_TIMEGM) || defined(HAVE_GMTOFF))
> apr_time_t mtime = 0L;
> #endif
>+ apr_int32_t lookup_flags;
>
> /* stuff for PASV mode */
> int connect = 0, use_port = 0;
>***************
>*** 820,825 ****
>--- 821,829 ----
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
> "proxy: FTP: serving URL %s", url);
>
>+ /* Get the appropriate DNS lookup flags */
>+ lookup_flags = ap_get_iplookup_flags(r);
>+
> /* create space for state information */
> backend = (proxy_conn_rec *) ap_get_module_config(c->conn_config, &proxy_ftp_module);
> if (!backend) {
>***************
>*** 914,920 ****
> "proxy: FTP: connecting %s to %s:%d", url, connectname, connectport);
>
> /* do a DNS lookup for the destination host */
>! err = apr_sockaddr_info_get(&connect_addr, connectname, APR_UNSPEC, connectport, 0, p);
>
> /* check if ProxyBlock directive on this host */
> if (OK != ap_proxy_checkproxyblock(r, conf, connect_addr)) {
>--- 918,924 ----
> "proxy: FTP: connecting %s to %s:%d", url, connectname, connectport);
>
> /* do a DNS lookup for the destination host */
>! err = apr_sockaddr_info_get(&connect_addr, connectname, APR_UNSPEC, connectport, lookup_flags, p);
>
> /* check if ProxyBlock directive on this host */
> if (OK != ap_proxy_checkproxyblock(r, conf, connect_addr)) {
>*** modules/proxy/proxy_connect.c Tue Mar 4 09:17:47 2003
>--- new/modules/proxy/proxy_connect.c Tue Mar 4 09:41:11 2003
>***************
>*** 135,140 ****
>--- 135,141 ----
> apr_int32_t pollcnt;
> apr_int16_t pollevent;
> apr_sockaddr_t *uri_addr, *connect_addr;
>+ apr_int32_t lookup_flags;
>
> apr_uri_t uri;
> const char *connectname;
>***************
>*** 165,178 ****
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
> "proxy: CONNECT: connecting %s to %s:%d", url, uri.hostname, uri.port);
>
> /* do a DNS lookup for the destination host */
>! err = apr_sockaddr_info_get(&uri_addr, uri.hostname, APR_UNSPEC, uri.port, 0, p);
>
> /* are we connecting directly, or via a proxy? */
> if (proxyname) {
> connectname = proxyname;
> connectport = proxyport;
>! err = apr_sockaddr_info_get(&connect_addr, proxyname, APR_UNSPEC, proxyport, 0, p);
> }
> else {
> connectname = uri.hostname;
>--- 166,181 ----
> ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
> "proxy: CONNECT: connecting %s to %s:%d", url, uri.hostname, uri.port);
>
>+ lookup_flags = ap_get_iplookup_flags(r);
>+
> /* do a DNS lookup for the destination host */
>! err = apr_sockaddr_info_get(&uri_addr, uri.hostname, APR_UNSPEC, uri.port, lookup_flags, p);
>
> /* are we connecting directly, or via a proxy? */
> if (proxyname) {
> connectname = proxyname;
> connectport = proxyport;
>! err = apr_sockaddr_info_get(&connect_addr, proxyname, APR_UNSPEC, proxyport, lookup_flags, p);
> }
> else {
> connectname = uri.hostname;
>
>
Re: [PATCH] IPLookups (revisited)
Posted by Bill Stoddard <bi...@wstoddard.com>.
Jeff Trawick wrote:
> Chris Monson wrote:
>
>> I still want to know whether it makes sense to change the entries in
>> proxy_util.c,
>
>
>
> what does "change the entries in proxy_util.c" mean?
>
>> and that requires a little discussion as to whether a
>> request_rec pointer should be required in the ap_get_iplookup_flags
>> function or not. Currently it is required so that we can allow for
>> per-directory configuration of IPLookups (which may seem like overkill,
>> but I am thinking of mod_rewrite and friends that may want to use it to
>> do things on a per-directory basis).
>
>
>
> does anybody else have an opinion here? I'm guessing it isn't
> necessary, but I don't have a good feel
I don't think enabling IPLookups per directory is necessary.
Bill
Re: [PATCH] IPLookups (revisited)
Posted by Chris Monson <ch...@orangatango.net>.
> what does "change the entries in proxy_util.c" mean?
There are some calls to apr_sockaddr_info_get in proxy_util.c, and I
wasn't really sure whether they should be looking at this flag or not.
In some cases (where we already have an IP address, etc) it is clearly
something that they should not do.
>> <snip>
>> do things on a per-directory basis).
>
>
> does anybody else have an opinion here? I'm guessing it isn't
> necessary, but I don't have a good feel
I think it is probably not necessary to do things on a per-directory
basis, either. I can certainly modify the patch to remove the request
dependency, but *some* information will need to be passed into the
configuration accessor (like the server_rec, etc). Any thoughts?
Again, my motivation for allowing per-directory stuff had more to do
with the fact that HostNameLookups does the same thing than anything
else. Is anyone using that functionality for HostNameLookups, or is
that also overkill?
> I would like us to create an interface that allows "IPLookups
> IPv[46]Only" to be possible with some follow-on enhancements, so
> I think this needs to tell the caller two things:
>
> what family to pass to apr_sockaddr_info_get
> what flags to turn on for apr_sockaddr_info_get
>
> If the conf file says
>
> IPLookups All
>
> returned family is APR_UNSPEC, returned flag is 0
>
> IPLookups IPv4OK
>
> returned family is APR_UNSPEC, returned flag is APR_IPV4_ADDR_OK
>
> IPLookups IPv6OK
>
> returned family is APR_UNSPEC, returned flag is APR_IPV6_ADDR_OK
>
> and after future enhancement
>
> IPLookups IPv4Only
>
> returned family is APR_INET, returned flag is 0
>
> IPLookups IPv6Only
>
> returned family is APR_INET6, returned flag is 0
>
> so no return code, and two apr_int32_t * parameters after request_rec *
I started to implement this, and then I checked the archives and found
that it had been discussed previously. Adding IPvxOnly was considered
the "optimization of an error path" and therefore unnecessary, but this
was a while ago. If there are fresh insights, I would love to hear
them. I don't have much of a feel for this particular issue, though the
implementation would be trivial.
C
Re: [PATCH] IPLookups (revisited)
Posted by Jeff Trawick <tr...@attglobal.net>.
Chris Monson wrote:
> I still want to know whether it makes sense to change the entries in
> proxy_util.c,
what does "change the entries in proxy_util.c" mean?
> and that requires a little discussion as to whether a
> request_rec pointer should be required in the ap_get_iplookup_flags
> function or not. Currently it is required so that we can allow for
> per-directory configuration of IPLookups (which may seem like overkill,
> but I am thinking of mod_rewrite and friends that may want to use it to
> do things on a per-directory basis).
does anybody else have an opinion here? I'm guessing it isn't
necessary, but I don't have a good feel
> + AP_DECLARE(apr_int32_t) ap_get_iplookup_flags(request_rec *r)
I would like us to create an interface that allows "IPLookups
IPv[46]Only" to be possible with some follow-on enhancements, so
I think this needs to tell the caller two things:
what family to pass to apr_sockaddr_info_get
what flags to turn on for apr_sockaddr_info_get
If the conf file says
IPLookups All
returned family is APR_UNSPEC, returned flag is 0
IPLookups IPv4OK
returned family is APR_UNSPEC, returned flag is APR_IPV4_ADDR_OK
IPLookups IPv6OK
returned family is APR_UNSPEC, returned flag is APR_IPV6_ADDR_OK
and after future enhancement
IPLookups IPv4Only
returned family is APR_INET, returned flag is 0
IPLookups IPv6Only
returned family is APR_INET6, returned flag is 0
so no return code, and two apr_int32_t * parameters after request_rec *