You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@httpd.apache.org by GitBox <gi...@apache.org> on 2021/06/01 14:31:13 UTC
[GitHub] [httpd] icing opened a new pull request #190: mod_proxy/mod_ssl interworking redesign
icing opened a new pull request #190:
URL: https://github.com/apache/httpd/pull/190
core/mod_proxy/mod_ssl:
* Adding `outgoing` flag to conn_rec, indicating a connection is
initiated by the server to somewhere, in contrast to incoming
connections from clients.
* Adding `ap_ssl_bind_outgoing()` function that marks a connection
as outgoing and is used by mod_proxy instead of the previous
optional function `ssl_engine_set`. This enables other SSL
module to secure proxy connections.
* The optional functions `ssl_engine_set`, `ssl_engine_disable` and
`ssl_proxy_enable` are now provided by the core to have backward
compatibility with non-httpd modules that might use them. mod_ssl
itself no longer registers these functions, but keeps them in its
header for backward compatibility.
* The core provided optional function wrap any registered function
like it was done for `ssl_is_ssl`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645630789
##########
File path: server/ssl.c
##########
@@ -79,6 +90,77 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ if (proxy) {
+ return ap_ssl_bind_outgoing(c, per_dir_config, enable) == OK;
+ }
+ else if (module_ssl_engine_set) {
+ return module_ssl_engine_set(c, per_dir_config, 0, enable);
+ }
+ else if (enable && module_ssl_proxy_enable) {
Review comment:
Ah ok right, the old ssl_proxy_enable() called ssl_engine_set() actually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r644914083
##########
File path: modules/ssl/ssl_engine_kernel.c
##########
@@ -2300,7 +2300,7 @@ void ssl_callback_Info(const SSL *ssl, int where, int rc)
/* If the reneg state is to reject renegotiations, check the SSL
* state machine and move to ABORT if a Client Hello is being
* read. */
- if (!sslconn->is_proxy &&
+ if (!conn->outgoing &&
Review comment:
I think this needs to be `c` instead of `conn` (see https://travis-ci.com/github/apache/httpd/jobs/510384470).
```suggestion
if (!c->outgoing &&
```
##########
File path: server/connection.c
##########
@@ -19,6 +19,7 @@
#include "ap_config.h"
#include "httpd.h"
+#include "http_config.h"
Review comment:
Why is this needed?
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
Review comment:
So far ssl_engine_set delivered back either 0 or 1. Now it delivers back 0 (OK) or -1 (DECLINED). Furthermore in case that `proxy` was 0 `sslcconn->disabled` was set based on `enable`.
##########
File path: modules/proxy/mod_proxy_ftp.c
##########
@@ -1980,7 +1980,6 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker,
proxy_ftp_cleanup(r, backend);
return HTTP_INTERNAL_SERVER_ERROR;
}
-
Review comment:
What is the purpose of this whitespace fix?
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
+ if (enable_ssl && !enabled) {
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, 0,
+ c, APLOGNO() " failed to enable ssl support "
Review comment:
I think we should move the old APLOGNO (01961) here, because it is the same issue as previously.
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
Review comment:
Should this only be done if `!enable_ssl && enabled`? Hence
```suggestion
else if (!enable_ssl && enabled) {
/* any existing optional funcs need to not enable here */
if (module_ssl_engine_set) {
module_ssl_engine_set(c, dir_conf, 1, 0);
}
else if (module_ssl_engine_disable) {
module_ssl_engine_disable(c);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645479113
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
Review comment:
I think this needs to be called for `!enable_ssl || enabled`:
* `!enable_ssl`: tell any existing OPTIONAL that this connection does not use SSL. An existing functions means that a module does not use the new hook and `c->outgoing`. It needs to be told as before to not engage here.
* `enabled`: a hook as enabled SSL on this connections. All OPTIONALs, not aware of the hook and `outgoing`, need to be told to not engage on this connection.
This all is a bit complicated, stemming from the fact that previously a SSL module *had* to be told not to do something. It could not differentiate between incoming and outgoing connections and would, without being told, assume the incoming case and check the c->base_server config for SSL support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645590310
##########
File path: modules/ssl/mod_ssl.c
##########
@@ -481,18 +481,23 @@ static SSLConnRec *ssl_init_connection_ctx(conn_rec *c,
* includes mod_proxy's later run_pre_connection call), sslconn->dc should
* be preserved if it's already set.
*/
+ /* icing: this method primarily initializes "sslconn" for a connection
+ * if it is not already there. But calling it with "new_proxy" re-inits
+ * the sslconn's dirconf parts. So "reinit" would be a more apt name.
+ */
Review comment:
EZ.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645577784
##########
File path: changes-entries/ssl_proxy.txt
##########
@@ -0,0 +1,16 @@
+ * core/mod_proxy/mod_ssl:
Review comment:
Nit, CHANGES entries start with `*)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645586307
##########
File path: modules/ssl/mod_ssl.c
##########
@@ -481,18 +481,23 @@ static SSLConnRec *ssl_init_connection_ctx(conn_rec *c,
* includes mod_proxy's later run_pre_connection call), sslconn->dc should
* be preserved if it's already set.
*/
+ /* icing: this method primarily initializes "sslconn" for a connection
+ * if it is not already there. But calling it with "new_proxy" re-inits
+ * the sslconn's dirconf parts. So "reinit" would be a more apt name.
+ */
Review comment:
+1 for the name change, and comment removal :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645648210
##########
File path: modules/ssl/mod_ssl.c
##########
@@ -539,53 +544,41 @@ static int ssl_engine_status(conn_rec *c, SSLConnRec *sslconn)
}
}
else {
- if (mySrvConfig(c->base_server)->enabled != SSL_ENABLED_TRUE) {
+ /* we decline by default for outgoing connections and for incoming
+ * where the base_server is not enabled. */
+ if (c->outgoing || mySrvConfig(c->base_server)->enabled != SSL_ENABLED_TRUE) {
return DECLINED;
}
}
return OK;
}
-static int ssl_engine_set(conn_rec *c,
- ap_conf_vector_t *per_dir_config,
- int proxy, int enable)
+static int ssl_hook_ssl_outgoing(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int enable_ssl)
{
SSLConnRec *sslconn;
int status;
-
- if (proxy) {
- sslconn = ssl_init_connection_ctx(c, per_dir_config, 1);
- }
- else {
- sslconn = myConnConfig(c);
- }
+ sslconn = ssl_init_connection_ctx(c, per_dir_config, 1);
status = ssl_engine_status(c, sslconn);
-
- if (proxy && status == DECLINED) {
- if (enable) {
+ if (enable_ssl) {
+ if (status == DECLINED) {
Review comment:
Changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645641636
##########
File path: server/ssl.c
##########
@@ -123,6 +205,13 @@ AP_DECLARE(void) ap_setup_ssl_optional_fns(apr_pool_t *pool)
module_ssl_var_lookup = (fn_ssl_var_lookup
&& fn_ssl_var_lookup != ssl_var_lookup)? fn_ssl_var_lookup : NULL;
APR_REGISTER_OPTIONAL_FN(ssl_var_lookup);
+
+ module_ssl_proxy_enable = APR_RETRIEVE_OPTIONAL_FN(ssl_proxy_enable);
+ APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
Review comment:
OK but if a module registered `ssl_proxy_enable` already, won't the uncontional `APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable)` here overwrite that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645607017
##########
File path: include/http_ssl.h
##########
@@ -49,6 +51,40 @@ AP_DECLARE_HOOK(int,ssl_conn_is_ssl,(conn_rec *c))
*/
AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c);
+/**
+ * This hook declares a connection to be outgoing and the configuration that applies to it.
+ * This hook can be called several times in the lifetime of an outgoing connection, e.g.
+ * when it is re-used in different request contexts. It will at least be called after the
+ * connection was created and before the pre-connection hooks is invoked.
+ * All outgoing-connection hooks are run until one returns something other than ok or decline.
+ * if enable_ssl != 0, a hook that sets up SSL for the connection needs to return DONE.
+ *
+ * @param c The connection on which requests/data are to be sent.
+ * @param dir_conf The directory configuration in which this connection is being used.
+ * @param enable_ssl If != 0, the SSL protocol should be enabled for this connection.
+ * @return OK or DECLINED, DONE when ssl was enabled
+ */
+AP_DECLARE_HOOK(int, ssl_outgoing,
+ (conn_rec *c, struct ap_conf_vector_t *dir_conf, int enable_ssl))
+
Review comment:
Maybe the hook could be called ssl_bind_outgoing, with the action being done one the outgoing connection (just like you have ap_bind_outgoing below)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r646320106
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
Review comment:
Yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem edited a comment on pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem edited a comment on pull request #190:
URL: https://github.com/apache/httpd/pull/190#issuecomment-856665848
Fine with me as well. Thanks for taking care.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645648466
##########
File path: include/http_ssl.h
##########
@@ -49,6 +51,40 @@ AP_DECLARE_HOOK(int,ssl_conn_is_ssl,(conn_rec *c))
*/
AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c);
+/**
+ * This hook declares a connection to be outgoing and the configuration that applies to it.
+ * This hook can be called several times in the lifetime of an outgoing connection, e.g.
+ * when it is re-used in different request contexts. It will at least be called after the
+ * connection was created and before the pre-connection hooks is invoked.
+ * All outgoing-connection hooks are run until one returns something other than ok or decline.
+ * if enable_ssl != 0, a hook that sets up SSL for the connection needs to return DONE.
+ *
+ * @param c The connection on which requests/data are to be sent.
+ * @param dir_conf The directory configuration in which this connection is being used.
+ * @param enable_ssl If != 0, the SSL protocol should be enabled for this connection.
+ * @return OK or DECLINED, DONE when ssl was enabled
+ */
+AP_DECLARE_HOOK(int, ssl_outgoing,
+ (conn_rec *c, struct ap_conf_vector_t *dir_conf, int enable_ssl))
+
Review comment:
Renamed. Thanks for the input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on pull request #190:
URL: https://github.com/apache/httpd/pull/190#issuecomment-854617124
Addressed the issues raised by @rpluem - thanks for watching out for me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645630147
##########
File path: server/ssl.c
##########
@@ -123,6 +205,13 @@ AP_DECLARE(void) ap_setup_ssl_optional_fns(apr_pool_t *pool)
module_ssl_var_lookup = (fn_ssl_var_lookup
&& fn_ssl_var_lookup != ssl_var_lookup)? fn_ssl_var_lookup : NULL;
APR_REGISTER_OPTIONAL_FN(ssl_var_lookup);
+
+ module_ssl_proxy_enable = APR_RETRIEVE_OPTIONAL_FN(ssl_proxy_enable);
+ APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
Review comment:
We are in `ap_setup_ssl_optional_fns`, which is called by `core_post_config` which is the first runner of the post config hook. Modules register optional functions typically in register hooks which runs before post config. Hence the pointers should be set if someone did.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645633976
##########
File path: modules/ssl/mod_ssl.c
##########
@@ -481,18 +481,23 @@ static SSLConnRec *ssl_init_connection_ctx(conn_rec *c,
* includes mod_proxy's later run_pre_connection call), sslconn->dc should
* be preserved if it's already set.
*/
+ /* icing: this method primarily initializes "sslconn" for a connection
+ * if it is not already there. But calling it with "new_proxy" re-inits
+ * the sslconn's dirconf parts. So "reinit" would be a more apt name.
+ */
if (!sslconn) {
sslconn = apr_pcalloc(c->pool, sizeof(*sslconn));
need_setup = 1;
}
- else if (!new_proxy) {
+ else if (!reinit) {
return sslconn;
}
/* Reinit dc in any case because it may be r->per_dir_config scoped
* and thus a caller like mod_proxy needs to update it per request.
*/
if (per_dir_config) {
+ ap_assert(c->outgoing);
Review comment:
You are right. There is the incoming upgrade situation as well. Quite some call chains going on.
Will remove the assert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645621975
##########
File path: server/ssl.c
##########
@@ -123,6 +205,13 @@ AP_DECLARE(void) ap_setup_ssl_optional_fns(apr_pool_t *pool)
module_ssl_var_lookup = (fn_ssl_var_lookup
&& fn_ssl_var_lookup != ssl_var_lookup)? fn_ssl_var_lookup : NULL;
APR_REGISTER_OPTIONAL_FN(ssl_var_lookup);
+
+ module_ssl_proxy_enable = APR_RETRIEVE_OPTIONAL_FN(ssl_proxy_enable);
+ APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
Review comment:
Doesn't APR_RETRIEVE_OPTIONAL_FN() always return NULL if it's called before APR_REGISTER_OPTIONAL_FN()?
I'm likely missing something here :p
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645584270
##########
File path: server/ssl.c
##########
@@ -79,6 +90,77 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ if (proxy) {
+ return ap_ssl_bind_outgoing(c, per_dir_config, enable) == OK;
+ }
+ else if (module_ssl_engine_set) {
+ return module_ssl_engine_set(c, per_dir_config, 0, enable);
+ }
+ else if (enable && module_ssl_proxy_enable) {
Review comment:
I think yes as the old ssl_engine_set in mod_ssl.c also does things in case that `proxy` is 0 e.g. switching `sslconn->disabled` based on `enable`:
```C
static int ssl_engine_set(conn_rec *c,
ap_conf_vector_t *per_dir_config,
int proxy, int enable)
{
SSLConnRec *sslconn;
int status;
if (proxy) {
sslconn = ssl_init_connection_ctx(c, per_dir_config, 1);
}
else {
sslconn = myConnConfig(c);
}
status = ssl_engine_status(c, sslconn);
if (proxy && status == DECLINED) {
if (enable) {
SSLSrvConfigRec *sc = mySrvConfig(sslconn->server);
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(01961)
"SSL Proxy requested for %s but not enabled "
"[Hint: SSLProxyEngine]", sc->vhost_id);
}
sslconn->disabled = 1;
}
else if (sslconn) {
sslconn->disabled = !enable;
}
return status != DECLINED;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on pull request #190:
URL: https://github.com/apache/httpd/pull/190#issuecomment-856600166
@ylavic and @rpluem : if the current state is ok with you, I'd like to apply this to trunk.
I have also some thoughts on how to improve here (adding SNI hostname, and APLN to the bind_outgoing()), but that can be discussed separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on pull request #190:
URL: https://github.com/apache/httpd/pull/190#issuecomment-856614264
Thanks for reviewing! This rework certainly was not easy and your keen eyes helped a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645471342
##########
File path: server/connection.c
##########
@@ -19,6 +19,7 @@
#include "ap_config.h"
#include "httpd.h"
+#include "http_config.h"
Review comment:
Will remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645642832
##########
File path: server/ssl.c
##########
@@ -123,6 +205,13 @@ AP_DECLARE(void) ap_setup_ssl_optional_fns(apr_pool_t *pool)
module_ssl_var_lookup = (fn_ssl_var_lookup
&& fn_ssl_var_lookup != ssl_var_lookup)? fn_ssl_var_lookup : NULL;
APR_REGISTER_OPTIONAL_FN(ssl_var_lookup);
+
+ module_ssl_proxy_enable = APR_RETRIEVE_OPTIONAL_FN(ssl_proxy_enable);
+ APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
Review comment:
ah! that's the point actually, nevermind..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645474144
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
Review comment:
You are correct. When someone directly calls this OPTIONAL function, the behaviour is not the same as before. I will update this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645607017
##########
File path: include/http_ssl.h
##########
@@ -49,6 +51,40 @@ AP_DECLARE_HOOK(int,ssl_conn_is_ssl,(conn_rec *c))
*/
AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c);
+/**
+ * This hook declares a connection to be outgoing and the configuration that applies to it.
+ * This hook can be called several times in the lifetime of an outgoing connection, e.g.
+ * when it is re-used in different request contexts. It will at least be called after the
+ * connection was created and before the pre-connection hooks is invoked.
+ * All outgoing-connection hooks are run until one returns something other than ok or decline.
+ * if enable_ssl != 0, a hook that sets up SSL for the connection needs to return DONE.
+ *
+ * @param c The connection on which requests/data are to be sent.
+ * @param dir_conf The directory configuration in which this connection is being used.
+ * @param enable_ssl If != 0, the SSL protocol should be enabled for this connection.
+ * @return OK or DECLINED, DONE when ssl was enabled
+ */
+AP_DECLARE_HOOK(int, ssl_outgoing,
+ (conn_rec *c, struct ap_conf_vector_t *dir_conf, int enable_ssl))
+
Review comment:
Maybe the hook could be called ssl_setup_outgoing, with the action being done one the outgoing connection (just like you have _bind_outgoing below)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645579632
##########
File path: server/ssl.c
##########
@@ -79,6 +90,77 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ if (proxy) {
+ return ap_ssl_bind_outgoing(c, per_dir_config, enable) == OK;
+ }
+ else if (module_ssl_engine_set) {
+ return module_ssl_engine_set(c, per_dir_config, 0, enable);
+ }
+ else if (enable && module_ssl_proxy_enable) {
Review comment:
Not sure. Theoretically, someone could have called with such parameters, although not from our source tree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645615954
##########
File path: modules/ssl/mod_ssl.c
##########
@@ -539,53 +544,41 @@ static int ssl_engine_status(conn_rec *c, SSLConnRec *sslconn)
}
}
else {
- if (mySrvConfig(c->base_server)->enabled != SSL_ENABLED_TRUE) {
+ /* we decline by default for outgoing connections and for incoming
+ * where the base_server is not enabled. */
+ if (c->outgoing || mySrvConfig(c->base_server)->enabled != SSL_ENABLED_TRUE) {
return DECLINED;
}
}
return OK;
}
-static int ssl_engine_set(conn_rec *c,
- ap_conf_vector_t *per_dir_config,
- int proxy, int enable)
+static int ssl_hook_ssl_outgoing(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int enable_ssl)
{
SSLConnRec *sslconn;
int status;
-
- if (proxy) {
- sslconn = ssl_init_connection_ctx(c, per_dir_config, 1);
- }
- else {
- sslconn = myConnConfig(c);
- }
+ sslconn = ssl_init_connection_ctx(c, per_dir_config, 1);
status = ssl_engine_status(c, sslconn);
-
- if (proxy && status == DECLINED) {
- if (enable) {
+ if (enable_ssl) {
+ if (status == DECLINED) {
Review comment:
Change to (status != OK) here for the ssl_engine_status() == SUSPENDED case to be DECLINED?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645470365
##########
File path: modules/proxy/mod_proxy_ftp.c
##########
@@ -1980,7 +1980,6 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker,
proxy_ftp_cleanup(r, backend);
return HTTP_INTERNAL_SERVER_ERROR;
}
-
Review comment:
will fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645630305
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
Review comment:
> Indeed a little bit complicated :-). Let me try to understand. If a module only has the optionals and no hook it should be told to step out of the way as another module with the hooks replied that it enabled SSL and hence we would do double SSL if both modules, would do, correct?
Yes.
> I think this also means that a module cannot implement both hooks and optional functions as in case it is the only module it would flip to on during the hook and to off here.
Indeed. It would need to due some extra thing to manage that. But I think there is not need to do both.
> Next question that pops up to me: If we have two hook implementing modules, mod_ssl and another one and the hook of the other one is run before the one of mod_ssl and the other hook enables SSL and thus returns OK and stops the hook from further processing how does mod_ssl know that it needs to step aside?
The idea is that a "modern" module knows that it will not engage on `c->outgoing` unless it has answered the hook with OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on pull request #190:
URL: https://github.com/apache/httpd/pull/190#issuecomment-856665848
Fine with me as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645578639
##########
File path: changes-entries/ssl_proxy.txt
##########
@@ -0,0 +1,16 @@
+ * core/mod_proxy/mod_ssl:
Review comment:
I have so much to learn...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645632461
##########
File path: modules/ssl/mod_ssl.c
##########
@@ -481,18 +481,23 @@ static SSLConnRec *ssl_init_connection_ctx(conn_rec *c,
* includes mod_proxy's later run_pre_connection call), sslconn->dc should
* be preserved if it's already set.
*/
+ /* icing: this method primarily initializes "sslconn" for a connection
+ * if it is not already there. But calling it with "new_proxy" re-inits
+ * the sslconn's dirconf parts. So "reinit" would be a more apt name.
+ */
if (!sslconn) {
sslconn = apr_pcalloc(c->pool, sizeof(*sslconn));
need_setup = 1;
}
- else if (!new_proxy) {
+ else if (!reinit) {
return sslconn;
}
/* Reinit dc in any case because it may be r->per_dir_config scoped
* and thus a caller like mod_proxy needs to update it per request.
*/
if (per_dir_config) {
+ ap_assert(c->outgoing);
Review comment:
I think you are correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645598447
##########
File path: modules/ssl/mod_ssl.c
##########
@@ -481,18 +481,23 @@ static SSLConnRec *ssl_init_connection_ctx(conn_rec *c,
* includes mod_proxy's later run_pre_connection call), sslconn->dc should
* be preserved if it's already set.
*/
+ /* icing: this method primarily initializes "sslconn" for a connection
+ * if it is not already there. But calling it with "new_proxy" re-inits
+ * the sslconn's dirconf parts. So "reinit" would be a more apt name.
+ */
if (!sslconn) {
sslconn = apr_pcalloc(c->pool, sizeof(*sslconn));
need_setup = 1;
}
- else if (!new_proxy) {
+ else if (!reinit) {
return sslconn;
}
/* Reinit dc in any case because it may be r->per_dir_config scoped
* and thus a caller like mod_proxy needs to update it per request.
*/
if (per_dir_config) {
+ ap_assert(c->outgoing);
Review comment:
Isn't upgrade_connection()->ssl_init_ssl_connection()->ssl_init_connection_ctx() a call chain where we'd fall through here with r->per_dir_config?
I don't the hard requirement on c->outgoing being set here, why enfore/assert it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645577963
##########
File path: server/ssl.c
##########
@@ -79,6 +90,77 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ if (proxy) {
+ return ap_ssl_bind_outgoing(c, per_dir_config, enable) == OK;
+ }
+ else if (module_ssl_engine_set) {
+ return module_ssl_engine_set(c, per_dir_config, 0, enable);
+ }
+ else if (enable && module_ssl_proxy_enable) {
Review comment:
Do we need this given that we are not in a proxy case here (per `if (proxy)` early return above)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] ylavic commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
ylavic commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645607017
##########
File path: include/http_ssl.h
##########
@@ -49,6 +51,40 @@ AP_DECLARE_HOOK(int,ssl_conn_is_ssl,(conn_rec *c))
*/
AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c);
+/**
+ * This hook declares a connection to be outgoing and the configuration that applies to it.
+ * This hook can be called several times in the lifetime of an outgoing connection, e.g.
+ * when it is re-used in different request contexts. It will at least be called after the
+ * connection was created and before the pre-connection hooks is invoked.
+ * All outgoing-connection hooks are run until one returns something other than ok or decline.
+ * if enable_ssl != 0, a hook that sets up SSL for the connection needs to return DONE.
+ *
+ * @param c The connection on which requests/data are to be sent.
+ * @param dir_conf The directory configuration in which this connection is being used.
+ * @param enable_ssl If != 0, the SSL protocol should be enabled for this connection.
+ * @return OK or DECLINED, DONE when ssl was enabled
+ */
+AP_DECLARE_HOOK(int, ssl_outgoing,
+ (conn_rec *c, struct ap_conf_vector_t *dir_conf, int enable_ssl))
+
Review comment:
Maybe the hook could be called ssl_bind_outgoing, with the action being done on the outgoing connection (just like you have ap_bind_outgoing below)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645474875
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
+ if (enable_ssl && !enabled) {
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, 0,
+ c, APLOGNO() " failed to enable ssl support "
Review comment:
Agreed. I will clear the (now DEBUG) logging in mod_ssl with this number and move it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r644914083
##########
File path: modules/ssl/ssl_engine_kernel.c
##########
@@ -2300,7 +2300,7 @@ void ssl_callback_Info(const SSL *ssl, int where, int rc)
/* If the reneg state is to reject renegotiations, check the SSL
* state machine and move to ABORT if a Client Hello is being
* read. */
- if (!sslconn->is_proxy &&
+ if (!conn->outgoing &&
Review comment:
I think this needs to be `c` instead of `conn` (see https://travis-ci.com/github/apache/httpd/jobs/510384470).
```suggestion
if (!c->outgoing &&
```
##########
File path: server/connection.c
##########
@@ -19,6 +19,7 @@
#include "ap_config.h"
#include "httpd.h"
+#include "http_config.h"
Review comment:
Why is this needed?
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
Review comment:
So far ssl_engine_set delivered back either 0 or 1. Now it delivers back 0 (OK) or -1 (DECLINED). Furthermore in case that `proxy` was 0 `sslcconn->disabled` was set based on `enable`.
##########
File path: modules/proxy/mod_proxy_ftp.c
##########
@@ -1980,7 +1980,6 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker,
proxy_ftp_cleanup(r, backend);
return HTTP_INTERNAL_SERVER_ERROR;
}
-
Review comment:
What is the purpose of this whitespace fix?
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
+ if (enable_ssl && !enabled) {
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, 0,
+ c, APLOGNO() " failed to enable ssl support "
Review comment:
I think we should move the old APLOGNO (01961) here, because it is the same issue as previously.
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
Review comment:
Should this only be done if `!enable_ssl && enabled`? Hence
```suggestion
else if (!enable_ssl && enabled) {
/* any existing optional funcs need to not enable here */
if (module_ssl_engine_set) {
module_ssl_engine_set(c, dir_conf, 1, 0);
}
else if (module_ssl_engine_disable) {
module_ssl_engine_disable(c);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing commented on pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing commented on pull request #190:
URL: https://github.com/apache/httpd/pull/190#issuecomment-856825511
Thank you all! Merged as r1890605 into trunk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645708729
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
Review comment:
> > Next question that pops up to me: If we have two hook implementing modules, mod_ssl and another one and the hook of the other one is run before the one of mod_ssl and the other hook enables SSL and thus returns OK and stops the hook from further processing how does mod_ssl know that it needs to step aside?
>
> The idea is that a "modern" module knows that it will not engage on `c->outgoing` unless it has answered the hook with OK.
`mod_ssl` changes its behaviour to step aside by default, because of https://github.com/apache/httpd/pull/190/files#diff-08c5183b966c9a370ab3549236a1542091c9ea0a521167516418b9ed1bc6145dL542-R546 , correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] icing closed pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
icing closed pull request #190:
URL: https://github.com/apache/httpd/pull/190
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
Posted by GitBox <gi...@apache.org>.
rpluem commented on a change in pull request #190:
URL: https://github.com/apache/httpd/pull/190#discussion_r645622629
##########
File path: server/ssl.c
##########
@@ -79,6 +90,65 @@ AP_DECLARE(int) ap_ssl_conn_is_ssl(conn_rec *c)
return r;
}
+static int ssl_engine_set(conn_rec *c,
+ ap_conf_vector_t *per_dir_config,
+ int proxy, int enable)
+{
+ return proxy? ap_ssl_bind_outgoing(c, per_dir_config, enable) : DECLINED;
+}
+
+static int ssl_proxy_enable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 1);
+}
+
+static int ssl_engine_disable(conn_rec *c)
+{
+ return ap_ssl_bind_outgoing(c, NULL, 0);
+}
+
+AP_DECLARE(int) ap_ssl_bind_outgoing(conn_rec *c, struct ap_conf_vector_t *dir_conf,
+ int enable_ssl)
+{
+ int rv, enabled = 0;
+
+ c->outgoing = 1;
+ rv = ap_run_ssl_outgoing(c, dir_conf, enable_ssl);
+ enabled = (rv == OK);
+ if (enable_ssl && !enabled) {
+ /* the hooks did not take over. Is there an old skool optional that will? */
+ if (module_ssl_engine_set) {
+ enabled = module_ssl_engine_set(c, dir_conf, 1, 1);
+ }
+ else if (module_ssl_proxy_enable) {
+ enabled = module_ssl_proxy_enable(c);
+ }
+ }
+ else {
+ /* !enable_ssl || enabled
+ * any existing optional funcs need to not enable here */
+ if (module_ssl_engine_set) {
+ module_ssl_engine_set(c, dir_conf, 1, 0);
+ }
+ else if (module_ssl_engine_disable) {
+ module_ssl_engine_disable(c);
+ }
+ }
Review comment:
Indeed a little bit complicated :-). Let me try to understand. If a module only has the optionals and no hook it should be told to step out of the way as another module with the hooks replied that it enabled SSL and hence we would do double SSL if both modules, would do, correct?
I think this also means that a module cannot implement both hooks and optional functions as in case it is the only module it would flip to on during the hook and to off here.
Next question that pops up to me: If we have two hook implementing modules, mod_ssl and another one and the hook of the other one is run before the one of mod_ssl and the other hook enables SSL and thus returns OK and stops the hook from further processing how does mod_ssl know that it needs to step aside?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@httpd.apache.org
For additional commands, e-mail: notifications-help@httpd.apache.org