Copilot commented on code in PR #13088:
URL: https://github.com/apache/trafficserver/pull/13088#discussion_r3104295213
##########
src/iocore/net/SSLClientUtils.cc:
##########
@@ -247,6 +249,18 @@ SSLInitClientContext(const SSLConfigParams *params)
}
#endif
+ if (params->client_cert_compression_algorithms) {
+ std::vector<std::string> algs;
+ SimpleTokenizer tok(params->client_cert_compression_algorithms,
',');
+ for (const char *token = tok.getNext(); token; token = tok.getNext()) {
+ algs.emplace_back(token);
Review Comment:
The client-side parsing of
`proxy.config.ssl.client.cert_compression.algorithms` doesn’t trim whitespace
around comma-separated tokens. This makes common configs like `zlib, brotli`
fail validation even though they’re logically correct. Trim each token (and
skip empty tokens) before passing the list to
`register_certificate_compression_preference()`.
```suggestion
std::string algorithm(token);
auto const begin = algorithm.find_first_not_of(" \t\r\n\f\v");
if (begin == std::string::npos) {
continue;
}
auto const end = algorithm.find_last_not_of(" \t\r\n\f\v");
algorithm = algorithm.substr(begin, end - begin + 1);
algs.emplace_back(std::move(algorithm));
```
##########
src/iocore/net/TLSCertCompression.cc:
##########
@@ -0,0 +1,128 @@
+/** @file
+
+ Functions for Certificate Compression
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include <openssl/ssl.h>
+
+#include "tscore/Diags.h"
+#include "TLSCertCompression.h"
+
+namespace
+{
+DbgCtl dbg_ctl_ssl_cert_compress{"ssl_cert_compress"};
+}
+
+constexpr unsigned int N_ALGORITHMS = 3;
+
+#if HAVE_SSL_CTX_ADD_CERT_COMPRESSION_ALG
+#include "TLSCertCompression_zlib.h"
+
+#if HAVE_BROTLI_ENCODE_H
+#include "TLSCertCompression_brotli.h"
+#endif
+
+#if HAVE_ZSTD_H
+#include "TLSCertCompression_zstd.h"
+#endif
+#endif
+
+struct alg_info {
+ const char *name;
+ int32_t number;
+#if HAVE_SSL_CTX_ADD_CERT_COMPRESSION_ALG
+ ssl_cert_compression_func_t compress_func;
+ ssl_cert_decompression_func_t decompress_func;
+#endif
+} supported_algs[] = {
+ {"zlib", 1,
+#if HAVE_SSL_CTX_ADD_CERT_COMPRESSION_ALG
+ compression_func_zlib, decompression_func_zlib
+#endif
+ },
+#if HAVE_BROTLI_ENCODE_H
+ {"brotli", 2,
+#if HAVE_SSL_CTX_ADD_CERT_COMPRESSION_ALG
+ compression_func_brotli, decompression_func_brotli
+#endif
+ },
+#endif
+#if HAVE_ZSTD_H
+ {"zstd", 3,
+#if HAVE_SSL_CTX_ADD_CERT_COMPRESSION_ALG
+ compression_func_zstd, decompression_func_zstd
+#endif
+ },
+#endif
+};
+
+int
+register_certificate_compression_preference(SSL_CTX *ctx,
std::vector<std::string> specified_algs)
+{
+ ink_assert(ctx != nullptr);
+ if (specified_algs.size() > N_ALGORITHMS) {
+ return 0;
+ }
+
+ if (specified_algs.empty()) {
+ return 1;
+ }
+
+#if HAVE_SSL_CTX_ADD_CERT_COMPRESSION_ALG
+ for (auto &&alg : specified_algs) {
+ struct alg_info *info = nullptr;
+
+ for (unsigned int i = 0; i < countof(supported_algs); ++i) {
+ if (strcmp(alg.c_str(), supported_algs[i].name) == 0) {
+ info = &supported_algs[i];
+ }
+ }
+ if (info != nullptr) {
+ if (SSL_CTX_add_cert_compression_alg(ctx, info->number,
info->compress_func, info->decompress_func) == 0) {
+ return 0;
+ }
+ Dbg(dbg_ctl_ssl_cert_compress, "Enabled %s", info->name);
+ } else {
+ Dbg(dbg_ctl_ssl_cert_compress, "Unrecognized algorithm: %s",
alg.c_str());
+ return 0;
+ }
+ }
+ return 1;
+#elif HAVE_SSL_CTX_SET1_CERT_COMP_PREFERENCE
+ int algs[N_ALGORITHMS];
+ int n = 0;
+
+ for (unsigned int i = 0; i < specified_algs.size(); ++i) {
+ for (unsigned int j = 0; j < countof(supported_algs); ++j) {
+ if (strcmp(specified_algs[i].c_str(), supported_algs[j].name) == 0) {
+ algs[n++] = supported_algs[j].number;
+ Dbg(dbg_ctl_ssl_cert_compress, "Enabled %s", supported_algs[j].name);
+ }
+ }
Review Comment:
In the `HAVE_SSL_CTX_SET1_CERT_COMP_PREFERENCE` branch, unrecognized
algorithm names are silently ignored (the loop only appends matches). This
means typos or unsupported algorithms may not be reported, and
`SSL_CTX_set1_cert_comp_preference()` may be called with `n == 0` even though
the config was non-empty. Track whether each specified algorithm matched; if
any didn’t, log/debug and return failure to keep behavior consistent with the
`SSL_CTX_add_cert_compression_alg` branch.
```suggestion
for (unsigned int i = 0; i < specified_algs.size(); ++i) {
bool matched = false;
for (unsigned int j = 0; j < countof(supported_algs); ++j) {
if (strcmp(specified_algs[i].c_str(), supported_algs[j].name) == 0) {
algs[n++] = supported_algs[j].number;
Dbg(dbg_ctl_ssl_cert_compress, "Enabled %s", supported_algs[j].name);
matched = true;
break;
}
}
if (!matched) {
Dbg(dbg_ctl_ssl_cert_compress, "Unrecognized algorithm: %s",
specified_algs[i].c_str());
return 0;
}
```
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -435,6 +436,26 @@ DH_get_2048_256()
}
#endif
+bool
+SSLMultiCertConfigLoader::_enable_cert_compression(SSL_CTX *ctx)
+{
+ std::vector<std::string> algs;
+
+ if (this->_params->server_cert_compression_algorithms) {
+ SimpleTokenizer tok(this->_params->server_cert_compression_algorithms,
',');
+ for (const char *token = tok.getNext(); token; token = tok.getNext()) {
+ algs.emplace_back(token);
Review Comment:
When parsing the comma-separated algorithm list, tokens aren’t trimmed.
Config values like `zlib, brotli` will produce a token with a leading space and
fail to match supported algorithms (resulting in unexpected startup failures).
Trim surrounding whitespace on each token before adding it to `algs` (and
consider skipping empty tokens).
```suggestion
std::string algorithm(token);
auto const begin = algorithm.find_first_not_of(" \t\n\r\f\v");
if (begin == std::string::npos) {
continue;
}
auto const end = algorithm.find_last_not_of(" \t\n\r\f\v");
algs.emplace_back(algorithm.substr(begin, end - begin + 1));
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
