(3.x) Known result tests for EC compression compatibility validation + #16624 changes #18320

hlandau · 2022-05-16T12:34:04Z

Follow-on from #18083.

These tests enforce 1.1 behaviour so they will naturally fail. I trust these results will be of interest.

t8m · 2022-05-16T13:34:03Z

Now, could we rebase #16624 on top of this?

…o UNCOMPRESSED Original patch by Nicola Tuveri.

hlandau · 2022-05-17T19:24:18Z

PR updated to incorporate Nicola's changes. These don't in themselves appear to be enough to have the tests pass. @romen

t8m · 2022-05-18T05:54:27Z

The SM2 curve test failure is expected and correct. We no longer support SM2 curve with EC in 3.x. So that part of the tests needs to be dropped.

Drops SM2.

hlandau · 2022-05-18T08:26:41Z

Updated to drop SM2.

openssl-machine · 2022-06-18T00:07:34Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

paulidale

I sort of wonder about the need for 1100 test files but more tests is better generally.

paulidale · 2022-06-20T03:58:11Z

test/evp_pkey_provided_test.c

@@ -1209,9 +1216,17 @@ static int test_fromdata_ec(void)
    if (OSSL_PARAM_BLD_push_utf8_string(bld, OSSL_PKEY_PARAM_GROUP_NAME,
                                        curve, 0) <= 0)
        goto err;
+    /*
+     * We intentionally provide the input point in compressed format,
+     * and avoid to set `OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT`.


and avoid setting ...

paulidale · 2022-06-20T03:58:39Z

test/evp_pkey_provided_test.c

-            || !TEST_true(out_pub[0] == (POINT_CONVERSION_COMPRESSED + 1))
+
+            /*
+             * Our providers use uncompressed format as default if


use uncompressed format by default if

paulidale · 2022-06-20T04:01:41Z

test/recipes/15-test_ec_comp.t

+}
+
+SKIP: {
+    skip "EC is not supported by this OpenSSL build", scalar @curves if disabled("ec");


Might a plan skip_all up at the plan be clearer?

paulidale · 2022-06-20T04:02:32Z

test/ec_comp_test.c

+    size_t i;
+
+    num_known_curves = EC_get_builtin_curves(NULL, 0);
+    known_curves = OPENSSL_malloc(num_known_curves*sizeof(*known_curves));


space around the multiply

paulidale · 2022-06-20T04:02:49Z

test/ec_comp_test.c

+    if (known_curve_names == NULL)
+        goto fail;
+
+    for (i=0; i<num_known_curves; ++i) {


spaces around the equals and less than.

paulidale · 2022-06-20T04:04:10Z

test/ec_comp_test.c

+    BIO *b = NULL;
+    char filename[256];
+
+    OPENSSL_assert(param_formats_i[param_format_i] >= 0);


TEST_xxx instead of asserts perhaps?

paulidale · 2022-06-20T04:05:10Z

test/ec_comp_test.c

+     * serialization and deserialization.
+     */
+    /* For each supported compression format (and unspecified) */
+    for (ci=0; ci<OSSL_NELEM(comp_formats_i); ++ci)


Spaces around operators here and elsewhere.

openssl-machine · 2022-07-21T00:07:46Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

jogme · 2022-10-06T07:39:40Z

@hlandau Hi, do you have any update on this? Do you need any help with it? It is open for quite a long time and I would like to have the changes.

slontis

This seems like overkill to me. @romen Is there a subset of this that would be sufficient for coverage?

romen · 2022-10-06T09:26:05Z

This seems like overkill to me. @romen Is there a subset of this that would be sufficient for coverage?

Well, that's for OTC to say: so far I only indicated what was asked by the OTC before evaluating the merge of #16624.

Looking at my notes, the OTC ask was to:

test both programmatically (i.e., via EVP_PKEY_* and EC_*) and through the relevant CLI apps, what are all the possible outcomes in 1.1.1

ensure 1.1.1 behavior is matched in 3.0, both for functions and apps.

Tests should cover both newly generated keys and keys parsed from input, across all curves and encoding combinations (i.e., [compressed, uncompressed, hybrid] * [named_curve, custom_curve, named_but_with_explicit_params])

hlandau · 2022-10-06T09:28:04Z

As far as I recall this PR revealed that the behaviour does not match that of 1.1.1. So revision to #16624 would be needed.

romen · 2022-10-06T09:38:25Z

As far as I recall this PR revealed that the behaviour does not match that of 1.1.1. So revision to #16624 would be needed.

This PR already includes the changes from #16624 AFAICT.

IIRC, by asking the thorough testing mentioned before, OTC basically wanted to see 3 things before deciding:

diff of testing results between 1.1.1 and 3.0, before PR#16624 (to justify PR#16624 as a bug fix)
diff of testing results between 1.1.1 and 3.0, after PR#16624 (to confirm PR#16624 is a complete fix)
incorporate the developed tests in 3.0, to ensure we detect changes on this matter in the future, as it can be subtle

hlandau · 2022-10-06T09:42:18Z

Yes, this PR includes #16624, however the tests fail, demonstrating that the behaviour is not the same between 1.1.1 and 3.0+#16624. So unless I'm mistaken, #16624 needs to be revised so that it creates the same behaviour as 1.1.1, or the OTC needs to decide what we want to do instead.

romen · 2022-10-06T09:49:06Z

Closing and reopening to trigger the tests again, and see what fails

romen · 2022-10-06T09:59:57Z

Also, looking at the last commit, I think that while dropping SM2 from testing for 3.0 makes some sense, as it is handled differently, the fact that it shares the same encoding format as EC keys means that OTC should take an explicit decision about what to do with it.

Is it fine to just drop SM2 from these tests?
Do we need to compensate with tests that ensure parsing SM2 keys works according to expectations in 3.0 (and that these expectations are slightly different than 1.1.1?)

beldmit · 2022-10-06T10:59:05Z

@InfoHunter could you please tell us anything about SM2?

Jakuje · 2022-12-21T16:35:18Z

If I am right, this was merged as #19681 and there is #19901 for the backport to 3.0 branch. Can we get this closed/updated as resolved?

hlandau · 2023-08-01T09:04:47Z

Closing as non-current; can be re-opened if this is revisited

Known result tests for EC compression compatibility validation

7cb2237

hlandau added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels May 16, 2022

hlandau requested review from romen and mattcaswell May 16, 2022 12:34

hlandau self-assigned this May 16, 2022

hlandau mentioned this pull request May 16, 2022

Known result tests for EC compression compatibility validation #18083

Closed

Honor OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT as set and default t…

f70cfac

…o UNCOMPRESSED Original patch by Nicola Tuveri.

hlandau changed the title ~~(3.x) Known result tests for EC compression compatibility validation~~ (3.x) Known result tests for EC compression compatibility validation + #16624 changes May 17, 2022

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 17, 2022

Adjustments to EC compression compatibility tests for 3.0

036f911

Drops SM2.

faramir-dev force-pushed the master branch from 36c269c to 3c0e8bc Compare May 24, 2022 04:50

paulidale reviewed Jun 20, 2022

View reviewed changes

t8m removed approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Jul 21, 2022

slontis reviewed Oct 6, 2022

View reviewed changes

romen closed this Oct 6, 2022

romen reopened this Oct 6, 2022

romen mentioned this pull request Oct 6, 2022

Honor OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT as set and default to UNCOMPRESSED #16624

Closed

2 tasks

romen mentioned this pull request Oct 9, 2022

[3.0] Known result tests for EC compression compatibility validation + #16624 changes #19365

Draft

This was referenced Nov 15, 2022

[3.1] Honor OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT as set and default to UNCOMPRESSED #19681

Closed

Vote: Accept PR #19681 in 3.1 subject to normal review process openssl/technical-policies#59

Closed

hlandau closed this Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(3.x) Known result tests for EC compression compatibility validation + #16624 changes #18320

(3.x) Known result tests for EC compression compatibility validation + #16624 changes #18320

hlandau commented May 16, 2022

t8m commented May 16, 2022

hlandau commented May 17, 2022

t8m commented May 18, 2022

hlandau commented May 18, 2022

openssl-machine commented Jun 18, 2022

paulidale left a comment

paulidale Jun 20, 2022

paulidale Jun 20, 2022

paulidale Jun 20, 2022

paulidale Jun 20, 2022

paulidale Jun 20, 2022

paulidale Jun 20, 2022

paulidale Jun 20, 2022

openssl-machine commented Jul 21, 2022

jogme commented Oct 6, 2022

slontis left a comment

romen commented Oct 6, 2022

hlandau commented Oct 6, 2022

romen commented Oct 6, 2022 •

edited

hlandau commented Oct 6, 2022

romen commented Oct 6, 2022

romen commented Oct 6, 2022

beldmit commented Oct 6, 2022

Jakuje commented Dec 21, 2022

hlandau commented Aug 1, 2023

(3.x) Known result tests for EC compression compatibility validation + #16624 changes #18320

(3.x) Known result tests for EC compression compatibility validation + #16624 changes #18320

Conversation

hlandau commented May 16, 2022

t8m commented May 16, 2022

hlandau commented May 17, 2022

t8m commented May 18, 2022

hlandau commented May 18, 2022

openssl-machine commented Jun 18, 2022

paulidale left a comment

Choose a reason for hiding this comment

paulidale Jun 20, 2022

Choose a reason for hiding this comment

paulidale Jun 20, 2022

Choose a reason for hiding this comment

paulidale Jun 20, 2022

Choose a reason for hiding this comment

paulidale Jun 20, 2022

Choose a reason for hiding this comment

paulidale Jun 20, 2022

Choose a reason for hiding this comment

paulidale Jun 20, 2022

Choose a reason for hiding this comment

paulidale Jun 20, 2022

Choose a reason for hiding this comment

openssl-machine commented Jul 21, 2022

jogme commented Oct 6, 2022

slontis left a comment

Choose a reason for hiding this comment

romen commented Oct 6, 2022

hlandau commented Oct 6, 2022

romen commented Oct 6, 2022 • edited

hlandau commented Oct 6, 2022

romen commented Oct 6, 2022

romen commented Oct 6, 2022

beldmit commented Oct 6, 2022

Jakuje commented Dec 21, 2022

hlandau commented Aug 1, 2023

romen commented Oct 6, 2022 •

edited