Support for other tls options such as client cert

[asilvas]

faye-websocket-node/lib/faye/websocket/client.js

Line 24 in 067df9a

if (options && options.ca) tlsOptions.ca = options.ca;

This is unnecessarily restrictive. Please pass on the full options object.

[jcoglan]

Passing the whole options object through would break an abstraction boundary, exposing faye-websocket clients to changes in the underlying TLS library, plus that options object contains loads of properties not related to TLS. I think we should either explicitly forward certain options on to TLS, or else introduce a tlsOptions option to faye-websocket that is expected to literally forward to TLS, with the name serving to advertise the abstraction leak.

What do you think? What options do you particularly need to use? Is making this change a substantial win over binding websocket-driver to the TLS module yourself?

[asilvas]

All tls options should be exposed, regardless of which I use. Exposing tlsOptions would be fine.

options.tls <-- forward this on

[jcoglan]

I'm asking which options you use because I want to understand the use cases before adding more surface to an API that couples client code directly to an implementation detail. In some cases these options need converting, e.g. between strings and buffers or cases where Faye takes a pathname, reads it and hands a buffer off to tls.

So, what problem are you trying to solve at the moment; which things precisely is the current API making difficult? There might be a better solution than simply leaking the implementation details.

[asilvas]

It's in the subject "client cert". This includes the key & cert properties.

[jcoglan]

Okay, but I was wondering if you could give me a bit more context around what you're doing because that helps me make API decisions. I need to know why people are doing certain things in order to get API right, in a way that won't need to break in future.

[asilvas]

Client certs are used for authentication or identity.

[jcoglan]

Sorry I dropped off this conversation back in May, I was busy with other things and I needed to fully explain my concerns on this issue which I didn't do very well before. I'll try to spell out more clearly what questions I have.

First, I completely agree that people need to be able to specify TLS options when using wss:, that is not in question. I was trying to get more context about where and how you need to do this, so that I can answer some API design questions I have. Those questions are as follows.

The current API for the client is (where I am listing all the available options):

var client = new faye.WebSocket.Client('wss://www.example.com/', ['subprotocol'], {
  maxLength:  1024,
  ca:         fs.readFileSync('path/to/cert'),
  ping:       5,
  headers:    {Authorization: 'Bearer $some_token'}
});

I cannot forward this whole options object as-is off to tls, since it contains unrelated options that might conflict with tls own options. There are similar issues upstream with faye.Client, which accepts ca as a constructor option along with other things, and it must forward a subset of those options on to faye.WebSocket.Client.

So, I could enumerate all the options accepted by tls, and manually gather them from the Client options and forward them on. This would leave ca as a 'top-level' option, and other tls options would be added alongside it. However, this causes a tight coupling problem between Client and tls, where we need to update our code whenever tls changes its option definitions, and it may cause us to only be interoperable with a specific version of Node as a result.

A second option would be to introduce only one new top-level option on Client, called tls or tlsOptions or similar. This would be an object containing only tls options that we could forward straight on to tls without inspection. We would however need to append the ca option, which people will be passing as a top-level object, onto this object, creating it if it is not given. That's a minor problem. A bigger problem is that the API of tls is now leaking through the API of Client. Client then cannot insulate users against changes in tls. Node core tends to be quite stable but this is still a question worth considering.

Finally, there's the question of how certain things are specified. Faye used to implement HTTPS bindings itself, and when this was the case it took the key and cert as pathnames that it would then read, rather than accepting them as buffers as the underlying https module does. The rationale for this was that those values are usually kept in files, and writing code to read files is annoying, so Faye let you pass in a pathname instead. faye-websocket accepts a buffer for a client cert, which it forwards unmodified to tls, but there might be an argument for indirecting some of the tls option API to provide a better interface.

So I was asking how and where you want this behaviour so I can address those questions. I'd appreciate any thoughts you have on the questions I've raised.

[jcoglan]

If you don't have any feedback on the questions I presented above I'm going to have to close this issue; I can't move forward without some design input.

[jcoglan]

I've now implemented this in 723f4ce.