Skip to content

Conversation

@icing
Copy link
Contributor

@icing icing commented Jun 1, 2021

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.

Stefan Eissing added 3 commits June 1, 2021 16:21
    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`.
…re than 1 module:

 * Change the `ssl_outgoing` hook to be a RUN_FIRST on the first OK
 * Documented the return values of the functions more clearly
 * Fixed ap_ssl_bind_outgoing() to call any existing, old style OPTIONAL
   functions. They are asked to engage if no hook returned OK or
   are asked to disable if there was.
 * Fixed mod_ssl to disable on default for outgoing connections.
   So, unless its hook takes over, it does not activate.
Comment on lines +127 to +136
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);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only be done if !enable_ssl && enabled? Hence

Suggested change
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);
}
}
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);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@icing
Copy link
Contributor Author

icing commented Jun 4, 2021

Addressed the issues raised by @rpluem - thanks for watching out for me!

Stefan Eissing added 2 commits June 4, 2021 16:51
…nf on outgoing

   connections as ylavic correctly notes, this may happen on Upgrades as well.
@icing
Copy link
Contributor Author

icing commented Jun 8, 2021

@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.

Copy link
Member

@ylavic ylavic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, thanks Stefan!

@icing
Copy link
Contributor Author

icing commented Jun 8, 2021

Thanks for reviewing! This rework certainly was not easy and your keen eyes helped a lot!

@rpluem
Copy link
Collaborator

rpluem commented Jun 8, 2021

Fine with me as well. Thanks for taking care.

@icing
Copy link
Contributor Author

icing commented Jun 8, 2021

Thank you all! Merged as r1890605 into trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants