Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
On Wed, 29 May 2013 17:06:14 +0100 Joe Orton wrote: > On Wed, May 29, 2013 at 11:37:14AM -0400, Matthew Steele wrote: > > Oops, yes, RUN_ALL semantics are desired; the misleading API > > description is my fault, sorry. (I confess I never really > > understood why RUN_ALL hooks accept both OK and DECLINED values, > > but then don't actually treat them any differently.) So probably > > we should update the doc comments. I'd be happy to draft new > > versions for those if that would be helpful, or not, as you prefer. > > (Really attached this time) This all looks great, thanks Joe! Pull my comments along with my the old backport proposal, because I'm certainly ready to support this change in 2.2 and 2.4. Addressed all of my concerns.
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
On Wed, May 29, 2013 at 03:04:30PM -0400, Matthew Steele wrote: > Looks good to me. Thanks! Thanks a lot for reviewing. http://svn.apache.org/viewvc?view=revision&revision=1487772 Gregg, thanks for confirming and sorry again about leaving the builds broken. Regards, Joe
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
On 5/29/2013 10:52 AM, Guenter Knauf wrote: Hi Joe, On 29.05.2013 18:06, Joe Orton wrote: On Wed, May 29, 2013 at 11:37:14AM -0400, Matthew Steele wrote: Oops, yes, RUN_ALL semantics are desired; the misleading API description is my fault, sorry. (I confess I never really understood why RUN_ALL hooks accept both OK and DECLINED values, but then don't actually treat them any differently.) So probably we should update the doc comments. I'd be happy to draft new versions for those if that would be helpful, or not, as you prefer. OK vs DECLINED has always confused me too ;) How does this look? I've specified the behaviour for OK and DONE as return codes for both the callbacks; since the caller doesn't pay attention to errors I've left behaviour undefined for any other values. (Really attached this time) thanks for looking into it! I will test tomorrow; need a rest just now since was the whole day busy at customers; but perhaps Gregg beats me soon ... Compiles fine now. Didn't do much testing other than access a simple page via https Gregg
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
On Wed, May 29, 2013 at 12:06 PM, Joe Orton wrote: > On Wed, May 29, 2013 at 11:37:14AM -0400, Matthew Steele wrote: > > Oops, yes, RUN_ALL semantics are desired; the misleading API description > is > > my fault, sorry. (I confess I never really understood why RUN_ALL hooks > > accept both OK and DECLINED values, but then don't actually treat them > any > > differently.) So probably we should update the doc comments. I'd be > happy > > to draft new versions for those if that would be helpful, or not, as you > > prefer. > > OK vs DECLINED has always confused me too ;) > > How does this look? I've specified the behaviour for OK and DONE as > return codes for both the callbacks; since the caller doesn't pay > attention to errors I've left behaviour undefined for any other values. > > (Really attached this time) > Looks good to me. Thanks! Cheers, -Matthew
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
Hi Joe, On 29.05.2013 18:06, Joe Orton wrote: On Wed, May 29, 2013 at 11:37:14AM -0400, Matthew Steele wrote: Oops, yes, RUN_ALL semantics are desired; the misleading API description is my fault, sorry. (I confess I never really understood why RUN_ALL hooks accept both OK and DECLINED values, but then don't actually treat them any differently.) So probably we should update the doc comments. I'd be happy to draft new versions for those if that would be helpful, or not, as you prefer. OK vs DECLINED has always confused me too ;) How does this look? I've specified the behaviour for OK and DONE as return codes for both the callbacks; since the caller doesn't pay attention to errors I've left behaviour undefined for any other values. (Really attached this time) thanks for looking into it! I will test tomorrow; need a rest just now since was the whole day busy at customers; but perhaps Gregg beats me soon ... Gün.
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
On Wed, May 29, 2013 at 11:37:14AM -0400, Matthew Steele wrote:
> Oops, yes, RUN_ALL semantics are desired; the misleading API description is
> my fault, sorry. (I confess I never really understood why RUN_ALL hooks
> accept both OK and DECLINED values, but then don't actually treat them any
> differently.) So probably we should update the doc comments. I'd be happy
> to draft new versions for those if that would be helpful, or not, as you
> prefer.
OK vs DECLINED has always confused me too ;)
How does this look? I've specified the behaviour for OK and DONE as
return codes for both the callbacks; since the caller doesn't pay
attention to errors I've left behaviour undefined for any other values.
(Really attached this time)
Regards, Joe
Index: modules/ssl/mod_ssl.c
===
--- modules/ssl/mod_ssl.c (revision 1487480)
+++ modules/ssl/mod_ssl.c (working copy)
@@ -285,18 +285,6 @@
AP_END_CMD
};
-/* Implement 'modssl_run_npn_advertise_protos_hook'. */
-APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(
-modssl, AP, int, npn_advertise_protos_hook,
-(conn_rec *connection, apr_array_header_t *protos),
-(connection, protos), OK, DECLINED)
-
-/* Implement 'modssl_run_npn_proto_negotiated_hook'. */
-APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(
-modssl, AP, int, npn_proto_negotiated_hook,
-(conn_rec *connection, const char *proto_name, apr_size_t proto_name_len),
-(connection, proto_name, proto_name_len), OK, DECLINED)
-
/*
* the various processing hooks
*/
@@ -444,6 +432,37 @@
return 1;
}
+static int modssl_register_npn(conn_rec *c,
+ ssl_npn_advertise_protos advertisefn,
+ ssl_npn_proto_negotiated negotiatedfn)
+{
+#ifdef HAVE_TLS_NPN
+SSLConnRec *sslconn = myConnConfig(c);
+
+if (!sslconn) {
+return DECLINED;
+}
+
+if (!sslconn->npn_advertfns) {
+sslconn->npn_advertfns =
+apr_array_make(c->pool, 5, sizeof(ssl_npn_advertise_protos));
+sslconn->npn_negofns =
+apr_array_make(c->pool, 5, sizeof(ssl_npn_proto_negotiated));
+}
+
+if (advertisefn)
+APR_ARRAY_PUSH(sslconn->npn_advertfns, ssl_npn_advertise_protos) =
+advertisefn;
+if (negotiatedfn)
+APR_ARRAY_PUSH(sslconn->npn_negofns, ssl_npn_proto_negotiated) =
+negotiatedfn;
+
+return OK;
+#else
+return DECLINED;
+#endif
+}
+
int ssl_init_ssl_connection(conn_rec *c, request_rec *r)
{
SSLSrvConfigRec *sc;
@@ -615,6 +634,7 @@
APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
APR_REGISTER_OPTIONAL_FN(ssl_engine_disable);
+APR_REGISTER_OPTIONAL_FN(modssl_register_npn);
ap_register_auth_provider(p, AUTHZ_PROVIDER_GROUP, "ssl",
AUTHZ_PROVIDER_VERSION,
Index: modules/ssl/mod_ssl.h
===
--- modules/ssl/mod_ssl.h (revision 1487480)
+++ modules/ssl/mod_ssl.h (working copy)
@@ -63,26 +63,40 @@
APR_DECLARE_OPTIONAL_FN(int, ssl_engine_disable, (conn_rec *));
-/** The npn_advertise_protos optional hook allows other modules to add entries
- * to the list of protocol names advertised by the server during the Next
- * Protocol Negotiation (NPN) portion of the SSL handshake. The hook callee is
- * given the connection and an APR array; it should push one or more char*'s
- * pointing to null-terminated strings (such as "http/1.1" or "spdy/2") onto
- * the array and return OK, or do nothing and return DECLINED. */
-APR_DECLARE_EXTERNAL_HOOK(modssl, AP, int, npn_advertise_protos_hook,
- (conn_rec *connection, apr_array_header_t *protos))
+/** The npn_advertise_protos callback allows another modules to add
+ * entries to the list of protocol names advertised by the server
+ * during the Next Protocol Negotiation (NPN) portion of the SSL
+ * handshake. The callback is given the connection and an APR array;
+ * it should push one or more char*'s pointing to NUL-terminated
+ * strings (such as "http/1.1" or "spdy/2") onto the array and return
+ * OK. To prevent further processing of (other modules') callbacks,
+ * return DONE. */
+typedef int (*ssl_npn_advertise_protos)(conn_rec *connection,
+apr_array_header_t *protos);
-/** The npn_proto_negotiated optional hook allows other modules to discover the
- * name of the protocol that was chosen during the Next Protocol Negotiation
- * (NPN) portion of the SSL handshake. Note that this may be the empty string
- * (in which case modules should probably assume HTTP), or it may be a protocol
- * that was never even advertised by the server. The hook callee is given the
- * connection, a non-null-terminated string containing the protocol name, and
- * the length of the string; it should do something appropriate (i.e. insert or
- * remove filters) and return OK, o
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
On Wed, May 29, 2013 at 11:14 AM, Joe Orton wrote: > On Wed, May 29, 2013 at 10:52:10AM -0400, Matthew Steele wrote: > > - In modssl_register_npn, it appears that the code creates > > new npn_advertfns and npn_negofns arrays on every call, even if they > > already exist. This would seem to prevent multiple modules from > > registering callbacks. Presumably this is not intended? Am I misreading > > the code? > > No, just a braino. Updated patch attached. > Gotcha. I'm not seeing a new attachment, though? > > - The old optional hooks were both declared as RUN_ALL, but the new > > implementation seems to stop calling callbacks as soon as one returns OK, > > which is different behavior than before. Is there a reason for this > change? > > I was wondering about this. The API description weakly implies that OK > does something different to DECLINED in both cases, which is not true > for RUN_ALL; so I presumed RUN_FIRST was desired. Are RUN_ALL semantics > desired? > Oops, yes, RUN_ALL semantics are desired; the misleading API description is my fault, sorry. (I confess I never really understood why RUN_ALL hooks accept both OK and DECLINED values, but then don't actually treat them any differently.) So probably we should update the doc comments. I'd be happy to draft new versions for those if that would be helpful, or not, as you prefer. Thanks, -Matthew
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
Hi Matthew - thanks for taking a look at the patch so quickly. On Wed, May 29, 2013 at 10:52:10AM -0400, Matthew Steele wrote: > Two questions about this change: > > - In modssl_register_npn, it appears that the code creates > new npn_advertfns and npn_negofns arrays on every call, even if they > already exist. This would seem to prevent multiple modules from > registering callbacks. Presumably this is not intended? Am I misreading > the code? No, just a braino. Updated patch attached. > - The old optional hooks were both declared as RUN_ALL, but the new > implementation seems to stop calling callbacks as soon as one returns OK, > which is different behavior than before. Is there a reason for this change? I was wondering about this. The API description weakly implies that OK does something different to DECLINED in both cases, which is not true for RUN_ALL; so I presumed RUN_FIRST was desired. Are RUN_ALL semantics desired? Regards, Joe
Re: mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
Hi Joe, Two questions about this change: - In modssl_register_npn, it appears that the code creates new npn_advertfns and npn_negofns arrays on every call, even if they already exist. This would seem to prevent multiple modules from registering callbacks. Presumably this is not intended? Am I misreading the code? - The old optional hooks were both declared as RUN_ALL, but the new implementation seems to stop calling callbacks as soon as one returns OK, which is different behavior than before. Is there a reason for this change? Best, -Matthew On Wed, May 29, 2013 at 10:36 AM, Joe Orton wrote: > Guenter, can you test if the attached compiles on Windows? It is > nothing special so it should be OK. > > This redesigns the NPN API with a cheap and crappy callback interface > which doesn't rely on the actual hooks API; it is not pretty but it > avoids the inter-module hard linkage issue (which is even more crap and > I should have noticed when committing, sorry about that). > > (Tested to compile and not segfault on Unix but no further, I'll mock up > something to test properly.) > > Regards, Joe >
mod_ssl NPN API rejig (was Re: Intent to revert commit r1332643)
Guenter, can you test if the attached compiles on Windows? It is
nothing special so it should be OK.
This redesigns the NPN API with a cheap and crappy callback interface
which doesn't rely on the actual hooks API; it is not pretty but it
avoids the inter-module hard linkage issue (which is even more crap and
I should have noticed when committing, sorry about that).
(Tested to compile and not segfault on Unix but no further, I'll mock up
something to test properly.)
Regards, Joe
Index: modules/ssl/mod_ssl.c
===
--- modules/ssl/mod_ssl.c (revision 1487480)
+++ modules/ssl/mod_ssl.c (working copy)
@@ -285,18 +285,6 @@
AP_END_CMD
};
-/* Implement 'modssl_run_npn_advertise_protos_hook'. */
-APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(
-modssl, AP, int, npn_advertise_protos_hook,
-(conn_rec *connection, apr_array_header_t *protos),
-(connection, protos), OK, DECLINED)
-
-/* Implement 'modssl_run_npn_proto_negotiated_hook'. */
-APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(
-modssl, AP, int, npn_proto_negotiated_hook,
-(conn_rec *connection, const char *proto_name, apr_size_t proto_name_len),
-(connection, proto_name, proto_name_len), OK, DECLINED)
-
/*
* the various processing hooks
*/
@@ -444,6 +432,35 @@
return 1;
}
+static int modssl_register_npn(conn_rec *c,
+ ssl_npn_advertise_protos advertisefn,
+ ssl_npn_proto_negotiated negotiatedfn)
+{
+#ifdef HAVE_TLS_NPN
+SSLConnRec *sslconn = myConnConfig(c);
+
+if (!sslconn) {
+return DECLINED;
+}
+
+sslconn->npn_advertfns =
+apr_array_make(c->pool, 0, sizeof(ssl_npn_advertise_protos));
+sslconn->npn_negofns =
+apr_array_make(c->pool, 0, sizeof(ssl_npn_proto_negotiated));
+
+if (advertisefn)
+APR_ARRAY_PUSH(sslconn->npn_advertfns, ssl_npn_advertise_protos) =
+advertisefn;
+if (negotiatedfn)
+APR_ARRAY_PUSH(sslconn->npn_negofns, ssl_npn_proto_negotiated) =
+negotiatedfn;
+
+return OK;
+#else
+return DECLINED;
+#endif
+}
+
int ssl_init_ssl_connection(conn_rec *c, request_rec *r)
{
SSLSrvConfigRec *sc;
@@ -615,6 +632,7 @@
APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
APR_REGISTER_OPTIONAL_FN(ssl_engine_disable);
+APR_REGISTER_OPTIONAL_FN(modssl_register_npn);
ap_register_auth_provider(p, AUTHZ_PROVIDER_GROUP, "ssl",
AUTHZ_PROVIDER_VERSION,
Index: modules/ssl/mod_ssl.h
===
--- modules/ssl/mod_ssl.h (revision 1487480)
+++ modules/ssl/mod_ssl.h (working copy)
@@ -63,26 +63,37 @@
APR_DECLARE_OPTIONAL_FN(int, ssl_engine_disable, (conn_rec *));
-/** The npn_advertise_protos optional hook allows other modules to add entries
- * to the list of protocol names advertised by the server during the Next
- * Protocol Negotiation (NPN) portion of the SSL handshake. The hook callee is
- * given the connection and an APR array; it should push one or more char*'s
- * pointing to null-terminated strings (such as "http/1.1" or "spdy/2") onto
- * the array and return OK, or do nothing and return DECLINED. */
-APR_DECLARE_EXTERNAL_HOOK(modssl, AP, int, npn_advertise_protos_hook,
- (conn_rec *connection, apr_array_header_t *protos))
+/** The npn_advertise_protos callback allows another modules to add
+ * entries to the list of protocol names advertised by the server
+ * during the Next Protocol Negotiation (NPN) portion of the SSL
+ * handshake. The callback is given the connection and an APR array;
+ * it should push one or more char*'s pointing to NUL-terminated
+ * strings (such as "http/1.1" or "spdy/2") onto the array and return
+ * OK, or do nothing and return DECLINED. */
+typedef int (*ssl_npn_advertise_protos)(conn_rec *connection,
+apr_array_header_t *protos);
-/** The npn_proto_negotiated optional hook allows other modules to discover the
- * name of the protocol that was chosen during the Next Protocol Negotiation
- * (NPN) portion of the SSL handshake. Note that this may be the empty string
- * (in which case modules should probably assume HTTP), or it may be a protocol
- * that was never even advertised by the server. The hook callee is given the
- * connection, a non-null-terminated string containing the protocol name, and
- * the length of the string; it should do something appropriate (i.e. insert or
- * remove filters) and return OK, or do nothing and return DECLINED. */
-APR_DECLARE_EXTERNAL_HOOK(modssl, AP, int, npn_proto_negotiated_hook,
- (conn_rec *connection, const char *proto_name,
- apr_size_t proto_name_len))
+/** The npn_proto_negotiated callback allows other modules to discover
+ * the name of the protocol that was chosen durin
