Fix tls-dh support for DHE parameters with OpenSSL v3+ #1949

sp-andwei · 2024-11-19T14:51:04Z

# When applying tls-dh=prime256v1:dhparams.pem configuration:
WARNING: Failed to decode EC parameters 'dhparams.pem'

# When forcing the use of FFDHE with something like
# openssl s_client -tls1_2 -cipher DHE-RSA-AES256-SHA256 -connect...
ERROR: failure while accepting a TLS connection on:
    SQUID_TLS_ERR_ACCEPT+TLS_LIB_ERR=A0000C1+TLS_IO_ERR=1

Squid https_port ... tls-dh=curve:dhparams.pem configuration is
supposed to support both ECDHE and FFDHE key exchange mechanisms (and
their cipher suites), depending on client-supported cipher suites. ECDHE
mechanism should use the named curve (e.g., prime256v1), and FFDHE
mechanism should use key exchange parameters loaded from the named PEM
file (e.g., ffdhe4096 named group specified in RFC 7919).

When 2022 commit 742236c added support for OpenSSL v3 APIs, new
loadDhParams() code misinterpreted curve name presence in tls-dh value
as an indication that the named parameters file contains ECDHE
parameters, setting OSSL_DECODER_CTX_new_for_pkey() type parameter to
"EC", and (when parameter file specified FFDHE details) triggering the
WARNING message quoted above.

Squid should not expect additional ECDHE parameters when the elliptic
curve group is already fully specified by naming it at the start of
tls-dh value. Squid now reverts to earlier (v4) behavior, where
the two mechanisms can coexist and can be configured separately as
described above:

$ openssl s_client -tls1_2 -cipher DHE-RSA-AES256-SHA256 -connect...
Server Temp Key: DH, 4096 bits

$ openssl s_client -connect...
Server Temp Key: ECDH, prime256v1, 256 bits

Furthermore, updateContextEecdh() code in commit 742236c continued to
load parsed parameters using old SSL_CTX_set_tmp_dh() call but should
have used SSL_CTX_set0_tmp_dh_pkey() API because the type of parsed
parameters (i.e. DhePointer) have changed from DH to EVP_PKEY pointer.
This second bug affected configurations with and without an explicit
curve name in tls-dh value.

Also report a failure to load parsed parameters into TLS context.

The #elif to #if OPENSSL_MAJOR_VERSION < 3 new code block introduced with 742236c does not work. Instead of trying to load parameters for DHE, it tries to parse the input file as EC Key if any curve is given to the tls-dh parameter, e.g. tls-dh=prime256v1:<path-to-dhparams>/dhparams4096.pem This causes TLS handshakes to fail if the client uses a configuration that does not accept ECDHE. This commit corrects this by setting the type to "DH" and to further also correctly load dhparams into the SSL_CTX from an EVP_PKEY (in contrast to the DH used with openssl < 3), the correct function is used.

rousskov

Thanks a lot for explaining and fixing this messy problem!

I do not know enough to review these low-level OpenSSL parameter management details, but I can help with fixing PR description formatting and satisfying Squid code styling guidelines. Are you OK with me making a few simple commits to your PR branch (instead of posting polishing change requests)?

sp-andwei · 2024-11-19T15:24:05Z

Hi Alex. Yeah of course, please go ahead.

"Failed" may be better, but all other ERRORs in this context are using (passable/reasonable) "Unable". Preserve that consistency.

This may help reduce triage iterations.

…call

IIRC, when working on this code earlier (before this branch), I, for one, did not realize that ECDHE curve name is orthogonal to DHE parameters. I incorrectly thought that the curve name is a part of DHE parameters.

src/security/ServerOptions.cc

rousskov

Thank you for testing updated PR code. I have edited PR title/description to make them more suitable for the future commit message and to fix formatting. Please check and adjust further as needed.

I have only one concern/question left. Please see the dedicated review comment for details.

rousskov · 2024-11-19T20:18:42Z

src/security/ServerOptions.cc


    Ssl::ForgetErrors();
    EVP_PKEY *rawPkey = nullptr;
    using DecoderContext = std::unique_ptr<OSSL_DECODER_CTX, HardFun<void, OSSL_DECODER_CTX*, &OSSL_DECODER_CTX_free> >;
-    if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, 0, nullptr, nullptr)}) {
+    if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, nullptr, nullptr)}) {


Should we also allow so called "other" parameters (i.e. OSSL_KEYMGMT_SELECT_OTHER_PARAMETERS) here? For example:

Suggested change

if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, nullptr, nullptr)}) {

if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_ALL_PARAMETERS, nullptr, nullptr)}) {

I cannot tell whether those "other" parameters can be compatible with how Squid uses parsed/loaded parameters. If you are not sure, and tests pass with more permissive OSSL_KEYMGMT_SELECT_ALL_PARAMETERS, then perhaps we should err on the side of allowing more?

This parameter was explicitly 0 ("set to 0 to indicate that the code will auto detect the selection") to account for the variance in which details need to be loaded for each type of cipher family being loaded.

Please revert.

This parameter was explicitly 0 ("set to 0 to indicate that the code will auto detect the selection") to account for the variance in which details need to be loaded for each type of cipher family being loaded.

Agreed. AFAICT, OSSL_KEYMGMT_SELECT_ALL_PARAMETERS would have the same effect if relevant "details" are limited to parameters and cannot be keys (because OSSL_KEYMGMT_SELECT_ALL_PARAMETERS excludes OSSL_KEYMGMT_SELECT_PRIVATE_KEY and OSSL_KEYMGMT_SELECT_PUBLIC_KEY bits).

Please revert.

I support reverting this polishing(?) change if it does not hurt known tests. If all other factors are equal, then OSSL_KEYMGMT_SELECT_ALL_PARAMETERS would have been better if it rejects parameter files that contain (irrelevant) keys, but we have bigger issues to solve and do not have to polish this code.

yadij

Squid https_port ... tls-dh=curve:dhparams.pem configuration is
supposed to support both ECDHE and FFDHE key exchange mechanisms

This is false. Squid has never supported FFDHE (RFC 7919). Squid implements RFC 4492. That only supports classical DH cipher, or ECDH cipher(s).

yadij · 2024-11-19T21:26:32Z

src/security/ServerOptions.cc

-    const auto type = eecdhCurve.isEmpty() ? "DH" : "EC";
+    const auto type = "DH";


Parameters contained within the file pointed to by tls-dh= contains parameters used by either Elliptic-Curve DH cipher(s) or the classical DH cipher. This type parameter determines which of the library interpreters is used to load the file.

Please revert. Any fixes to this logic need to work regardless of this type value.

Amos: This type parameter determines which of the library interpreters is used to load the file.

Agreed.

PR: It does not make sense to try and load additional parameters when the elliptic curve group is already fully specified by naming it as first part of the tls-dh parameter

Amos: Parameters contained within the file pointed to by tls-dh= contains parameters used by either Elliptic-Curve DH cipher(s) or the classical DH cipher.

This PR effectively asserts that ECDH mechanisms with named curves have no (i.e. cannot have any) additional parameters. Do you dispute that assertion? Developing an agreement regarding this assertion validity is key to handling several other change requests!

Please revert. Any fixes to this logic need to work regardless of this type value.

Reverting these changes will not help with the primary problem this PR is trying to address -- concurrent support for two different mechanism families. Let's table this suggestion for a second and come back to it once the "no additional parameters" claim is validated or rejected.

Parameters contained within the file pointed to by tls-dh= contains parameters used by either Elliptic-Curve DH cipher(s) or the classical DH cipher.

This is just not true, as I stated and shown in my introductory post. You could and we actually have configured squid to configure a ECDHE curve and DHE parameters with a group read from the file system. I've shown the relevant excerpt from the openssl s_client in my introductory post (Server Tmp Key: ...). This has been the behavior for years. And this was also the only behavior. There was no successful loading of ECDH parameters at all.

Please refer to src/security/ServerOptions.cc. AFAICS, you have

parsedDhParams, dhParamsFile

eecdhCurve

for OPENSSL_MAJOR_VERSION < 3 you have

Security::ServerOptions::loadDhParams: unconditionally loads DHE Params from dhParamsFile using PEM_read_DHparams and stores them into the DH-type struct parsedDhParams; no EC-param loading at all

Security::ServerOptions::updateContextEecdh: derives nid + uses SSL_CTX_set_tmp_ecdh to set the ECDH curve to use; then calls SSL_CTX_set_tmp_dh if parsedDhParams is set

unless I'm really mistaken, this code (as used for Squid versions 4 to 5) does never ever actually pass anything from the dhParamsFile that is related to ECDH

for the #else (openssl >= 3.x) branch:

Security::ServerOptions::loadDhParams: does not work if a curve and dhparams are specified; it loads either some EC or DH(E) Parameters into an EVP_PKEY structure

updateContextEecdh: derives nid and uses SSL_CTX_set1_groups to set the curve for ECDH; this still works (otherwise, ECDHE would currently be completely broken) --- this is all that is ever needed to successfully setup ECDHE AFAICT

updateContextEecdh: continues with still calling SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get());, which will always fail, no matter if type above was EC or DH, because it expects a DH structure and not an EVP_PKEY.

Thus, the PR tries to restore the bevahavior that Squid exhibited in the last major versions and that is aligned with reference documention:

tls-dh=[curve:]file File containing DH parameters for temporary/ephemeral DH key exchanges, optionally prefixed by a curve for ephemeral ECDH key exchanges. See OpenSSL documentation for details on how to create the DH parameter file. Supported curves for ECDH can be listed using the "openssl ecparam -list_curves" command. WARNING: EDH and EECDH ciphers will be silently disabled if this option is not set.

Instead I'm now asked to cater for some envisioned behavior that does neither work in the current v6 branch (SSL_CTX_set_tmp_dh does not work with any EVP_PKEY structure, whether it contains additional ECDHE parameters or DHE parameters) nor is aligned with the actual reference documentation OR how squid behaved in the past. As far as I understand the openssl documentation with regard to that function, it has also never worked in the past, even if there would have been curves that took additional parameters.

I would argue that it does make much more sense to repair and restore documented and actual behavior exhibited by squid4 and 5 and if that loading of additional ECDHE parameters is an important feature treat it as such and implement it for a future version without breaking compatibility.

Or phrased differently: How many users would gain anything right now from being able to load additional ECDHE parameters for those theoretically defined curves? 0, because nobody uses it at the moment, because it does not work anyway. How many would gain from restoring the documented and actual behavior? At least one 👋, and potentially some more, who may not even know it because for TLS-1.3 or standard TLS-1.2, it just works as long as ECDHE is an option for the client and configured, see above.

Btw: citing doc for openssl ecparam

OpenSSL is currently not able to generate new groups and therefore this command can only create EC parameters from known (named) curves.

I'm now asked to cater for some envisioned behavior that does neither work in the current v6 branch nor is aligned with the actual reference documentation OR how squid behaved in the past.

I agree with many of your statements, but just want to clarify that we are currently in the process of reaching consensus of what should be changed (if anything). Since reviewers are not yet on the same page, I do not recommend making changes until there is consensus about the primary change request (at least).

The only change request were there seems to be consensus right now is replacing OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS with the old 0 value (even there, there is no consensus that zero is best; only potential consensus that polishing that parameter, if any polishing is needed, can be done in a separate PR!).

yadij · 2024-11-19T21:56:47Z

src/security/ServerOptions.cc


    Ssl::ForgetErrors();
    EVP_PKEY *rawPkey = nullptr;
    using DecoderContext = std::unique_ptr<OSSL_DECODER_CTX, HardFun<void, OSSL_DECODER_CTX*, &OSSL_DECODER_CTX_free> >;
-    if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, 0, nullptr, nullptr)}) {
+    if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, nullptr, nullptr)}) {


This parameter was explicitly 0 ("set to 0 to indicate that the code will auto detect the selection") to account for the variance in which details need to be loaded for each type of cipher family being loaded.

Please revert.

yadij · 2024-11-19T22:15:53Z

src/security/ServerOptions.cc

-                // TODO: verify that the loaded parameters match the curve named in eecdhCurve
-


This is still valid. Not all EC curves accept the same parameter values, and some RFC 4492 section 5.1.1 specifies "explicit-prime" curves which have no name but are determined by their parameters.

Not all EC curves accept the same parameter values,

For curves named in tls-dh value, the validity of the above assertion is effectively being decided in another change request. Let's table this change request until that one is resolved.

some RFC 4492 section 5.1.1 specifies "explicit-prime" curves which have no name but are determined by their parameters.

AFAICT, this assertion is not relevant to this TODO because this TODO is specific to cases with a named curve.

AFAIK, at least at the current moment in time, at least openssl does not support the generation of any arbitrary EC parameters. Neither do I see any support for

arbitrary_explicit_prime_curves(0xFF01), arbitrary_explicit_char2_curves(0xFF02),

in openssl ecparam -list_curves (which I know is not the final truth but probably a good indication of how widespread those are in actual use)

yadij · 2024-11-19T22:44:46Z

src/security/ServerOptions.cc

+            debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context (using legacy OpenSSL): " << Ssl::ReportAndForgetErrors);
+        }
+#else
+        const auto tmp = EVP_PKEY_dup(parsedDhParams.get());


Do we really need to consume more memory duplicating the parameters?
They should be reference-locked by OpenSSL and the parsedDhParams smart-Pointer holds our lock for that library-internal counter.

Do we really need to consume more memory duplicating the parameters?

AFAICT, a short answer is "yes". Longer answer: We have to duplicate the key for the SSL_CTX_set0_tmp_dh_pkey() call below because that function transfers ownership (i.e. it does not lock the given pointer): "Ownership of the dhpkey value is passed to the SSL_CTX ... object as a result of this call, and so the caller should not free it if the function call is successful."

They should be reference-locked by OpenSSL

I agree, but that is not what OpenSSL does in this case (and in some other cases) AFAICT from the function documentation. This smells like an OpenSSL API/design problem. PR code uses that problematic API correctly.

Do we really need to worry about a single memory allocation at the time of configuration? Apart from that, its exactly as Alex says, the function takes ownership and demands being passed a copy.

Apart from that, its exactly as Alex says, the function takes ownership and demands being passed a copy.

That transfer of ownership fact is the only thing that should matter for this change request. I will answer the question below in case that answer is useful for future pull requests, but I hope that my answer does not trigger an out-of-scope discussion that is going to keep this particular change request unresolved.

Do we really need to worry about a single memory allocation at the time of configuration?

Yes, we do, but not too much: An extra memory allocation (that does not leak!) is still unwanted because the problematic code may be copied to or mimicked in a place where this overhead actually matters (but where it may be overlooked or even insisted upon using "but we already do it elsewhere!" arguments). An extra allocation may also give code reader a pause (e.g., to answer a "Does this leak?" question), increasing development overheads.

yadij · 2024-11-19T22:46:07Z

src/security/ServerOptions.cc

+            debugs(83, DBG_IMPORTANT, "ERROR: Unable to duplicate DH parameters: " << Ssl::ReportAndForgetErrors);
+            return;
+        }
+        if (SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp) != 1) {


This is only possible when the dhparams file contained DHE parameters. When it contains EC parameters or and explicit-EC algorithm definition this function is expected to fail.

This is only possible when the dhparams file contained DHE parameters. When it contains EC parameters or and explicit-EC algorithm definition this function is expected to fail.

I assume that official Squid code supports parameter files with EC parameters (e.g., the curve name). I agree that we do not want to lose that support. It sounds like we need to add code that determines whether configured parameter file contains EC or DH parameters and then process loaded parameters accordingly. Depending on another change request outcome, that determination may be completely independent from whether any curve was named in tls-dh value!

Let's table this important change request until that other change request is resolved and then come back here.

This is only possible when the dhparams file contained DHE parameters. When it contains EC parameters or and explicit-EC algorithm definition this function is expected to fail.

If it is expected to fail, what use then has the whole "let's support loading additional ECDHE parameters"-support? This seems like a contradiction to your own argument.

@sp-andwei, to avoid that contradiction, I interpret this change request as "please use a different function for EC cases, the one that is not expected to fail in those cases". Again, I recommend waiting for the resolution of the primary change request before making any changes to address this request (or even discussing this request details). I am requesting @yadij review in hope to make progress in that direction.

rousskov

@sp-andwei, I am sorry that you will have to deal with two reviewers that have potentially contradictory views; it is never fun! FWIW, I will do my best to massage these arguments, so that they can be addressed without triggering negative votes from one of the reviewers, but if I fail, then I will step back and wait for you to address Amos' requests.

AFAICT, here is a set of all possible use cases (some of which we may, as the result of this PR, designate as invalid, useless, and/or unsupported):

nil:nil: no explicit curve name; no parameters
nil:PC: no explicit curve name; parameters for an ECDH curve mechanism
nil:PD: no explicit curve name; parameters for a non-curve¹ mechanism
C:nil: explicit curve name X; no parameters
CX:PCX: explicit curve name X; parameters for ECDH curve name X
CX:PCY: explicit curve name X; parameters for ECDH curve name Y
C:PD: explicit curve name X; parameters for a non-curve¹ mechanism

I am using "non-curve" phrase to mark FFDHE and/or "classical DH" mechanisms; the correct terminology is being discussed in a dedicated change request. Please ignore those terminology disagreements for the purpose of this enumeration of use cases. ↩ ↩²

rousskov · 2024-11-20T14:34:50Z

src/security/ServerOptions.cc

-    const auto type = eecdhCurve.isEmpty() ? "DH" : "EC";
+    const auto type = "DH";


Amos: This type parameter determines which of the library interpreters is used to load the file.

Agreed.

PR: It does not make sense to try and load additional parameters when the elliptic curve group is already fully specified by naming it as first part of the tls-dh parameter

Amos: Parameters contained within the file pointed to by tls-dh= contains parameters used by either Elliptic-Curve DH cipher(s) or the classical DH cipher.

This PR effectively asserts that ECDH mechanisms with named curves have no (i.e. cannot have any) additional parameters. Do you dispute that assertion? Developing an agreement regarding this assertion validity is key to handling several other change requests!

Please revert. Any fixes to this logic need to work regardless of this type value.

Reverting these changes will not help with the primary problem this PR is trying to address -- concurrent support for two different mechanism families. Let's table this suggestion for a second and come back to it once the "no additional parameters" claim is validated or rejected.

rousskov · 2024-11-20T14:41:50Z

src/security/ServerOptions.cc


    Ssl::ForgetErrors();
    EVP_PKEY *rawPkey = nullptr;
    using DecoderContext = std::unique_ptr<OSSL_DECODER_CTX, HardFun<void, OSSL_DECODER_CTX*, &OSSL_DECODER_CTX_free> >;
-    if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, 0, nullptr, nullptr)}) {
+    if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, nullptr, nullptr)}) {


This parameter was explicitly 0 ("set to 0 to indicate that the code will auto detect the selection") to account for the variance in which details need to be loaded for each type of cipher family being loaded.

Agreed. AFAICT, OSSL_KEYMGMT_SELECT_ALL_PARAMETERS would have the same effect if relevant "details" are limited to parameters and cannot be keys (because OSSL_KEYMGMT_SELECT_ALL_PARAMETERS excludes OSSL_KEYMGMT_SELECT_PRIVATE_KEY and OSSL_KEYMGMT_SELECT_PUBLIC_KEY bits).

Please revert.

I support reverting this polishing(?) change if it does not hurt known tests. If all other factors are equal, then OSSL_KEYMGMT_SELECT_ALL_PARAMETERS would have been better if it rejects parameter files that contain (irrelevant) keys, but we have bigger issues to solve and do not have to polish this code.

rousskov · 2024-11-20T14:45:47Z

src/security/ServerOptions.cc

-                // TODO: verify that the loaded parameters match the curve named in eecdhCurve
-


Not all EC curves accept the same parameter values,

For curves named in tls-dh value, the validity of the above assertion is effectively being decided in another change request. Let's table this change request until that one is resolved.

some RFC 4492 section 5.1.1 specifies "explicit-prime" curves which have no name but are determined by their parameters.

AFAICT, this assertion is not relevant to this TODO because this TODO is specific to cases with a named curve.

rousskov · 2024-11-20T15:08:46Z

src/security/ServerOptions.cc

+            debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context (using legacy OpenSSL): " << Ssl::ReportAndForgetErrors);
+        }
+#else
+        const auto tmp = EVP_PKEY_dup(parsedDhParams.get());


Do we really need to consume more memory duplicating the parameters?

AFAICT, a short answer is "yes". Longer answer: We have to duplicate the key for the SSL_CTX_set0_tmp_dh_pkey() call below because that function transfers ownership (i.e. it does not lock the given pointer): "Ownership of the dhpkey value is passed to the SSL_CTX ... object as a result of this call, and so the caller should not free it if the function call is successful."

They should be reference-locked by OpenSSL

I agree, but that is not what OpenSSL does in this case (and in some other cases) AFAICT from the function documentation. This smells like an OpenSSL API/design problem. PR code uses that problematic API correctly.

rousskov · 2024-11-20T15:16:09Z

src/security/ServerOptions.cc

+            debugs(83, DBG_IMPORTANT, "ERROR: Unable to duplicate DH parameters: " << Ssl::ReportAndForgetErrors);
+            return;
+        }
+        if (SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp) != 1) {


This is only possible when the dhparams file contained DHE parameters. When it contains EC parameters or and explicit-EC algorithm definition this function is expected to fail.

I assume that official Squid code supports parameter files with EC parameters (e.g., the curve name). I agree that we do not want to lose that support. It sounds like we need to add code that determines whether configured parameter file contains EC or DH parameters and then process loaded parameters accordingly. Depending on another change request outcome, that determination may be completely independent from whether any curve was named in tls-dh value!

Let's table this important change request until that other change request is resolved and then come back here.

rousskov · 2024-11-20T15:19:32Z

src/security/ServerOptions.cc

@@ -390,12 +396,12 @@ Security::ServerOptions::loadDhParams()
    parsedDhParams.resetWithoutLocking(dhp);


PR: Squid https_port ... tls-dh=curve:dhparams.pem configuration is
supposed to support both ECDHE and FFDHE key exchange mechanisms

Amos: This is false. Squid has never supported FFDHE (RFC 7919). Squid implements RFC 4492. That only supports classical DH cipher, or ECDH cipher(s).

I assume this is a disagreement on terminology (i.e. "FFDHE" vs "classical DH") rather than on the primary point of that statement (i.e. "supposed to support both") that this PR is based on. @yadij, please correct me if this assumption is wrong.

FWIW, I am OK with any correct terminology, but I like FFDHE term better because, unlike "classical DH", FFDHE is pretty well defined (RFC 7919 Section 1.2) and because @sp-andwei tests with ffdhe4096 named group appear to demonstrate that at least one FFDHE mechanism is supported by Squid. @yadij, I assume that you are not disputing the correctness of those tests, but please correct me if my assumption is wrong.

As far as I understand, FFDHE and DHE are used interchangably (RFC 7919):.

The terms "DHE" or "FFDHE" are used in this document to refer to the
finite-field-based Diffie-Hellman ephemeral key exchange mechanism in
TLS.

Squid definitely supported DHE in the past via the tls-dh parameter.

As additional comment to your inital statement:

nil:PC

That case never worked and does not currently work in v5/v6/master:

CX:PCX or CX:PCY

That case have never worked in a sense that any additional parameters have been loaded and used and does not currently work in that sense in the v6 or master branches. They do work if a valid name for the curve is specified, because the curve-to-be-used is always set by name, see my long post above:

nil:PD and (missing above) D:PD

(addressed by PR) both broken in v5/v6/master, used to work in (v4); loading of PD successful, but loading it into SSL_CTX ist not

C:PD

addressed by PR, used used to work in v4, with v5/v6/master throws errors and only configures ECDHE successfully

C:nil and nil:nil

those two should work like expected

FYI: Tested my patch with openssl 3.0, squid v6 + patch

tls-dh=<path>/dhparams4096.pem: connecting with openssl s_client defaults: TLS-1.3 with Server Temp Key: X25519, 253 bits

as in this case SSL_CTX_set1_groups is not called and I assume openssl just falls back to some default behavior (using curve X25519 in this case)

DHE works, too, when forcing it using TLS-1.2 and an explicit DHE cihper: openssl s_client -tls1_2 -cipher DHE-RSA-AES256-SHA256 -connect rproxytest.intern01.local:8443 --> Server Temp Key: DH, 4096 bits

tls-dh=ffdhe4096:<path>/dhparams4096.pem

with openssl s_client defaults TLS-1.3 with Server Temp Key: DH, 4096 bits

with openssl s_client -tls1_2 -cipher DHE-RSA-AES256-SHA256 -connect rproxytest.intern01.local:8443 --> Server Temp Key: DH, 4096 bits

forcing ECDHE: openssl s_client -tls1_2 -cipher ECDHE-RSA-AES256-SHA384 -connect rproxytest.intern01.local:8443 --> handshake failure (as would be expected)

C:PD: used used to work in v4/v5

I added that info to the PR description that used a more vague "v4+" term.

CX:PCY: work[s] if a valid name for the curve is specified.

To avoid misunderstanding, this is a hypothetical case where one EC curve name (i.e. X) is specified at the beginning of the tsl-dh parameter value and another EC curve name (i.e. Y) is specified via parameters file named at the end of the tsl-dh parameter value. This case assumes that it is generally possible to specify an EC curve name via parameters file, regardless of how current Squid code handles such parameters file. In other words, the assumption here is that the parameters file syntax and semantics support specifying an EC curve name.

@sp-andwei, with the above in mind, please clarify: Does this particular CX:PCY case work in PR code built with modern OpenSSL, where "work" is defined as "clients insisting on using EC curve X and clients insisting on using EC curve Y can successfully establish connections with the corresponding https_port (i.e. both curves are supported by the same port)"?

My interpretation of the "only possible when the dhparams file contained DHE parameters" assertion is that this case cannot work in PR code built with modern OpenSSL because parameters containing EC curve name cannot be successfully used by SSL_CTX_set0_tmp_dh_pkey() even if OSSL_DECODER_CTX_new_for_pkey("DH") manages to parse such a parameters file successfully despite the apparent DH-is-not-EC conflict.

I added some confusion by bringing squid v5 into that. I got the commit that added the openssl < 3 #if wrong, squid v5 uses basically the same code as squid v6. Sorry about that.

Some more verifying:

openssl ecparam -name brainpoolP256r1 -out secp384r1.pem openssl ecparam -name prime256v1 -out prime256v1.pem openssl ecparam -in secp384r1.pem -text -noout ASN1 OID: secp384r1 NIST CURVE: P-384 openssl ecparam -in prime256v1.pem -text -noout ASN1 OID: prime256v1 NIST CURVE: P-256

CX:PCY, loading of PCY does not work with the PR. Neither did it work with squid v4, v5, v6, master. CX is used and that's it.

tls-dh=prime256v1:<path>/secp384r1.pem

v4: openssl s_client -connect rproxytest.intern01.local:8443 --> Server Temp Key: ECDH, prime256v1, 256 bits

v6: openssl s_client -connect rproxytest.intern01.local:8443 --> Server Temp Key: ECDH, prime256v1, 256 bits

tls-dh=secp384r1:<path>/prime256v1.pem

v4: openssl s_client -connect rproxytest.intern01.local:8443 --> Server Temp Key: ECDH, secp384r1, 384 bits

v6: openssl s_client -connect rproxytest.intern01.local:8443 --> Server Temp Key: ECDH, secp384r1, 384 bits

nil:PCY, that case does not work with the PR. Neither did it work with squid v4, v5, v6, master. openssl defaults are used (currently, X25519 curve for ECDHE ist installed)

tls-dh=/prime256v1.pem`

v4: openssl s_client -connect rproxytest.intern01.local:8443 --> Server Temp Key: X25519, 253 bits

v6: WARNING: Failed to decode DH parameters '/rootdisk/config/prime256v1.pem'; openssl s_client -connect rproxytest.intern01.local:8443-->Server Temp Key: X25519, 253 bits`

The time it took to verify those combinations would have probably been enough to implement some logic to allow specifying a list of groups...

I just wanted to fix that one case relevant for us that has been broken going from v4 to v6. I think I have demonstrated that my quite minimalistic PR does exactly that and does not alter or worsen the behavior in any other way.

squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Nov 19, 2024

sp-andwei force-pushed the fix/load_dhparams_openssl_3.x_master branch from d26f363 to 2041b85 Compare November 19, 2024 14:54

rousskov requested changes Nov 19, 2024

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Nov 19, 2024

rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Nov 19, 2024

rousskov self-requested a review November 19, 2024 15:59

rousskov added 9 commits November 19, 2024 11:12

fixup: Undo out-of-scope formatting change

ba6e0f1

fixup: Avoid Yoda conditions in new code

dc04ed6

fixup: AAA and const-correctness

69be7fe

fixup: More consistent error phrasing and capitalization

f0106c6

"Failed" may be better, but all other ERRORs in this context are using (passable/reasonable) "Unable". Preserve that consistency.

fixup: Avoid identical messages in two error handling paths

6a2de08

This may help reduce triage iterations.

fixup: Do not assume EVP_PKEY_dup() never fails

aa71fdf

fixup: Report current OpenSSL errors _before_ making another OpenSSL …

c382c01

…call

fixup: Decorate longish/nested #if statement end

404d15e

Documented loadDhParams() and removed incorrect/misleading TODO

1972cf5

IIRC, when working on this code earlier (before this branch), I, for one, did not realize that ECDHE curve name is orthogonal to DHE parameters. I incorrectly thought that the curve name is a part of DHE parameters.

rousskov reviewed Nov 19, 2024

View reviewed changes

src/security/ServerOptions.cc Show resolved Hide resolved

rousskov self-requested a review November 19, 2024 19:11

rousskov changed the title ~~Fix loading of dhparams for DHE with Openssl >= 3~~ Fix tls-dh support for DHE parameters with OpenSSL v3+ Nov 19, 2024

squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Nov 19, 2024

rousskov reviewed Nov 19, 2024

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 19, 2024

yadij requested changes Nov 19, 2024

View reviewed changes

rousskov reviewed Nov 20, 2024

View reviewed changes

rousskov requested a review from yadij November 21, 2024 20:26

rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Nov 21, 2024

	if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, nullptr, nullptr)}) {
	if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_ALL_PARAMETERS, nullptr, nullptr)}) {

		const auto type = eecdhCurve.isEmpty() ? "DH" : "EC";
		const auto type = "DH";

		// TODO: verify that the loaded parameters match the curve named in eecdhCurve

		@@ -390,12 +396,12 @@ Security::ServerOptions::loadDhParams()
		parsedDhParams.resetWithoutLocking(dhp);

Fix tls-dh support for DHE parameters with OpenSSL v3+ #1949

Are you sure you want to change the base?

Fix tls-dh support for DHE parameters with OpenSSL v3+ #1949

Conversation

sp-andwei commented Nov 19, 2024 • edited Loading

rousskov left a comment

Choose a reason for hiding this comment

sp-andwei commented Nov 19, 2024

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yadij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp-andwei Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rousskov left a comment • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp-andwei Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp-andwei commented Nov 19, 2024 •

edited

Loading

sp-andwei Nov 20, 2024 •

edited

Loading

rousskov left a comment •

edited

Loading

sp-andwei Nov 20, 2024 •

edited

Loading