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

Jump to bottom

Doc update - legacy provider's cipher algorithms #15197

Closed
wants to merge 2 commits into from
Closed

Doc update - legacy provider's cipher algorithms #15197

wants to merge 2 commits into from

Conversation

beldmit
Copy link
Member

@beldmit beldmit commented May 7, 2021

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

@beldmit beldmit added branch: master Merge to master branch approval: otc review pending This pull request needs review by an OTC member labels May 7, 2021
paulidale
paulidale requested changes May 8, 2021
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Minor wording changes mostly.

doc/man3/EVP_des_cbc.pod Outdated Show resolved Hide resolved
doc/man3/EVP_desx_cbc.pod Outdated Show resolved Hide resolved
doc/man7/OSSL_PROVIDER-legacy.pod Outdated
@@ -52,6 +52,30 @@ The OpenSSL legacy provider supports these operations and algorithms:

=back

=head2 Symmetric Ciphers

Not all the symmetric cipher algorithms can be enabled in a particular OpenSSL build.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all of these symmetric cipher algorithms are enabled by default.

Although, it would also be good to indicate which aren't: This cipher is not enabled by default. Use the enable-XXX configuration to enable it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs changing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the mentioning to the rc5 section (which seems the only disabled by default). Could you please clarify what else should be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Change the text to Not all of these symmetric cipher algorithms are enabled by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks!

@beldmit
Copy link
Member Author

beldmit commented May 8, 2021

Hopefully fixed

paulidale
paulidale reviewed May 8, 2021
View reviewed changes
doc/man3/EVP_des_cbc.pod Outdated Show resolved Hide resolved
@slontis
Copy link
Member

slontis commented May 10, 2021

Not sure the third party implementation should even be mentioned - these are fairly DEAD algorithms..

@paulidale
Copy link
Contributor

I agree, I think it was better without that mention.

@slontis
Copy link
Member

slontis commented May 10, 2021 •
edited

'Enumerating the ' sounds like a code change..

@slontis slontis closed this May 10, 2021
@slontis
Copy link
Member

slontis commented May 10, 2021

Oops - that wasnt what I meant to do,,

@slontis slontis reopened this May 10, 2021
@levitte
Copy link
Member

levitte commented May 10, 2021

What about saying that they aren't available with OpenSSL's default provider, and that to use them, you will for example have to load OpenSSL's legacy provider?

@paulidale
Copy link
Contributor

That seems reasonable.

paulidale
paulidale reviewed May 10, 2021
View reviewed changes
doc/man3/EVP_des_cbc.pod Outdated Show resolved Hide resolved
@beldmit beldmit changed the title Enumerating the legacy provider's cipher algorithms Doc update - legacy provider's cipher algorithms May 10, 2021
paulidale
paulidale reviewed May 10, 2021
View reviewed changes
doc/man3/EVP_des_cbc.pod Outdated
@@ -54,6 +54,10 @@ EVP_des_ofb()
DES in CBC, ECB, CFB with 64-bit shift, CFB with 1-bit shift, CFB with 8-bit
shift and OFB modes.

All these algorithms are not provided by the default OpenSSL provider.
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these algorithms...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "None of these algorithms are provided" is more clear.

Copy link
Member

@romen romen May 11, 2021 •
edited

Choose a reason for hiding this comment

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

Not a native here, so feel free to disregard this completely, but would "OpenSSL default provider" be more appropriate here? To me here default is more akin to the name of a particular provider instance than a generic adjective qualifying the provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworded. @romen, would you mind reapproving?

paulidale
paulidale approved these changes May 11, 2021
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Approved subject to the second of being added.

@beldmit
Copy link
Member Author

beldmit commented May 11, 2021

I'm sorry, @paulidale, I don't understand it the phrasing should be polished a bit more (and then could you please give an exact formula) or it's OK to you (and then could you please add the approval label)?

Many thanks!

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels May 11, 2021
@beldmit beldmit removed the approval: done This pull request has the required number of approvals label May 11, 2021
@beldmit beldmit added the approval: otc review pending This pull request needs review by an OTC member label May 11, 2021
@beldmit
Copy link
Member Author

beldmit commented May 12, 2021

Ping @openssl/otc to re-approve after, hopefully, final rewording

paulidale
paulidale reviewed May 12, 2021
View reviewed changes
doc/man3/EVP_des_cbc.pod Outdated Show resolved Hide resolved
doc/man3/EVP_des_cbc.pod Outdated Show resolved Hide resolved
doc/man3/EVP_desx_cbc.pod Outdated Show resolved Hide resolved
paulidale
paulidale approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Finally 👍

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels May 12, 2021
@beldmit
Copy link
Member Author

beldmit commented May 12, 2021

Finally

(here was a long quote from "Parkinson's law")

@openssl-machine
Copy link
Collaborator

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.

openssl-machine pushed a commit that referenced this pull request May 13, 2021
@beldmit
Enumerating the legacy provider's cipher algorithms
6581b17 
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15197)
openssl-machine pushed a commit that referenced this pull request May 13, 2021
@beldmit
clarification about the DES status
4639772 
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15197)
@beldmit
Copy link
Member Author

beldmit commented May 13, 2021

Finally merged. Thanks!

@beldmit beldmit closed this May 13, 2021
@beldmit beldmit deleted the legacy_doc_improvement branch May 13, 2021 10:25
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@beldmit @devnexen
Enumerating the legacy provider's cipher algorithms
dc38aea 
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15197)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@beldmit @devnexen
clarification about the DES status
3d7dde8 
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15197)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romen romen romen left review comments

@levitte levitte levitte left review comments

@kaduk kaduk kaduk left review comments

@richsalz richsalz richsalz left review comments

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@beldmit @slontis @paulidale @levitte @openssl-machine @romen @kaduk @richsalz