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

tls: allow client-side sockets to be half-opened #27836

Closed
wants to merge 2 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented May 23, 2019

Make tls.connect() support an allowHalfOpen option which specifies
whether or not to allow the connection to be half-opened when the
socket option is not specified.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@lpinca
Copy link
Member Author

lpinca commented May 23, 2019

I didn't add the option in the list of options allowed for the tls.TLSSocket constructor because the socket argument is required there even though that is not true.

doc/api/tls.md Outdated Show resolved Hide resolved
@lpinca lpinca force-pushed the add/allow-half-open-option branch from a04d2ec to e810a28 Compare May 23, 2019 14:41
@sam-github
Copy link
Contributor

Is there a test somewhere that shows that half-open actually works? If there isn't, then I think there should be a test showing it working, not just that the option is passing through.

There is a fair bit of back-an-forth in the TLS protocol, its not at all clear that a TCP socket allowing half-open will automatically have half-open semantics at the TLS layer.

@lpinca
Copy link
Member Author

lpinca commented May 24, 2019

Is there a test somewhere that shows that half-open actually works? If there isn't, then I think there should be a test showing it working, not just that the option is passing through.

Added one in c6d8518. There is another for the server in https://github.com/nodejs/node/blob/77b9ce5/test/parallel/test-tls-server-parent-constructor-options.js

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this got another sign-off since Sam's comment... I agree with Sam that the semantics of { allowHalfOpen: true } in a TLS context are underspec'd, perhaps even nonsensical. That needs to be hashed out first. What is the expected behavior? What is the expected interaction between the TLS and TCP socket levels?

@sam-github
Copy link
Contributor

IIRC, TLS itself has half-close semantics in emulation of TCP's bi-directional close, it has a signed uni-directional finish alert (I forget the exact name) that goes each way. Its just I recall that while looking through the TLS tests that it wasn't clear to me that tests existed that checked this functionality. Also, openssl might need to write messages even when doing SSL_read, so its not impossible that admin records will be written back to the peer, even after receiving the signed read-close alert. All of which is just to say, that the existing code might be working perfectly with TLS half-close, and/or with TLS half-close on top of TCP half-close, but it does deserve a test.

@lpinca
Copy link
Member Author

lpinca commented May 24, 2019

I've added a couple. Suggestions on what else should be tested are welcome.

@lpinca
Copy link
Member Author

lpinca commented May 24, 2019

Expected behavior is to prevent _handle.close() from being called automatically as soon as the 'end' event is emitted. Or better to prevent socket.end() from being called and in turn _handle.close().

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two suggestions, but otherwise LGTM

socket.on('end', common.mustCall(() => {
setTimeout(() => {
assert(socket.writable);
assert(socket.write('Hello'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest changing this to 'Bye'


socket.setEncoding('utf8');
socket.on('data', common.mustCall((chunk) => {
assert.strictEqual(chunk, 'Hello');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit thrown by the conditional, it seemed to me the test should know exactly whether socket.writable was true or false, but then I read the rest of the test and I see why. I suggest changing this to something like below:

if (chunk === 'Hello') {
socket.end(chunk);
} else {
assert(chunk === 'Bye');
assert(!socket.writable);
}

doc/api/tls.md Outdated
@@ -1215,6 +1218,10 @@ changes:
connection/disconnection/destruction of `socket` is the user's
responsibility, calling `tls.connect()` will not cause `net.connect()` to be
called.
* `allowHalfOpen` {boolean} If the `socket` option is missing, indicates
whether or not to allow the internally created socket to be half-opened,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/half-opened/half-open/.

@lpinca lpinca force-pushed the add/allow-half-open-option branch 2 times, most recently from ccc0c18 to d1332da Compare May 25, 2019 09:06
@addaleax addaleax added the tls Issues and PRs related to the tls subsystem. label Jun 2, 2019
@lpinca lpinca force-pushed the add/allow-half-open-option branch from d1332da to aae8969 Compare June 7, 2019 13:02
@lpinca
Copy link
Member Author

lpinca commented Aug 14, 2019

@bnoordhuis are you concerns still valid?

@bnoordhuis
Copy link
Member

I didn't re-review but I dismissed my old review.

@lpinca
Copy link
Member Author

lpinca commented Aug 14, 2019

Ok, thanks. I will land this in 48 hours if there are no objections.


socket.setEncoding('utf8');
socket.on('data', common.mustCall((chunk) => {
if (chunk === 'Hello') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that you receive the string as a single chunk. Which is probably true nearly all of the time but it isn't guaranteed, it might be split across multiple chunks.

The only thing socket.setEncoding() does is ensure the stream won't emit partial multi-byte sequences.

Copy link
Member Author

@lpinca lpinca Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true and it is not an excuse but there are a lot of tests written like this. I would not add unneeded complexity unless the test turns out to be flaky.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if the 'data' handler is not called with a chunk which is not 'Hello' or 'Bye', the assertion in the else branch will fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but our CI is really good at shaking out edge cases and debugging a failing test takes 10x as long as just writing it correctly in the first place.

Copy link
Member Author

@lpinca lpinca Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis does this look better to you:

diff --git a/test/parallel/test-tls-connect-allow-half-open-option.js b/test/parallel/test-tls-connect-allow-half-open-option.js
index 129d1d1ced..1bc7eda1ca 100644
--- a/test/parallel/test-tls-connect-allow-half-open-option.js
+++ b/test/parallel/test-tls-connect-allow-half-open-option.js
@@ -21,6 +21,7 @@ const tls = require('tls');
   assert.strictEqual(socket.allowHalfOpen, false);
 }
 
+const hello = Buffer.from('Hello');
 
 const server = tls.createServer({
   key: fixtures.readKey('agent1-key.pem'),
@@ -28,15 +29,24 @@ const server = tls.createServer({
 }, common.mustCall((socket) => {
   server.close();
 
-  socket.setEncoding('utf8');
-  socket.on('data', common.mustCall((chunk) => {
-    if (chunk === 'Hello') {
-      socket.end(chunk);
+  const chunks = [];
+
+  socket.on('data', (chunk) => {
+    chunks.push(chunk);
+
+    const buf = Buffer.concat(chunks);
+
+    if (buf.equals(hello)) {
+      chunks.length = 0;
+      socket.end(buf);
     } else {
-      assert.strictEqual(chunk, 'Bye');
       assert(!socket.writable);
     }
-  }, 2));
+  });
+
+  socket.on('end', common.mustCall(() => {
+    assert.deepStrictEqual(Buffer.concat(chunks), Buffer.from('Bye'));
+  }));
 }));
 
 server.listen(0, common.mustCall(() => {
@@ -45,19 +55,22 @@ server.listen(0, common.mustCall(() => {
     rejectUnauthorized: false,
     allowHalfOpen: true,
   }, common.mustCall(() => {
-    socket.on('data', common.mustCall((chunk) => {
-      assert.strictEqual(chunk, 'Hello');
-    }));
+    const chunks = [];
+
+    socket.on('data', (chunk) => {
+      chunks.push(chunk);
+    });
+
     socket.on('end', common.mustCall(() => {
+      assert.deepStrictEqual(Buffer.concat(chunks), hello);
+
       setTimeout(() => {
         assert(socket.writable);
         assert(socket.write('Bye'));
         socket.end();
-      }, 50);
+      }, 100);
     }));
 
     socket.write('Hello');
   }));
-
-  socket.setEncoding('utf8');
 }));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that works. You can also keep the socket.setEncoding('utf8') and concatenate until you see the shibboleth.

Make `tls.connect()` support an `allowHalfOpen` option which specifies
whether or not to allow the connection to be half-opened when the
`socket` option is not specified.
@nodejs-github-bot
Copy link
Collaborator

lpinca added a commit that referenced this pull request Aug 17, 2019
Make `tls.connect()` support an `allowHalfOpen` option which specifies
whether or not to allow the connection to be half-opened when the
`socket` option is not specified.

PR-URL: #27836
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@lpinca
Copy link
Member Author

lpinca commented Aug 17, 2019

Landed in c3b8e50.

@lpinca lpinca closed this Aug 17, 2019
@lpinca lpinca deleted the add/allow-half-open-option branch August 17, 2019 04:48
targos pushed a commit that referenced this pull request Aug 19, 2019
Make `tls.connect()` support an `allowHalfOpen` option which specifies
whether or not to allow the connection to be half-opened when the
`socket` option is not specified.

PR-URL: #27836
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants