Deprecate SRP #14132

mattcaswell · 2021-02-08T16:02:25Z

The OTC recently decided to deprecate the SRP APIs. This PR implements that deprecation.

Unfortunately, at this time, SRP is not available via EVP so there is no replacement available.

The OTC decided that all low level APIs should be deprecated. This extends to SRP, even though at the current time there is no "EVP" interface to it. This could be added in a future release.

The low level SRP implementation has been deprecated with no replacement. Therefore the libssl level APIs need to be similarly deprecated.

Ensure all the man pages correctly reflect the deprecated status of SRP. Fixes openssl#13917

mattcaswell · 2021-02-08T16:06:54Z

Note:

Since 1.1.1 some work has occurred on the SRP APIs to make them very slightly less bad than they already were which involved the introduction of some new functions. There's also a handful of functions that gained "_ex" versions in order to take a libctx and propq. These are currently necessary for the tests to pass. Removing them would be a non-trivial piece of work as it would require some restructuring of the tests - and they do actually add value for anyone who still wants to use SRP anyway (bearing in mind that this probably will happen because we're not introducing a replacement). For that reason I've chosen not to remove the newly added functions in this PR, even though they are immediately being deprecated.

mattcaswell · 2021-02-08T17:11:52Z

Fixup pushed addressing CI failure

mattcaswell · 2021-02-09T11:31:38Z

Ping? All CIs have passed.

richsalz · 2021-02-09T13:24:59Z

Could you please ask the community if this should be done? Start with the TLS working group at the IETF for example.

Or use a separate define so that SRP is still available even if someone wants a 'clean' no-deprecated build.

t8m · 2021-02-09T15:13:38Z

Or use a separate define so that SRP is still available even if someone wants a 'clean' no-deprecated build.

That would make no sense as the no-deprecated build is not something that is targeted for production. The no-deprecated build is there to check whether you still do not use something that will be removed (at some point) later. The SRP API as is is not provider based and so it has to be removed at some point. There might be a replacement added but not in 3.0.

richsalz · 2021-02-09T15:18:46Z

That would make no sense as the no-deprecated build is not something that is targeted for production.

Oh really? That should be written down in INSTALLmd if so because it is most definitely not even implied there right now. And that brings up the obvious next question: then what is the purpose of no-deprecated ? That should also be documented.

I understand that 3.0 is really late, but tossing things out because the project couldn't meet its goals seem wrong.

crypto/srp/srp_lib.c

crypto/srp/srp_vfy.c

include/openssl/srp.h.in

t8m · 2021-02-09T15:30:17Z

include/openssl/srp.h.in

@@ -39,6 +39,8 @@ use OpenSSL::stackhash qw(generate_stack_macros);
 extern "C" {
 # endif

+# ifndef OPENSSL_NO_DEPRECATED_3_0


Or actually when we have the disable cascade to OPENSSL_NO_SRP, perhaps we do not actually need this ifndef?

I think we need it to support "Configure --api=1.1.1 no-deprecated"

t8m · 2021-02-09T15:37:12Z

That would make no sense as the no-deprecated build is not something that is targeted for production.

Oh really? That should be written down in INSTALLmd if so because it is most definitely not even implied there right now. And that brings up the obvious next question: then what is the purpose of no-deprecated ? That should also be documented.

Maybe I used a little bit strong and incorrect words. You certainly can use no-deprecated build in production, except you cannot expect the deprecated APIs to be available. On the other hand there is IMO no obligation of the project to support all the existing features of OpenSSL 1.1.1 in no-deprecated build. Marking an API to be deprecated is not making it removed and on the other hand not marking it deprecated basically means it has to be still supported in future 4.0 version which would be really bad.

richsalz · 2021-02-09T16:36:43Z

Well of course if you say no-deprecated you can't expect deprecated API's to be available :)

I think it is a big mistake to deprecate SRP when there is no replacement; it sends the signal that it's going away,not that an EVP replacement is coming. And yes, I understand the support impact. That is unfortunate. Maybe get some of the other people doing non-release stuff to work on it?

And to be clear, I am no fan of SRP. But deprecating it because the project isn't going to be able to get the EVP work done for 3.0 is not right.

t8m · 2021-02-09T16:48:17Z

No, the notion that the SRP apis are deprecated means just that the existing deprecated SRP apis are going away in some future release. It does not mean anything else (like that SRP as a RFC or anything is deprecated). And it would be silly if we tried to accommodate for everyone's skewed meaning of deprecation.

richsalz · 2021-02-09T16:50:46Z

Do you have another example of where something is deprecated that

Doesn't have a preferred replacement (e.g., AES_encrypt); or
Isn't really going away (e.g., BN_pseudo_rand)

I claim my understanding is not a skewed understanding of how the project deprecates things.

t8m · 2021-02-09T16:57:46Z

There are certainly some (albeit minor) low-level stuff you can do with for example the RSA/DSA/EC_KEY APIs that won't be possible with the EVP_PKEY anymore. Yes, nothing is on the level of whole API set, but that does not make my point of not automatically inferring general obsolescence of a functionality from the fact that API for that functionality is deprecated wrong.

richsalz · 2021-02-09T17:02:23Z

but that does not make my point of not automatically inferring general obsolescence of a functionality from the fact that API for that functionality is deprecated wrong.

On the contrary, it does show that you are wrong. There is no plan for those low-level things to come back. And yet, here the expectation is that the SRP algorithm will come back. I think your understanding of how openssl does deprecation is skewed (to borrow the word). You can't use a term in one way 99% of the time and expect users to follow the change in meaning for that last 1%.

mattcaswell · 2021-02-10T15:09:32Z

And yet, here the expectation is that the SRP algorithm will come back. I think your understanding of how openssl does deprecation is skewed (to borrow the word)

We are deprecating the low level APIs (not the algorithm) in accordance with a decision by the OTC. This much is in alignment with all of the other low level APIs we have deprecated recently. What is different about this time is that there isn't an EVP layer replacement. I think that can be seen as a bug or deficiency - we should have one, but no such implementation exists. Implementing it is a significant piece of work (not least because it is not clear how SRP would fit into any of the existing EVP operations).

Given all of the above OTC was faced with a dilemma, and there were 3 choices:

Do nothing
Deprecate the API and defer an EVP implementation until some later time
Deprecate the API and spend the time providing an EVP implementation

The OTC view was that we do not want the low level API to persist any longer that we have to. Frankly, IMO, the SRP API is the worst API ever in the history of all APIs that ever existed. It is not fit for purpose. With that in mind deprecating it seems the right thing to do. However, as you well know, we need to get 3.0 out and you yourself have been pushing us hard to make difficult choices. This is one that we made. We don't think that delaying 3.0 while we go off and write an SRP EVP implementation is a good trade off.

That said if someone external to the core dev resources were to contribute a high quality SRP EVP implementation in the 3.0 timescale (i.e. before beta1) then I personally see no reason why we wouldn't take it (although that would be an OTC decision).

richsalz · 2021-02-10T15:11:48Z

I think the OTC should have gone with 2, but I'm just a bystander.

mattcaswell · 2021-02-10T15:12:51Z

I think the OTC should have gone with 2, but I'm just a bystander.

Is that what you meant to say? We did go with 2, i.e. deprecate the APIs now, and defer the SRP EVP implementation until later. Did you mean 3?

mattcaswell · 2021-02-10T15:13:52Z

Fixup pushed addressing feedback. Please take another look.

richsalz · 2021-02-10T15:17:05Z

Thanks matt. I meant "do nothing" Perhaps you could mitigate things by putting some of the rationale for why SRP is different from all other deprecations (without saying mean things about it) into the CHANGES file:

The SRP API is also deprecated. Unlike other deprecations it has no EVP replacement. The project wanted to remove all
low-level crypto API's, even though the replacement will have to appear in a future release.

And saying the SRP is the worst OpenSSL API ever... :)

levitte · 2021-02-10T15:18:57Z

And saying the SRP is the worst OpenSSL API ever... :)

Er, I'd say there are strong contenders, and I'm not at all sure I view the SRP API as the worst...

mattcaswell · 2021-02-10T15:20:30Z

Er, I'd say there are strong contenders, and I'm not at all sure I view the SRP API as the worst...

You've clearly never used it :-)

mattcaswell · 2021-02-10T15:23:09Z

Isn't that more-or-less what I already said in the CHANGES file although not quite in the same words:

The SRP APIs have been deprecated. The old APIs do not work via providers,
and there is no EVP interface to them. Unfortunately there is no replacement
for these APIs at this time.

richsalz · 2021-02-10T15:31:33Z

The difference is that I think the project should commit to doing the work .

levitte · 2021-02-10T16:06:40Z

You've clearly never used it :-)

I raise you ASN1_item_verify_ctx() 😉

t8m

LGTM

openssl-machine · 2021-02-11T19:00:15Z

This pull request is ready to merge

mattcaswell · 2021-02-12T08:59:45Z

Pushed. I resolved the trivial CHANGES.md conflict while merging.

The OTC decided that all low level APIs should be deprecated. This extends to SRP, even though at the current time there is no "EVP" interface to it. This could be added in a future release. Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #14132)

The low level SRP implementation has been deprecated with no replacement. Therefore the libssl level APIs need to be similarly deprecated. Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #14132)

Ensure all the man pages correctly reflect the deprecated status of SRP. Fixes #13917 Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #14132)

mattcaswell added 3 commits February 8, 2021 15:53

Deprecate the low level SRP APIs

257c2f9

The OTC decided that all low level APIs should be deprecated. This extends to SRP, even though at the current time there is no "EVP" interface to it. This could be added in a future release.

Deprecate the libssl level SRP APIs

75ff0df

The low level SRP implementation has been deprecated with no replacement. Therefore the libssl level APIs need to be similarly deprecated.

Update documentation following deprecation of SRP

bffbb89

Ensure all the man pages correctly reflect the deprecated status of SRP. Fixes openssl#13917

mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Feb 8, 2021

mattcaswell added this to the 3.0.0 beta1 milestone Feb 8, 2021

mattcaswell self-assigned this Feb 8, 2021

mattcaswell modified the milestones: 3.0.0 beta1, Sprint Hydrogen Feb 8, 2021

mattcaswell closed this Feb 8, 2021

mattcaswell reopened this Feb 8, 2021

mattcaswell mentioned this pull request Feb 8, 2021

SRP is not EVP enabled or provider aware or deprecated #13917

Closed

fixup! Deprecate the libssl level SRP APIs

5b4f8de

t8m reviewed Feb 9, 2021

View reviewed changes

fixup! Deprecate the low level SRP APIs

76e43b6

fixup! Deprecate the libssl level SRP APIs

984822a

t8m approved these changes Feb 10, 2021

View reviewed changes

t8m 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, 2021

openssl-machine removed the approval: done This pull request has the required number of approvals label Feb 11, 2021

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Feb 11, 2021

mattcaswell closed this Feb 12, 2021

openssl-machine pushed a commit that referenced this pull request Feb 12, 2021

Update documentation following deprecation of SRP

13888e7

Ensure all the man pages correctly reflect the deprecated status of SRP. Fixes #13917 Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #14132)

richsalz mentioned this pull request Feb 12, 2021

SRP needs an EVP API #14167

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate SRP #14132

Deprecate SRP #14132

mattcaswell commented Feb 8, 2021

mattcaswell commented Feb 8, 2021

mattcaswell commented Feb 8, 2021

mattcaswell commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m Feb 9, 2021

mattcaswell Feb 10, 2021

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

mattcaswell commented Feb 10, 2021

richsalz commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

richsalz commented Feb 10, 2021

levitte commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

richsalz commented Feb 10, 2021

levitte commented Feb 10, 2021

t8m left a comment

openssl-machine commented Feb 11, 2021

mattcaswell commented Feb 12, 2021

Deprecate SRP #14132

Deprecate SRP #14132

Conversation

mattcaswell commented Feb 8, 2021

mattcaswell commented Feb 8, 2021

mattcaswell commented Feb 8, 2021

mattcaswell commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m Feb 9, 2021

Choose a reason for hiding this comment

mattcaswell Feb 10, 2021

Choose a reason for hiding this comment

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

t8m commented Feb 9, 2021

richsalz commented Feb 9, 2021

mattcaswell commented Feb 10, 2021

richsalz commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

richsalz commented Feb 10, 2021

levitte commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

mattcaswell commented Feb 10, 2021

richsalz commented Feb 10, 2021

levitte commented Feb 10, 2021

t8m left a comment

Choose a reason for hiding this comment

openssl-machine commented Feb 11, 2021

mattcaswell commented Feb 12, 2021