Skip to content

Add a default provider and implement SHA256 in it#8513

Closed
mattcaswell wants to merge 9 commits intoopenssl:masterfrom
mattcaswell:provider-skeletons
Closed

Add a default provider and implement SHA256 in it#8513
mattcaswell wants to merge 9 commits intoopenssl:masterfrom
mattcaswell:provider-skeletons

Conversation

@mattcaswell
Copy link
Member

This is marked as WIP because it still needs tests and documentation. It is also built on the commits in #8341 and #8480.

This runs through "make test" successfully using the default provider implementation of SHA256 where applicable. If engines are involved or we are invoking the EVP_Digest* functions as part of an EVP_DigestSign* operation then we fallback to the legacy handling.

@mattcaswell mattcaswell added the branch: master Applies to master branch label Mar 18, 2019
@mattcaswell
Copy link
Member Author

Rebased now that #8341 went in.

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.

Looks good so far.

@paulidale paulidale mentioned this pull request Mar 19, 2019
2 tasks
@mattcaswell
Copy link
Member Author

I rebased this now that #8480 has gone in. This incorporates all fixes to the feedback so far. The previously noted dependencies for this PR (#8341 and #8480) have both been merged, but now we additionally depend on #8519.

@mattcaswell mattcaswell changed the title WIP: Add a default provider and implement SHA256 in it Add a default provider and implement SHA256 in it Mar 20, 2019
@mattcaswell
Copy link
Member Author

I've rebased this and removed the dependency on #8519 (which got closed without merge). So this PR is now completely standalone.

I've taken it out of WIP. Please take a look.

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.

One thing that needs to be fixed -- prototype removal in providers/default/defltprov.c.
The rest are suggestions to take or leave.

@mattcaswell
Copy link
Member Author

Fixup commits pushed. @paulidale please can you reconfirm?

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.

LGTM.

@paulidale paulidale added the approval: done This pull request has the required number of approvals label Mar 20, 2019
@paulidale
Copy link
Contributor

AppVeyor failure isn't relevant.

@levitte
Copy link
Member

levitte commented Mar 21, 2019

I restarted the failed appveyor build

@mattcaswell
Copy link
Member Author

Appveyor passed this time.

@mattcaswell
Copy link
Member Author

Pushed. Thanks.

levitte pushed a commit that referenced this pull request Mar 21, 2019
Reviewed-by: Paul Dale <[email protected]>
(Merged from #8513)
levitte pushed a commit that referenced this pull request Mar 21, 2019
Reviewed-by: Paul Dale <[email protected]>
(Merged from #8513)
levitte pushed a commit that referenced this pull request Mar 21, 2019
levitte pushed a commit that referenced this pull request Mar 21, 2019
levitte pushed a commit that referenced this pull request Mar 21, 2019
Reviewed-by: Paul Dale <[email protected]>
(Merged from #8513)
levitte pushed a commit that referenced this pull request Mar 21, 2019
@opensignature opensignature mentioned this pull request Apr 15, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants