zenhack / go-websocket-capnp

Apache License 2.0
3 stars 2 forks source link

How to wait for connection or failure? #3

Open hspaay opened 1 year ago

hspaay commented 1 year ago

Any suggestion on how best to wait until the connection is established or fails?

The (private) ready channel looks useful but it would also need an error channel. What do you think of adding a method, 'wait for connection with timeout' which returns an error on failure?

zenhack commented 1 year ago

(I'm assuming this is specific to the js implementation). I'd be open to tweaking the API to support this.

zenhack commented 1 year ago

In terms of the details of the API, we could instead of New do something like:

func Connect(ctx context.Context, url string, subprotocols []string) (*Conn, error)
hspaay commented 1 year ago

Thanks for the quick reply.

func Connect(ctx context.Context, url string, subprotocols []string) (*Conn, error)

  1. Using 'Connect' instead of New makes sense as that is what is does :)

  2. Currently, New returns immediately. Does the use of context imply you are thinking of making it blocking? If so, that solves the error problem I'm running into. In my fork I've closed the ready channel on error, and added a function WaitForConnection that waits on the ready channel. The error variable is then used to determine if it was successful. I like your proposed API better though.

  3. To be able to TLS can we add an options struct to the Connect function?

    func Connect(ctx context.Context, url string, subprotocols []string, opts map[string]interface{})
zenhack commented 1 year ago

Quoting Henk (2023-02-11 12:24:09)

1. Using 'Connect' instead of New makes sense as that is what is
   does
   :)

Actually, now that I think of it, Dial() would be more consistent with Go conventions.

2. Currently, New returns immediately. Does the use of context imply
   you are thinking of making it blocking? If so, that solves the
   error problem I'm running into.

Yes, that was my thinking.

3. To be able to TLS can we add an options struct to the Connect
   function?

func Connect(ctx context.Context, url string, subprotocols []string, opts map [string]interface{})

Can't we just use a wss URL for this?

hspaay commented 1 year ago

Can't we just use a wss URL for this?

For the machine-to-machine use-case where a service is written in (node) JS, there is a use-case for certificate based authentication. In my case, a zwave-js service running on Node publishes events using capnp over websockets. For security reasons all services that use websockets must use TLS and a client certificates for authentication. In addition the service has the CA certificate to resist a MiM attack. The second use-case is similar. Any IoT device that is written in JS will obtain a client certificate when provisioning and use it to authenticate themselves to publish events.

zenhack commented 1 year ago

Aha, you're using this with node -- I'm only doing this in the browser.

I guess I still don't have a clear picture of what this would look like; how would the options struct actually be used? The websocket constructor doesn't take any arguments that we aren't already supplying:

https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/WebSocket

Does node have an extended version of this that you wanted to use?

Quoting Henk (2023-02-11 17:52:16)

 Can't we just use a wss URL for this?

For the machine-to-machine use-case where a service is written in (node) JS, there is a use-case for certificate based authentication. In my case, a zwave-js service running on Node publishes events using capnp over websockets. For security reasons all services that use websockets must use TLS and a client certificates for authentication. In addition the service has the CA certificate to resist a MiM attack. The second use-case is similar. Any IoT device that is written in JS (yeah too wonder why they would want to, but thats okay) will obtain a client certificate when provisioning and use it to authenticate themselves to publish events.

-- Reply to this email directly, [1]view it on GitHub, or [2]unsubscribe. You are receiving this because you commented. Message ID: @.***>

Verweise

  1. https://github.com/zenhack/go-websocket-capnp/issues/3#issuecomment-1426892538
  2. https://github.com/notifications/unsubscribe-auth/AAGXYPUOYB2YHETZWPXUIRTWXAJ2BANCNFSM6AAAAAAUYPOWCM
hspaay commented 1 year ago

Yes, Node supports the option parameter. I don't know what the browser does with it. I've tested this and it works. I hope that client cert auth also works in the browser...

What I'm currently doing looks like this in the caller to New:

opts := make(map[string]interface{})
opts["rejectUnauthorized"] = false      // only needed when no CA certificate is provided and the service isn't using a well known CA
opts["cert"] = string(clientCertPem)     // loaded certificate in PEM format
opts["key"] = string(clientKeyPem)
opts["ca"] = string(caCertPem)
wsCodec := wsockcap.New(fullURL, nil, opts)

In New (modified from go-websocket-capnp/websocket.go code):

func New(url string, subprotocols []string, opts map[string]interface{}) *Conn {

    websocketCls := js.Global().Get("WebSocket")
    var value js.Value
    if subprotocols == nil {
        optsJs := js.ValueOf(opts)
        value = websocketCls.New(url, optsJs)
...

What I don't like about this approach is that there is a 'fuzzy' dependency on the opts struct of nodejs/browser, which is not well documented.

hspaay commented 1 year ago

If you don't mind I can put together a PR:

Any other?

zenhack commented 1 year ago

If the options work as expected in the browser I'm fine adding them, though the documentation I've found suggests they aren't supported? If these don't do anything in the browser I'd like to do something to make that clear to potential users; I wouldn't want someone to be fighting with a bug that came down to "the options argument doesn't actually do anything."

If we are going to add options, I'd like a strongly-typed struct with the relevant fields, rather than just passing a map, but otherwise sounds good.

hspaay commented 1 year ago

Makes sense. I'll figure out how this behaves in the browser. The primary objective is to be able to use client certificate authentication and use self signed server certificates, so might just limit the options to that.