Overhauled test suite.#180
Conversation
ssl/statem/extensions_srvr.c
Outdated
|
|
||
| /* Get our list of supported groups */ | ||
| tls1_get_supported_groups(s, &srvrgroups, &srvr_num_groups); | ||
| oqs_get_all_supported_groups(s, &srvrgroups, &srvr_num_groups); |
There was a problem hiding this comment.
So does this mean that none of the non-OQS curves will be in the list?
There was a problem hiding this comment.
I don't think so: Contrary to its name, this function indeed returns all (also real) curves. The function should rather be called tls1_get_server_supported_groups as it's creating a deviation from standard-OpenSSL functionality (to treat server and client side curve list identically).
What I find 'suboptimal' is that only one occurrence of the call to the original tls1_get_supported_groups is replaced in this file and not both. Why?
If I don't understand things wrong, this is another consequence of the decision to reduce the number of curves announced causing ever more changes (and this code now effectively tries to roll back this decision only for the server). Should we possibly revisit that decision instead? OR introduce a true OQS algorithm selection mechanism instead of "abusing" EC curves? I have the gut feeling that this might also be the reason for #179 (where a plain (EC) s_server seems to be run against an OQS/kyber512 client). It definitely was the reason for #172 and #173 ...
There was a problem hiding this comment.
Indeed it does return all possible curves, and I just named the function poorly.
The other instance of tls1_get_supported_groups in that file is in the tls_construct_stoc_supported_groups function, which I assume is used to advertise the server's supported groups to the client. I'm not sure I understand what you mean by "roll back": the server was advertising a reduced set of groups even before this change, and I did not seek to alter that behaviour.
The purpose of this change was for the server to obtain the shared group from a larger set of all the supported groups rather than the pared-down set returned by tls1_get_supported_groups (which is used to populate the ClientHello and ServerHello). This means that, for example, when the client specifies -curves frodo640aes, with this change, the server gets its shared groups from the larger set, and correctly settles on frodo640aes for the rest of the handshake. Prior to the change, the server would've gotten its supported groups from the smaller list returned by tls1_get_supported_groups, which would've resulted in an error since frodo640aes would not be on that list.
|
@xvzcf Have you tested your Python scripts to make sure that they actually report failures when they occur? If I recall correctly in the past we didn't always test that in our bash scripts, and failures went through unreported. |
|
The use of liboqs-formatted KEM names in openssl was a consequence of this suggestion and ultimately of not having the KEMs part of the EVP API (#59 ). So we might have to revisit that ... Re-enabling KEM speed testing using the openssl names would be sensible: #181. |
The KEM name change is not a hard requirement, it's just that we'd need to add the liboqs names to the templating script for the test suites to run the speed tests algorithm by algorithm (which is desirable since it can be parallelized). |
Understood. So, what's your preference? Parallelizing it with |
|
I also prefer the |
baentsch
left a comment
There was a problem hiding this comment.
OK then for me. I'll work on a PR to bring back speed with openssl KEM names.
The speed tests have been temporarily taken out the CI. Once,
openssl speedcan parse OQS key-exchanges in the form of the names given here, a parallelized version of the KEM and SIG tests can be added to a nightly build.