Skip to content
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

crypto keygen benchmark failing after OpenSSL 3.0 upgrade #40410

Closed
mscdex opened this issue Oct 11, 2021 · 1 comment · Fixed by #40416
Closed

crypto keygen benchmark failing after OpenSSL 3.0 upgrade #40410

mscdex opened this issue Oct 11, 2021 · 1 comment · Fixed by #40416
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@mscdex
Copy link
Contributor

mscdex commented Oct 11, 2021

Version

master

Platform

Linux foo 5.4.0-58-generic #64~18.04.1-Ubuntu SMP Wed Dec 9 17:11:11 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

crypto

What steps will reproduce the bug?

Run the crypto/keygen benchmark.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

No response

Additional information

Stack trace:

node:internal/crypto/keygen:110
    throw err;
    ^

Error: error:05000072:dsa routines::bad ffc parameters
    at generateKeyPairSync (node:internal/crypto/keygen:101:63)
    at Object.dsaSync (/home/bench/node/benchmark/crypto/keygen.js:45:7)
    at main (/home/bench/node/benchmark/crypto/keygen.js:70:18)
    at /home/bench/node/benchmark/common.js:42:9
    at processTicksAndRejections (node:internal/process/task_queues:78:11)

I guess we should be putting some of these function calls from the benchmark into our existing crypto tests to catch things like this in the future?

/cc @nodejs/crypto

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Oct 11, 2021
mscdex added a commit to mscdex/io.js that referenced this issue Oct 11, 2021
OpenSSL 3.0 increased the minimum values for these parameters.

Fixes: nodejs#40410
@richardlau
Copy link
Member

I guess we should be putting some of these function calls from the benchmark into our existing crypto tests to catch things like this in the future?

I thought our normal CI runs did a basic check that our benchmarks worked? e.g. I would have expected https://github.com/nodejs/node/blob/master/test/benchmark/test-benchmark-crypto.js to flag this... unless we're running them without going once through the loop (n==0?)?

mscdex added a commit to mscdex/io.js that referenced this issue Oct 16, 2021
OpenSSL 3.0 increased the minimum values for these parameters.

Fixes: nodejs#40410
@mscdex mscdex closed this as completed in ed01811 Oct 16, 2021
targos pushed a commit that referenced this issue Nov 4, 2021
OpenSSL 3.0 increased the minimum values for these parameters.

PR-URL: #40416
Fixes: #40410
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants