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/03 16:30:23 UTC
[GitHub] [httpd] rpluem commented on a change in pull request #190: mod_proxy/mod_ssl interworking redesign
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