This allows old ssl_multicert.config to still function on reload #1488
Conversation
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1613/ for details. |
|
Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/46/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1508/ for details. |
| if (ssl_store_ssl_context(params, lookup, &sslMultiCertSettings) == nullptr) { | ||
| Error("failed to load SSL server contexts"); | ||
| return false; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I'm a bit confused why this fixes anything. I don't see any callers checking the return value.
There was a problem hiding this comment.
Yeh, it's not used, but the point is that we stop parsing the configuration at this point, which leaves lookup->is_valid == false (correctly). I believe in the old version, the status of the lookup is the status of the last certificate in the configuration.
There was a problem hiding this comment.
I'd be fine adding additional actionable checks here if you like, I'm just not familiar with most of this code.
|
clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/178/ for details. |
|
I see now, there's a new configuration, proxy.config.ssl.server.multicert.exit_on_load_fail, which would exit on load errors. Surprised that this is not the default to "1" though. However, the fix above would retain (what I think is) correct behavior on reloads, and if proxy.config.ssl.server.multicert.exit_on_load_fail is off (0), you'd still be able to load the initially broken configuration. Why anyone wants a broken configuration is beyond me, but I'm ok with this, but think it should be changed to "1" for v8.0.0. |
|
I'm gonna close this PR for now, I think this code could be cleaned up, but setting proxy.config.ssl.server.multicert.exit_on_load_fail=1 seems to behave more reasonable than the defaults. |
The problem is that if a certificate fails to load, for whatever reason that might be, ATS still switches the configuration, leaving the server in a crippled state. This patch fixes this, to the normal behavior of ATS retaining the old configurations if the new fails to load.