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

crypto(7) no longer exists; EVP_*(3) point to a five-level manual chase instead of EVP_MD-*(7) #22420

Closed
wants to merge 1 commit into from

Conversation

nabijaczleweli
Copy link
Contributor

@nabijaczleweli nabijaczleweli commented Oct 18, 2023

I'd wanted to fix
image
but crypto(7) doesn't exist anymore:

$ git show d5b7d0a6a2c7263c4bc22a9403e05bc80d324f9a --stat
commit d5b7d0a6a2c7263c4bc22a9403e05bc80d324f9a
Author: Matt Caswell <matt@openssl.org>
Date:   Thu Jul 13 15:02:40 2023 +0100

    Incorporate the crypto man page into the OpenSSL guide

    Some content has been moved out into the general libraries introduction.
    Reformat and fill in some gaps with what remains.

    Reviewed-by: Hugo Landau <hlandau@openssl.org>
    Reviewed-by: Tim Hudson <tjh@openssl.org>
    Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
    Reviewed-by: Anton Arapov <anton@openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/21560)

 doc/man7/crypto.pod                            | 580 ---------------------------------------------------------------------------------------------------------------------
 doc/man7/ossl-guide-libcrypto-introduction.pod | 398 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 398 insertions(+), 580 deletions(-)

and yet git grep crypto\(7 yields precisely 100 lines. Not great. Trivial fix, this is patch 1.


Earlier today I finally buckled and tried to find out what the

Developers should be aware of the negative performance implications of calling these functions multiple times and should consider using EVP_MD_fetch(3) instead. See "Performance" in crypto(7) for further information.

paragraph means.

This took me 5 (five!) manuals:

  • EVP_sha1(3)
  • crypto(7)
  • EVP_MD_fetch(3) (but not there! don't read that!)
  • OSSL_PROVIDER-default(7)
  • EVP_MD-SHA1(7)

just to find out what I'm supposed to give EVP_MD_fetch() (well okay, SHA was easy, but what "providers" call BLAKE2 was too nebulous to guess). Which is not great, considering these are supposed to be replacing the EVP_* functions, but what you're supposed to be replacing it with is deadass buried. Thus I made the paragraphs that contained "and should consider using" be

Developers should be aware of the negative performance implications of calling these functions multiple times and should consider using EVP_MD_fetch(3) with EVP_MD-BLAKE2(7) instead. See "Performance" in ossl-guide-libcrypto-introduction(7) for further information.

Which, I think, is useful to the developer instead of damning them for using the "legacy" function with no obvious recourse. This is patch 2.

@t8m
Copy link
Member

t8m commented Oct 18, 2023

@nabijaczleweli The first commit is not needed. The crypto(7) manpage still exists as a link to the guide. See for example: https://www.openssl.org/docs/manmaster/man7/crypto.html

@t8m t8m added branch: master Merge to master branch triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Oct 18, 2023
@t8m
Copy link
Member

t8m commented Oct 18, 2023

Unfortunately this is also outside of what would be acceptable with CLA: trivial. Would you be willing to sign a regular CLA?
https://www.openssl.org/policies/cla.html

@t8m t8m added hold: cla required The contributor needs to submit a license agreement cla: trivial One of the commits is marked as 'CLA: trivial' labels Oct 18, 2023
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Oct 18, 2023
Earlier today, it took me five manuals! to find what on earth the
"Performance"/"EVP_MD_fetch(3)" crosslinks actually mean:
  EVP_sha1(3)
  crypto(7)
  EVP_MD_fetch(3) (but not there! don't read that!)
  OSSL_PROVIDER-default(7)
  EVP_MD-SHA1(7)

If, instead, EVP_sha1(3) referenced EVP_MD-SHA1(7) at /all/,
which it should do, since it's supposed to be what you're replacing it
with, but it doesn't actually say that, maybe people would use it.
I know I didn't because it's basically just deadass buried

As found by git grep -l 'and should consider using'
@t8m t8m added the hold: cla required The contributor needs to submit a license agreement label Oct 18, 2023
@nabijaczleweli
Copy link
Contributor Author

That wasn't immediately obvious to me, the crypto.7 alias doesn't get created with just make all (but make install does make /usr/local/share/man/man7/crypto.7ossl, so that checks out). Patch 1 dropped.

"CLA: trivial" stanza dropped, ICLA mailed to legal@ as <pnojsdlcsdow2cpjktdndxcmc3fuvtfssg6jtao7chnh43ykig@tarta.nabijaczleweli.xyz>

@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Oct 19, 2023
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Oct 19, 2023
@t8m t8m removed the cla: trivial One of the commits is marked as 'CLA: trivial' label Oct 19, 2023
@t8m t8m added approval: done This pull request has the required number of approvals tests: exempted The PR is exempt from requirements for testing and removed approval: review pending This pull request needs review by a committer labels Oct 19, 2023
@openssl-machine openssl-machine 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 Oct 20, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@mattcaswell
Copy link
Member

Pushed to master/3.1/3.0. Thanks!

openssl-machine pushed a commit that referenced this pull request Oct 20, 2023
Earlier today, it took me five manuals! to find what on earth the
"Performance"/"EVP_MD_fetch(3)" crosslinks actually mean:
  EVP_sha1(3)
  crypto(7)
  EVP_MD_fetch(3) (but not there! don't read that!)
  OSSL_PROVIDER-default(7)
  EVP_MD-SHA1(7)

If, instead, EVP_sha1(3) referenced EVP_MD-SHA1(7) at /all/,
which it should do, since it's supposed to be what you're replacing it
with, but it doesn't actually say that, maybe people would use it.
I know I didn't because it's basically just deadass buried

As found by git grep -l 'and should consider using'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22420)

(cherry picked from commit b6eb95f)
openssl-machine pushed a commit that referenced this pull request Oct 20, 2023
Earlier today, it took me five manuals! to find what on earth the
"Performance"/"EVP_MD_fetch(3)" crosslinks actually mean:
  EVP_sha1(3)
  crypto(7)
  EVP_MD_fetch(3) (but not there! don't read that!)
  OSSL_PROVIDER-default(7)
  EVP_MD-SHA1(7)

If, instead, EVP_sha1(3) referenced EVP_MD-SHA1(7) at /all/,
which it should do, since it's supposed to be what you're replacing it
with, but it doesn't actually say that, maybe people would use it.
I know I didn't because it's basically just deadass buried

As found by git grep -l 'and should consider using'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22420)
openssl-machine pushed a commit that referenced this pull request Oct 20, 2023
Earlier today, it took me five manuals! to find what on earth the
"Performance"/"EVP_MD_fetch(3)" crosslinks actually mean:
  EVP_sha1(3)
  crypto(7)
  EVP_MD_fetch(3) (but not there! don't read that!)
  OSSL_PROVIDER-default(7)
  EVP_MD-SHA1(7)

If, instead, EVP_sha1(3) referenced EVP_MD-SHA1(7) at /all/,
which it should do, since it's supposed to be what you're replacing it
with, but it doesn't actually say that, maybe people would use it.
I know I didn't because it's basically just deadass buried

As found by git grep -l 'and should consider using'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22420)

(cherry picked from commit b6eb95f)
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Oct 26, 2023
Earlier today, it took me five manuals! to find what on earth the
"Performance"/"EVP_MD_fetch(3)" crosslinks actually mean:
  EVP_sha1(3)
  crypto(7)
  EVP_MD_fetch(3) (but not there! don't read that!)
  OSSL_PROVIDER-default(7)
  EVP_MD-SHA1(7)

If, instead, EVP_sha1(3) referenced EVP_MD-SHA1(7) at /all/,
which it should do, since it's supposed to be what you're replacing it
with, but it doesn't actually say that, maybe people would use it.
I know I didn't because it's basically just deadass buried

As found by git grep -l 'and should consider using'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#22420)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Oct 26, 2023
Earlier today, it took me five manuals! to find what on earth the
"Performance"/"EVP_MD_fetch(3)" crosslinks actually mean:
  EVP_sha1(3)
  crypto(7)
  EVP_MD_fetch(3) (but not there! don't read that!)
  OSSL_PROVIDER-default(7)
  EVP_MD-SHA1(7)

If, instead, EVP_sha1(3) referenced EVP_MD-SHA1(7) at /all/,
which it should do, since it's supposed to be what you're replacing it
with, but it doesn't actually say that, maybe people would use it.
I know I didn't because it's basically just deadass buried

As found by git grep -l 'and should consider using'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#22420)

(cherry picked from commit b6eb95f)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Oct 26, 2023
Earlier today, it took me five manuals! to find what on earth the
"Performance"/"EVP_MD_fetch(3)" crosslinks actually mean:
  EVP_sha1(3)
  crypto(7)
  EVP_MD_fetch(3) (but not there! don't read that!)
  OSSL_PROVIDER-default(7)
  EVP_MD-SHA1(7)

If, instead, EVP_sha1(3) referenced EVP_MD-SHA1(7) at /all/,
which it should do, since it's supposed to be what you're replacing it
with, but it doesn't actually say that, maybe people would use it.
I know I didn't because it's basically just deadass buried

As found by git grep -l 'and should consider using'

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl/openssl#22420)

(cherry picked from commit b6eb95f)
Signed-off-by: fly2x <fly2x@hitls.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants