[test][15-test_genec] Improve EC tests with genpkey by romen · Pull Request #12307 · openssl/openssl

romen · 2020-06-28T17:11:32Z

Test separately EC parameters and EC key generation.

For some curves we have had cases in which generating the parameters
under certain conditions failed, while generating and serializing a key
under the same conditions did not.
See #12306 for more details.

This follows up on #12080 (master) and #12085 (1.1.1, merging is not trivial due to the TODO annotations required in master until #12102 is fixed, a separate PR will follow).

Checklist

tests are added or updated

…d keys The following built-in curves do not have an assigned OID: - Oakley-EC2N-3 - Oakley-EC2N-4 In general we shouldn't assume that an OID is always available. This commit detects such cases, raises an error and returns appropriate return values so that the condition can be detected and correctly handled by the callers, when serializing EC parameters or EC keys with the default `ec_param_enc:named_curve`. Fixes openssl#12306

…ters and keys make update

If the key is to be serialized or printed as text and the framework returns an error, the app should signal the failure to the user using a non-zero exit status.

…put errors

Test separately EC parameters and EC key generation. For some curves we have had cases in which generating the parameters under certain conditions failed, while generating and serializing a key under the same conditions did not. See <openssl#12306> for more details.

[test][15-test_genec] Some curves only support explicit params encoding

romen · 2020-06-29T02:33:27Z

Rebased on top of #12313 and #12305 .

Individually the above PRs fail the tests (intentionally), because they need the changes to the tests included in this PR.

… parameters and keys Detect missing OID in serializer implementation

[extended tests]

…zing EC parameters and keys EC_R_MISSING_OID -> PROV_R_MISSING_OID

[extended tests]

romen · 2020-06-30T04:43:43Z

@DDvO I see Travis already has some red crosses which seem related to the cmp cli tests failing again with ubsan, can you look into it?

romen · 2020-06-30T06:22:38Z

@DDvO I see Travis already has some red crosses which seem related to the cmp cli tests failing again with ubsan, can you look into it?

The problems seem to be with both ubsan and msan.

romen · 2020-06-30T06:23:21Z

The other 2 errors are timeouts, I just restarted those jobs to see if kicking the container helps

DDvO · 2020-06-30T14:05:11Z

@DDvO I see Travis already has some red crosses which seem related to the cmp cli tests failing again with ubsan, can you look into it?

The problems seem to be with both ubsan and msan.
* https://travis-ci.com/github/openssl/openssl/jobs/355619805

* https://travis-ci.com/github/openssl/openssl/jobs/355619806

These issues are duplicates of issue that @mattcaswell for the most part already solved #12275
yet he has paused pursuing that for a couple of days.

If you cherry-pick his two commits and also do the following patch I proposed there:

diff --git a/test/recipes/81-test_cmp_cli.t b/test/recipes/81-test_cmp_cli.t
index 8ddb3c2daf..98833e04aa 100644
--- a/test/recipes/81-test_cmp_cli.t
+++ b/test/recipes/81-test_cmp_cli.t
@@ -14,7 +14,7 @@ use warnings;
 use POSIX;
 use File::Spec::Functions qw/catfile/;
 use File::Compare qw/compare_text/;
-use OpenSSL::Test qw/:DEFAULT with data_file data_dir bldtop_dir/;
+use OpenSSL::Test qw/:DEFAULT with data_file data_dir bldtop_dir bldtop_file/;
 use OpenSSL::Test::Utils;
 use Data::Dumper; # for debugging purposes only
 
@@ -24,8 +24,10 @@ plan skip_all => "These tests are not supported in a no-cmp build"
     if disabled("cmp");
 plan skip_all => "These tests are not supported in a no-ec build"
     if disabled("ec");
+open(CONFIG, bldtop_file("configdata.pm"));
 plan skip_all => "These tests are not supported in a fuzz build"
-    if !disabled("fuzz-libfuzzer") || !disabled("fuzz-afl");
+    if (grep { /-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION/ } <CONFIG>);
+close CONFIG;
 plan skip_all => "Tests involving server not available on Windows or VMS"
     if $^O =~ /^(VMS|MSWin32)$/;

those test failures should be gone.

DDvO · 2020-06-30T20:41:01Z

Thanks again @richsalz for the hint on detecting -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION which you gave very I asked for help about that in #12275 (comment).

mattcaswell

I'm assuming I'm just supposed to be reviewing the test changes right? The other changes are from the other PRs? On that assumption this looks good.

romen · 2020-07-03T11:34:18Z

I'm assuming I'm just supposed to be reviewing the test changes right? The other changes are from the other PRs? On that assumption this looks good.

Yes @mattcaswell, the commits prefixed with drop are from the other PRs!

Test separately EC parameters and EC key generation. Some curves only support explicit params encoding. For some curves we have had cases in which generating the parameters under certain conditions failed, while generating and serializing a key under the same conditions did not. See <openssl#12306> for more details. Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#12307)

openssl-machine · 2020-07-04T12:00:16Z

This pull request is ready to merge

Test separately EC parameters and EC key generation. Some curves only support explicit params encoding. For some curves we have had cases in which generating the parameters under certain conditions failed, while generating and serializing a key under the same conditions did not. See <#12306> for more details. Reviewed-by: Matt Caswell <[email protected]> (Merged from #12307)

romen · 2020-07-07T09:06:35Z

Merged to master as:

1c9761d [test][15-test_genec] Improve EC tests with genpkey

Thanks!

romen added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Jun 28, 2020

romen requested a review from levitte June 28, 2020 17:11

romen self-assigned this Jun 28, 2020

romen mentioned this pull request Jun 28, 2020

[1.1.1][test][15-test_genec] Improve EC tests with genpkey #12308

Closed

3 tasks

romen added 7 commits June 29, 2020 05:27

drop! fixup! [EC][ASN1] Detect missing OID when serializing EC parame…

994c866

…ters and keys make update

drop! [apps/genpkey] exit status should not be 0 on output errors

9b48e9b

If the key is to be serialized or printed as text and the framework returns an error, the app should signal the failure to the user using a non-zero exit status.

drop! fixup! [apps/genpkey] exit status should not be 0 on output errors

6f45c26

drop! fixup! fixup! [apps/genpkey] exit status should not be 0 on out…

fda1ef8

…put errors

squash! [test][15-test_genec] Improve EC tests with genpkey

e23e9b0

[test][15-test_genec] Some curves only support explicit params encoding

romen force-pushed the improve_test_genec branch from da3534d to e23e9b0 Compare June 29, 2020 02:29

This was referenced Jun 29, 2020

[1.1.1][EC][ASN1] Detect missing OID when serializing EC parameters and keys #12312

Closed

[apps/genpkey] exit status should not be 0 on output errors #12305

Closed

romen added 2 commits June 30, 2020 02:24

drop! fixup! fixup! [EC][ASN1] Detect missing OID when serializing EC…

974eb27

… parameters and keys Detect missing OID in serializer implementation

drop! Trigger extended tests

02aa18c

[extended tests]

romen mentioned this pull request Jun 29, 2020

[EC][ASN1] Detect missing OID when serializing EC parameters and keys #12313

Closed

romen added 3 commits June 30, 2020 02:52

fixup! squash! [test][15-test_genec] Improve EC tests with genpkey

133b689

drop! fixup! fixup! fixup! [EC][ASN1] Detect missing OID when seriali…

bc51e0e

…zing EC parameters and keys EC_R_MISSING_OID -> PROV_R_MISSING_OID

drop! Trigger extended tests

bd094ec

[extended tests]

mattcaswell approved these changes Jul 3, 2020

View reviewed changes

romen removed the approval: review pending This pull request needs review by a committer label Jul 3, 2020

romen added the approval: done This pull request has the required number of approvals label Jul 3, 2020

openssl-machine removed the approval: done This pull request has the required number of approvals label Jul 4, 2020

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jul 4, 2020

romen closed this Jul 7, 2020

romen mentioned this pull request Jul 7, 2020

genpkey -genparam fails for some EC curves #12306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

[test][15-test_genec] Improve EC tests with genpkey#12307

[test][15-test_genec] Improve EC tests with genpkey#12307
romen wants to merge 12 commits intoopenssl:masterfrom
romen:improve_test_genec

romen commented Jun 28, 2020

Uh oh!

romen commented Jun 29, 2020

Uh oh!

romen commented Jun 30, 2020

Uh oh!

romen commented Jun 30, 2020

Uh oh!

romen commented Jun 30, 2020

Uh oh!

DDvO commented Jun 30, 2020

Uh oh!

DDvO commented Jun 30, 2020 •

edited

Loading

Uh oh!

mattcaswell left a comment

Uh oh!

romen commented Jul 3, 2020

Uh oh!

openssl-machine commented Jul 4, 2020

Uh oh!

romen commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Comments

Conversation

romen commented Jun 28, 2020

Checklist

Uh oh!

romen commented Jun 29, 2020

Uh oh!

romen commented Jun 30, 2020

Uh oh!

romen commented Jun 30, 2020

Uh oh!

romen commented Jun 30, 2020

Uh oh!

DDvO commented Jun 30, 2020

Uh oh!

DDvO commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

romen commented Jul 3, 2020

Uh oh!

openssl-machine commented Jul 4, 2020

Uh oh!

romen commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DDvO commented Jun 30, 2020 •

edited

Loading