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: Fix calls to evp_pkey_export_to_provider() #11550

Closed
wants to merge 5 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Apr 15, 2020

The calls weren't quite right, as this function has changed its behaviour.
We also change the internal documentation of this function, and document
evp_pkey_downgrade().

Fixes #11549

@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Apr 15, 2020
@levitte levitte added this to the 3.0.0 milestone Apr 15, 2020
@levitte levitte added this to In progress in 3.0 New Core + FIPS via automation Apr 15, 2020
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

I would like to see a test case for this scenario also (similar to the listing in the bug).
The code looks ok.

@levitte
Copy link
Member Author

levitte commented Apr 15, 2020

Please suggest a spot for such a test... or do you expect a whole new test program?

(I was toying with the idea of making a library to collect test cases like this, and a program test/corners.c that goes through them all, as well as the recipe test/recipes/90-test_corner_cases.t)

@mattcaswell
Copy link
Member

Please suggest a spot for such a test

evp_extra_test?

@levitte
Copy link
Member Author

levitte commented Apr 15, 2020

evp_extra_test?

Ah! That's the pile of random tests! Thanks :-)

@levitte
Copy link
Member Author

levitte commented Apr 15, 2020

Test added

@slontis
Copy link
Member

slontis commented Apr 16, 2020

The new test fails :)

@levitte
Copy link
Member Author

levitte commented Apr 16, 2020

The new test fails :)

I was tired when I wrote that :-/

Fixed, I hope

@levitte levitte mentioned this pull request Apr 16, 2020
2 tasks
paulidale
paulidale previously approved these changes Apr 16, 2020
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 typo :)

test/evp_extra_test.c Outdated Show resolved Hide resolved
3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Apr 16, 2020
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 16, 2020
crypto/evp/pmeth_gn.c Outdated Show resolved Hide resolved
The calls weren't quite right, as this function has changed its behaviour.
We also change the internal documentation of this function, and document
evp_pkey_downgrade().

Fixes openssl#11549
…est.c

We do it with RSA, which may seem strange.  However, an RSA "template"
is generally ignored, so this is safe.  This is modelled after the test
code given in github issue openssl#11549.
@levitte levitte dismissed paulidale’s stale review April 16, 2020 08:36

Re-approval will be needed in a bit

3.0 New Core + FIPS automation moved this from Reviewer approved to Needs review Apr 16, 2020
@levitte levitte removed the approval: done This pull request has the required number of approvals label Apr 16, 2020
@levitte levitte added the approval: review pending This pull request needs review by a committer label Apr 16, 2020
3.0 New Core + FIPS automation moved this from Needs review to Reviewer approved Apr 16, 2020
@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 16, 2020
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

@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Apr 17, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Apr 17, 2020
@slontis
Copy link
Member

slontis commented Apr 17, 2020

Pauli you wanna merge this? I am off for dinner..

@paulidale
Copy link
Contributor

Yep, merging.

@paulidale
Copy link
Contributor

Merged.

@paulidale paulidale closed this Apr 17, 2020
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Apr 17, 2020
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
The calls weren't quite right, as this function has changed its behaviour.
We also change the internal documentation of this function, and document
evp_pkey_downgrade().

Fixes #11549

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #11550)
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
…est.c

We do it with RSA, which may seem strange.  However, an RSA "template"
is generally ignored, so this is safe.  This is modelled after the test
code given in github issue #11549.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #11550)
@levitte levitte deleted the fix-11549 branch February 3, 2021 22:30
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
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

EVP_PKEY_keygen from EVP_PKEY_CTX_new seems to be broken
5 participants