Copilot commented on code in PR #13088: URL: https://github.com/apache/trafficserver/pull/13088#discussion_r3083300294
########## src/iocore/net/TLSCertCompression.cc: ########## @@ -0,0 +1,109 @@ +/** @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 + +struct alg_info { + const char *name; + int32_t number; + ssl_cert_compression_func_t compress_func; + ssl_cert_decompression_func_t decompress_func; +} supported_algs[] = { + {"zlib", 1, compression_func_zlib, decompression_func_zlib }, +#if HAVE_BROTLI_ENCODE_H + {"brotli", 2, compression_func_brotli, decompression_func_brotli}, +#endif +#if HAVE_ZSTD_H + {"zstd", 3, 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); + ink_assert(specified_algs.size() <= N_ALGORITHMS); + + 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); + } + } + return 1; +#elif HAVE_SSL_CTX_SET1_CERT_COMP_PREFERENCE + int algs[N_ALGORITHMS]; + int n = 0; + + for (int i = 0; i < specified_algs.size(); ++i) { + for (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); + } + } + } + return SSL_CTX_set1_cert_comp_preference(ctx, algs, n); Review Comment: In the HAVE_SSL_CTX_SET1_CERT_COMP_PREFERENCE branch, this code references supported_algs, but supported_algs is only declared under HAVE_SSL_CTX_ADD_CERT_COMPRESSION_ALG. This will fail to compile when only the *_SET1_* API is available. Move the supported_algs mapping out so it is available to both branches (or create an equivalent mapping for the SET1 branch). ########## src/iocore/net/TLSCertCompression_brotli.h: ########## @@ -0,0 +1,29 @@ +/** @file + + Functions for brotli compression/decompression + + @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. + */ + +#pragma once + +#include <openssl/bytestring.h> Review Comment: This header declares functions taking SSL* and CRYPTO_BUFFER** but does not include a header that declares those types. Since the corresponding .cc includes <openssl/ssl.h> after this header, this can cause a compile failure. Include <openssl/ssl.h> here (or forward declare the needed types) to make the header self-contained. ```suggestion #include <openssl/bytestring.h> #include <openssl/ssl.h> ``` ########## src/iocore/net/TLSCertCompression.cc: ########## @@ -0,0 +1,109 @@ +/** @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 + +struct alg_info { + const char *name; + int32_t number; + ssl_cert_compression_func_t compress_func; + ssl_cert_decompression_func_t decompress_func; +} supported_algs[] = { + {"zlib", 1, compression_func_zlib, decompression_func_zlib }, +#if HAVE_BROTLI_ENCODE_H + {"brotli", 2, compression_func_brotli, decompression_func_brotli}, +#endif +#if HAVE_ZSTD_H + {"zstd", 3, 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); + ink_assert(specified_algs.size() <= N_ALGORITHMS); + + 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); + } + } + return 1; +#elif HAVE_SSL_CTX_SET1_CERT_COMP_PREFERENCE + int algs[N_ALGORITHMS]; + int n = 0; + + for (int i = 0; i < specified_algs.size(); ++i) { + for (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: The only protection against too many/duplicate algorithm entries is ink_assert(specified_algs.size() <= N_ALGORITHMS), but specified_algs comes from user-configured strings. In release builds this assert is compiled out, and the SET1 branch can write past the end of algs[] via algs[n++] when duplicates or >3 values are provided. Replace the assert with runtime validation (dedupe + cap at N_ALGORITHMS, or return an error) so n never exceeds the array size. ########## src/iocore/net/TLSCertCompression_brotli.cc: ########## @@ -0,0 +1,73 @@ +/** @file + + Functions for brotli compression/decompression + + @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 "TLSCertCompression_brotli.h" +#include "SSLStats.h" +#include <openssl/ssl.h> +#include <brotli/decode.h> +#include <brotli/encode.h> + +int +compression_func_brotli(SSL * /* ssl */, CBB *out, const uint8_t *in, size_t in_len) +{ + // TODO Need a cache mechanism inside this function for better performance. + + uint8_t *buf; + unsigned long buf_len = BrotliEncoderMaxCompressedSize(in_len); + CBB_reserve(out, &buf, buf_len); + Review Comment: CBB_reserve() return value is not checked. If the reserve fails, buf is undefined and the subsequent BrotliEncoderCompress() call can crash or corrupt memory. Check the return value and treat failure as a compression failure (increment failure metric and return 0). ```suggestion if (!CBB_reserve(out, &buf, buf_len)) { CBB_did_write(out, 0); Metrics::Counter::increment(ssl_rsb.cert_compress_brotli_failure); return 0; } ``` ########## src/iocore/net/TLSCertCompression.h: ########## @@ -0,0 +1,36 @@ +/** @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. + */ + +#pragma once + +#include <openssl/ssl.h> + +/** + * Common function to set certificate compression preference + * + * @param ctx SSL_CTX + * @param algs An array that contains compression algorithm names ("zlib", "brotli", or "zstd") + * @param count The count of algorithm names in algs Review Comment: The doc comment mentions a "count" parameter that no longer exists in the function signature, which makes the API documentation misleading. Update the comment to match the actual parameters and return semantics. ```suggestion * @param algs Compression algorithm names ("zlib", "brotli", or "zstd") ``` ########## src/iocore/net/TLSCertCompression_zstd.cc: ########## @@ -0,0 +1,79 @@ +/** @file + + Functions for zstd compression/decompression + + @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 "TLSCertCompression_zstd.h" +#include "SSLStats.h" +#include <openssl/ssl.h> +#include <zstd.h> + +int +compression_func_zstd(SSL * /* ssl */, CBB *out, const uint8_t *in, size_t in_len) +{ + // TODO Need a cache mechanism inside this function for better performance. + + uint8_t *buf; + unsigned long buf_len = ZSTD_compressBound(in_len); + + if (ZSTD_isError(buf_len) == 1) { + Metrics::Counter::increment(ssl_rsb.cert_compress_zstd_failure); + return 0; + } + + CBB_reserve(out, &buf, buf_len); + + // For better performance ZSTD_compressCCtx, which reuses a context object, should be used. + // One context object need to be made for each thread. + size_t ret = ZSTD_compress(buf, buf_len, in, in_len, ZSTD_CLEVEL_DEFAULT); Review Comment: CBB_reserve() return value is not checked. If the reserve fails, buf is undefined and the subsequent ZSTD_compress() call can crash or corrupt memory. Check the return value and treat failure as a compression failure (increment failure metric and return 0). ########## src/iocore/net/TLSCertCompression_zlib.h: ########## @@ -0,0 +1,29 @@ +/** @file + + Functions for zlib compression/decompression + + @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. + */ + +#pragma once + +#include <openssl/bytestring.h> Review Comment: This header declares functions taking SSL* and CRYPTO_BUFFER** but does not include a header that declares those types. Since the corresponding .cc includes <openssl/ssl.h> after this header, this can cause a compile failure. Include <openssl/ssl.h> here (or forward declare the needed types) to make the header self-contained. ```suggestion #include <openssl/bytestring.h> #include <openssl/ssl.h> ``` ########## src/iocore/net/TLSCertCompression_zstd.h: ########## @@ -0,0 +1,29 @@ +/** @file + + Functions for zstd compression/decompression + + @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. + */ + +#pragma once + +#include <openssl/bytestring.h> Review Comment: This header declares functions taking SSL* and CRYPTO_BUFFER** but does not include a header that declares those types. Since the corresponding .cc includes <openssl/ssl.h> after this header, this can cause a compile failure. Include <openssl/ssl.h> here (or forward declare the needed types) to make the header self-contained. ```suggestion #include <openssl/bytestring.h> #include <openssl/ssl.h> ``` ########## src/iocore/net/TLSCertCompression_zlib.cc: ########## @@ -0,0 +1,71 @@ +/** @file + + Functions for zlib compression/decompression + + @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 "TLSCertCompression_zlib.h" +#include "SSLStats.h" +#include <openssl/ssl.h> +#include <zlib.h> + +int +compression_func_zlib(SSL * /* ssl */, CBB *out, const uint8_t *in, size_t in_len) +{ + // TODO Need a cache mechanism inside this function for better performance. + + uint8_t *buf; + unsigned long buf_len = compressBound(in_len); + CBB_reserve(out, &buf, buf_len); + Review Comment: CBB_reserve() return value is not checked. If the reserve fails, buf is undefined and the subsequent compress() call can crash or corrupt memory. Check the return value and treat failure as a compression failure (increment failure metric and return 0). ```suggestion if (!CBB_reserve(out, &buf, buf_len)) { Metrics::Counter::increment(ssl_rsb.cert_compress_zlib_failure); return 0; } ``` ########## src/iocore/net/TLSCertCompression.h: ########## @@ -0,0 +1,36 @@ +/** @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. + */ + +#pragma once + +#include <openssl/ssl.h> + +/** + * Common function to set certificate compression preference + * + * @param ctx SSL_CTX + * @param algs An array that contains compression algorithm names ("zlib", "brotli", or "zstd") + * @param count The count of algorithm names in algs + * @return 1 on success + */ +int register_certificate_compression_preference(SSL_CTX *ctx, std::vector<std::string> algs); Review Comment: This header uses std::vector<std::string> but does not include the required standard headers, which can break compilation depending on include order. Add the appropriate includes (e.g., <string> and <vector>) so the header is self-contained. -- 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]
