Re: svn commit: r1840265 - in /httpd/httpd/trunk: include/ap_mmn.h include/util_filter.h modules/http/http_request.c server/core_filters.c server/util_filter.c
I'd suggest you did a better job in the commit log explaning this than in the doxygen where it really is needed. Private declares don't belong in util_filter.h IMO. On Thu, Sep 6, 2018, 17:48 wrote: > Author: ylavic > Date: Thu Sep 6 22:48:28 2018 > New Revision: 1840265 > > URL: http://svn.apache.org/viewvc?rev=1840265=rev > Log: > Follow up to r1840149: core input filter pending data. > > Since r1840149 ap_core_input_filter() can't use use f->[priv->]bb > directly, so > ap_filter_input_pending() stopped accounting for its pending data. > > But ap_core_input_filter() can't (and doesn't need to) setaside its socket > bucket, so ap_filter_setaside_brigade() is not an option. This commit adds > ap_filter_adopt_brigade() which simply moves the given buckets (brigade) > into > f->priv->bb, and since this is not something to be done blindly (the > buckets > need to have c->pool/bucket_alloc lifetime, which is the case in the core > filter) the function is not AP_DECLAREd/exported thus can be used in core > only. > > With ap_filter_adopt_brigade() and ap_filter_reinstate_brigade(), the core > input is now ap_filter_input_pending() friendly. > > Also, ap_filter_recycle() is no more part of the API (AP_DECLARE removed > too), > there really is no point to call it outside core code. MAJOR bumped once > again > because of this. > > Modified: > httpd/httpd/trunk/include/ap_mmn.h > httpd/httpd/trunk/include/util_filter.h > httpd/httpd/trunk/modules/http/http_request.c > httpd/httpd/trunk/server/core_filters.c > httpd/httpd/trunk/server/util_filter.c > > Modified: httpd/httpd/trunk/include/ap_mmn.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1840265=1840264=1840265=diff > > == > --- httpd/httpd/trunk/include/ap_mmn.h (original) > +++ httpd/httpd/trunk/include/ap_mmn.h Thu Sep 6 22:48:28 2018 > @@ -606,12 +606,13 @@ > * ap_acquire_brigade()/ap_release_brigade(), and > * in ap_filter_t replace pending/bb/deferred_pool > * fields by struct ap_filter_private *priv > + * 20180906.1 (2.5.1-dev) Don't export ap_filter_recycle() anymore > */ > > #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */ > > #ifndef MODULE_MAGIC_NUMBER_MAJOR > -#define MODULE_MAGIC_NUMBER_MAJOR 20180905 > +#define MODULE_MAGIC_NUMBER_MAJOR 20180906 > #endif > #define MODULE_MAGIC_NUMBER_MINOR 1 /* 0...n */ > > > Modified: httpd/httpd/trunk/include/util_filter.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/include/util_filter.h?rev=1840265=1840264=1840265=diff > > == > --- httpd/httpd/trunk/include/util_filter.h (original) > +++ httpd/httpd/trunk/include/util_filter.h Thu Sep 6 22:48:28 2018 > @@ -596,6 +596,16 @@ AP_DECLARE(int) ap_filter_prepare_brigad > AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f, > apr_bucket_brigade > *bb); > > +/* > + * Adopt a bucket brigade as is (no setaside nor copy). > + * @param f The current filter > + * @param bb The bucket brigade adopted. This brigade is always empty > + * on return > + * @remark httpd internal, not exported, needed by > + * ap_core_input_filter > + */ > +void ap_filter_adopt_brigade(ap_filter_t *f, apr_bucket_brigade *bb); > + > /** > * Reinstate a brigade setaside earlier, and calculate the amount of data > we > * should write based on the presence of flush buckets, size limits on in > @@ -656,14 +666,17 @@ AP_DECLARE_NONSTD(int) ap_filter_output_ > */ > AP_DECLARE_NONSTD(int) ap_filter_input_pending(conn_rec *c); > > -/** > +/* > * Recycle removed request filters so that they can be reused for filters > * added later on the same connection. This typically should happen after > * each request handling. > * > * @param c The connection. > + * @remark httpd internal, not exported, needed by > + * ap_process_request_after_handler > + * > */ > -AP_DECLARE(void) ap_filter_recycle(conn_rec *c); > +void ap_filter_recycle(conn_rec *c); > > /** > * Flush function for apr_brigade_* calls. This calls ap_pass_brigade > > Modified: httpd/httpd/trunk/modules/http/http_request.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_request.c?rev=1840265=1840264=1840265=diff > > == &g
Re: svn commit: r1838055 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/http_core.h include/http_vhost.h server/core.c server/protocol.c server/vhost.c
> Am 06.09.2018 um 16:21 schrieb Eric Covener : > > On Thu, Sep 6, 2018 at 10:16 AM Eric Covener wrote: >> >> Today I learned that ServerAlias is not permitted in global scope. >> while writing tests in a different framework for $bigco. >> >> I would like to allow them in trunk. They are normally not needed as >> aliases are really only a way to pick the best NVH from a set of >> virtual host sharing an TCP address specification -- and if the global >> config is the best TCP match, there is only one. But this would allow >> the StrictHostCheck "don't respond to unknown hostnames" to add stuff >> when NVH'es are not (totally) used. >> >> I plan to keep it in trunk. Any concerns? > > Moments later, I learned that this might not be a very good idea after > all, as the aliases are a little too ingrained in the NVH gorp in > vhost.c. > I will probably just doc that if using this feature, one should cover > listening ports with vhosts or deal with a single name [servername] Was just about to write that there are some checks in vhost.c that seem to recognize the main server by s->names == NULL. ;-) From usability point of view, seems like an arbitrary restriction to not allow ServerAlias in global scope. It does not need to inherit... -Stefan
Re: svn commit: r1838055 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/http_core.h include/http_vhost.h server/core.c server/protocol.c server/vhost.c
On Thu, Sep 6, 2018 at 10:16 AM Eric Covener wrote: > > Today I learned that ServerAlias is not permitted in global scope. > while writing tests in a different framework for $bigco. > > I would like to allow them in trunk. They are normally not needed as > aliases are really only a way to pick the best NVH from a set of > virtual host sharing an TCP address specification -- and if the global > config is the best TCP match, there is only one. But this would allow > the StrictHostCheck "don't respond to unknown hostnames" to add stuff > when NVH'es are not (totally) used. > > I plan to keep it in trunk. Any concerns? Moments later, I learned that this might not be a very good idea after all, as the aliases are a little too ingrained in the NVH gorp in vhost.c. I will probably just doc that if using this feature, one should cover listening ports with vhosts or deal with a single name [servername]
Re: svn commit: r1838055 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/http_core.h include/http_vhost.h server/core.c server/protocol.c server/vhost.c
Today I learned that ServerAlias is not permitted in global scope.
while writing tests in a different framework for $bigco.
I would like to allow them in trunk. They are normally not needed as
aliases are really only a way to pick the best NVH from a set of
virtual host sharing an TCP address specification -- and if the global
config is the best TCP match, there is only one. But this would allow
the StrictHostCheck "don't respond to unknown hostnames" to add stuff
when NVH'es are not (totally) used.
I plan to keep it in trunk. Any concerns?
On Tue, Aug 14, 2018 at 5:47 PM wrote:
>
> Author: covener
> Date: Tue Aug 14 21:47:22 2018
> New Revision: 1838055
>
> URL: http://svn.apache.org/viewvc?rev=1838055=rev
> Log:
> Add StrictHostCheck
>
> .. to allow ucnonfigured hostnames to be rejected.
>
> The checks happen during NVH mapping and checks that the
> mapped VH itself has the host as a name or alias.
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/manual/mod/core.xml
> httpd/httpd/trunk/include/http_core.h
> httpd/httpd/trunk/include/http_vhost.h
> httpd/httpd/trunk/server/core.c
> httpd/httpd/trunk/server/protocol.c
> httpd/httpd/trunk/server/vhost.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1838055=1838054=1838055=diff
> ==
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Tue Aug 14 21:47:22 2018
> @@ -1,6 +1,9 @@
> -*- coding: utf-8
> -*-
> Changes with Apache 2.5.1
>
> + *) core: Add StrictHostCheck to allow ucnonfigured hostnames to be
> + rejected. [Eric Covener]
> +
>*) mod_status: Cumulate CPU time of exited child processes in the
> "cu" and "cs" values. Add CPU time of the parent process to the
> "c" and "s" values.
>
> Modified: httpd/httpd/trunk/docs/manual/mod/core.xml
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/core.xml?rev=1838055=1838054=1838055=diff
> ==
> --- httpd/httpd/trunk/docs/manual/mod/core.xml (original)
> +++ httpd/httpd/trunk/docs/manual/mod/core.xml Tue Aug 14 21:47:22 2018
> @@ -5240,6 +5240,40 @@ as if 'QualifyRedirectURL ON' was config
>
>
>
> +
> +StrictHostCheck
> +Controls whether the server requires the requested hostname be
> + listed enumerated in the virtual host handling the request
> +
> +StrictHostCheck ON|OFF
> +StrictHostCheck OFF
> +server configvirtual host
> +
> +Added in 2.5.1
>
> +
> +By default, the server will respond to requests for any hostname,
> +including requests addressed to unexpected or unconfigured hostnames.
> +While this is convenient, it is sometimes desirable to limit what
> hostnames
> +a backend application handles since it will often generate
> self-referential
> +responses.
> +
> +By setting StrictHostCheck to ON,
> +the server will return an HTTP 400 error if the requested hostname
> +hasn't been explicitly listed by either +>ServerName or +>ServerAlias in the virtual host that best matches the
> +details of the incoming connection.
> +
> + This directive also allows matching of the requested hostname to
> hostnames
> + specified within the opening module="core">VirtualHost
> + tag, which is a relatively obscure configuration mechanism that acts like
> + additional ServerAlias entries.
> +
> + This directive has no affect in non-default virtual hosts. The value
> + inherited from the global server configuration, or the default virtualhost
> + for the ip:port the underlying connection, determine the effective
> value.
> +
> +
>
>
>
> Modified: httpd/httpd/trunk/include/http_core.h
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_core.h?rev=1838055=1838054=1838055=diff
> ==
> --- httpd/httpd/trunk/include/http_core.h (original)
> +++ httpd/httpd/trunk/include/http_core.h Tue Aug 14 21:47:22 2018
> @@ -770,6 +770,7 @@ typedef struct {
>
> apr_size_t flush_max_threshold;
> apr_int32_t flush_max_pipelined;
> +unsigned int strict_host_check;
> } core_server_config;
>
> /* for AddOutputFiltersByType in core.c */
>
> Modified: httpd/httpd/trunk/include/http_vhost.h
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_vhost.h?rev=1838055=1838054=1838055=diff
> ==
> --- httpd/httpd/trunk/include/http_vhost.h (original)
> +++ httpd/httpd/trunk/include/http_vhost.h Tue Aug 14 21:47:22 2018
> @@ -100,6 +100,19 @@ AP_DECLARE(void) ap_update_vhost_given_i
> AP_DECLARE(void) ap_update_vhost_from_headers(request_rec *r);
>
> /**
> +
Re: Rationale on my latest util_filter changes
I like this work a lot and als like reading this explanation and details
about intentions and rabbit holes encountered. I think there could
be more of it from everyone, including myself.
About these changes in particular: making io handling more and
easier async is excellent. I'd like to use that in h2.
+1 for ap_filter_private.
re ap_filter_recycle() would a new meta bucket help solving this?
Something to insert after an EOR?
Cheers, Stefan
> Am 05.09.2018 um 19:32 schrieb Yann Ylavic :
>
> Hi,
>
> you may have noticed my commits on util_filter lately, which I didn't
> propose/argument on dev@ first, so here I am...
> My preference was to proceed like this (sequentially on each patch),
> with an idea of where to go but not necessarily the words to explain
> each part beforehand; I wish you didn't take (too much) offense for
> that, and I can always revert if you don't like it finally...
>
> Sorry for the long message by the way (you were warned :)
>
>
> First this is only about trunk only code, namely the async write
> completion improvements (started in r1706669) which introduced the
> notion of pending filters with retained/setaside bucket brigades.
>
> A few times ago I changed that code a bit by using an APR_RING instead
> of an apr_hash_t to maintain the pending filters (with a deterministic
> order, the same as the filters chain) so that flushing pending data
> from MPM event always happens from outer most filters first.
>
> This introduced a bug (PR 62668) which could have been addressed quite
> simply (once reproduced/diagnosed) by only saving the "prev" filter
> before running the current one in ap_filter_output_pending() loop.
>
> But while fixing this, I noticed that we had a bigger issue with
> ap_filter_t's lifetime for request filters: *f is destroyed with
> *f->r, so ap_filter_output_pending() can't check f->bb after f is
> called, so it can't check whether there are still pending data for f
> to stop processing (its very role...).
>
> So passing EOR downstream for ap_filter_output_pending() or any
> request filter is a big pain since it can't touch *f afterward, not to
> talk about the passed in brigade or temporary brigades it may use
> which can also be destroyed at the same time.
>
> We already discussed some aspects of this previously IIRC, and made
> minimal fixes for some brigades (ap_reuse_brigade_from_pool) in
> filters where this kind of issue was identified, but the issue with
> request filters remained for *f.
>
>
> So in r1839997, besides fixing PR 62668 as explained above, the goal
> is to allocate filters (ap_filter_t) on c->pool in any case, and
> recycle them in a ring to avoid leaks (since multiple request filters
> are added/removed during the lifetime of a conn_rec).
> But this requires more than the existing pending_{in,out}put_filters
> context in conn_rec, so I defined an opaque ap_filter_conn_ctx,
> attached to each connection, also containing the needed rings.
>
> Follow up r1840002 is about when we should recycle the ap_filter_t's,
> because if we do this at the time a filter removes itself from the
> chain (i.e. ap_remove_{in,out}put_filter), another filter might reuse
> the ap_filter_t before the former filter had a chance to return, thus
> *f might end up being misinterpreted or corrupted (think of
> ap_remove_output_filter(f) followed by ap_add_output_filter("some
> filter") followed by ap_pass_brigade(f->next), f->next is not what one
> expect it to be...).
> So what ap_remove_output_filter() now does is recycle filters in
> temporary ring (namely dead_filters) so that they can't be reused
> until the new ap_filter_recycle() function is called, which simply
> moves dead_filters to the reusable spare_filters ring.
> Now ap_filter_recycle() can be called by both
> ap_process_request_after_handler() and ap_filter_output_pending(),
> where all the filters have returned, ap_add_*_filter() can't race, and
> the more reusable filters for the next request the better.
> I would have liked to find a callback where to do this automagically,
> but couldn't think of one; any idea welcome because
> ap_filter_recycle() looks like an implementation detail for me (maybe
> at least we could not AP_DECLARE it).
> Ideas welcome!
>
> Next follow up r1840028 is optimizations of what was done in previous commits.
>
> Finally r1840149, aimed to protect struct ap_filter_t from (ab)users,
> namely f->pending, f->bb and f->deferred_pool are all util_filter use
> only and hidding them (in an opaque struct ap_filter_private) allows
> nice assertions (thus optimizations) in "util_filter.c", which would
> break if the user touched anything in there.
> This commit also adds ap_acquire_brigade() and ap_acquire_brigade() to
> replace ap_reuse_brigade_from_pool() added not so long ago. Besides
> being more efficient (ring's O(1) instead of O(1 + n/k) for pool's
> apr_hash), I think acquire/release semantics allow to cleanup the
> brigade at the right place (where
Re: TLSv1.3 supprt for 2.4.x?
> Am 05.09.2018 um 18:52 schrieb William A Rowe Jr : > > On Wed, Sep 5, 2018 at 10:52 AM, Dennis Clarke wrote: > On 09/05/2018 07:36 AM, Stefan Eissing wrote: > A member of the OpenSSL project gave me a "go ahead" and we now have branch: > > https://svn.apache.org/repos/asf/httpd/httpd/branches/tlsv1.3-for-2.4.x > > as a copy of 2.4.x with > 1827912,1827924,1827992,1828222,1828720,1828723,1833588,1833589,1839920,1839946 > merged in. If was not a clean merge as some feature from trunk are not > present in 2.4.x, so peer review/test is definitely desired. > > I put a backport proposal into 2.4.x/STATUS > > Cheers, Stefan > > > Awesome but there are plenty of folks that will want a simple tarball > with the usual autoconf/configure magic done for them. Could be a waste > of effort given that OpenSSL 1.1.1 release is 6 days away. > > Not a waste of effort. > > The project can't realistically deliver such a large changeset without wider > testing, the number of issues raised on multiple forums demonstrate that. > (Thankfully > 50% are users who were unaware of draft vs. final TLS > handshake signatures, and such inattentive users aren't productively > contributing to interoperability review.) Users who are prepared to > *constructively* engage on any proposed changeset should have few > problems with a couple extra steps. If there are interop problems at the TLS layer itself, these are not directly a concert to httpd, but a matter of IETF draft versions and implementations to sort out. At this point, we will not help resolving interop issues by holding back a release that can use TLSv1.3. > I can't imagine the project releasing this changeset without first releasing > a stable 2.4.35, followed shortly thereafter with a less stable TLS 1.3 > release. It appears to introduce a set of required(?) config changes, > something we've never purposefully done in a major.minor update. I think we will find common ground in that we do not want to interrupt existing httpd deployments with such a feature. On the other hand, we have a responsibility also as one of the leading HTTP servers on this planet, to enable our users to protect themselves by applying the latest tech in security. This, we have to balance. Precisely for this feature, we need to evaluate: - do we introduce config breaking changes? I added the optional parameter to SSLCipherSuite in a way that does not (or should not) affect existing configurations. If I erred, it would be good to find out. - does this change affect servers linked with OpenSSL 1.0.x or older? The intention of the change is to not do that. The guarding of changes by #ifdef make it work with OpenSSL 1.0.2 for me. I invited Bernard explicitly to test his libressl setups to get broader coverage. Maybe Rainer and Rüdiger will find the time to tests their various setups. - servers linking with a latest *SSL library (or distros shipping it that way) will see TLSv1.3 enabled, unless the configurations remove it explicitly. I think, based on feedback from others, this is what we want to happen. All this can and should be discussed by bringing forth technical arguments or counter examples, IMO. For the release, there will be voting, just as with other backport proposals, will it not? Cheers, Stefan
