Test genpkey app for EC keygen with various args by romen · Pull Request #12080 · openssl/openssl

romen · 2020-06-07T18:34:47Z

This PR adds a new recipe to test EC key generation with the genpkey CLI app.

For each built-in curve, it tests key generation with text output, in PEM and in DER format, using explicit and named_curve for parameters encoding.

The list of built-in curves is static at the moment, as this allows to differentiate between prime curves and binary curves to avoid failing when ec2m is disabled.

Issue

At this time these new tests, that are passing in 1.1.1 (#12085) are failing due to changes introduced in #11328.

Issue #12102 tracks the progress on solving the issue.

`TODO: {}` block

For now, the failing tests are annotated within a TODO: {} block that informs the testing harness that the tests are expected to fail because development is not yet completed.

The PR fixing #12102 will address also the removal of this TODO: {} annotation.

Credits

Thanks to @bbbrumley and the rest of the team behind the OpenSSL Triggerflow CI (paper) that detected and reported this problem!

Checklist

tests are added or updated
issue is created ([3.0.0] genpkey -algorithm EC -pkeyopt ec_param_enc:explicit fails #12102)
add more curve aliases

This commit adds a new recipe to test EC key generation with the `genpkey` CLI app. For each built-in curve, it tests key generation with text output, in PEM and in DER format, using `explicit` and `named_curve` for parameters encoding. The list of built-in curves is static at the moment, as this allows to differentiate between prime curves and binary curves to avoid failing when ec2m is disabled.

!!!DO NOT MERGE!!! This commit should be dropped before merging !!!DO NOT MERGE!!! Test only P-256 and in the `-text` format only: this is to highlight the current failure with ec_param_enc:explicit.

romen · 2020-06-07T19:05:32Z

I need @levitte's feedback on how to address the issue.

Currently we have this

openssl/crypto/evp/pmeth_lib.c

Lines 1000 to 1015 in da56060

    
               /* Special cases that we intercept */ 
        
           # ifndef OPENSSL_NO_EC 
        
               /* 
        
                * We don't support encoding settings for providers, i.e. the only 
        
                * possible encoding is "named_curve", so we simply fail when something 
        
                * else is given, and otherwise just pretend all is fine. 
        
                */ 
        
               if (strcmp(name, "ec_param_enc") == 0) { 
        
                   if (strcmp(value, "named_curve") == 0) { 
        
                       return 1; 
        
                   } else { 
        
                       ERR_raise(ERR_LIB_EVP, EVP_R_COMMAND_NOT_SUPPORTED); 
        
                       return -2; 
        
                   } 
        
               } 
        
           # endif

but this was from before further developments for serializers.

If we don't provide a way for the framework to keep track of encoding settings, what are the alternatives?

modify apps to downgrade the key to a legacy key and use deprecated functions?
accept the breaking change and alter the documentation saying that ec_param_enc in OpenSSL 3.0 is not supported anymore?

romen · 2020-06-07T19:19:52Z

I am not very familiar with the serializer code. But this is another tentative plan for a solution.

Is serializing a separate operation that can have its own ctx?
If yes, could this be solved by passing both pkeyopts to both the keygen and the serialize ctx?
- keygen could then use ec_paramgen_curve and ignore ec_param_enc
- serialize could then use ec_param_enc:explicit (or ignore it if it does not support it) and ignore ec_paramgen_curve
- apps/genpkey could detect if ec_param_enc has been passed and barf an error if the provider of serialize does not support it

This reverts commit da56060.

Make SM2 curve testing conditional to sm2 feature support. (cherry picked from commit 321ebf3)

Add more aliases (cherry picked from commit 5c87108)

Add extra negative test with unknown curve (cherry picked from commit e52dee3)

!!!DO NOT MERGE!!! This commit should be dropped before merging !!!DO NOT MERGE!!! Test only P-256 and in the `-text` format only: this is to highlight the current failure with ec_param_enc:explicit.

romen · 2020-06-08T10:39:56Z

The last set of commits cherry-picks the additional tests (and a fix) included in #12085 (the 1.1.1 counterpart of this PR).

The very last commit disables anyway most of these tests to keep only P-256 in the list of tested curves, as every curve is currently failing in master upon ec_param_enc:explicit (and the extra curves are just cluttering the test output with no extra benefit).

levitte · 2020-06-09T10:38:48Z

@romen, #12097 contains stuff to mark up certain tests as TODOs and let the recipe pass

This reverts commit baf8fa8.

There currently do not support 'ec_param_enc:explicit' with provider side key generation. Reflect that by encoding the expected failure with a Test::More TODO section for those particular tests. Because the tests in this recipe are data driven, we implement this mechanism with two functions, one for stuff that's supported and one for stuff that isn't.

I just rearranged the code with the subroutines to be at the beginning, it might be just a cosmetic thing, but it really makes a difference for me (with basically no knowledge of Perl) in readability to know in advance the meaning of the subroutines and how they are used, so I can better understand what is happening in the foreach loops.

romen · 2020-06-09T15:21:33Z

@romen, #12097 contains stuff to mark up certain tests as TODOs and let the recipe pass

@levitte I cherry-picked your amendment in this PR, and added a small change to move the subroutines towards the beginning of the file (as it seems more readable to me).

Can you find in the documentation if the TODO annotation in Test::More is supported in all the Perl versions we care about?

levitte · 2020-06-09T15:49:25Z

I looked through early Test::More changelog, and it shows that the TODO feature has been around for a long time (12 years back at least)

levitte

Lambdas are a thing 😉

levitte · 2020-06-09T15:55:21Z

test/recipes/15-test_genec.t

+     'named_curve'      => \&supported,
+     'explicit'         => \&unsupported


Another way would be this:

Suggested change

'named_curve' => \&supported,

'explicit' => \&unsupported

'named_curve' => sub { my $str = shift;

ok(run(@_), $str); },

'explicit' => sub { my $str = shift;

TODO: { local $TODO = "Currently not supported";

ok(run(@_), $str); }

},

That would get rid of the function definitions higher up.

I think the current approach is fine, using \&<name>.

What I find a bit unreadable of having the subroutines at the end is that when you first encounter \&supported or \&unsupported reading from top to bottom, there is very little context to understand what is going on.

Having them first, with the comment describing what is happening, makes it easier to grasp what is going on in the associative arrays and the loops even for Perl-scared people like me!

romen · 2020-06-09T17:21:06Z

I took this out of WIP:

title edited
draft status removed in GitHub
description updated
separate issue to track the defect created ([3.0.0] genpkey -algorithm EC -pkeyopt ec_param_enc:explicit fails #12102)

romen · 2020-06-12T12:28:32Z

@levitte would you be willing to review this?

romen · 2020-06-16T09:18:56Z

@levitte would this require further changes?
Do you prefer the lambda option to the current one?

romen · 2020-06-21T15:15:22Z

Ping @levitte

levitte · 2020-06-22T05:33:42Z

Why was this added to the "New Core + FIPS" project?

romen · 2020-06-22T06:31:48Z

Why was this added to the "New Core + FIPS" project?

It's adding tests for something we broke during the development of the new core+providers architecture.
It's not enough to qualify?

levitte · 2020-06-22T10:51:48Z

It's adding tests for something we broke during the development of the new core+providers architecture.
It's not enough to qualify?

Not in my opinion. It's affected by new core / fips work, but itself doesn't touch anything that's distinctly new core / fips.

openssl-machine · 2020-06-23T06:00:17Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

This commit adds a new recipe to test EC key generation with the `genpkey` CLI app. For each built-in curve, it tests key generation with text output, in PEM and in DER format, using `explicit` and `named_curve` for parameters encoding. The list of built-in curves is static at the moment, as this allows to differentiate between prime curves and binary curves to avoid failing when ec2m is disabled. Reviewed-by: Richard Levitte <[email protected]> (Merged from #12080)

There currently do not support 'ec_param_enc:explicit' with provider side key generation. Reflect that by encoding the expected failure with a Test::More TODO section for those particular tests. Because the tests in this recipe are data driven, we implement this mechanism with two functions, one for stuff that's supported and one for stuff that isn't. Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12080)

romen · 2020-06-26T10:59:33Z

Merged to master with:

0c2bddb Test genpkey app for EC keygen with various args
c65b1d0 TEST: Add TODO segments in test/recipes/15-test_genec.t

Thanks!

romen added 2 commits June 7, 2020 21:22

drop! disable most new tests

da56060

!!!DO NOT MERGE!!! This commit should be dropped before merging !!!DO NOT MERGE!!! Test only P-256 and in the `-text` format only: this is to highlight the current failure with ec_param_enc:explicit.

romen added the branch: master Applies to master branch label Jun 7, 2020

romen added this to the 3.0.0 milestone Jun 7, 2020

romen self-assigned this Jun 7, 2020

romen marked this pull request as draft June 7, 2020 18:35

romen force-pushed the issues/3.0.0/ec_explicit_params_encoding branch from d211135 to da56060 Compare June 7, 2020 18:39

romen mentioned this pull request Jun 8, 2020

[1.1.1] Test genpkey app for EC keygen with various args #12085

Closed

2 tasks

romen added 5 commits June 8, 2020 13:23

drop! Revert "drop! disable most new tests"

029bdd6

This reverts commit da56060.

fixup! Test genpkey app for EC keygen with various args

fa81168

Make SM2 curve testing conditional to sm2 feature support. (cherry picked from commit 321ebf3)

fixup! Test genpkey app for EC keygen with various args

56dcaf0

Add more aliases (cherry picked from commit 5c87108)

fixup! Test genpkey app for EC keygen with various args

47e8e19

Add extra negative test with unknown curve (cherry picked from commit e52dee3)

drop! disable most new tests

baf8fa8

!!!DO NOT MERGE!!! This commit should be dropped before merging !!!DO NOT MERGE!!! Test only P-256 and in the `-text` format only: this is to highlight the current failure with ec_param_enc:explicit.

romen and others added 3 commits June 9, 2020 18:05

drop! Revert "drop! disable most new tests"

bdcc371

This reverts commit baf8fa8.

romen mentioned this pull request Jun 9, 2020

Amendment to #12080 #12097

Closed

levitte reviewed Jun 9, 2020

View reviewed changes

romen mentioned this pull request Jun 9, 2020

[3.0.0] genpkey -algorithm EC -pkeyopt ec_param_enc:explicit fails #12102

Closed

romen changed the title ~~[WIP] Test genpkey app for EC keygen with various args~~ Test genpkey app for EC keygen with various args Jun 9, 2020

romen marked this pull request as ready for review June 9, 2020 17:19

romen requested a review from levitte June 9, 2020 17:21

romen added the approval: review pending This pull request needs review by a committer label Jun 10, 2020

levitte approved these changes Jun 22, 2020

View reviewed changes

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 Jun 22, 2020

levitte 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 23, 2020

romen closed this Jun 26, 2020

romen linked an issue Jun 26, 2020 that may be closed by this pull request

[3.0.0] genpkey -algorithm EC -pkeyopt ec_param_enc:explicit fails #12102

Closed

romen mentioned this pull request Jun 28, 2020

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

Closed

1 task

-     'named_curve'      => \&supported,
-     'explicit'         => \&unsupported
+     'named_curve'      => sub { my $str = shift;
+                                 ok(run(@_), $str); },
+     'explicit'         => sub { my $str = shift;
+                                 TODO: { local $TODO = "Currently not supported";
+                                         ok(run(@_), $str); }
+                               },

Uh oh!

Conversation

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

Uh oh!

Issue

TODO: {} block

Credits

Checklist

Uh oh!

romen commented Jun 7, 2020

Uh oh!

romen commented Jun 7, 2020

Uh oh!

romen commented Jun 8, 2020

Uh oh!

levitte commented Jun 9, 2020

Uh oh!

romen commented Jun 9, 2020

Uh oh!

levitte commented Jun 9, 2020

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

levitte Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

levitte Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

romen Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

romen commented Jun 9, 2020

Uh oh!

romen commented Jun 12, 2020

Uh oh!

romen commented Jun 16, 2020

Uh oh!

romen commented Jun 21, 2020

Uh oh!

levitte commented Jun 22, 2020

Uh oh!

romen commented Jun 22, 2020

Uh oh!

levitte commented Jun 22, 2020

Uh oh!

openssl-machine commented Jun 23, 2020

Uh oh!

romen commented Jun 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

romen commented Jun 7, 2020 •

edited

Loading

`TODO: {}` block