CI: sshserver: skip -m PEM for ed25519 key generation#21360
CI: sshserver: skip -m PEM for ed25519 key generation#21360herbenderbler wants to merge 1 commit into
Conversation
|
Analysis of PR #21360 at 2776218a: Test 671 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Generated by Testclutch |
herbenderbler
force-pushed
the
sshserver-ed25519-pem-fix
branch
from
4ffaebf to
e1c7c54
Compare
herbenderbler
changed the title
| needs: build-cache | ||
| runs-on: windows-2022 | ||
| timeout-minutes: 10 | ||
| timeout-minutes: 30 |
There was a problem hiding this comment.
Can you explain what is this intending to fix?
There was a problem hiding this comment.
It increases the GitHub Actions job time limit so Windows test-ci can finish. Previously, the whole job was often killed before the test step completed, which produced misleading non-TESTFAIL failures.
| @@ -407,13 +407,21 @@ sub logmsg { | |||
|
|
|||
| my @sshkeygenopt; | |||
| if(($sshid =~ /OpenSSH/) && ($sshvernum >= 560)) { | |||
There was a problem hiding this comment.
| if(($sshid =~ /OpenSSH/) && ($sshvernum >= 560)) { | |
| if(($sshid =~ /OpenSSH/) && ($sshvernum >= 560) && ($keyalgo ne 'ed25519')) { |
Perhaps this one-liner change would fix it in a simpler way?
There was a problem hiding this comment.
Thanks! I applied a simplified version of your suggestion. I think we still need a single if (($sshid =~ /OpenSSH/) && ($sshvernum >= 560)) block so CURL_TEST_SSH_KEY_FORMAT can set -m for any algorithm when the env is set. Putting && ($keyalgo ne 'ed25519') on that outer if would skip the whole block for ed25519 and would no longer apply an explicit format from the env.
Instead, default -m PEM is now only added in elsif ($keyalgo ne 'ed25519') after handling the env var, which is the same idea in a flatter form. Pushed in 9a388d0.
Is that accurate or am I wildly offbase?
There was a problem hiding this comment.
I don't see an actual reason to combine CURL_TEST_SSH_KEY_FORMAT with ed25519
now or in the foreseeable future in this context. The one-liner is fine IMO.
| test_setopt(pooh.curl, CURLOPT_XFERINFOFUNCTION, t670_xferinfo); | ||
| test_setopt(pooh.curl, CURLOPT_NOPROGRESS, 0L); | ||
| result = curl_easy_perform(pooh.curl); | ||
| } |
There was a problem hiding this comment.
I suggest addressing this unrelated issue in a separate PR.
- sshserver.pl: OpenSSH ssh-keygen cannot use -m PEM with ed25519; default PEM only for other algorithms, explicit CURL_TEST_SSH_KEY_FORMAT first (simplified elsif per review). - tests/libtest/lib670.c: use curl_multi_* for 670-series; reliable unpause. - .github/workflows/windows.yml: raise job and test-ci step timeouts.
herbenderbler
force-pushed
the
sshserver-ed25519-pem-fix
branch
from
9a388d0 to
ca61025
Compare
ssh-keygen option `-m` cannot be combined with `-t ed25519`. Reported-by: herbenderbler on github Fixes curl#21360 Follow-up to acda4ea curl#21223
3 participants
ssh-keygen does not support exporting ed25519 keys in PEM format. On Windows/MSYS this causes a hard failure ("Saving key ... failed: invalid format") while on macOS it silently ignores the flag and writes OpenSSH format anyway. Either way, passing -m PEM with ed25519 is wrong.
Only default to -m PEM for key algorithms that support it (rsa, ecdsa). Ed25519 keys now use the ssh-keygen default format unless CURL_TEST_SSH_KEY_FORMAT is explicitly set.
This fixes SSH server startup failures on Windows CI where CURL_TEST_SSH_KEYALGO is set to ed25519.
Follow-up to acda4ea
Ref: #21223