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

Undeprecate tls.createSecurePair #29559

Closed
Ayms opened this issue Sep 14, 2019 · 10 comments
Closed

Undeprecate tls.createSecurePair #29559

Ayms opened this issue Sep 14, 2019 · 10 comments
Labels
tls Issues and PRs related to the tls subsystem. wontfix Issues that will not be fixed.

Comments

@Ayms
Copy link

Ayms commented Sep 14, 2019

Restarting https://github.com/Ayms/node-Tor, making the whole (non public) code open source in clear, clean, modular, ES6 and working with latest nodejs version, not easy work, getting up to speed with the code, did not work on it since some time so maybe not seeing the simple fix to the below issue

node-Tor can be used from the browser implementing WS encapsulating TLS with the Tor protocol (and not the contrary, ie wss)

The former code was using cleartext and encrypted pairs, while receiving a message it uses ws_decode and passes the data to encrypted, on the other way the data is piped to encrypted with ws_encode, then we use cleartext to get the decrypted data

As far as I understand TLSSocket extends the socket object to a cleartext object, therefore I don't see how to include the encrypted ws_decode/encode except hacking into TLSSocket

@addaleax addaleax added the tls Issues and PRs related to the tls subsystem. label Sep 14, 2019
@addaleax
Copy link
Member

While I’m not entirely understanding your use case here, two things that should be pointed out:

  • SecurePair and TLSSocket should be functionally identical. If you can do something with SecurePair, you can do it with TLSSocket, too.
  • As of 9301b8a, SecurePair is implemented in terms of TLSSocket, so the overhead of having this second implementation is neglegible and I personally wouldn’t have an issue with removing the runtime deprecation on it.

That being said:

  • You can probably make this work by using the fact that you can pass any Duplex stream, not just network sockets, as the first argument to new TLSSocket(stream).
  • If you do want to stick to the SecurePair API, you can also consider copying the implementation from Node core (less than 30 lines of JS code + the DuplexPair utility, which is published to npm as duplexpair), and use that.

Does this help you?

@bnoordhuis
Copy link
Member

#29421 (comment) is allegedly a use case where tls.TLSSocket isn't a substitute for tls.createSecurePair().

(I say 'allegedly' because I didn't look into it much. It might just be a bug rather than a design flaw.)

@Ayms
Copy link
Author

Ayms commented Sep 15, 2019

The use case is encapsulating tls (who encapsulates the Tor protocol) inside the websocket protocol (ie the browser connects to node via ws)

Thanks for the examples, probably the fix is to use a duplex stream who performs ws_encode/decode before passing/sending to TLSSocket (not sure for #29421 case)

Now, since the code is about the same and this is not deprecated finally since 0.11.4, and given that some people like myself likes it the way it is, and given that some non usual cases could exist requiring securepair, what is the purpose finally to deprecate it?

@sam-github
Copy link
Contributor

Deprecation history: #6063 (and #8783, to a lesser extent)

createSecurePair() is implemented in terms of TLSSocket, so its demonstably (see #29559 (comment)) a wrapper function, it doesn't allow anything that wasn't possible before. But, given that, its maintenance overhead is small, and likelihood of actually being removed even smaller.

Reading through history, there is a long-standing point that when deprecated, the docs should have said how to use TLSSocket to replace it (without having to read the Node.js source). On the other hand, does forcing npm modules to copy js out of the Node.js' lib folder (or documentation) into their own code really improve anything?

Unless this API is on track for actually being deleted at a specific release, I think there is no reason to hope (based on history) that the API will be used less and less over time until the point it can be removed and no one will notice, which I suspect was the original hope.

So, perhaps it should just be undeprecated? @nodejs/crypto thoughts?

@bnoordhuis
Copy link
Member

I don't remember full-on removal having been discussed. The reason for deprecation (in general, not just here) is that there should be One Way Of Doing Things.

I'm inclined to say it's a documentation issue. It would be good if the documentation for tls.createSecurePair() shows how to port existing code to tls.TLSSocket.

@Ayms
Copy link
Author

Ayms commented Sep 18, 2019

One way of doing things, yes, but that's not the issue here, TLSSocket is designed to retrieve cleartext (one way thing), securePair was designed to handle clear and encrypted (second way thing), both are at the end the very same in fact in terms of code, not well documented indeed, now, again, what is the big issue with keeping it like it is?

Taking another example, then you could deprecate https too so people pipe TLSSocket to http

@bnoordhuis
Copy link
Member

Not a great analogy because the https module does quite a bit more than that. SecurePair on the other hand is just a flimsy wrapper around TLSSocket.

The sole reason to keep it around, documentation issues aside, is inertia. I'm not worried about maintenance overhead but having different ways to accomplish the same thing is not a good thing.

Keeping it deprecated is the best way forward, IMO.

@jasnell
Copy link
Member

jasnell commented Sep 18, 2019

I definitely agree with @bnoordhuis on this. Not seeing a strong reason to undeprecate

@bnoordhuis
Copy link
Member

I'll close this out seeing that no collaborators are championing undeprecation.

@bnoordhuis bnoordhuis added the wontfix Issues that will not be fixed. label Sep 23, 2019
@Ayms
Copy link
Author

Ayms commented Oct 2, 2019

Maybe you are right but an example would be good, I first used the duplexpair from @addaleax, then ended up with (probably there is a smarter way using pipe to wsdecode/encode):

//request is a network Socket (here a WebSocket)
let socket=new stream.Duplex();
let cleartext=new tls.TLSSocket(socket,{secureContext:sslcontext,isServer:true});
request.encrypted=socket;
request.on('data',function(data) {
	this.encrypted.push(wsdecode(data));
};
request.encrypted._read=function() {
	const cb=this['cb'];
	if (cb) {
		this['cb']=null;
		cb();
	};
};
request.encrypted._write=function(chunk,encoding,cb) {
	this['cb']=cb;
	request.write(wsencode(chunk));
};

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. wontfix Issues that will not be fixed.
Projects
None yet
Development

No branches or pull requests

5 participants