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

Test failures with OpenSSL 3.2.2 #53382

Open
26 tasks done
richardlau opened this issue Jun 7, 2024 · 6 comments
Open
26 tasks done

Test failures with OpenSSL 3.2.2 #53382

richardlau opened this issue Jun 7, 2024 · 6 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. openssl Issues and PRs related to the OpenSSL dependency.

Comments

@richardlau
Copy link
Member

richardlau commented Jun 7, 2024

A number of Node.js tests fail when Node.js is compiled dynamically linked against OpenSSL 3.2 (tested with OpenSSL 3.2.2).
i.e.

./configure --shared-openssl
make -j 4 test-ci

Full tap results: https://gist.github.com/richardlau/ce642daf2ffd581755232a924f9f8f63

Failures:

In our Jenkins CI we currently test in node-test-commit-linux-containered Node.js dynamically linked against OpenSSL 3.0 and 3.1. Addressing the above test failures would be required before we can add testing against OpenSSL 3.2 to the CI.

@richardlau richardlau added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. openssl Issues and PRs related to the OpenSSL dependency. labels Jun 7, 2024
@nodejs nodejs deleted a comment Jun 7, 2024
nodejs-github-bot pushed a commit that referenced this issue Jun 9, 2024
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: #53384
Refs: #53382
Refs: openssl/openssl#19950
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@richardlau
Copy link
Member Author

FWIW I've added testing against OpenSSl 3.2 to my copy of node-test-commit-linux-containered: richardlau-node-test-commit-linux-containered

e.g. https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/nodes=ubuntu2204_sharedlibs_openssl32_x64/26/consoleFull which is showing the current test failures with main when linked against OpenSSL 3.2.2.

@richardlau
Copy link
Member Author

In terms of updating what is included in Node.js, for now we want to stay on the LTS version of OpenSSL (3.0) for as long as possible. OpenSSL have not yet decided/announced what the next LTS version of OpenSSL will be. Making sure Node.js can build and execute tests successfully on later OpenSSL versions should hopefully ease the migration when it is eventually time.

targos pushed a commit that referenced this issue Jun 20, 2024
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: #53384
Refs: #53382
Refs: openssl/openssl#19950
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@mhdawson
Copy link
Member

For those that want to help move it forward, you can run this job to confirm if you have fixed one of the tests - https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/

nodejs-github-bot pushed a commit that referenced this issue Jun 26, 2024
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: #50395
PR-URL: #53503
Refs: #53382
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 pushed a commit that referenced this issue Jul 12, 2024
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: #50395
PR-URL: #53503
Refs: #53382
Reviewed-By: Luigi Pinca <[email protected]>
aduh95 pushed a commit that referenced this issue Jul 16, 2024
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: #50395
PR-URL: #53503
Refs: #53382
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
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: #53384
Refs: #53382
Refs: openssl/openssl#19950
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
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: #53384
Refs: #53382
Refs: openssl/openssl#19950
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
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: #50395
PR-URL: #53503
Refs: #53382
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
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: #50395
PR-URL: #53503
Refs: #53382
Reviewed-By: Luigi Pinca <[email protected]>
mhdawson added a commit to mhdawson/io.js that referenced this issue Aug 27, 2024
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 <[email protected]>
@richardlau
Copy link
Member Author

richardlau commented Aug 28, 2024

OpenSSL 3.2 does document:

The default security level can be configured when OpenSSL is compiled by setting -DOPENSSL_TLS_SECURITY_LEVEL=level. If not set then 2 is used.

https://docs.openssl.org/3.2/man3/SSL_CTX_set_security_level/#notes

Compare this to earlier versions (3.1, 3.0) where the default seclevel is 1:

We don't set -DOPENSSL_TLS_SECURITY_LEVEL when compiling OpenSSL in Node.js in upstream so will inherit the default.

Since we've had other issues relating to key sizes – perhaps the right long term solution would be to provide an API (either public or just for tests) to get the seclevel from OpenSSL at runtime and then Node.js' test could be selective (i.e. match the size of keys being used to the seclevel being run at).

The changing default seclevel also highlights that while OpenSSL 3.x is ABI compatible, updating (e.g. 3.0 -> 3.2) may still cause observable changes (i.e. with the increased seclevel smaller keys are no longer allowed out of the box without dropping the seclevel down to what it was previously).

@richardlau
Copy link
Member Author

I ran the CI variant again on main: https://ci.nodejs.org/job/richardlau-node-test-commit-linux-containered/32/nodes=ubuntu2204_sharedlibs_openssl32_x64/

This is showing a new failure in parallel.test-tls-set-sigalgs:

  (node:2717679) [DEP0060] DeprecationWarning: The `util._extend` API is deprecated. Please use Object.assign() instead.
  (Use `node --trace-deprecation ...` to show where the warning was created)
  node:assert:126
    throw new AssertionError(obj);
    ^

  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected

  + 'ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE'
  - 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE'
      at /home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/parallel/test-tls-set-sigalgs.js:51:16
      at /home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/common/index.js:488:15
      at /home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/common/index.js:488:15
      at maybeCallback (/home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/fixtures/tls-connect.js:97:7)
      at TLSSocket.<anonymous> (/home/iojs/build/workspace/richardlau-node-test-commit-linux-containered/test/fixtures/tls-connect.js:73:13)
      at TLSSocket.emit (node:events:520:28)
      at emitErrorNT (node:internal/streams/destroy:170:8)
      at emitErrorCloseNT (node:internal/streams/destroy:129:3)
      at process.processTicksAndRejections (node:internal/process/task_queues:90:21) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: 'ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE',
    expected: 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE',
    operator: 'strictEqual'
  }

  Node.js v23.0.0-pre

This didn't fail before (see description) likely due to the changes from #54208 landing since the run for the description.

mhdawson added a commit to mhdawson/io.js that referenced this issue Aug 28, 2024
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 <[email protected]>
mhdawson added a commit to mhdawson/io.js that referenced this issue Aug 29, 2024
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 <[email protected]>
nodejs-github-bot pushed a commit that referenced this issue Aug 29, 2024
Refs: #44498
Refs: #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
#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 <[email protected]>
PR-URL: #54599
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 2, 2024
Refs: #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 <[email protected]>
PR-URL: #54909
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 2, 2024
Refs: #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 <[email protected]>
PR-URL: #54926
Refs: #53382
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Oct 2, 2024
Refs: #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 <[email protected]>
PR-URL: #54739
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 2, 2024
Refs: #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 <[email protected]>
PR-URL: #54903
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 2, 2024
Refs: #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 <[email protected]>
PR-URL: #54909
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 2, 2024
Refs: #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 <[email protected]>
PR-URL: #54926
Refs: #53382
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Oct 4, 2024
Refs: #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 <[email protected]>
PR-URL: #54968
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
targos pushed a commit that referenced this issue Oct 4, 2024
Refs: #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 <[email protected]>
PR-URL: #54972
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Oct 4, 2024
Refs: #54968
Refs: #53382

Add additional asserts as suggestd by Richard in:
#54968

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #54997
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Oct 4, 2024
Refs: #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 <[email protected]>
PR-URL: #55016
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 4, 2024
Refs: #53382

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

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #55030
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Oct 4, 2024
Refs: #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 <[email protected]>
PR-URL: #55089
Refs: #52482
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54599
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54610
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54739
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54903
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54909
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54926
Refs: nodejs#53382
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54968
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#54972
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
Refs: nodejs#54968
Refs: nodejs#53382

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

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: nodejs#54997
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#55016
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
Refs: nodejs#53382

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

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: nodejs#55030
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
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 <[email protected]>
PR-URL: nodejs#55089
Refs: nodejs#52482
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this issue Nov 16, 2024
Refs: #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 <[email protected]>
PR-URL: #54968
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
marco-ippolito pushed a commit that referenced this issue Nov 16, 2024
Refs: #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 <[email protected]>
PR-URL: #54972
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this issue Nov 16, 2024
Refs: #54968
Refs: #53382

Add additional asserts as suggestd by Richard in:
#54968

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #54997
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this issue Nov 16, 2024
Refs: #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 <[email protected]>
PR-URL: #55016
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this issue Nov 16, 2024
Refs: #53382

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

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #55030
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this issue Nov 16, 2024
Refs: #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 <[email protected]>
PR-URL: #55089
Refs: #52482
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

No branches or pull requests

2 participants