Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2041b85
fix loading of dhparams for DHE with Openssl >= 3
Nov 18, 2024
ba6e0f1
fixup: Undo out-of-scope formatting change
rousskov Nov 19, 2024
dc04ed6
fixup: Avoid Yoda conditions in new code
rousskov Nov 19, 2024
69be7fe
fixup: AAA and const-correctness
rousskov Nov 19, 2024
f0106c6
fixup: More consistent error phrasing and capitalization
rousskov Nov 19, 2024
6a2de08
fixup: Avoid identical messages in two error handling paths
rousskov Nov 19, 2024
aa71fdf
fixup: Do not assume EVP_PKEY_dup() never fails
rousskov Nov 19, 2024
c382c01
fixup: Report current OpenSSL errors _before_ making another OpenSSL …
rousskov Nov 19, 2024
404d15e
fixup: Decorate longish/nested #if statement end
rousskov Nov 19, 2024
1972cf5
Documented loadDhParams() and removed incorrect/misleading TODO
rousskov Nov 19, 2024
4657834
fixup: Undo out-of-scope OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS change
rousskov Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions src/security/ServerOptions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,12 @@ Security::ServerOptions::loadClientCaFile()
return bool(clientCaStack);
}

/// Interprets DHE parameters stored in a previously configured dhParamsFile.
/// These DHE parameters are orthogonal to ECDHE curve name that may also be
/// configured when naming that DHE parameters configuration file. When both are
/// configured, the server selects either FFDHE or ECDHE key exchange mechanism
/// (and its cipher suites) depending on client-supported cipher suites.
/// \sa Security::ServerOptions::updateContextEecdh() and RFC 7919 Section 1.2
void
Security::ServerOptions::loadDhParams()
sp-andwei marked this conversation as resolved.
Show resolved Hide resolved
{
Expand Down Expand Up @@ -390,7 +396,7 @@ Security::ServerOptions::loadDhParams()
parsedDhParams.resetWithoutLocking(dhp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sp-andwei sp-andwei Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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
  1. 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
  1. 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.

Copy link
Contributor

@rousskov rousskov Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

Thank you for checking and keeping PR description up to date!

Some more verifying: ...
2. CX:PCY, loading of PCY does not work with PR, ... v4, v5, v6, master. CX is used and that's it.
3. nil:PCY, does not work with PR, ... v4, v5, v6, master. openssl defaults are used

Thank you for testing these. Both outcomes appear to support PR's assertion that parameters file is unusable for configuring EC curves (before and after PR changes).

The speculations below record my understanding of why the above use cases are not (and have not been) supported. You do not have to validate these speculations, but I would appreciate if you can do that (without heroic efforts) for my own edification.

Also, if speculations below are correct, then I wonder whether this PR can avoid specifying parameters key type (i.e. "DH") and let OpenSSL decide what that parameter file contains. Could something like that work and add support for CX:PCY and nil:PCY cases while removing one more hard-coded string/decision from Squid code?? OSSL_DECODER_CTX_new_for_pkey documentation looks encouraging, but there may be some post-call caveats I do not know about:

The search of decoder implementations can also be limited with keytype and selection, which specifies the
expected resulting keytype and contents.  NULL and zero are valid and signify that the decoder
implementations will find out the keytype and key contents on their own from the input they get.

I speculate that official code does not support CX:PCY case because, as already explained in this PR description, official code calls SSL_CTX_set_tmp_dh() with the wrong second parameter object type (EVP_PKEY instead of DH). PR code does not support this use case because it sets parameters file type to "DH" instead of "EC".

I suspect nil:PCY case does not work because all code versions set parameters file type to to "DH" (instead of "EC") in this case.

Copy link
Contributor Author

@sp-andwei sp-andwei Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(without heroic efforts)

touché ;)

official code calls SSL_CTX_set_tmp_dh()

That is the main reason why this never has worked and also does not work in v5/v6 to load any additional ECDHE parameters (which I have yet to actually encounter in the wild). The ECDHE-groups to use were always and are now successfully set by SSL_CTX_set_tmp_ecdh or SSL_CTX_set1_groups earlier in the code. But the parameters for those are solely derived from the name given before the colon, i.e., CX.

TBH, this is a probably a rather minor thing on the whole, but we did use the option to specify a curve and some locally generated dh-groups (using the DHE parameter loading). So that configuration option is "out there" and it would be nice if it worked (especially since loading DH parameters does no longer work at all currently, due to the DH* vs EVP_PKEY issue).

Could something like that work and add support for CX:PCY and nil:PCY cases while removing one more hard-coded string/decision from Squid code??

I guess it could work. But then again, it would probably make more sense to be able to specify a list of group/param pairs and then I still fail to see how you would go about providing any custom EC-params:

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

Copy link
Contributor

@rousskov rousskov Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it could work.

Great, it sounds like we are more-or-less on the same page.

But then again, it would probably make more sense to be able to specify a list of group/param pairs

Yes, perhaps something like tls-dh-groups=<group1>,<group2>,... where each entry uses either ec-curve:<name> or file:paramters.pem format. I was just hoping that we could allow folks that want to use files for all groups to use parameters.pem entries to specify EC curve names (instead of using a special syntax for them).

Any such enhancement is outside this PR scope, of course!

I still fail to see how you would go about providing any custom EC-params

I assume they have no practical value in Squid deployments. If the overall configuration approach is flexible enough, then we can add code to support them if/when there is a real use case (important enough to warrant that extra work). Until then, naming an EC curve using a parameters file could be helpful in some situations (as mentioned above), but even that is not essential.


#else // OpenSSL 3.0+
const auto type = eecdhCurve.isEmpty() ? "DH" : "EC";
const auto type = "DH";
Comment on lines -393 to +399
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sp-andwei sp-andwei Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!).


Ssl::ForgetErrors();
EVP_PKEY *rawPkey = nullptr;
Expand All @@ -413,8 +419,6 @@ Security::ServerOptions::loadDhParams()
if (OSSL_DECODER_from_fp(dctx.get(), in)) {
assert(rawPkey);
const Security::DhePointer pkey(rawPkey);
// TODO: verify that the loaded parameters match the curve named in eecdhCurve

Comment on lines -416 to -417
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (const Ssl::EVP_PKEY_CTX_Pointer pkeyCtx{EVP_PKEY_CTX_new_from_pkey(nullptr, pkey.get(), nullptr)}) {
switch (EVP_PKEY_param_check(pkeyCtx.get())) {
case 1: // success
Expand Down Expand Up @@ -566,9 +570,23 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx)
// set DH parameters into the server context
#if USE_OPENSSL
if (parsedDhParams) {
SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get());
#if OPENSSL_VERSION_MAJOR < 3
if (SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get()) != 1) {
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors);
EVP_PKEY_free(tmp);
}
#endif // OPENSSL_VERSION_MAJOR
}
#endif
#endif // USE_OPENSSL
}

void
Expand Down