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

EVP digest list: one hash algorithm per file, synchronize EVP list, o… #4628

Closed

Conversation

ronaldtse
Copy link
Contributor

As described by @levitte in #4564 and #4616 , this is similar work to break up the EVP digest list into individual pages.

Specifically:

  • Added documentation for the missing SHAKE, WHIRLPOOL, MD4 hash algorithms
  • Added the SSL-specific MD5-SHA1 hash algorithm
  • Split all digests into separate files

This is a contribution from Ribose Inc..

Checklist
  • documentation is added or updated


=head1 CONFORMING TO

N/A.
Copy link
Member

Choose a reason for hiding this comment

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

In a case like this, I think it would be preferable to remove this section. What's your motivation for keeping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for consistency among all the files. We can remove it alright if it is preferred.

Copy link
Member

Choose a reason for hiding this comment

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

It is at least my preference. I don't think we've ever had a N/A section before, so this is a new situation for us. What says the rest of @openssl/omc and @openssl/committers ?

Copy link
Member

Choose a reason for hiding this comment

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

How about referring to https://tools.ietf.org/html/rfc7693 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call @bernd-edlinger , thank you! Maybe the answer to @levitte is : there can always be a reference...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I write man pages in my other life as a FreeBSD documentation committer, and CONFORMING TO is not a required section, so it should be omitted when empty.

Copy link
Member

Choose a reason for hiding this comment

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

yes, but it would normally be the case that it conforms to something, like an RFC or something.


=head1 CONFORMING TO

N/A.
Copy link
Member

Choose a reason for hiding this comment

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

Here's another one.

I heard your argument about having this as a placeholder, in case we find a reference later on... Unfortunately, placeholders aren't in our habits. As a matter of fact, we have had some, but removed them all at some point.

I did check to see if I could quickly find something to put here. Alas...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The answer is actually in MDC2_Init.pod, the reference is ISO/IEC 10118-2 👍

Copy link
Member

Choose a reason for hiding this comment

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

Feel free :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still trying to find out which exact algorithm it is in that document... one sec 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.


=head1 CONFORMING TO

N/A.
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ISO/IEC 10118-3:2016 Dedicated Hash-Function 1 (RIPEMD-160).

Copy link
Member

Choose a reason for hiding this comment

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

Do feel free :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

Awesome

@ronaldtse ronaldtse force-pushed the ronaldtse-update-evpdigestlist branch from 8d47c59 to 7289300 Compare October 31, 2017 10:19

=item EVP_md_null()

Returns a pointer to the corresponding B<EVP_MD> structures.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this sentence, is it in the wrong context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bernd-edlinger I think there's something wrong with the diff?

I'm seeing the right description here: https://github.com/openssl/openssl/pull/4628/files#diff-b285e033e98ee2d9eff6aec1a7bd6dbfR156

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the description looks right, but under return values I see something like:

EVP_md_null()
    Returns a pointer to the corresponding EVP_MD structures

That sounds a bit funny I think you mean to say something about the return value of any
EVP_<digest> function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bernd-edlinger in the diff the content is:
"A "null" message digest that does nothing: i.e. the hash it returns is of zero length."

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean the return value section,
previously the text was:

EVP_md_null(), EVP_md2(), EVP_md5(), EVP_sha1(),
EVP_mdc2(), EVP_ripemd160(), EVP_blake2b512(), and EVP_blake2s256() return
pointers to the corresponding EVP_MD structures.

which made sense, but now it is:

EVP_md_null()
Returns a pointer to the corresponding B<EVP_MD> structures.

So, unless I am completely mistaken,
the return value is the EVP_MD structure of the NULL MD or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I was entirely confused. I've now fixed hopefully the right issue 😉

Copy link
Member

@bernd-edlinger bernd-edlinger left a comment

Choose a reason for hiding this comment

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

Thank you

@ronaldtse
Copy link
Contributor Author

Thank you @bernd-edlinger !

@levitte
Copy link
Member

levitte commented Oct 31, 2017

Squashed and merged. bbda8ce

@levitte levitte closed this Oct 31, 2017
levitte pushed a commit that referenced this pull request Oct 31, 2017
…verall cleanup.

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #4628)
@ronaldtse ronaldtse deleted the ronaldtse-update-evpdigestlist branch November 2, 2017 09:28
@ronaldtse
Copy link
Contributor Author

Thank you @levitte !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants