Copilot commented on code in PR #13349: URL: https://github.com/apache/trafficserver/pull/13349#discussion_r3536958264
########## src/iocore/net/SSLKeyUtils.cc: ########## @@ -0,0 +1,186 @@ +/** @file + + @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 "SSLKeyUtils.h" +#include "P_SSLUtils.h" + +#include <tscore/Diags.h> +#ifdef OPENSSL_IS_OPENSSL3 +#include <tscore/ink_assert.h> +#else +#include <tscore/ink_config.h> +#endif + +#ifdef OPENSSL_IS_OPENSSL3 +#include <openssl/evp.h> +#include <openssl/decoder.h> +#include <openssl/params.h> +#include <openssl/ssl.h> +#else +#include <openssl/bio.h> +#include <openssl/bn.h> +#include <openssl/pem.h> +#include <openssl/ssl.h> +#endif + +#ifdef OPENSSL_IS_OPENSSL3 + +EVP_PKEY * +gen_dh_2048_256_pkey() +{ + scoped_PKEY_CTX pctx{EVP_PKEY_CTX_new_from_name(NULL, "DH", NULL)}; + if (!pctx) { + Error("failed to create OpenSSL pkey context"); + return nullptr; + } + + if (EVP_PKEY_keygen_init(pctx.get()) <= 0) { + Error("failed to initialize OpenSSL keygen"); + return nullptr; + } + + char prime_group[]{"dh_2048_256"}; + OSSL_PARAM const params[]{OSSL_PARAM_utf8_string("group", prime_group, 0), OSSL_PARAM_END}; + + if (!EVP_PKEY_CTX_set_params(pctx.get(), params)) { + Error("SSL dhparams source returned invalid parameters"); + return nullptr; + } + + EVP_PKEY *pkey{}; + EVP_PKEY_generate(pctx.get(), &pkey); + + return pkey; Review Comment: In the OpenSSL 3 path, this helper initializes the context with EVP_PKEY_keygen_init() but then only needs DH *parameters* for SSL_CTX_set0_tmp_dh_pkey(). Using keygen here is unnecessary work (generates private key material) and can consume extra entropy; also EVP_PKEY_generate()’s return value is not checked, so failures silently return a null/partial key. ########## src/iocore/net/SSLKeyUtils.cc: ########## @@ -0,0 +1,186 @@ +/** @file + + @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 "SSLKeyUtils.h" +#include "P_SSLUtils.h" + +#include <tscore/Diags.h> +#ifdef OPENSSL_IS_OPENSSL3 +#include <tscore/ink_assert.h> +#else +#include <tscore/ink_config.h> +#endif + +#ifdef OPENSSL_IS_OPENSSL3 +#include <openssl/evp.h> +#include <openssl/decoder.h> +#include <openssl/params.h> +#include <openssl/ssl.h> +#else +#include <openssl/bio.h> +#include <openssl/bn.h> +#include <openssl/pem.h> +#include <openssl/ssl.h> +#endif + +#ifdef OPENSSL_IS_OPENSSL3 + +EVP_PKEY * +gen_dh_2048_256_pkey() +{ + scoped_PKEY_CTX pctx{EVP_PKEY_CTX_new_from_name(NULL, "DH", NULL)}; + if (!pctx) { + Error("failed to create OpenSSL pkey context"); + return nullptr; + } + + if (EVP_PKEY_keygen_init(pctx.get()) <= 0) { + Error("failed to initialize OpenSSL keygen"); + return nullptr; + } + + char prime_group[]{"dh_2048_256"}; + OSSL_PARAM const params[]{OSSL_PARAM_utf8_string("group", prime_group, 0), OSSL_PARAM_END}; + + if (!EVP_PKEY_CTX_set_params(pctx.get(), params)) { + Error("SSL dhparams source returned invalid parameters"); + return nullptr; + } + + EVP_PKEY *pkey{}; + EVP_PKEY_generate(pctx.get(), &pkey); + + return pkey; +} + +EVP_PKEY * +load_dhparams_file(char const *dhparams_file) +{ + EVP_PKEY *pkey{}; + scoped_Decoder_CTX dctx{OSSL_DECODER_CTX_new_for_pkey(&pkey, "PEM", NULL, "DH", OSSL_KEYMGMT_SELECT_ALL_PARAMETERS, NULL, NULL)}; + if (!dctx) { + Error("failed to create OpenSSL decoder context"); + return nullptr; + } + + ink_assert(OSSL_DECODER_CTX_get_num_decoders(dctx.get()) > 0); + scoped_BIO bio{BIO_new_file(dhparams_file, "r")}; + if (!OSSL_DECODER_from_bio(dctx.get(), bio.get())) { + Error("SSL dhparams source returned invalid parameters"); + return nullptr; + } + + return pkey; +} Review Comment: load_dhparams_file() assumes BIO_new_file() succeeds and passes bio.get() to OSSL_DECODER_from_bio() unconditionally. If the file can’t be opened (e.g., nonexistent path), bio will be null and this can crash. Also, using ink_assert() on decoder availability turns a missing/disabled decoder/provider into a hard abort instead of a clean configuration error. ########## src/iocore/net/unit_tests/test_SSLDHParams.cc: ########## @@ -0,0 +1,186 @@ +/** @file + + Catch based unit tests for the DH-parameter handling behavior of + SSLMultiCertConfigLoader::init_server_ssl_ctx, which is the inknet + public boundary that transitively invokes ssl_context_enable_dhe + and (when a file is configured) load_dhparams_file. + + @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 <iocore/net/SSLMultiCertConfigLoader.h> +#include "../P_SSLCertLookup.h" +#include "../P_SSLConfig.h" +#include "../P_SSLUtils.h" + +#include <tscore/ink_memory.h> +#include <tscore/ink_platform.h> + +#include <catch2/catch_test_macros.hpp> + +#include <openssl/bio.h> +#include <openssl/core_names.h> +#include <openssl/evp.h> +#include <openssl/pem.h> +#include <openssl/ssl.h> + +#include <cstdio> +#include <string> + +namespace +{ + +std::string +make_valid_dh_pem() +{ + EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_from_name(nullptr, "DH", nullptr); + REQUIRE(pctx != nullptr); + REQUIRE(EVP_PKEY_paramgen_init(pctx) > 0); + char prime_group[]{"dh_2048_256"}; + OSSL_PARAM const params[2] = { + OSSL_PARAM_construct_utf8_string("group", prime_group, 0), + OSSL_PARAM_construct_end(), + }; + REQUIRE(EVP_PKEY_CTX_set_params(pctx, params) > 0); + EVP_PKEY *pkey = nullptr; + REQUIRE(EVP_PKEY_generate(pctx, &pkey) > 0); + + BIO *bio = BIO_new(BIO_s_mem()); + REQUIRE(PEM_write_bio_Parameters(bio, pkey) == 1); + BUF_MEM *bm = nullptr; + BIO_get_mem_ptr(bio, &bm); + std::string out{bm->data, bm->length}; + BIO_free(bio); + EVP_PKEY_free(pkey); + EVP_PKEY_CTX_free(pctx); + return out; +} + +std::string +make_rsa_pem() +{ + EVP_PKEY *pkey = EVP_RSA_gen(1024); Review Comment: This test generates a 1024-bit RSA key to produce a “wrong key type” PEM. 1024-bit RSA is rejected in some OpenSSL 3 configurations/policies (notably FIPS), which can make the unit test fail for reasons unrelated to DH parameter handling. Using 2048 avoids that class of policy failures while still exercising the DH-only decoder rejection path. -- 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]
