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.

Sign up for GitHub

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

Move BLAKE2 digests completely to the default provider #9075

Closed
wants to merge 5 commits into from
Closed

Move BLAKE2 digests completely to the default provider #9075

wants to merge 5 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Jun 4, 2019

This leaves minimal implementations of EVP_blake2b512 and EVP_blake2s256,
that are now only there to provide a name for implicit fetches.

@levitte levitte added the branch: master Merge to master branch label Jun 4, 2019
@levitte levitte added this to In progress in 3.0 New Core + FIPS via automation Jun 4, 2019
@levitte levitte force-pushed the move-blake2-more-20190604 branch from b07c102 to 2ca6d14 Compare June 4, 2019 12:03
@mattcaswell
Copy link
Member

This suffers from the same problems as #9076 (see my comments there).

@slontis
Copy link
Member

slontis commented Jun 4, 2019

When this actually does work do you perhaps need to leave the size fields in the old legacy EVP_MD structures?

@levitte
Copy link
Member Author

levitte commented Jun 5, 2019

When this actually does work do you perhaps need to leave the size fields in the old legacy EVP_MD structures?

Why? For DigestInit, only the type is interesting...

@slontis
Copy link
Member

slontis commented Jun 5, 2019

Why? For DigestInit, only the type is interesting...

So what does EVP_MD_size(md) return? it doesnt have a md->prov right?

@levitte
Copy link
Member Author

levitte commented Jun 5, 2019

So what does EVP_MD_size(md) return? it doesnt have a md->prov right?

Ohhhhhhhhhhhhhhhh!!!!!! Now this raises an interesting question, how is that size relevant for legacy (constant) EVP_MDs if any actual use of that EVP_MD will result in an implicit fetch, i.e. a completely different set of data? There's a potential that EVP_MD_size() will give the wrong answer...

@mattcaswell
Copy link
Member

So what does EVP_MD_size(md) return? it doesnt have a md->prov right?

Good point. EVP_MD_block_size has a similar issue.

@slontis
Copy link
Member

slontis commented Jun 5, 2019

There's a potential that EVP_MD_size() will give the wrong answer...

But at least it would be right in most cases :)

@levitte
Copy link
Member Author

levitte commented Jun 5, 2019 •
edited

But at least it would be right in most cases :)

yeeeaaahhh I dunno how I like that... but OK, for the moment being, I'll keep the sizes. I wonder if we should have checks of such things in DigestInit, though.

@levitte levitte mentioned this pull request Jun 6, 2019
Closed
@levitte levitte force-pushed the move-blake2-more-20190604 branch 2 times, most recently from b4ae803 to 5fe7e85 Compare September 30, 2019 12:56
@levitte
Copy link
Member Author

levitte commented Sep 30, 2019

Ping. We really should do this kind of cleanup for everything that we move to the provider.

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Oct 1, 2019
mattcaswell
mattcaswell requested changes Oct 2, 2019
View reviewed changes
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 suffers from the same problem that I noted in #10059 so should not be merged before that is resolved.

3.0 New Core + FIPS automation moved this from Reviewer approved to Needs review Oct 2, 2019
@levitte levitte force-pushed the move-blake2-more-20190604 branch 3 times, most recently from b458d1b to f4b07e8 Compare October 9, 2019 19:41
@levitte
Copy link
Member Author

levitte commented Oct 17, 2019

Ping

3.0 New Core + FIPS automation moved this from Needs review to Reviewer approved Oct 17, 2019
@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Oct 17, 2019
levitte added a commit that referenced this pull request Oct 17, 2019
@levitte
Move BLAKE2 digests completely to the default provider
8c77d45 
This leaves minimal implementations of EVP_blake2b512 and EVP_blake2s256,
that are now only there to provide a name for implicit fetches.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #9075)
@levitte
Copy link
Member Author

levitte commented Oct 17, 2019

Merged.

8c77d45 Move BLAKE2 digests completely to the default provider

@levitte levitte closed this Oct 17, 2019
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

@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
No open projects
3.0 New Core + FIPS
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@levitte @mattcaswell @slontis @paulidale