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.

Sign up for GitHub

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

Jump to bottom

[v18.x backport] test: update TLS tests for OpenSSL 3.2 #55101

Merged
merged 21 commits into from
Sep 27, 2024
Merged

[v18.x backport] test: update TLS tests for OpenSSL 3.2 #55101

merged 21 commits into from
Sep 27, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 24, 2024 •
edited
Loading

We should add #55089 as well. EDIT: that's done!

The only test that's still failing for me is test-tls-multi-key (on v20.x and v18.x) with the following stack:

    node:events:495
          throw er; // Unhandled 'error' event
          ^
    
    Error: certificate signature failure
        at TLSSocket.onConnectSecure (node:_tls_wrap:1659:34)
        at TLSSocket.emit (node:events:517:28)
        at TLSSocket._finishInit (node:_tls_wrap:1070:8)
        at ssl.onhandshakedone (node:_tls_wrap:856:12)
    Emitted 'error' event on TLSSocket instance at:
        at emitErrorNT (node:internal/streams/destroy:151:8)
        at emitErrorCloseNT (node:internal/streams/destroy:116:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
      code: 'CERT_SIGNATURE_FAILURE'
    }

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Sep 24, 2024
@richardlau
Copy link
Member

Looks like a21d087 and be7bcba trigger no-extra-semi lint failures on v18.x-staging.

@richardlau
Copy link
Member

The only test that's still failing for me is test-tls-multi-key (on v20.x and v18.x) with the following stack:

v18.x-staging and main have the same commits for test/parallel/test-tls-multi-key.js, so presumably the fixtures being loaded by the test must differ.

@aduh95 aduh95 mentioned this pull request Sep 24, 2024
Merged
@richardlau
Copy link
Member

The only test that's still failing for me is test-tls-multi-key (on v20.x and v18.x) with the following stack:

For me, with v18.x-staging, test-tls-multi-key fails with OpenSSL 3.2.3 but passes with 01f751b cherry-picked (which looks to be included in this PR). 🤔

@richardlau
Copy link
Member

richardlau commented Sep 26, 2024 •
edited
Loading

https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/66/nodes=ubuntu2204_sharedlibs_openssl32_x64/ passed with the branch for this PR.

(except for flaky js-native-api/test_cannot_run_js -- I'll see if #51898 cherry-picks cleanly to v18.x-staging)

@richardlau
Copy link
Member

(except for flaky js-native-api/test_cannot_run_js -- I'll see if #51898 cherry-picks cleanly to v18.x-staging)

It does cherry-pick cleanly -- I've pushed 627d399 up to v18.x-staging.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62785/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62794/

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 26, 2024

In the end, there were no conflicts to solve (just a few lint errors to address). Should I close this PR and mark the other PRs with lts-watch-v18.x PRs that may need to be released in v18.x. ? Or is it still useful to use that backport PR?

richardlau and others added 13 commits September 27, 2024 14:05
@richardlau
src: fix dynamically linked OpenSSL version
6615fe5 
Report the version of OpenSSL that Node.js is running with instead
of the version of OpenSSL that Node.js was compiled against.

PR-URL: nodejs#53456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@richardlau
test: update TLS tests for OpenSSL 3.2
fc43c68 
Update the following TLS tests to account for error code changes
in OpenSSL 3.2 and later.
- `parallel/test-tls-empty-sni-context`
- `parallel/test-tls-psk-circuit`

PR-URL: nodejs#53384
Refs: nodejs#53382
Refs: openssl/openssl#19950
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@richardlau
test: check against run-time OpenSSL version
cc3cdf7 
Update `common.hasOpenSSL3*` to check against the run-time version of
OpenSSL instead of the version of OpenSSL that Node.js was compiled
against.

Add a generalized `common.hasOpenSSL()` so we do not need to keep adding
new checks for each new major/minor of OpenSSL.

PR-URL: nodejs#53456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@richardlau
test: update TLS trace tests for OpenSSL >= 3.2
1320fb9 
Update tests to allow for a slight change to the TLS trace messages
starting from OpenSSL 3.2.

Refs: openssl/openssl@45aac10
PR-URL: nodejs#53229
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
@sonsurim @richardlau
test: use assert.{s,deepS}trictEqual()
e9e3306 
Use `asset.strictEqual()` and `asset.deepStrictEqual()` in
`test/parallel/test-tls-set-sigalgs.js`.

PR-URL: nodejs#54208
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@mhdawson @richardlau
test: fix test-tls-client-auth test for OpenSSL32
a1f0c87 
Refs: nodejs#53382
Refs: nodejs#53384

Same change as in 53384 where OpenSSL32 returns a slightly
different error but for a different test.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54610
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@richardlau
test: update TLS test for OpenSSL 3.2
5316314 
Update `parallel/test-tls-set-sigalgs` to account for error code changes
in OpenSSL 3.2 and later.

PR-URL: nodejs#54612
Refs: nodejs#53384
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@mhdawson @richardlau
test: increase key size for ca2-cert.pem
d5b73e5 
Refs: nodejs#44498
Refs: nodejs#53382

Key sizes were increased to 2048 in PR 44498 including
the configuration file for the generation of ca2-cert.pem.
However, it seems like updating ca2-cert.pem and related files
themselves were missed as they were not updated in the PR and
the ca2-cert.pem reported as being associated with a 1024 bit key.
I believe that was the cause of some of the failures mentioned in
nodejs#53382 as OpenSSL 3.2
increased the default security level from 1 to 2 and that
would mean that certificates associated with keys of 1024 bits
would no longer be accepted.

This PR updates the key size for ca2-cert.pem. It was not
necessary to change the config, only run the generation for
the ca2-cert.pem and related files.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54599
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson @richardlau
test: fix test-tls-client-mindhsize for OpenSSL32
68156cb 
Refs: nodejs#53382

- OpenSSL32 has a minimum dh key size by 2048 by
  default.
- Create larter 3072 dh key needed for testing and
  adjust tests to use it for builds with OpenSSL32

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54739
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson @richardlau
test: fix test test-tls-dhe for OpenSSL32
c7de027 
Refs: nodejs#53382

- OpenSSL32 has a minimum dh key size by 2048 by default.
- Adjust test to use larger 3072 key instead of 1024
  when OpenSSL32 is present.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54903
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson @richardlau
test: adjust tls test for OpenSSL32
e999738 
Refs: nodejs#53382

Looks like test is forcing an error through bad data and
the error code we get is different for OpenSSL32. Adjust
test to cope with the variation across versions.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54909
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson @richardlau
test: adjust test-tls-junk-server for OpenSSL32
b097d85 
Refs: nodejs#53382

OpenSSL32 returns different error text. Looking through
the test it seems like the expected error text has been adjusted
for different OpenSSL versions in the past and what the test
is testing is not related to the error being returned.

Update test to allow for error returned by OpenSSL32

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54926
Refs: nodejs#53382
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@panva @richardlau
crypto: reject Ed25519/Ed448 in Sign/Verify prototypes
d244204 
fixes: nodejs#52097
PR-URL: nodejs#52340
Fixes: nodejs#52097
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
JLHwung and others added 8 commits September 27, 2024 14:05
@JLHwung @richardlau
crypto: reject dh,x25519,x448 in {Sign,Verify}Final
6e7274f 
Fixes: nodejs#53742
PR-URL: nodejs#53774
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@mhdawson @richardlau
test: update test to support OpenSSL32
75ff0cd 
Refs: nodejs#53382

This test fails on OpenSSL32 because it complains the key being
used is too short.

It seems to have been missed when the test suite was udpated to have
a Makefile to generate key material as the keys are hard coded in the
test as opposed to being read in from the fixtures/key directory.

Update the test to use keys/certs from the fixtures directory and
to remove newlines at the end of the key and cert to retain the
inteded test.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54968
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@mhdawson @richardlau
test: adjust key sizes to support OpenSSL32
37a2f7e 
Refs: nodejs#53382

This test fails on OpenSSL32 because it complains the key
being used is too short.

Adjust the key sizes so that they will pass on OpenSSL32 in
addition to other OpenSSL3 versions.

Since the keys are not public key related I don't think the
increase in key size will be too bad in terms of performance so
I've just increased versus guarding for OpenSSL32

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54972
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@mhdawson @richardlau
test: add asserts to validate test assumptions
341496a 
Refs: nodejs#54968
Refs: nodejs#53382

Add additional asserts as suggestd by Richard in:
nodejs#54968

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#54997
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@mhdawson @richardlau
test: adjust tls-set-ciphers for OpenSSL32
1a4d497 
Refs: nodejs#53382

The test failed as it was using AES128 which is not supported
in OpenSSL32 due to default security level and because
some error messages have changed.

Adjusted to use AES256 where it made sense and not run
tests on OpenSSL32 where test was specific to AES128.

Adjust to use the expected error messages based on version.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#55016
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson @richardlau
test: update tls test to support OpenSSL32
9824827 
Refs: nodejs#53382

OpenSSL32 does not support AES128 and DH 1024 to
update test to use newer algorithms.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#55030
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau
test: fix OpenSSL version checks
c8520ff 
As per the original pull request that introduced the OpenSSL version
check in `parallel/test-crypto-dh`:

```
Error message change is test-only and uses the right error message for
versions >=3.0.12 in 3.0.x and >= 3.1.4 in 3.1.x series.
```

Fix the check so that:
- The older message is expected for OpenSSL 3.1.0.
- The newer message is expected for OpenSSL from 3.1.4 (e.g. 3.2.x).

Refs: nodejs#50395
PR-URL: nodejs#53503
Refs: nodejs#53382
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@mhdawson @richardlau
test: fix test-tls-junk-closes-server
ac3a390 
Refs: nodejs#53382

TLS spec seems to indicate there should should be a response
sent when TLS handshake fails. See
https://datatracker.ietf.org/doc/html/rfc8446#page-85

When compiled with OpenSSL32 we see the
the following response '15 03 03 00 02 02 16' which
decodes as a fatal (0x02) TLS error alert number 22 (0x16).
which corresponds to TLS1_AD_RECORD_OVERFLOW which matches
the error we see if NODE_DEBUG is turned on once you get
through the define aliases.

If there is a response from the server the test used to
hang because the end event will not be emitted until after
the response is consumed. This PR fixes the test so
it consumes the response.

Some earlier OpenSSL versions did not seem to send a response but
the error handling seems to have been re-written/improved
in OpenSSL32.

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#55089
Refs: nodejs#52482
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@richardlau richardlau merged commit ac3a390 into nodejs:v18.x-staging Sep 27, 2024
21 checks passed
@richardlau
Copy link
Member

In the end, there were no conflicts to solve (just a few lint errors to address). Should I close this PR and mark the other PRs with lts-watch-v18.x PRs that may need to be released in v18.x. ? Or is it still useful to use that backport PR?

I was flitting between just cherry-picking the commits across (due to no conflicts to solve) vs landing the backport PR (minor lint issues) -- in the end I decided to just cherry pick. This has had the side effect of merging this PR, as I'd pushed to this PR to rebase it to pick up 627d399 and since the commit hashes lined up GH decided this PR was merged.

@aduh95 aduh95 deleted the openssl32-backports branch September 27, 2024 22:53
@aduh95 aduh95 mentioned this pull request Sep 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@aduh95 @nodejs-github-bot @richardlau @sonsurim @mhdawson @panva @JLHwung