-
Notifications
You must be signed in to change notification settings - Fork 74
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
Adding Locust as performance OQS tool #304
Conversation
Thanks for the PR, @davidgca! Would you be willing to list yourself as a maintainer of this new demo in the README? Also, please feel free to add yourself to the list of contributors in the same file :) |
I assume you say adding directly to README for OQS Demo? I also add a new line for the new tool in the table |
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.
Hi @davidgca, thanks for the updates. I left a few style-related comments/questions.
I also followed the instructions in locust/README.md but got a 100% error rate. Not sure if this is expected?
|
Not at all, 100% error not sounds good!. The first step (and most important) is build the docker image: |
Signed-off-by: davidgca <[email protected]>
Signed-off-by: davidgca <[email protected]>
Signed-off-by: davidgca <[email protected]>
Signed-off-by: davidgca <[email protected]>
Signed-off-by: davidgca <[email protected]>
Signed-off-by: davidgca <[email protected]>
Hi @SWilson4 , I have signed-off all commits, do we need something else? Have you checked if all Readme steps are working? |
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.
I'm currently working on moving all the demos to pinning on the latest version (and double checking arm/amd support) in #298 It would help a bunch if you could make the same changes here (otherwise I'll just make the updates once this is merged). To help I noted them individually below:
Thanks for taking a look at this. I will incorporate all these suggestions into a single commit. |
…nges Signed-off-by: davidgca <[email protected]>
Signed-off-by: davidgca <[email protected]>
Hi @SWilson4 and @ajbozarth, could we go ahead and merge this PR? Or is there anything you think might still be missing?" |
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 for the integration @davidgca ! Please see my single comments. Please seriously consider changing the term curve to group. If there were interest to put this on docker hub (?) so users don't have to build this but can just "get going", please also add the corresponding CI build job.
@SWilson4 @ajbozarth please consider pulling such remote origin PR into the OQS org such as to enable CI to run. In this case, possibly not necessary as there is no (new) test. I personally do not find merging new (and maintained!) code without test (build & exec) good as that is not (automatically) ascertaining the continued quality/validity of code, though. Please all together also consider whether you'd want (a successful build) residing on docker hub and if so, move USAGE.md over after the first successful push. |
Signed-off-by: davidgca <[email protected]>
As I mentioned before, I've already modified this and refactored it by changing 'curve' to 'group.' Thanks a lot for this comment! I actually had some doubts about considering Kyber a curve, as my confusion came from OpenSSL using the 'curve' option for any post-quantum scheme |
Thanks for these updates, @davidgca !
That should not be the case any more... Can I ask where we do that? Direct links would be best such as to know where to change. Thanks in advance! |
sure, here, As you cans see, we are using |
Well, that's your choice, isn't it? In OpenSSL this command line option is kept for backwards compatibility with TLS1.2; for TLS1.3 (which is the only spec supporting PQC) the term (and option to use) is "-groups" (https://docs.openssl.org/master/man3/SSL_CONF_cmd/#supported-command-line-commands btw lists "-curves" only as a synonym ... but looking at the documentation I think it would be prudent to add verbiage that providers can extend the list given there; thus, thanks for giving me reason to check @davidgca). |
Signed-off-by: davidgca <[email protected]>
Thanks! In my initial research, I saw the curves option and didn't look into the groups option. I've tested it successfully and made the change here |
Signed-off-by: davidgca <[email protected]>
Ping @SWilson4 @ajbozarth : Objections to merge? |
That's to be expected; our CI run unfortunately fails unless you have sufficient perms to access repository secrets. I believe @ajbozarth will be looking at a fix for this in the near future. |
That is correct, it is one of the pain points I most was to address in my upcoming CI work.
IIUC I believe you can input the expected secrets in GitHub Actions for your own fork and it should work if you really want to |
Merged. Thanks for the contribution @davidgca ! |
Purpose:
The primary goal of this contribution is to enable Locust to perform quantum-safe load testing scenarios, utilizing quantum-safe cryptographic algorithms in TLS 1.3. This is achieved by building a Docker image that incorporates the OQS provider into the OpenSSL library used by Locust.
Key Changes:
An example: