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.

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

Deprecate DH harder #13138

Closed
wants to merge 12 commits into from
Closed

Conversation

mattcaswell
Copy link
Member

This is very much an early sight WIP PR. It depends on #13136 and #13135 - those commits are included here.

Still to be done:

  • There's still more of DH to deprecate, although I think I've solved most problem areas
  • make update is doing some strange stuff. Needs investigation
  • I had to deprecate some libssl functions/macros because they pass around a DH object. I need to implement EVP friendly replacements
  • Better tests of dhparam

@mattcaswell mattcaswell added the branch: master Merge to master branch label Oct 14, 2020
@mattcaswell mattcaswell added this to In progress in 3.0 New Core + FIPS via automation Oct 14, 2020
ssl/statem/statem_clnt.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Oct 15, 2020

Do we actually need non-deprecated replacement for SSL_set_tmp_dh now that the libssl should automatically choose right safe prime from the known good ones? I propose to not add such replacement.

@mattcaswell
Copy link
Member Author

Do we actually need non-deprecated replacement for SSL_set_tmp_dh now that the libssl should automatically choose right safe prime from the known good ones? I propose to not add such replacement.

I believe there are many people who want to use their specific DH params, and would be broken by removing that capability.

@t8m
Copy link
Member

t8m commented Oct 15, 2020

The reason they used specific DH params was mostly that the OpenSSL itself historically did not provide reasonable ones built in. But OK I suppose OpenSSL should still allow users to shoot themselves into their feet.

include/openssl/dh.h Outdated Show resolved Hide resolved
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Do .pod files need updating also?

@mattcaswell
Copy link
Member Author

Do .pod files need updating also?

Yes.

@slontis
Copy link
Member

slontis commented Oct 16, 2020

No problems.. I am mainly asking because I am doing the EC deprecation currently - just making sure that was not deliberate.

@mattcaswell mattcaswell changed the title [WIP, Pending on #13136, #13135]: Deprecate DH harder [WIP]: Deprecate DH harder Oct 16, 2020
@mattcaswell
Copy link
Member Author

I've rebased this to pick up #13136 and #13135 which have now been merged, so this PR is no longer dependent on them.

There's also been numerous updates, including deprecating the remaining DH functions and a number of fixes that resulted. Fixed the "make update" strangeness.

Still to do: documentation updates; more testing

@kroeckx
Copy link
Member

kroeckx commented Oct 16, 2020

So I'm confused what you think "no-dh" means. To me it means we have no support for DH at all, and that the EVP functions return an error in case you try to use them with DH.

@mattcaswell
Copy link
Member Author

So I'm confused what you think "no-dh" means.

It means there is no low-level DH implementation. It so happens that if you have no DH implementation then the EVP functions will return an error. Unless you happen to plug in a third party provider that does have DH support, in which case it will start to work.

@kroeckx
Copy link
Member

kroeckx commented Oct 16, 2020 via email

@kroeckx
Copy link
Member

kroeckx commented Oct 16, 2020

This PR no longer support reading parameters from stdin, master still support this.

include/openssl/dh.h Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Oct 19, 2020

So that means that the SSL library always has DHE support, but just fails to negiotate a cipher using it? Or does it detect that DH is not provided, and then not announce it supports it/not select that as cipher?

It should be the latter - it should properly detect whether DH is provided or not and not announce/negotiate it if it isn't. Anything else would be a bug.

@mattcaswell
Copy link
Member Author

So that means that the SSL library always has DHE support, but
just fails to negiotate a cipher using it? Or does it detect that
DH is not provided, and then not announce it supports it/not
select that as cipher?

Most algorithms (e.g. symmetric ciphers and hashes), it detects up front and does not offer ciphersuites based on that if there is no support. It also detects key exchange algorithms up front and does not offer groups in the supported_groups extension that it cannot support (including DH groups). However, checking the code, it looks to me like it doesn't actually suppress DH ciphersuites if it doesn't have DH support. It should do. I'll raise an issue for that.

@mattcaswell
Copy link
Member Author

#13184

@mattcaswell
Copy link
Member Author

This PR no longer support reading parameters from stdin, master still support this.

Hmm. I just tried this on master and got this output:

$ cat dh.pem | openssl dhparam -noout -text
Error, unable to load DH parameters
80C24B4C7A7F0000:error::STORE routines:ossl_store_get0_loader_int:unregistered scheme:crypto/store/store_register.c:240:scheme=file
80C24B4C7A7F0000:error::DECODER routines:decoder_process:BIO lib:crypto/encode_decode/decoder_lib.c:511:

Trying the same thing on 1.1.1 works fine. So it looks to me like this is broken even in master.

@kroeckx
Copy link
Member

kroeckx commented Oct 19, 2020 via email

@mattcaswell
Copy link
Member Author

The difference between this PR and master, is that on master it
still waits for the data while this PR just directly returns an
error.

It still seems to fail at the same line in the OSSL_DECODER code though. I've raised an issue (#13185).

openssl-machine pushed a commit that referenced this pull request Nov 27, 2020
d2i_DHparams and i2d_DHparam as well as the equivalent DHX functions are
deprecated.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13138)
openssl-machine pushed a commit that referenced this pull request Nov 27, 2020
EVP_PKEY_set1_DH is deprecated so there is no need to test it in a
no-deprecated build.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13138)
openssl-machine pushed a commit that referenced this pull request Nov 27, 2020
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13138)
openssl-machine pushed a commit that referenced this pull request Nov 27, 2020
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13138)
openssl-machine pushed a commit that referenced this pull request Nov 27, 2020
Extend the existing CHANGES.md entry with information about the
additional functions that have also been deprecated.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13138)
openssl-machine pushed a commit that referenced this pull request Nov 27, 2020
d2i_RSAPrivateKey.pod is the more generic page for these deprecated
functions and provides advice and guidance on how to translate the old
style functions into new ones.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #13138)
@mattcaswell
Copy link
Member Author

Pushed. Thanks.

3.0 New Core + FIPS automation moved this from Reviewer approved to Done Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants