DSA: deprecate the low level functions. by paulidale · Pull Request #10977 · openssl/openssl

paulidale · 2020-01-31T03:34:40Z

documentation is added or updated
tests are added or updated

paulidale · 2020-01-31T03:36:51Z

Still WIP: evp_extra_test needs to be updated to not use the low level calls.

slontis · 2020-01-31T03:41:14Z

I foresee some merge problems for me.

CHANGES

include/openssl/dsa.h

levitte

Poor @paulidale... between @mattcaswell and me, life can't be easy 😉

include/openssl/dsa.h

paulidale · 2020-02-03T03:16:37Z

Poor @paulidale...

This is the least of my problems...

paulidale · 2020-02-03T09:02:22Z

Open questions are:

DSA_SIG related function -- deprecate or not?
ASN.1 functions -- deprecate or not?
printing functions -- deprecate or not?
DSA_size, DSA_bits, DSA_security_bits functions -- deprecate or not?

levitte · 2020-02-03T09:52:36Z

The general viewpoint that at least @mattcaswell and I share nowadays is that the key creation functions should not be deprecated, but the rest goes. In other words, it should remain possible to DSA_new(), then a bit of DSA_set0_pqg() and DSA_set0_key(), and finish it off with EVP_PKEY_assign_DSA(), and anything else should become a matter of using functions that accept a EVP_PKEY.

That's at least how I understood things. But it dawns on me now that @mattcaswell talked about deprecating the functions that do calculations using the keys (encryption, decryption, ...), so it seems like he has a more conservative viewpoint than me, after all.

But on direct questions:

DSA_SIG functions need to stay for the moment, for the simple reason that we don't have a replacement. We should, however, have a think of making a signature not only a provided method, but also a first class object on the provider level, allowing us to pick out its pieces with OSSL_PARAM arrays just like any other provider object.
ASN.1 encoding/decoding functions should essentially become increasingly deprecated, as we add serializers and deserializers. We already have the serializers in place, but not the deserializers, and I think it's fine to wait until they are implemented.
I would deprecate the printing functions without any hesitation, especially since they are just wrappers around the EVP_PKEY printing functions.

mattcaswell · 2020-02-03T10:05:45Z

IMO:

DSA_SIG functions should stay.
ASN1 encoding/decoding should stay
I'm ok with deprecating the printing functions
size/bits/security_bits seems like a bit of a grey area to me. Is it more like getting parameters "p/q/g" which we allow, or is it more like a "crypto" operation. On reflection, I suppose there may actually be some "calculation" involved to return these values - so I'm ok to deprecate.

paulidale · 2020-02-04T02:02:50Z

Thanks for the prompt feedback. Okay, DSA_SIG and ASN.1 are staying. The printing and bits/size functions are deprecated. Clarity is achieved :)

Travis is relevant...

paulidale · 2020-02-04T04:46:19Z

Travis is fixed.

Time to open the review floodgates ...

apps/dhparam.c

paulidale · 2020-02-05T01:34:24Z

Rebased to avoid conflicts.
Added deprecation warnings when command line apps are started.

levitte

In a no-deprecated configuration, we shouldn't even build the dsa (and dh, since you're already on a path to include that in this deprecation) related apps source. The formula for this in build.info is:

IF[{- !$disabled{deprecated} || $config{api} < 30000 -}]
  SOURCE[openssl]=dsa.c dsaparam.c
ENDIF

apps/progs.pl

levitte · 2020-02-05T08:36:54Z

apps/progs.pl

Er, did you actually mean to have that in this PR?

Yes, I intend to push this as a separate commit.
I am willing to make it a separate PR if required, but it's easier here.

[Reopening this conversation after seeing comments from Matt on the subject.]

It turns out that these deprecations are premature.

apps/progs.pl

paulidale · 2020-02-05T09:44:06Z

In a no-deprecated configuration, we shouldn't even build the dsa (and dh, since you're already on a path to include that in this deprecation) related apps source.

This cascades to test failures. I tried :)

paulidale · 2020-02-05T09:45:07Z

The partial dh deprecation is here only because dh relies on dsa for parameter generation. Fully deprecating dh is a lot more effort which I've got underway.

levitte · 2020-02-05T10:23:11Z

This cascades to test failures. I tried :)

Yup, there are a lot of tests that need a similar check. #10797 had a lot of failures related to exactly this, but from a different viewpoint.

mattcaswell

LGTM

Use of the low level DSA functions has been informally discouraged for a long time. We now formally deprecate them.

Do not run programs that depend on deprecated APIs when 'no-deprecated' is configured. We still retain the conversion tests that use 'openssl pkey', and add the one that's missing.

Use 'openssl genpkey' instead.

speed is updated to not support DSA instead of being removed. The dhparam, dsaparam, dsa and gendsa commands are deprecated but still exist without NO_DEPRECATED defined.

petrovr · 2020-02-11T19:48:53Z

Richard Levitte wrote:

> Common --api=3.0.0 no-deprecated effectively removes today ! I will say this again: those are not default configuration options, *and they are not recommended for general use and cannot be considered "normal" use*. The intention with `no-deprecated` is to check if your application can build with all deprecated symbols removed, for those who really want to be that much on the bleeding edge. If that's not your application, *then do not use `no-deprecated`*, it's as simple as that.

And the error is in design - some symbols are marked depreciated without alternative. And this is my point - to mark as deprecated in 5.0 when is expected to exist valuable alternative. Regards Roumen Petrov

levitte · 2020-02-11T20:40:02Z

And this is my point - to mark as deprecated in 5.0 when is expected to exist valuable alternative.

Huh? Of course, it depends on what you mean with "valuable", but the EVP API with provider backing is the alternative.
(as a matter of fact, we have been pushing for EVP use for a long time, so even without the providers, it would have been perfectly sensible to deprecate the low-level functions)

mattcaswell · 2020-02-11T22:35:51Z

And the error is in design - some symbols are marked depreciated without
alternative.

If you believe that then I'd like to understand what symbols you think they are. AFAIK all symbols that we are deprecating have an alternative.

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

Use of the low level DSA functions has been informally discouraged for a long time. We now formally deprecate them. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

Do not run programs that depend on deprecated APIs when 'no-deprecated' is configured. We still retain the conversion tests that use 'openssl pkey', and add the one that's missing. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #10977)

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

Use 'openssl genpkey' instead. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #10977)

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

speed is updated to not support DSA instead of being removed. The dhparam, dsaparam, dsa and gendsa commands are deprecated but still exist without NO_DEPRECATED defined. Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

paulidale · 2020-02-11T22:55:49Z

Merged to master. Thanks for the reviews and feedback.

paulidale added the branch: master Applies to master branch label Jan 31, 2020

mattcaswell requested changes Jan 31, 2020

View reviewed changes

CHANGES Outdated Show resolved Hide resolved

include/openssl/dsa.h Outdated Show resolved Hide resolved

include/openssl/dsa.h Outdated Show resolved Hide resolved

include/openssl/dsa.h Outdated Show resolved Hide resolved

paulidale force-pushed the dep-dsa branch from 50b921f to 9250ed2 Compare February 1, 2020 03:02

levitte requested changes Feb 2, 2020

View reviewed changes

include/openssl/dsa.h Outdated Show resolved Hide resolved

include/openssl/dsa.h Outdated Show resolved Hide resolved

include/openssl/dsa.h Outdated Show resolved Hide resolved

paulidale force-pushed the dep-dsa branch 2 times, most recently from 4555edd to 9427410 Compare February 3, 2020 02:53

paulidale force-pushed the dep-dsa branch 3 times, most recently from 2a847ad to 946cda9 Compare February 3, 2020 04:52

paulidale force-pushed the dep-dsa branch 3 times, most recently from 9647406 to ce3c254 Compare February 4, 2020 00:46

paulidale force-pushed the dep-dsa branch from ce3c254 to 367012a Compare February 4, 2020 02:40

paulidale marked this pull request as ready for review February 4, 2020 04:46

levitte reviewed Feb 4, 2020

View reviewed changes

apps/dhparam.c Outdated Show resolved Hide resolved

paulidale force-pushed the dep-dsa branch from ba75f40 to 444fa07 Compare February 5, 2020 01:28

paulidale force-pushed the dep-dsa branch from 444fa07 to 5b86c9a Compare February 5, 2020 02:00

levitte requested changes Feb 5, 2020

View reviewed changes

paulidale force-pushed the dep-dsa branch from 1a64978 to 995b085 Compare February 10, 2020 22:50

mattcaswell approved these changes Feb 10, 2020

View reviewed changes

mattcaswell 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 10, 2020

paulidale and others added 7 commits February 11, 2020 09:07

Deprecate the low level DSA functions.

2ae87f8

Use of the low level DSA functions has been informally discouraged for a long time. We now formally deprecate them.

test/recipes/15-test_dsa.t: Deal with deprecation of 'openssl dsa'

8784bfe

Do not run programs that depend on deprecated APIs when 'no-deprecated' is configured. We still retain the conversion tests that use 'openssl pkey', and add the one that's missing.

test_dsa: fix deprecation logic

644962c

test/recipes/80-test_ssl_old.t: Replace 'openssl gendsa'

eac8e63

Use 'openssl genpkey' instead.

app: add a deprecation warning to all deprecated commands.

ea03109

dsa: deprecate applications that depend on the low level DSA functions.

d05ce6e

speed is updated to not support DSA instead of being removed. The dhparam, dsaparam, dsa and gendsa commands are deprecated but still exist without NO_DEPRECATED defined.

Add NEWS entry about deprecation of command line public tools

437be6c

paulidale force-pushed the dep-dsa branch from 995b085 to 437be6c Compare February 10, 2020 23:07

openssl-machine pushed a commit that referenced this pull request Feb 11, 2020

DSA: fix the DSA parameter logic in test.

c5d9414

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

openssl-machine pushed a commit that referenced this pull request Feb 11, 2020

dsa.h: fix preprocessor indentation

7f6deaf

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

openssl-machine pushed a commit that referenced this pull request Feb 11, 2020

test_dsa: fix deprecation logic

57ea7a7

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

openssl-machine pushed a commit that referenced this pull request Feb 11, 2020

test/recipes/80-test_ssl_old.t: Replace 'openssl gendsa'

343f0e4

Use 'openssl genpkey' instead. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #10977)

openssl-machine pushed a commit that referenced this pull request Feb 11, 2020

app: add a deprecation warning to all deprecated commands.

c2ec4a1

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

openssl-machine pushed a commit that referenced this pull request Feb 11, 2020

Add NEWS entry about deprecation of command line public tools

663247b

Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #10977)

paulidale closed this Feb 11, 2020

paulidale deleted the dep-dsa branch February 11, 2020 22:55

paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Feb 11, 2020

tomiii00 mentioned this pull request Feb 26, 2021

How to free the result of EVP_PKEY_get1_DSA() ? #14323

Closed

Uh oh!

Conversation

paulidale commented Jan 31, 2020

Uh oh!

paulidale commented Jan 31, 2020

Uh oh!

slontis commented Jan 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulidale commented Feb 3, 2020

Uh oh!

paulidale commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Feb 3, 2020

Uh oh!

mattcaswell commented Feb 3, 2020

Uh oh!

paulidale commented Feb 4, 2020

Uh oh!

paulidale commented Feb 4, 2020

Uh oh!

Uh oh!

paulidale commented Feb 5, 2020

Uh oh!

levitte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

levitte Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

paulidale Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

levitte Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulidale commented Feb 5, 2020

Uh oh!

paulidale commented Feb 5, 2020

Uh oh!

levitte commented Feb 5, 2020

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

petrovr commented Feb 11, 2020 via email

Uh oh!

levitte commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Feb 11, 2020

Uh oh!

paulidale commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

paulidale commented Feb 3, 2020 •

edited

Loading

levitte left a comment •

edited

Loading

levitte commented Feb 11, 2020 •

edited

Loading