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

Test encoders/decoders with a non-default library context, fix up defects #14587

Closed
wants to merge 4 commits into from
Closed

Test encoders/decoders with a non-default library context, fix up defects #14587

wants to merge 4 commits into from

Conversation

jon-oracle
Copy link
Contributor

@jon-oracle jon-oracle commented Mar 17, 2021 •
edited

This PR adds a non-default library context option to endecode_test and fixes issues caused by ciphers/digests being retrieved without a _fetch. It also adds a couple of APIs (b2i_PVK_bio_ex & i2b_PVK_bio_ex) to allow the library context and property query to be passed to where it's needed.

This work also overlaps #12694

  • documentation is added or updated
  • tests are added or updated

@slontis
Copy link
Member

slontis commented Mar 18, 2021

Jon - can you check that you cover everything that I did in #12694. If so I will close that one..

@jon-oracle jon-oracle mentioned this pull request Apr 7, 2021
Closed
2 tasks
@jon-oracle jon-oracle changed the title [WIP] Test encoders/decoders with a non-default library context, fix up defects Test encoders/decoders with a non-default library context, fix up defects Apr 14, 2021
levitte
levitte requested changes May 6, 2021
View reviewed changes
providers/implementations/encode_decode/encode_key2ms.c Outdated Show resolved Hide resolved
test/endecode_test.c Outdated Show resolved Hide resolved
test/endecode_test.c Outdated Show resolved Hide resolved
test/endecode_test.c Outdated Show resolved Hide resolved
test/recipes/04-test_encoder_decoder.t Outdated Show resolved Hide resolved
test/endecode_test.c Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented May 6, 2021

I forget, do we have a test of implicit fetching somewhere? I noticed that the PVK code was changed to fetch RC4 explicitly, and am worried that we forget to test the implicit variants.

@slontis
Copy link
Member

slontis commented May 7, 2021

LGTM

t8m
t8m reviewed May 7, 2021
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

If you rebase and make update the fips checksum should not be modified anymore.

t8m
t8m requested changes May 18, 2021
View reviewed changes
providers/fips-sources.checksums Outdated Show resolved Hide resolved
@slontis
Copy link
Member

slontis commented May 18, 2021

Can you rebase this and clean up the commit messages that you would want to appear into the logs please.

@jon-oracle
Copy link
Contributor Author

Rebased and squashed/reworded some commits.

slontis
slontis reviewed May 18, 2021
View reviewed changes
crypto/pem/pvkfmt.c Outdated Show resolved Hide resolved
slontis
slontis reviewed May 18, 2021
View reviewed changes
test/endecode_test.c Outdated Show resolved Hide resolved
slontis
slontis reviewed May 18, 2021
View reviewed changes
crypto/pem/pvkfmt.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented May 19, 2021

Please do rebase -i --autosquash again to avoid touching the fips checksums in the intermediate commits. Otherwise I suspect this will cause issues with ghmerge. Also the commit message of db8ee15 needs a cleanup.

@jon-oracle
Copy link
Contributor Author

Rebased.

I've modified the arguments to endecode_test to add two new named arguments for the ones merged from master (RSA and RSA-PSS key files).

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 31, 2021
@t8m
Copy link
Member

t8m commented May 31, 2021

Rebased to drop the spurious fips-sources.checksums change.

t8m
t8m approved these changes May 31, 2021
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

@slontis please reconfirm after the rebase.

slontis
slontis approved these changes May 31, 2021
View reviewed changes
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.

reapproved

paulidale
paulidale approved these changes Jun 1, 2021
View reviewed changes
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.

Subject to the two blank lines and a reason for the odd conditional

doc/man3/b2i_PVK_bio_ex.pod Show resolved Hide resolved
doc/man3/b2i_PVK_bio_ex.pod Show resolved Hide resolved
test/recipes/04-test_encoder_decoder.t Show resolved Hide resolved
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Jun 1, 2021
@jon-oracle
fixup! Enhance the encoder/decoder tests to allow testing with a non-…
66cec02 
…default library context and configurable providers
@paulidale
Copy link
Contributor

No need to reset the timer for the two blank lines added.

@paulidale paulidale 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 Jun 1, 2021
@slontis slontis closed this Jun 1, 2021
@slontis slontis reopened this Jun 1, 2021
@t8m
Copy link
Member

t8m commented Jun 1, 2021

Merging

openssl-machine pushed a commit that referenced this pull request Jun 1, 2021
@jon-oracle @t8m
Enhance the encoder/decoder tests to allow testing with a non-default…
169eca6 
… library context and configurable providers

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #14587)
openssl-machine pushed a commit that referenced this pull request Jun 1, 2021
@jon-oracle @t8m
Fix up encoder/decoder issues caused by not passing a library context…
e3c7595 
… to the PKCS8 encrypt/decrypt

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #14587)
openssl-machine pushed a commit that referenced this pull request Jun 1, 2021
@jon-oracle @t8m
Pass library context and property query into private key decoders
5e2d22d 
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #14587)
@t8m
Copy link
Member

t8m commented Jun 1, 2021

Merged to master, finally. Thank you all!

@t8m t8m closed this Jun 1, 2021
@slontis
Copy link
Member

slontis commented Jun 1, 2021

Thanks!

devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@jon-oracle @devnexen
Enhance the encoder/decoder tests to allow testing with a non-default…
9969129 
… library context and configurable providers

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#14587)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@jon-oracle @devnexen
Fix up encoder/decoder issues caused by not passing a library context…
54ee5a6 
… to the PKCS8 encrypt/decrypt

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#14587)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
@jon-oracle @devnexen
Pass library context and property query into private key decoders
91e1084 
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#14587)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte requested changes

@slontis slontis slontis approved these changes

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources
Projects
None yet
Milestone
3.0.0 beta1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jon-oracle @slontis @levitte @t8m @openssl-machine @paulidale