Skip to content

Deprecate low level Diffie Hellman functions.#11024

Closed
paulidale wants to merge 3 commits intoopenssl:masterfrom
paulidale:dep-dh
Closed

Deprecate low level Diffie Hellman functions.#11024
paulidale wants to merge 3 commits intoopenssl:masterfrom
paulidale:dep-dh

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Feb 6, 2020

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added the branch: master Applies to master branch label Feb 6, 2020
@paulidale
Copy link
Contributor Author

paulidale commented Feb 6, 2020

This is extremely early. Accepting the suite of deprecated functions is the first step.

  1. The command line commit is a clone of the one in DSA: deprecate the low level functions. #10977 and will be removed once that is merged.
  2. Documentation isn't updated.
  3. The conversions in ssl3_lib.c have problems.
  4. Tests are failing.

@paulidale paulidale force-pushed the dep-dh branch 3 times, most recently from 29e9586 to d6c9e86 Compare February 12, 2020 03:01
@paulidale paulidale changed the title WIP: Deprecate low level Diffie Hellman functions. Deprecate low level Diffie Hellman functions. Feb 12, 2020
@paulidale paulidale added the approval: review pending This pull request needs review by a committer label Feb 12, 2020
@paulidale paulidale marked this pull request as ready for review February 12, 2020 03:18
@paulidale
Copy link
Contributor Author

paulidale commented Feb 12, 2020

Bringing this out of WIP. There are two outstanding functions: DH_new_by_nid and DH_check_params which should be deprecated but are used by TLS with no immediately obvious replacement.

Travis failures do not appear to be relevant.

@levitte
Copy link
Member

levitte commented Feb 12, 2020

Travis failures not relevant here

@levitte
Copy link
Member

levitte commented Feb 12, 2020

The corresponding thing for DH_check_params would be the key validation stuff, right?

@levitte
Copy link
Member

levitte commented Feb 12, 2020

DH_new_by_nid() is a constructor, just like DH_new(), even though very special case. I suggest not to deprecate it now.

levitte
levitte reviewed Feb 12, 2020
View reviewed changes
@paulidale
Copy link
Contributor Author

paulidale commented Feb 12, 2020 •
edited
Loading

Yes, DH_check_params would be some kind of key validation. I tried creating an EVP_PKEY_CTX and calling some of the check functions but none worked.

Happy to leave DH_new_by_nid non-deprecated (it isn't currently). I guess NIDs will work for a while yet. There will need to be some refactoring to avoid NIDs internally.

@levitte
Copy link
Member

levitte commented Feb 12, 2020

There will need to be some refactoring to avoid NIDs internally.

Yes, agreed. I guess that we can work on that when EC stuff has transferred over to providers

@levitte
Copy link
Member

levitte commented Feb 16, 2020

Yes, DH_check_params would be some kind of key validation. I tried creating an EVP_PKEY_CTX and calling some of the check functions but none worked.

That's because there currently is no validation code added in the KEYMGMT implementations for DSA and DH.

@levitte
Copy link
Member

levitte commented Feb 16, 2020

Travis and Appveyor are a bit unhappy...

levitte
levitte requested changes Feb 16, 2020
View reviewed changes
@paulidale paulidale force-pushed the dep-dh branch 3 times, most recently from d79a252 to 19ec26a Compare February 16, 2020 07:31
@paulidale
Copy link
Contributor Author

paulidale commented Feb 16, 2020

That's because there currently is no validation code added in the KEYMGMT implementations for DSA and DH.

As I remembered after getting stuck into it.

@paulidale paulidale force-pushed the dep-dh branch 2 times, most recently from 5b4b4c8 to a028270 Compare February 18, 2020 12:44
@paulidale
Copy link
Contributor Author

paulidale commented Feb 18, 2020

Travis isn't relevant, ping for review.

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 19, 2020
@paulidale
DH: add CHANGES entry listing the deprecated DH functions.
16265e2
@paulidale
Deprecate the low level Diffie-Hellman functions.
8ec7654 
Use of the low level DH functions has been informally discouraged for a
long time.  We now formally deprecate them.
@paulidale
Copy link
Contributor Author

paulidale commented Feb 20, 2020

Rebased and conflicts merged -- no code changes just infrastructure.
I'll wait for the CIs before merging.

@paulidale
Copy link
Contributor Author

paulidale commented Feb 20, 2020

Merged to master. Thanks for the feedback and assistance.

@paulidale paulidale closed this Feb 20, 2020
openssl-machine pushed a commit that referenced this pull request Feb 20, 2020
@paulidale
DH: fix header file indentation
2c64df6 
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #11024)
openssl-machine pushed a commit that referenced this pull request Feb 20, 2020
@paulidale
DH: add CHANGES entry listing the deprecated DH functions.
0ad05b1 
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #11024)
openssl-machine pushed a commit that referenced this pull request Feb 20, 2020
@paulidale
Deprecate the low level Diffie-Hellman functions.
ada66e7 
Use of the low level DH functions has been informally discouraged for a
long time.  We now formally deprecate them.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #11024)
@paulidale paulidale deleted the dep-dh branch February 20, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@levitte levitte levitte approved these changes

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@paulidale @levitte