-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Known result tests for EC compression compatibility validation #18083
Known result tests for EC compression compatibility validation #18083
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hlandau, it's a very good start!
Here's a first round of comments for discussion.
test/recipes/15-test_ec_comp.t
Outdated
|
||
plan tests => 1; | ||
|
||
ok(run(test(["ec_comp_test"])), "running ec_comp_test"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should take care of the API usage.
next we should test the CLI apps: we should augment the recipe to also run all the equivalent combinations piping openssl ec
, openssl pkey
, etc. either deserializing from the files created with the generate
script you wrote or generating a new key/params on the fly.
It should be very similar to the loops written in the C test, but replacing function calls with the equivalent cmdline tool.
@romen Updated. Looking into CLI tests. |
@romen I don't think we can retrieve the compression type from the CLI. Am I missing something or can we not test this? |
EC_GROUP *g = NULL; | ||
|
||
int asn1_flag = param_formats_i[param_format_i]; | ||
int point_form = comp_formats_i[comp_format_i]; | ||
|
||
g = EC_GROUP_new_by_curve_name(known_curves[curve_i].nid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a request for changes yet:
@mattcaswell do you think we should increase test coverage here also by generating the key using EC_GROUP_new_curve_GFp()
or the GF2m
version?
IIRC parameters matching happens when decoding, but not when intentionally using the API to build an explicit parameters group.
} | ||
|
||
static EC_KEY *get_existing_key(size_t curve_i, size_t param_format_i, size_t comp_format_i, struct default_config_st *dcfg) | ||
static EC_KEY *get_existing_key(size_t curve_i, size_t param_format_i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use EVP_PKEY *
objects whenever possible
test/ec_comp_test.c
Outdated
if (obj_type == OBJ_TYPE_PARAMS) { | ||
if (PEM_read_bio_ECPKParameters(b, &g, NULL, NULL) == 0) | ||
goto fail; | ||
|
||
dcfg->asn1_flag = EC_GROUP_get_asn1_flag(EC_KEY_get0_group(k)); | ||
dcfg->point_form = EC_GROUP_get_point_conversion_form(EC_KEY_get0_group(k)); | ||
k = EC_KEY_new(); | ||
if (k == NULL) | ||
goto fail; | ||
|
||
if (param_formats_i[param_format_i] >= 0) | ||
EC_KEY_set_asn1_flag(k, param_formats_i[param_format_i]); | ||
if (comp_formats_i[comp_format_i] >= 0) | ||
EC_KEY_set_conv_form(k, comp_formats_i[comp_format_i]); | ||
if (EC_KEY_set_group(k, g) == 0) | ||
goto fail; | ||
|
||
BIO_free(b); | ||
fclose(f); | ||
return k; | ||
EC_GROUP_free(g); | ||
g = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use PEM_read_bio_Parameters
and gain an EVP_PKEY *
object from which we can retrieve an EC_GROUP
, without manually creating an EC_KEY
object.
Going through EVP will increase coverage while still ultimately testing the EC_KEY
codepaths.
test/ec_comp_test.c
Outdated
EC_GROUP_free(g); | ||
g = NULL; | ||
} else { | ||
if (PEM_read_bio_ECPrivateKey(b, &k, NULL, NULL) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, transitioning to EVP_PKEY
objects: we should use PEM_read_bio_PrivateKey()
return 0; | ||
} | ||
|
||
static EC_KEY *generate_new_key(size_t curve_i, size_t param_format_i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can uniform to use EVP_PKEY *
objects. For key generation, in 1.1.1 this needs to generate the EC_KEY
first and then use EVP_PKEY_set1_EC_KEY
, but when we will port these tests to 3.0 and later EC_KEY
will be deprecated (we will still need to test it as long as it is part of the public API) and additionally we could call EVP_PKEY_Q_keygen()
.
/* | ||
* When we put keys through serialization and then deserialization, they | ||
* always become uncompressed. (The same is not true for parameters, | ||
* however.) | ||
*/ | ||
new_comp_format = POINT_CONVERSION_UNCOMPRESSED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is surprising in 1.1.1: if we ask for compressed point serialization we should get the output in compressed format, and the decoding process should detect the input is in compressed form and retain that information.
@mattcaswell any idea what might be going on?
We could parse the output of
Compare: openssl asn1parse -dump -in secp521r1-explicit-uncompressed.priv
0:d=0 hl=4 l= 670 cons: SEQUENCE
4:d=1 hl=2 l= 1 prim: INTEGER :01
7:d=1 hl=2 l= 66 prim: OCTET STRING
0000 - 00 9f 3a be a0 e9 20 bd-e6 17 ca 98 61 a6 02 61 ..:... .....a..a
0010 - 77 35 ec e0 4b 58 77 cd-86 f2 3d c9 89 66 7e 5c w5..KXw...=..f~\
0020 - cf 89 b0 e2 0c 19 a2 57-dc b5 0a ee f7 46 64 53 .......W.....FdS
0030 - 6b a5 80 69 eb f7 86 1a-2f 84 6e 13 82 16 98 29 k..i..../.n....)
0040 - 12 a1 ..
75:d=1 hl=4 l= 455 cons: cont [ 0 ]
79:d=2 hl=4 l= 451 cons: SEQUENCE
83:d=3 hl=2 l= 1 prim: INTEGER :01
86:d=3 hl=2 l= 77 cons: SEQUENCE
88:d=4 hl=2 l= 7 prim: OBJECT :prime-field
97:d=4 hl=2 l= 66 prim: INTEGER :01FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
165:d=3 hl=3 l= 159 cons: SEQUENCE
168:d=4 hl=2 l= 66 prim: OCTET STRING
0000 - 01 ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0010 - ff ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0020 - ff ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0030 - ff ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0040 - ff fc ..
236:d=4 hl=2 l= 66 prim: OCTET STRING
0000 - 00 51 95 3e b9 61 8e 1c-9a 1f 92 9a 21 a0 b6 85 .Q.>.a......!...
0010 - 40 ee a2 da 72 5b 99 b3-15 f3 b8 b4 89 91 8e f1 @...r[..........
0020 - 09 e1 56 19 39 51 ec 7e-93 7b 16 52 c0 bd 3b b1 ..V.9Q.~.{.R..;.
0030 - bf 07 35 73 df 88 3d 2c-34 f1 ef 45 1f d4 6b 50 ..5s..=,4..E..kP
0040 - 3f 00 ?.
304:d=4 hl=2 l= 21 prim: BIT STRING
0000 - 00 d0 9e 88 00 29 1c b8-53 96 cc 67 17 39 32 84 .....)..S..g.92.
0010 - aa a0 da 64 ba ...d.
327:d=3 hl=3 l= 133 prim: OCTET STRING
0000 - 04 00 c6 85 8e 06 b7 04-04 e9 cd 9e 3e cb 66 23 ............>.f#
0010 - 95 b4 42 9c 64 81 39 05-3f b5 21 f8 28 af 60 6b ..B.d.9.?.!.(.`k
0020 - 4d 3d ba a1 4b 5e 77 ef-e7 59 28 fe 1d c1 27 a2 M=..K^w..Y(...'.
0030 - ff a8 de 33 48 b3 c1 85-6a 42 9b f9 7e 7e 31 c2 ...3H...jB..~~1.
0040 - e5 bd 66 01 18 39 29 6a-78 9a 3b c0 04 5c 8a 5f ..f..9)jx.;..\._
0050 - b4 2c 7d 1b d9 98 f5 44-49 57 9b 44 68 17 af bd .,}....DIW.Dh...
0060 - 17 27 3e 66 2c 97 ee 72-99 5e f4 26 40 c5 50 b9 .'>f,..r.^.&@.P.
0070 - 01 3f ad 07 61 35 3c 70-86 a2 72 c2 40 88 be 94 .?..a5<p..r.@...
0080 - 76 9f d1 66 50 v..fP
463:d=3 hl=2 l= 66 prim: INTEGER :01FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFA51868783BF2F966B7FCC0148F709A5D03BB5C9B8899C47AEBB6FB71E91386409
531:d=3 hl=2 l= 1 prim: INTEGER :01
534:d=1 hl=3 l= 137 cons: cont [ 1 ]
537:d=2 hl=3 l= 134 prim: BIT STRING
0000 - 00 04 01 01 18 09 1a ad-22 26 96 b4 51 20 f5 10 ........"&..Q ..
0010 - 8c 0b 9b c1 03 ad 0b 6a-19 9a 0a 13 05 78 85 9b .......j.....x..
0020 - e3 f7 b9 ca fb 4a f7 89-fb 41 54 49 12 36 31 46 .....J...ATI.61F
0030 - d3 e9 89 d2 25 5c aa fd-6c 81 99 9d a2 13 39 fd ....%\..l.....9.
0040 - 30 73 3a f1 01 35 64 31-1e 5d b1 5d 6c 37 7e bf 0s:..5d1.].]l7~.
0050 - 85 da 2a 31 12 36 4d b1-cc a4 2b d5 c2 62 54 d1 ..*1.6M...+..bT.
0060 - 60 d4 2e 96 46 e4 9f 0b-17 5c 93 a5 73 11 eb 61 `...F....\..s..a
0070 - 68 5d 50 44 a2 87 0d 70-0a fa ac af 33 4c dc 97 h]PD...p....3L..
0080 - 1d d6 93 a1 9c 68 .....h $ openssl asn1parse -dump -in secp521r1-named_curve-compressed.priv
0:d=0 hl=3 l= 152 cons: SEQUENCE
3:d=1 hl=2 l= 1 prim: INTEGER :01
6:d=1 hl=2 l= 66 prim: OCTET STRING
0000 - 01 8b 9c e9 20 d8 70 4a-9a b0 fe 28 82 10 7f ab .... .pJ...(....
0010 - 1c 45 0b d2 2c d7 c4 4f-dd 18 90 f5 48 d7 78 6a .E..,..O....H.xj
0020 - d7 b0 68 9b 0d 47 fa 97-9b 49 55 42 a7 37 38 ad ..h..G...IUB.78.
0030 - 2c c1 ee 35 12 ac c6 69-c5 41 fd eb 90 f8 89 13 ,..5...i.A......
0040 - f5 84 ..
74:d=1 hl=2 l= 7 cons: cont [ 0 ]
76:d=2 hl=2 l= 5 prim: OBJECT :secp521r1
83:d=1 hl=2 l= 70 cons: cont [ 1 ]
85:d=2 hl=2 l= 68 prim: BIT STRING
0000 - 00 02 01 12 af e1 2c 2b-83 c0 4d 7b 22 be f6 69 ......,+..M{"..i
0010 - b4 4e 1b 48 10 71 af 50-d1 5e 00 dd b0 38 8b 41 .N.H.q.P.^...8.A
0020 - 9e ce 8d e9 af d3 4f 62-20 e7 b1 9a 8f 5e b4 79 ......Ob ....^.y
0030 - 80 e9 d5 e7 0c dc 84 89-94 5a 3b aa 09 32 bd 59 .........Z;..2.Y
0040 - 73 49 bf 12 sI.. Notice that when you use explicit parameters, there are 2 points in the output (and both are affected by the chosen $ openssl asn1parse -i -dump -in secp521r1-explicit-hybrid.priv
0:d=0 hl=4 l= 670 cons: SEQUENCE
4:d=1 hl=2 l= 1 prim: INTEGER :01
7:d=1 hl=2 l= 66 prim: OCTET STRING
0000 - 00 9f 3a be a0 e9 20 bd-e6 17 ca 98 61 a6 02 61 ..:... .....a..a
0010 - 77 35 ec e0 4b 58 77 cd-86 f2 3d c9 89 66 7e 5c w5..KXw...=..f~\
0020 - cf 89 b0 e2 0c 19 a2 57-dc b5 0a ee f7 46 64 53 .......W.....FdS
0030 - 6b a5 80 69 eb f7 86 1a-2f 84 6e 13 82 16 98 29 k..i..../.n....)
0040 - 12 a1 ..
75:d=1 hl=4 l= 455 cons: cont [ 0 ]
79:d=2 hl=4 l= 451 cons: SEQUENCE
83:d=3 hl=2 l= 1 prim: INTEGER :01
86:d=3 hl=2 l= 77 cons: SEQUENCE
88:d=4 hl=2 l= 7 prim: OBJECT :prime-field
97:d=4 hl=2 l= 66 prim: INTEGER :01FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
165:d=3 hl=3 l= 159 cons: SEQUENCE
168:d=4 hl=2 l= 66 prim: OCTET STRING
0000 - 01 ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0010 - ff ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0020 - ff ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0030 - ff ff ff ff ff ff ff ff-ff ff ff ff ff ff ff ff ................
0040 - ff fc ..
236:d=4 hl=2 l= 66 prim: OCTET STRING
0000 - 00 51 95 3e b9 61 8e 1c-9a 1f 92 9a 21 a0 b6 85 .Q.>.a......!...
0010 - 40 ee a2 da 72 5b 99 b3-15 f3 b8 b4 89 91 8e f1 @...r[..........
0020 - 09 e1 56 19 39 51 ec 7e-93 7b 16 52 c0 bd 3b b1 ..V.9Q.~.{.R..;.
0030 - bf 07 35 73 df 88 3d 2c-34 f1 ef 45 1f d4 6b 50 ..5s..=,4..E..kP
0040 - 3f 00 ?.
304:d=4 hl=2 l= 21 prim: BIT STRING
0000 - 00 d0 9e 88 00 29 1c b8-53 96 cc 67 17 39 32 84 .....)..S..g.92.
0010 - aa a0 da 64 ba ...d.
327:d=3 hl=3 l= 133 prim: OCTET STRING
0000 - 06 00 c6 85 8e 06 b7 04-04 e9 cd 9e 3e cb 66 23 ............>.f#
0010 - 95 b4 42 9c 64 81 39 05-3f b5 21 f8 28 af 60 6b ..B.d.9.?.!.(.`k
0020 - 4d 3d ba a1 4b 5e 77 ef-e7 59 28 fe 1d c1 27 a2 M=..K^w..Y(...'.
0030 - ff a8 de 33 48 b3 c1 85-6a 42 9b f9 7e 7e 31 c2 ...3H...jB..~~1.
0040 - e5 bd 66 01 18 39 29 6a-78 9a 3b c0 04 5c 8a 5f ..f..9)jx.;..\._
0050 - b4 2c 7d 1b d9 98 f5 44-49 57 9b 44 68 17 af bd .,}....DIW.Dh...
0060 - 17 27 3e 66 2c 97 ee 72-99 5e f4 26 40 c5 50 b9 .'>f,..r.^.&@.P.
0070 - 01 3f ad 07 61 35 3c 70-86 a2 72 c2 40 88 be 94 .?..a5<p..r.@...
0080 - 76 9f d1 66 50 v..fP
463:d=3 hl=2 l= 66 prim: INTEGER :01FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFA51868783BF2F966B7FCC0148F709A5D03BB5C9B8899C47AEBB6FB71E91386409
531:d=3 hl=2 l= 1 prim: INTEGER :01
534:d=1 hl=3 l= 137 cons: cont [ 1 ]
537:d=2 hl=3 l= 134 prim: BIT STRING
0000 - 00 06 01 01 18 09 1a ad-22 26 96 b4 51 20 f5 10 ........"&..Q ..
0010 - 8c 0b 9b c1 03 ad 0b 6a-19 9a 0a 13 05 78 85 9b .......j.....x..
0020 - e3 f7 b9 ca fb 4a f7 89-fb 41 54 49 12 36 31 46 .....J...ATI.61F
0030 - d3 e9 89 d2 25 5c aa fd-6c 81 99 9d a2 13 39 fd ....%\..l.....9.
0040 - 30 73 3a f1 01 35 64 31-1e 5d b1 5d 6c 37 7e bf 0s:..5d1.].]l7~.
0050 - 85 da 2a 31 12 36 4d b1-cc a4 2b d5 c2 62 54 d1 ..*1.6M...+..bT.
0060 - 60 d4 2e 96 46 e4 9f 0b-17 5c 93 a5 73 11 eb 61 `...F....\..s..a
0070 - 68 5d 50 44 a2 87 0d 70-0a fa ac af 33 4c dc 97 h]PD...p....3L..
0080 - 1d d6 93 a1 9c 68 .....h
|
|
The Windows failure is a bit of a mystery to me as I can't replicate it under mingw. |
…ion compatibility validation
…ompression compatibility validation
Fixed Windows issue. This was a pain to debug... this was caused by using This is fixed by using BIO_new_file instead, which clears BIO_FLAGS_UPLINK before calling BIO_set_fd. |
…or EC compression compatibility validation
@romen CLI tests added. I've limited this to a small number of curves to avoid the tests taking excessive time. If you prefer all curves, I can do this, but it will be a long test. |
…tests for EC compression compatibility validation
…result tests for EC compression compatibility validation
@romen Tests pass, ready for review. |
I do not think this PR should be actually merged to 1.1.1 branch. However we should use the known results to test against the master and 3.0 branches and fix the problems in the code where we fail to match these known results. |
@hlandau What is the status on porting the tests to 3.0? My work is blocked on this, so if you need any help, please let me know, I will try to help you with it. |
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 92 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 123 days ago |
@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. |
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 92 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 123 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 154 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 185 days ago |
This PR is in a state where it requires action by @openssl/otc but the last update was 216 days ago |
Closing as non-current; can be re-opened if this is revisited |
Known answer tests for how OpenSSL 1.1 processes EC compression parameters. See #16624.
My idea of what is required here is a little vague, feedback welcomed.