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

Pass node 18.x branch tests when compiled against shared openssl 3.2 #52482

Closed
kapouer opened this issue Apr 11, 2024 · 4 comments
Closed

Pass node 18.x branch tests when compiled against shared openssl 3.2 #52482

kapouer opened this issue Apr 11, 2024 · 4 comments
Labels
openssl Issues and PRs related to the OpenSSL dependency.

Comments

@kapouer
Copy link
Contributor

kapouer commented Apr 11, 2024

Version

18.20.1

Platform

linux debian/trixie

Subsystem

No response

What steps will reproduce the bug?

Compile node against shared openssl 3.2.1 on debian/trixie, run tests.

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

Always reproduce.

What is the expected behavior? Why is that the expected behavior?

Tests pass or fail in an expected way :)

What do you see instead?

One test hangs:
test/parallel/test-tls-junk-closes-server.js

TLS 3832: server emit tlsClientError: [Error: 00E8D1B9EE7F0000:error:0A0000C6:SSL routines:tls_get_more_records:packet length too long:../ssl/record/methods/tls_common.c:654:
] {
  library: 'SSL routines',
  reason: 'packet length too long',
  code: 'ERR_SSL_PACKET_LENGTH_TOO_LONG'
}
NET 3832: destroy
NET 3832: close
NET 3832: close handle
NET 3832: has server
NET 3832: SERVER _emitCloseIfDrained
NET 3832: SERVER handle? true   connections? 0
NET 3832: emit close
NET 3832: emit close
STREAM 3832: readableAddChunk <Buffer 15 03 03 00 02 02 16>
STREAM 3832: emitReadable true false
STREAM 3832: emitReadable null
STREAM 3832: emitReadable_ false 7 false
STREAM 3832: flow null
STREAM 3832: maybeReadMore read 0
STREAM 3832: read 0
STREAM 3832: need readable true
STREAM 3832: length less than watermark true
STREAM 3832: do read
NET 3832: _read - n 16384 isConnecting? false hasHandle? true
STREAM 3832: readableAddChunk null
STREAM 3832: onEofChunk
STREAM 3832: emitReadable_ false 7 true
STREAM 3832: flow null
STREAM 3832: read 0

to be compared with the same setup but shared openssl 3.0:

TLS 510254: server emit tlsClientError: [Error: 00786A80367F0000:error:0A00010B:SSL routines:ssl3_get_record:wrong version number:../ssl/record/ssl3_record.c:354:
] {
  library: 'SSL routines',
  reason: 'wrong version number',
  code: 'ERR_SSL_WRONG_VERSION_NUMBER'
}
NET 510254: destroy
NET 510254: close
NET 510254: close handle
NET 510254: has server
NET 510254: SERVER _emitCloseIfDrained
NET 510254: SERVER handle? true   connections? 0
NET 510254: emit close
NET 510254: emit close
STREAM 510254: readableAddChunk null
STREAM 510254: onEofChunk
STREAM 510254: emitReadable_ false 0 true
STREAM 510254: flow null
STREAM 510254: read 0
STREAM 510254: endReadable false
STREAM 510254: endReadableNT false 0
NET 510254: SERVER _emitCloseIfDrained
NET 510254: SERVER: emit close
NET 510254: _final: not ended, call shutdown()
NET 510254: afterShutdown destroyed=false
NET 510254: destroy
NET 510254: close
NET 510254: close handle
NET 510254: emit close

Additional information

The fact openssl 3.2 continues to provide (probably garbarge) data is reminiscent of "marvin" issue.

@kapouer
Copy link
Contributor Author

kapouer commented Apr 12, 2024

Regarding quictls/quictls#14 since it's the only non-trivial issue besides anything quic-related, it might be worth the help :)

@targos
Copy link
Member

targos commented Apr 12, 2024

Is it only an issue with 18.x?

@kapouer
Copy link
Contributor Author

kapouer commented Apr 12, 2024

Absolutely not, I was just focused on that version :)

@targos targos added openssl Issues and PRs related to the OpenSSL dependency. shared_lib Issues and PRs related to use of Node.js as a shared library. and removed shared_lib Issues and PRs related to use of Node.js as a shared library. labels Apr 13, 2024
nodejs-github-bot pushed a commit that referenced this issue Sep 25, 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]>
aduh95 pushed a commit to aduh95/node that referenced this issue Sep 25, 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]>
richardlau pushed a commit to aduh95/node that referenced this issue Sep 27, 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]>
@aduh95
Copy link
Contributor

aduh95 commented Sep 28, 2024

With #55101 merged, I think we can close this issue as completed :)

@aduh95 aduh95 closed this as completed Sep 28, 2024
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#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

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]>
marco-ippolito pushed a commit that referenced this issue Nov 17, 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]>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

No branches or pull requests

3 participants