Remove RSA padding SSLv23#14248
Remove RSA padding SSLv23#14248richsalz wants to merge 2 commits intoopenssl:masterfrom richsalz:deprecate-rsa-padding
Conversation
include/openssl/core_names.h
Outdated
There was a problem hiding this comment.
Maybe we should just drop this define? So we do not add immediately deprecated defines in 3.0?
There was a problem hiding this comment.
I agree adding something and deprecating it seems silly, but it's used in two files so have to keep it I think.
There was a problem hiding this comment.
Could it be defined in some internal header? @levitte any opinions?
I'd just drop it from the document and keep it only as an internal implementation detail that there is another possible value for the pad mode. It is really useless and I would not expect any third party provider to implement this. |
|
Once again I can't seem to get no-deprecated correct. help please. |
crypto/rsa/rsa_ossl.c
Outdated
There was a problem hiding this comment.
This change has no effect because you have the #include "internal/deprecated.h" above. Unfortunately that means the rsa_ssl.c cannot be among the !$disabled{'deprecated-3.0'} files.
There was a problem hiding this comment.
This is the cause for the no-deprecated build failure.
There was a problem hiding this comment.
rsa_ossl needs to use deprecated stuff and rsa_ssl implements deprecated stuff. So it seems that using the standard deprecation flags will not work. Does anyone have suggestions on how to do this deprecation? One possibility I am about to try is to add a shim file that doesn't include internal/deprecated.h Let's see what happens.
There was a problem hiding this comment.
No, you're misunderstanding. The fix is just to drop the move of rsa_ssl.c into the !$disabled{'deprecated-3.0'} block. Just drop the build.info change.
There was a problem hiding this comment.
Just drop the build.info change.
well heck, that's easier. :)
|
Dang I really twisted up my repo. sigh. closing this for now. |
|
Now, thinking about this more, and I am sorry for not proposing this earlier, I'd say this should be removed completely and not just deprecated. However that needs an OMC (or OTC?) approval. |
|
This is on hold pending OTC decision in #14283. And if that is approved, I'd be happy to update this. |
|
The vote for removal has passed. So the RSA_SSLV23_PADDING and any related things should be completely removed. |
|
I forgot we have an issue for the removal already. So closing this one as it is duplicate. |
|
I really need to get some coffee break - this one is a PR and not an issue. So @richsalz do you want to morph this into removal PR? Or do you want to open a new PR for the removal? Or, if you do not want to or have time to work on the removal, I can as well do it. |
|
I had it lying around in anticipation :) I only documented in CHANGES, let me know if that's not okay. |
|
You'll need to rebase against master and fix the remaining places. |
|
Yeah, I missed one line; updated commit pushed. Based on current master. FYI the edits to |
|
fixup commit pushed address @t8m 's feedback. |
t8m
left a comment
There was a problem hiding this comment.
Approving provided the last nit is fixed as well.
OpenSSL removes support for it in gh#openssl/openssl#14248. Fixes #294.
|
For your enjoyment, https://gitlab.com/m2crypto/m2crypto/-/merge_requests/264 |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #14248)
|
Merged to master. Thank you for the contribution. |
As per OpenSSL 3 (since alpha 13), the support of SSL23 has been removed (openssl/openssl#14248). Reference: pocoproject#3223
Fixes: #14216
I am not sure how to handle some of the documentation updates, particularly
man7/EVP_SIGNATURE-RSA.podJust mark it inline as deprecated? I kinda did that forman3/EVP_PKEY_CTX_ctrl.pod