Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Wed, Jan 19, 2022 at 03:32:35PM +0100, Willy Tarreau wrote: > Subject: Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with > set server ssl > > On Wed, Jan 19, 2022 at 03:24:44PM +0100, William Lallemand wrote: > > On Tue, Jan 18, 2022 at 12:07:21PM +0100, Willy Tarreau wrote: > > > > > > On Mon, Jan 17, 2022 at 07:46:24PM +0100, William Dauchy wrote: > > > > Hello Christopher, > > > > > > > > On Wed, Jan 12, 2022 at 12:45 PM William Dauchy > > > > wrote: > > > > > my approach was to say: > > > > > - remove the implicit behavior > > > > > - then work on the missing commands for the health checks > > > > > > > > Do you think we can conclude on it? > > > > > > Just merged after our discussion on it :-) > > > > > > > Can we also mark it as deprecated in 2.5? patch attached > > Sure, works for me! > > Willy > Thanks, pushed in master. -- William Lallemand
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Wed, Jan 19, 2022 at 03:24:44PM +0100, William Lallemand wrote: > On Tue, Jan 18, 2022 at 12:07:21PM +0100, Willy Tarreau wrote: > > > > On Mon, Jan 17, 2022 at 07:46:24PM +0100, William Dauchy wrote: > > > Hello Christopher, > > > > > > On Wed, Jan 12, 2022 at 12:45 PM William Dauchy wrote: > > > > my approach was to say: > > > > - remove the implicit behavior > > > > - then work on the missing commands for the health checks > > > > > > Do you think we can conclude on it? > > > > Just merged after our discussion on it :-) > > > > Can we also mark it as deprecated in 2.5? patch attached Sure, works for me! Willy
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Tue, Jan 18, 2022 at 12:07:21PM +0100, Willy Tarreau wrote: > > On Mon, Jan 17, 2022 at 07:46:24PM +0100, William Dauchy wrote: > > Hello Christopher, > > > > On Wed, Jan 12, 2022 at 12:45 PM William Dauchy wrote: > > > my approach was to say: > > > - remove the implicit behavior > > > - then work on the missing commands for the health checks > > > > Do you think we can conclude on it? > > Just merged after our discussion on it :-) > Can we also mark it as deprecated in 2.5? patch attached -- William Lallemand >From 9998a33d3a027ff6863eab71bcc2f2d7158319b4 Mon Sep 17 00:00:00 2001 From: William Lallemand Date: Wed, 19 Jan 2022 15:17:08 +0100 Subject: [PATCH] DOC: management: mark "set server ssl" as deprecated This command was integrated in 2.4 when it was not possible to handle SSL with dynamic servers, this is now possible so we should prefer this way. Must be backported in 2.5. --- doc/management.txt | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/management.txt b/doc/management.txt index b96cbc8789..b9b7ebdfb7 100644 --- a/doc/management.txt +++ b/doc/management.txt @@ -2185,11 +2185,14 @@ set server / fqdn Change a server's FQDN to the value passed in argument. This requires the internal run-time DNS resolver to be configured and enabled for this server. -set server / ssl [ on | off ] +set server / ssl [ on | off ] (deprecated) This option configures SSL ciphering on outgoing connections to the server. When switch off, all traffic becomes plain text; health check path is not changed. + This command is deprecated, create a new server dynamically with or without + SSL instead, using the "add server" command. + set severity-output [ none | number | string ] Change the severity output format of the stats socket connected to for the duration of the current session. -- 2.32.0
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Mon, Jan 17, 2022 at 07:46:24PM +0100, William Dauchy wrote: > Hello Christopher, > > On Wed, Jan 12, 2022 at 12:45 PM William Dauchy wrote: > > my approach was to say: > > - remove the implicit behavior > > - then work on the missing commands for the health checks > > Do you think we can conclude on it? Just merged after our discussion on it :-) Thanks! Willy
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
Hello Christopher, On Wed, Jan 12, 2022 at 12:45 PM William Dauchy wrote: > my approach was to say: > - remove the implicit behavior > - then work on the missing commands for the health checks Do you think we can conclude on it? -- William
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Wed, Jan 12, 2022 at 10:02 AM Christopher Faulet wrote: > I don't know what is the expected behavior on the stable releases for users. > Honestly, I've misread you patch and kept in mind you alternative solution... > But as said, if there is no implicit change (and I'm fine with this > solution), a > new command or an option to current ones must be introduced to be able to > change > SSL setting for health-check too. Otherwise, it will be unusable. my approach was to say: - remove the implicit behavior - then work on the missing commands for the health checks -- William
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Wed, Jan 12, 2022 at 10:02:30AM +0100, Christopher Faulet wrote: > I don't know what is the expected behavior on the stable releases for users. > The actual state is buggy because health-check are only updated when ssl is > disabled. When SSL is enabled on a server, there is no implicit change on > the check. when SSL is disabled, there is an implicit change. So we must > first state for the expected behavior on stable releases. But, keep in mind > there is no way to dynamically enable/disable SSL on health-checks. So if a > user configures the SSL on its server but disables it on startup, when he > enables ssl, he probably want to do so on the health-check, explicitly or > not. Also I have no idea what possible issues could cause a sudden change of transport on the health checks while they are running :-/ Willy
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Tue, Jan 11, 2022 at 06:55:10PM +0100, Christopher Faulet wrote:
> Le 1/10/22 à 23:19, Willy Tarreau a écrit :
> > At this point I'm starting to think that we should probably avoid as
> > much as possible to use implicit settings for whatever is dynamic.
> > Originally a lot of settings were implicit because we don't want to
> > have huge config lines to enforce lots of obvious settings. On the CLI
> > it's less of a problem and for example if "ssl" only deals with the
> > traffic without manipulating the checks, I'm personally not shocked,
> > because these are normally sent by bots and we don't care if they
> > have to set a few more settings to avoid multiple implicit changes
> > that may not always be desired. This is where the CLI (or any future
> > API) might differ a bit from the config, and an agent writing some
> > config might have to explicitly state certain things like "no-check-ssl"
> > for example to stay safe and avoid such implicit dependencies.
> >
> I agree with Willy on this point. Especially because, depending the order of
> commands, the result can be different because of implicit changes and it is
> pretty hard to predict how it will behave in all cases.
> For instance, to fix William's bug about "set server ssl" command, in a way
> or another, we must stop to dynamically update the health-check if its port
> or its address is explicitly specified. With this change, the result of
> following set of commands will be different:
> $ set server BK/SRV ssl on
> $ set server BK/SRV check-port XXX
> ==> SSL is enabled for the server and the health-check
> $ set server BK/SRV check-port XXX
> $ set server BK/SRV ssl on
> ==> because the check's port was updated first, the SSL is only enabled for
> the server.
> > Note that such a brainstorming doesn't apply to your fix and should
> > not hold it from being merged in any way, I'm just speaking in the
> > general sense, as I don't feel comfortable with keep all these special
> > cases in a newly introduced API, that are only there for historical
> > reasons.
> Agree. But, if possible, a warning may be added in the documentation to warn
> about implicit changes.
> About the fix, I checked the code, and, at first glance, there is no reason
> to change "srv->check.use_ssl" value when the health-check uses the server
> settings. Thus, we may fix "set server ssl" command this way:
> diff --git a/src/check.c b/src/check.c
> index f0ae81504..8cf8a1c5b 100644
> --- a/src/check.c
> +++ b/src/check.c
> @@ -1565,10 +1565,8 @@ int init_srv_check(struct server *srv)
> * default, unless one is specified.
> */
> if (!srv->check.port && !is_addr(&srv->check.addr)) {
> - if (!srv->check.use_ssl && srv->use_ssl != -1) {
> - srv->check.use_ssl = srv->use_ssl;
> - srv->check.xprt= srv->xprt;
> - }
> + if (!srv->check.use_ssl && srv->use_ssl != -1)
> + srv->check.xprt = srv->xprt;
> else if (srv->check.use_ssl == 1)
> srv->check.xprt = xprt_get(XPRT_SSL);
> srv->check.send_proxy |= (srv->pp_opts);
> diff --git a/src/server.c b/src/server.c
> index 2061153bc..a18836a71 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -2113,10 +2113,11 @@ void srv_set_ssl(struct server *s, int use_ssl)
> return;
> s->use_ssl = use_ssl;
> - if (s->use_ssl)
> - s->xprt = xprt_get(XPRT_SSL);
> - else
> - s->xprt = s->check.xprt = s->agent.xprt = xprt_get(XPRT_RAW);
> + s->xprt = (s->use_ssl == 1) ? xprt_get(XPRT_SSL) : xprt_get(XPRT_RAW);
> +
> + if ((s->check.state & CHK_ST_CONFIGURED) && !s->check.use_ssl &&
> + !s->check.port && !is_addr(&s->check.addr))
> + s->check.xprt = s->xprt;
> }
> #endif /* USE_OPENSSL */
> This may be done with the following 3 steps:
> * First, stop to change "srv->check.use_ssl" value
> * Then, stop to change the agent settings in srv_set_ssl() because there
> is no ssl support for agent-check.
> * Finally, fix the bug identified by William, adding the according
> documentation.
> Note I don't know if all this stuff works properly with the server-state
> file...
> --
> Christopher Faulet
>
To add more context on this discussion, I looked at dynamic servers.
Currently, the behavior is identical with the configuration as
init_srv_check() is used in the "add server" CLI handler. This is really
problematic as "no-check-ssl" is not available for dynamic servers, so
if I understand correctly it's not possible to add a dynamic SSL server
with checks on the same port without SSL on checks.
Christopher's patch for init_srv_check() is probably a good idea, but if
not taken it should be probably at least used for servers with the
SRV_F_DYNAMIC flag.
--
Amaury Denoyelle
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
Le 1/11/22 à 22:47, William Dauchy a écrit : Agree. But, if possible, a warning may be added in the documentation to warn about implicit changes. From the discussion, I would be tempted to say the opposite, as I feel like keeping implicit things for this command is worse. I don't know what is the expected behavior on the stable releases for users. The actual state is buggy because health-check are only updated when ssl is disabled. When SSL is enabled on a server, there is no implicit change on the check. when SSL is disabled, there is an implicit change. So we must first state for the expected behavior on stable releases. But, keep in mind there is no way to dynamically enable/disable SSL on health-checks. So if a user configures the SSL on its server but disables it on startup, when he enables ssl, he probably want to do so on the health-check, explicitly or not. as mentioned above I am not sure I am aligned here; I would rather remove the side effect of changing s->check. Honestly, I've misread you patch and kept in mind you alternative solution... But as said, if there is no implicit change (and I'm fine with this solution), a new command or an option to current ones must be introduced to be able to change SSL setting for health-check too. Otherwise, it will be unusable. Note I don't know if all this stuff works properly with the server-state file... I am always scared to look at the server state functionality... I truly understand :( ... -- Christopher Faulet
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
Hello Christopher,
Thanks for your research,
On Tue, Jan 11, 2022 at 6:55 PM Christopher Faulet wrote:
> Le 1/10/22 à 23:19, Willy Tarreau a écrit :
> > At this point I'm starting to think that we should probably avoid as
> > much as possible to use implicit settings for whatever is dynamic.
> > Originally a lot of settings were implicit because we don't want to
> > have huge config lines to enforce lots of obvious settings. On the CLI
> > it's less of a problem and for example if "ssl" only deals with the
> > traffic without manipulating the checks, I'm personally not shocked,
> > because these are normally sent by bots and we don't care if they
> > have to set a few more settings to avoid multiple implicit changes
> > that may not always be desired. This is where the CLI (or any future
> > API) might differ a bit from the config, and an agent writing some
> > config might have to explicitly state certain things like "no-check-ssl"
> > for example to stay safe and avoid such implicit dependencies.
> >
>
> I agree with Willy on this point. Especially because, depending the order of
> commands, the result can be different because of implicit changes and it is
> pretty hard to predict how it will behave in all cases.
yup totally agree with both of you. I can't really remember why I did
that in the first place.
> For instance, to fix William's bug about "set server ssl" command, in a way or
> another, we must stop to dynamically update the health-check if its port or
> its
> address is explicitly specified. With this change, the result of following set
> of commands will be different:
>
>$ set server BK/SRV ssl on
>$ set server BK/SRV check-port XXX
>
> ==> SSL is enabled for the server and the health-check
interesting one :))
>$ set server BK/SRV check-port XXX
>$ set server BK/SRV ssl on
>
> ==> because the check's port was updated first, the SSL is only enabled for
> the
> server.
> Agree. But, if possible, a warning may be added in the documentation to warn
> about implicit changes.
>From the discussion, I would be tempted to say the opposite, as I feel
like keeping implicit things for this command is worse.
> About the fix, I checked the code, and, at first glance, there is no reason to
> change "srv->check.use_ssl" value when the health-check uses the server
> settings. Thus, we may fix "set server ssl" command this way:
>
> diff --git a/src/check.c b/src/check.c
> index f0ae81504..8cf8a1c5b 100644
> --- a/src/check.c
> +++ b/src/check.c
> @@ -1565,10 +1565,8 @@ int init_srv_check(struct server *srv)
> * default, unless one is specified.
> */
> if (!srv->check.port && !is_addr(&srv->check.addr)) {
> - if (!srv->check.use_ssl && srv->use_ssl != -1) {
> - srv->check.use_ssl = srv->use_ssl;
> - srv->check.xprt= srv->xprt;
> - }
> + if (!srv->check.use_ssl && srv->use_ssl != -1)
> + srv->check.xprt = srv->xprt;
> else if (srv->check.use_ssl == 1)
> srv->check.xprt = xprt_get(XPRT_SSL);
> srv->check.send_proxy |= (srv->pp_opts);
indeed this is simplified that way
> diff --git a/src/server.c b/src/server.c
> index 2061153bc..a18836a71 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -2113,10 +2113,11 @@ void srv_set_ssl(struct server *s, int use_ssl)
> return;
>
> s->use_ssl = use_ssl;
> - if (s->use_ssl)
> - s->xprt = xprt_get(XPRT_SSL);
> - else
> - s->xprt = s->check.xprt = s->agent.xprt = xprt_get(XPRT_RAW);
> + s->xprt = (s->use_ssl == 1) ? xprt_get(XPRT_SSL) : xprt_get(XPRT_RAW);
> +
> + if ((s->check.state & CHK_ST_CONFIGURED) && !s->check.use_ssl &&
> + !s->check.port && !is_addr(&s->check.addr))
> + s->check.xprt = s->xprt;
> }
as mentioned above I am not sure I am aligned here; I would rather
remove the side effect of changing s->check.
> #endif /* USE_OPENSSL */
>
> This may be done with the following 3 steps:
>
>* First, stop to change "srv->check.use_ssl" value
>
>* Then, stop to change the agent settings in srv_set_ssl() because there is
> no ssl support for agent-check.
good point, I did not know
>* Finally, fix the bug identified by William, adding the according
> documentation.
>
> Note I don't know if all this stuff works properly with the server-state
> file...
I am always scared to look at the server state functionality...
--
William
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
Le 1/10/22 à 23:19, Willy Tarreau a écrit :
At this point I'm starting to think that we should probably avoid as
much as possible to use implicit settings for whatever is dynamic.
Originally a lot of settings were implicit because we don't want to
have huge config lines to enforce lots of obvious settings. On the CLI
it's less of a problem and for example if "ssl" only deals with the
traffic without manipulating the checks, I'm personally not shocked,
because these are normally sent by bots and we don't care if they
have to set a few more settings to avoid multiple implicit changes
that may not always be desired. This is where the CLI (or any future
API) might differ a bit from the config, and an agent writing some
config might have to explicitly state certain things like "no-check-ssl"
for example to stay safe and avoid such implicit dependencies.
I agree with Willy on this point. Especially because, depending the order of
commands, the result can be different because of implicit changes and it is
pretty hard to predict how it will behave in all cases.
For instance, to fix William's bug about "set server ssl" command, in a way or
another, we must stop to dynamically update the health-check if its port or its
address is explicitly specified. With this change, the result of following set
of commands will be different:
$ set server BK/SRV ssl on
$ set server BK/SRV check-port XXX
==> SSL is enabled for the server and the health-check
$ set server BK/SRV check-port XXX
$ set server BK/SRV ssl on
==> because the check's port was updated first, the SSL is only enabled for the
server.
Note that such a brainstorming doesn't apply to your fix and should
not hold it from being merged in any way, I'm just speaking in the
general sense, as I don't feel comfortable with keep all these special
cases in a newly introduced API, that are only there for historical
reasons.
Agree. But, if possible, a warning may be added in the documentation to warn
about implicit changes.
About the fix, I checked the code, and, at first glance, there is no reason to
change "srv->check.use_ssl" value when the health-check uses the server
settings. Thus, we may fix "set server ssl" command this way:
diff --git a/src/check.c b/src/check.c
index f0ae81504..8cf8a1c5b 100644
--- a/src/check.c
+++ b/src/check.c
@@ -1565,10 +1565,8 @@ int init_srv_check(struct server *srv)
* default, unless one is specified.
*/
if (!srv->check.port && !is_addr(&srv->check.addr)) {
- if (!srv->check.use_ssl && srv->use_ssl != -1) {
- srv->check.use_ssl = srv->use_ssl;
- srv->check.xprt= srv->xprt;
- }
+ if (!srv->check.use_ssl && srv->use_ssl != -1)
+ srv->check.xprt = srv->xprt;
else if (srv->check.use_ssl == 1)
srv->check.xprt = xprt_get(XPRT_SSL);
srv->check.send_proxy |= (srv->pp_opts);
diff --git a/src/server.c b/src/server.c
index 2061153bc..a18836a71 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2113,10 +2113,11 @@ void srv_set_ssl(struct server *s, int use_ssl)
return;
s->use_ssl = use_ssl;
- if (s->use_ssl)
- s->xprt = xprt_get(XPRT_SSL);
- else
- s->xprt = s->check.xprt = s->agent.xprt = xprt_get(XPRT_RAW);
+ s->xprt = (s->use_ssl == 1) ? xprt_get(XPRT_SSL) : xprt_get(XPRT_RAW);
+
+ if ((s->check.state & CHK_ST_CONFIGURED) && !s->check.use_ssl &&
+ !s->check.port && !is_addr(&s->check.addr))
+ s->check.xprt = s->xprt;
}
#endif /* USE_OPENSSL */
This may be done with the following 3 steps:
* First, stop to change "srv->check.use_ssl" value
* Then, stop to change the agent settings in srv_set_ssl() because there is
no ssl support for agent-check.
* Finally, fix the bug identified by William, adding the according
documentation.
Note I don't know if all this stuff works properly with the server-state file...
--
Christopher Faulet
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
Le 1/10/22 à 23:19, Willy Tarreau a écrit : w options were still configurable on the CLI by then. "check-ssl" has been available for a long time, so that's not the reason behind it, but I guess you were referring to something else. I suspect I did a dumb copy/paste from the new_server function and probably never thought was possibly wrong as my previous production never had any check using tls. Maybe but then I don't remember about all the detailed rules in place that indicate when check-ssl *has* to be used. I'll have to read the doc. For a health-check, if no port or address is specified and no transport layer is forced, then the transport layer used by the check is the same than for the production traffic. So, the same must be done for dynamic changes. But it is not so simple because, when the check inherits from the server settings, "srv->check.use_ssl" is also changed. I don't remember why this field is updated. But this may prevent any dynamic change on healtcheck. I must read the code to be sure. -- Christopher Faulet
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
Hi William! On Mon, Jan 10, 2022 at 10:49:38PM +0100, William Dauchy wrote: > On Mon, Jan 10, 2022 at 7:51 AM Willy Tarreau wrote: > > It's important to always keep in mind that checks are not necessarily > > related to the production traffic, and that configuring one part should > > not have any impact on the other one. By default a server running in SSL > > will not be checked using SSL unless "check-ssl" is set. > > note it is only true in your example if you use another port. Hmmm I didn't remember about this :-( > > You could for > > example have a server forwarding to multiple ports (say 80 and 443) and > > decide to check only one of them, or even check another one. > > > > As such, I think your patch is correct as it only affects what the user > > attempts to modify. I suspect that the reason for your initial choice was > > that it was not yet possible by then to enable SSL checks manually, > > sorry what do you mean by manually? I meant "on the CLI". I.e. you added support for enabling/disabling SSL while few options were still configurable on the CLI by then. > "check-ssl" has been available for a long time, so that's not the > reason behind it, but I guess you were referring to something else. I > suspect I did a dumb copy/paste from the new_server function and > probably never thought was possibly wrong as my previous production > never had any check using tls. Maybe but then I don't remember about all the detailed rules in place that indicate when check-ssl *has* to be used. I'll have to read the doc. At this point I'm starting to think that we should probably avoid as much as possible to use implicit settings for whatever is dynamic. Originally a lot of settings were implicit because we don't want to have huge config lines to enforce lots of obvious settings. On the CLI it's less of a problem and for example if "ssl" only deals with the traffic without manipulating the checks, I'm personally not shocked, because these are normally sent by bots and we don't care if they have to set a few more settings to avoid multiple implicit changes that may not always be desired. This is where the CLI (or any future API) might differ a bit from the config, and an agent writing some config might have to explicitly state certain things like "no-check-ssl" for example to stay safe and avoid such implicit dependencies. Note that such a brainstorming doesn't apply to your fix and should not hold it from being merged in any way, I'm just speaking in the general sense, as I don't feel comfortable with keep all these special cases in a newly introduced API, that are only there for historical reasons. > > it > > would be worth rechecking, because if that's the case, maybe we should > > not backport it to 2.4 and only document a behavior change between 2.4 > > and 2.5. > > If you could have a double-check at the history behind this, that would > > be nice so that we know how far to backport it. By the way, maybe your > > proposed alternative would be acceptable for older versions which do not > > allow to enable SSL health checks on the CLI. > > unless I missed something, for me the current behavior is broken as > you can't come back to a working state if you are using tls on both > traffic and health check path. That's my understanding as well. > The only working setup is when you are > using `no-check-ssl` in your default server. In that sense I believe > it should be backported to v2.4. If you're *certain* that we're not going to break existing 2.4 deployments that could rely on the current behavior, I'm fine with that. It's just that I absolutely hate behavior changes in stable versions because we tend to think we've seen all corner cases, and after a release someone reports an issue :-/ I can't think of a case which would benefit from the current bug, I'm just trying to be careful. Thanks! Willy
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Sat, Jan 8, 2022 at 3:03 PM Tim Düsterhus wrote: > Causes issues when applying the patch, because git gets confused and > believes this to be the patch. > I tend to indent this type of "literal code block" within my commit > message with 4 spaces for clarity. indeed, good point, will fix if I have to resend a v2 On Mon, Jan 10, 2022 at 7:51 AM Willy Tarreau wrote: > It's important to always keep in mind that checks are not necessarily > related to the production traffic, and that configuring one part should > not have any impact on the other one. By default a server running in SSL > will not be checked using SSL unless "check-ssl" is set. note it is only true in your example if you use another port. > You could for > example have a server forwarding to multiple ports (say 80 and 443) and > decide to check only one of them, or even check another one. > > As such, I think your patch is correct as it only affects what the user > attempts to modify. I suspect that the reason for your initial choice was > that it was not yet possible by then to enable SSL checks manually, sorry what do you mean by manually? "check-ssl" has been available for a long time, so that's not the reason behind it, but I guess you were referring to something else. I suspect I did a dumb copy/paste from the new_server function and probably never thought was possibly wrong as my previous production never had any check using tls. > it > would be worth rechecking, because if that's the case, maybe we should > not backport it to 2.4 and only document a behavior change between 2.4 > and 2.5. > If you could have a double-check at the history behind this, that would > be nice so that we know how far to backport it. By the way, maybe your > proposed alternative would be acceptable for older versions which do not > allow to enable SSL health checks on the CLI. unless I missed something, for me the current behavior is broken as you can't come back to a working state if you are using tls on both traffic and health check path. The only working setup is when you are using `no-check-ssl` in your default server. In that sense I believe it should be backported to v2.4. -- William
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
On Thu, Jan 06, 2022 at 04:57:15PM +0100, William Dauchy wrote: > While giving a fresh try to `set server ssl` (which I wrote), I realised > the behavior is a bit inconsistent. Indeed when using this command over > a server with ssl enabled for the data path but also for the health > check path we have: > > - data and health check done using tls > - emit `set server be_foo/srv0 ssl off` > - data path and health check path becomes plain text > - emit `set server be_foo/srv0 ssl on` > - data path becomes tls and health check path remains plain text > > while I thought the end result would be: > - data path and health check path comes back in tls > > In the current code we indeed erase all connections while deactivating, > but restore only the data path while activating. I made this mistake in > the past because I was testing with a case where the health check plain > text by default. It's important to always keep in mind that checks are not necessarily related to the production traffic, and that configuring one part should not have any impact on the other one. By default a server running in SSL will not be checked using SSL unless "check-ssl" is set. You could for example have a server forwarding to multiple ports (say 80 and 443) and decide to check only one of them, or even check another one. As such, I think your patch is correct as it only affects what the user attempts to modify. I suspect that the reason for your initial choice was that it was not yet possible by then to enable SSL checks manually, it would be worth rechecking, because if that's the case, maybe we should not backport it to 2.4 and only document a behavior change between 2.4 and 2.5. If you could have a double-check at the history behind this, that would be nice so that we know how far to backport it. By the way, maybe your proposed alternative would be acceptable for older versions which do not allow to enable SSL health checks on the CLI. Thanks, Willy
Re: [PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
William,
as a heads up, this part of the commit message:
On 1/6/22 4:57 PM, William Dauchy wrote:
The alternative solution was to restore the previous state, but I
believe this will create even more confusion in the future:
--- a/src/server.c
+++ b/src/server.c
@@ -2113,8 +2113,11 @@ void srv_set_ssl(struct server *s, int use_ssl)
return;
s->use_ssl = use_ssl;
- if (s->use_ssl)
+ if (use_ssl) {
s->xprt = xprt_get(XPRT_SSL);
+ if (s->check.use_ssl)
+ s->check.xprt = s->agent.xprt = xprt_get(XPRT_SSL);
+ }
else
s->xprt = s->check.xprt = s->agent.xprt = xprt_get(XPRT_RAW);
}
Causes issues when applying the patch, because git gets confused and
believes this to be the patch.
I tend to indent this type of "literal code block" within my commit
message with 4 spaces for clarity.
Best regards
Tim Düsterhus
[PATCH] BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
While giving a fresh try to `set server ssl` (which I wrote), I realised
the behavior is a bit inconsistent. Indeed when using this command over
a server with ssl enabled for the data path but also for the health
check path we have:
- data and health check done using tls
- emit `set server be_foo/srv0 ssl off`
- data path and health check path becomes plain text
- emit `set server be_foo/srv0 ssl on`
- data path becomes tls and health check path remains plain text
while I thought the end result would be:
- data path and health check path comes back in tls
In the current code we indeed erase all connections while deactivating,
but restore only the data path while activating. I made this mistake in
the past because I was testing with a case where the health check plain
text by default.
There are several ways to solve this issue. The cleanest one would
probably be to avoid changing the health check connection when we use
`set server ssl` command, and create a new command `set server
ssl-check` to change this. For now I assumed this would be ok to simply
avoid changing the health check path and be more consistent.
This patch tries to address that and also update the documentation. It
should not break the existing usage with health check on plain text, as
in this case they should have `no-check-ssl` in defaults. Without this
patch, it makes the command unusable in an env where you have a list of
server to add along the way with initial `server-template`, and all
using tls for data and healthcheck path.
For 2.6 we should probably reconsider and add `set server ssl-check`
command for better granularity of cases.
If this solution is accepted, this patch should be backported up to >=
2.4.
The alternative solution was to restore the previous state, but I
believe this will create even more confusion in the future:
--- a/src/server.c
+++ b/src/server.c
@@ -2113,8 +2113,11 @@ void srv_set_ssl(struct server *s, int use_ssl)
return;
s->use_ssl = use_ssl;
- if (s->use_ssl)
+ if (use_ssl) {
s->xprt = xprt_get(XPRT_SSL);
+ if (s->check.use_ssl)
+ s->check.xprt = s->agent.xprt = xprt_get(XPRT_SSL);
+ }
else
s->xprt = s->check.xprt = s->agent.xprt = xprt_get(XPRT_RAW);
}
Signed-off-by: William Dauchy
---
doc/management.txt | 2 ++
src/server.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/doc/management.txt b/doc/management.txt
index 147e059f6..b96cbc878 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -2187,6 +2187,8 @@ set server / fqdn
set server / ssl [ on | off ]
This option configures SSL ciphering on outgoing connections to the server.
+ When switch off, all traffic becomes plain text; health check path is not
+ changed.
set severity-output [ none | number | string ]
Change the severity output format of the stats socket connected to for the
diff --git a/src/server.c b/src/server.c
index 2061153bc..d165f50b9 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2116,7 +2116,7 @@ void srv_set_ssl(struct server *s, int use_ssl)
if (s->use_ssl)
s->xprt = xprt_get(XPRT_SSL);
else
- s->xprt = s->check.xprt = s->agent.xprt = xprt_get(XPRT_RAW);
+ s->xprt = xprt_get(XPRT_RAW);
}
#endif /* USE_OPENSSL */
--
2.34.1
