search

MatrixAI / js-quic

QUIC Networking for TypeScript & JavaScript
https://matrixai.github.io/js-quic/
Apache License 2.0
13 stars 1 forks source link

Create QUIC library that can be exposed to JS and uses the Node `dgram` module #1

Closed CMCDragonkai closed 1 year ago

CMCDragonkai commented 1 year ago

Specification

We need QUIC in order to simplify our networking stack in PK.

QUIC is a superior UDP layer that can make use of any UDP socket, and create multiplexed reliable streams. It is also capable of hole punching either just by attempting to send ping frames on the stream, or through the unreliable datagrams.

Our goals is to make use of a QUIC library, something that is compilable to desktop and mobile operating systems, expose its functionality to JS, but have the JS runtime manage the actual sockets.

On NodeJS, it can already manage the underlying UDP sockets, and by relying on NodeJS, it will also ensure that these sockets will mix well with the concurrency/parallelism used by the rest of the NodeJS system due to libuv and thus avoid creating a second IO system running in parallel.

On Mobile runtimes, they may not have a dgram module readily available. In such cases, having an IO runtime to supply the UDP sockets may be required. But it is likely there are already existing libraries that provide this like https://github.com/tradle/react-native-udp.

The underlying QUIC library there is expected to be agnostic to the socket runtime. It will give you data that you need to the UDP socket, and it will take data that comes of the UDP socket.

However it does need 2 major duties:

  1. The multiplexing and managing of streams.
  2. The encryption/decryption TLS side

Again if we want to stay cross platform, we would not want to bind into Node.js's openssl crypto. It would require instead that the library can take a callback of crypto routines to use. However I've found that this is generally not the case with most existing QUIC libraries. But let's see how we go with this.

Additional context

QUIC and NAPI-RS

Sub issues

Tasks

  1. [x] Experiment with Neon or Napi-rs
  2. [x] Experiment with quiche by cloudflare
  3. [x] Create bridge code plumbing UDP sockets and QUIC functions
  4. [x] Create self signed TLS certificate during development - https://github.com/MatrixAI/js-quic/issues/1#issuecomment-1356229150
  5. ~Extract out the TLS configuration so that it can be set via in-memory PEM variable and key variable. - 2 day~ - see #2
  6. ~Decide whether application protocols are necessary here, or abstract the quiche Config so the user can decide this (especially since this is not a HTTP3 library). - 0.5 day~ - see #13
  7. [x] Fix the timeout events and ensure that when a timeout occurs, that the connection gets cleaned up, and we are not inadvertently clearing the timeout due to null. Right now when a quiche client connects to the server, even after closing, the server side is keeping the connection alive. - 1 day
  8. [x] We need lifecycle events for QUICConnection and QUICStream and QUICServer and QUICSocket. This will allow users to hook into the destruction of the object, and perhaps remove their event listeners. These events must be post-facto events. - 0.5 day
  9. ~[ ] Test the QUICStream and change to BYOB style, so that way there can be a byte buffer for it. Testing code should be able generator functions similar to our RPC handlers. - 1 day~ - see #5.
  10. [x] Complete the QUICClient with the shared socket QUICSocket. - 3 day
  11. ~Test the multiplexing/demultipexing of the UDP socket with multiple QUICClient and a single QUICServer. - 1 day~ - See #14
  12. ~[ ] Test the error handling of the QUIC stream, and life cycle and destruction routines. - 1 day~ - #10
  13. ~Benchmark this system, by sending lots of data through. - 1 day~ - See #15
  14. ~Propagate the rinfo from the UDP datagram into the conn.recv() so that the streams (either during construction or otherwise) can have its rinfo updated. Perhaps we can just "set" the rinfo properties of the connection every time we do a conn.recv(). Or... we just mutate the conn parameters every time we receive a UDP packet.~ - See #16
  15. ~Ensure that when a user asks stream.connection they can acquire the remote information and also all the remote peer's certificate chain.~ - See #16
  16. ~[ ] Integrate dgram sending and recving for hole punching logic.~ - see #4
  17. ~[ ] Integrate the napi program into the scripts/prebuild.js so we can actually build the package in a similar to other native packages we are using like js-db~ - see #7
CMCDragonkai commented 1 year ago

So a stream can only have 1 reader or writer.

If you call getWriter() twice, you'll get a TypeError.

The only way to "destroy" the first writer is by calling writer.releaseLock().

Now if you were to do writer.close(), the stream is now closed, so that actually means you cannot get a second writer with stream.getWriter().

This means this is the only way to get 2 writers:

const writer1 = stream.getWriter();
writer1.releaseLock();
const writer2 = stream.getWriter();

After calling writer1.releaseLock it should be assumed that this writer is no longer usable, and should be left to be garbage collected.

tegefaulkes commented 1 year ago
  1. I don't know the full details but if something is consuming a stream then it is locked. there can only be one consumer of a stream. in this case if if you take a writer of a writable with const writer = writableStream.getWriter() then ultimate control of that writable is done through that writer. So if you call await writer.close() then what will close out the writer and work down the connected chain of streams closing everything. hence the error when you close the writer and then the writableStream.

If you wanted to close the writeable stream then you'd have to check if it's locked first. Alternatively if this is part of clean up then you could throw an error into the writable stream and I think everything chained to it should end up closing and throwing if they're still being used.

  1. When consuming the readableStreams I either read it to completion or close it early with readableStream.close() If it was consumed with a for await (const thing of thing){} loop then closing is handled automatically even if it ends early. For the writable I have to call writer.end() in some way. I think the only way to end a stream if you're not holding onto the end of the chain is to make it throw an error.

  2. I know it's possible to end a stream without closing the whole chain. there's an option preventClose that does it, releaseLock() likely does the same. I'll need it for the raw handlers to work. I haven't looked at it recently to recall the exact usage.

CMCDragonkai commented 1 year ago

Therefore while handling a stream in the RPC handler, it would be important that the handler would release the lock to any writer it has.

However this is only possible if the RPC handler were a raw handler of RWP and were to acquire a writer with stream.writable.getWriter().

If they don't release their writer with writer.releaseLock() there's no standard way of force closing the writable stream. This means if a raw RPC handler doesn't release its writer lock, then the stream cannot be closed.

We can however extend the getWriter method such that we keep a reference to any writer that was acquired, and then explicitly release the lock and then close the stream. I'm not sure if this is a good idea, and primarily would only be used to force close the streams even in the case where the raw handlers are written incorrectly or they are still running.

CMCDragonkai commented 1 year ago

@tegefaulkes here's a demonstration of how the locking works in the presence of derived streams:

import { ReadableStream, WritableStream, TransformStream } from 'stream/web';

const inputStream = new ReadableStream({
  start(controller) {
    controller.enqueue('Hello ');
    controller.enqueue('World!');
    controller.close();
  }
});

const transformStream = new TransformStream({
  transform(chunk, controller) {
    controller.enqueue(chunk.toUpperCase());
  }
});

console.log('Input Stream is locked', inputStream.locked); // false

const outputStream = inputStream.pipeThrough(transformStream);

console.log('Input Stream is locked', inputStream.locked); // true
console.log('Output Stream is locked', outputStream.locked); // false

const reader = outputStream.getReader();

console.log('Input Stream is locked', inputStream.locked); // true
console.log('Output Stream is locked', outputStream.locked); // true

reader.releaseLock();

console.log('Input Stream is locked', inputStream.locked); // true
console.log('Output Stream is locked', outputStream.locked); // false

Here you can see that as soon as the input stream is composed, it becomes locked. However the output stream is not locked, not until I get a reader. At that point both input and output streams are locked. Then if I release the reader, the input stream is still locked, while the output stream is now unlocked.

This is actually quite important, it means that even if say the raw RPC handler ends up releasing the readers/writers, the original QUICStream will still end up being locked, which could prevent garbage collection here if I'm trying to see if the stream is closed yet.

There's one issue though, it appears "cancelling" the reader does not actually release the reader. Note that readers can only be cancelled and not closed, but cancelling is the equivalent of closing the reader. This technically means you still have to call reader.releaseLock() to ensure that the reader is no longer locked.

With this example:

import { ReadableStream, WritableStream, TransformStream } from 'stream/web';

async function main () {

  const inputStream = new ReadableStream({
    start(controller) {
      controller.enqueue('Hello ');
      controller.enqueue('World!');
      controller.close();
    },
    cancel: (reason) => {
      console.log('INPUT STREAM CANCELLED');
    }
  });

  const transformStream = new TransformStream({
    transform(chunk, controller) {
      controller.enqueue(chunk.toUpperCase());
    }
  });

  console.log('Input Stream is locked', inputStream.locked); // false

  const outputStream = inputStream.pipeThrough(
    transformStream,
    {
      preventClose: false,
      preventAbort: false,
      preventCancel: false
    }
  );

  console.log('Input Stream is locked', inputStream.locked); // true
  console.log('Output Stream is locked', outputStream.locked); // false

  const reader = outputStream.getReader();

  console.log('Input Stream is locked', inputStream.locked); // true
  console.log('Output Stream is locked', outputStream.locked); // true

  await reader.cancel();
  await reader.closed;
  reader.releaseLock();

  console.log('Input Stream is locked', inputStream.locked); // true
  console.log('Output Stream is locked', outputStream.locked); // false

}

void main();

Notice that the input stream is still locked even when the output stream's reader is cancelled, closed and released...

This breaks our assumptions for how to destroy the QUICStream, if the input stream is always locked due to composition even if the end result is that is is closed... then we can't really rely on the locked boolean to know whether we can proceed to closing or not.

CMCDragonkai commented 1 year ago

I've tried a few things. You cannot call inputStream.cancel on a stream that is already locked. And in this case the inputStream stays locked when it is piped to the output stream.

Basically this means once a stream is composed, it stays locked forever even if the output stream is unlocked.

So our problem now is that with respect to QUICStream.destroy, there's no way for the QUICStream to know whether the resulting derived stream is locked or not. Because its own readable and writable will basically be locked forever because it is assumed it would be composed.

CMCDragonkai commented 1 year ago

This means there's no way for QUICStream.destroy to be able cancel or close the streams if the streams have been composed.

Once they are composed, the "ownership"/responsibility of the stream lifecycles becomes the job of the RPC server or whatever is controlling those things.

Which means QUICStream cannot force destroy the streams, it can only destroy them when they are not locked, and that only occurs when the they haven't been composed.

@tegefaulkes this means the RPC server needs to keep track of every stream it's handling, and explicitly close them if the RPCServer.destroy is called. Or is RPCServer.stop. It means that RPCServer takes ownership of the lifecycle of the streams.

CMCDragonkai commented 1 year ago

Now with respect to QUICStream.destroy there's a choice we have to make here.

If the streams are locked, but the this._recvClosed or this._sendClosed is not true, then we can either:

  1. Ignore them, since they are locked, we assume owership/control of the lifecycle is another place's responsibility, then we just delete the stream and assume they don't exist.
  2. Throw an error, if they were locked and closed, that's ok, no error, if they are not locked and we can close it, that's ok, no error, but if they are locked AND not closed, that means they are in-use and the other system hasn't shut them all down, thus we must prevent our own destruction.

I think I'm going with 2., in the sense that the streams are locked (responsibility has shifted) but also they aren't closed. I'll adapt the ErrorQUICStreamLocked erro and change it to ErrorQUICStreamLockedAndNotClosed.

CMCDragonkai commented 1 year ago

So now the QUICStream.destroy works like this:

  /**
   * Explicit destruction of the stream
   */
  public async destroy() {
    this.logger.info(`Destroy ${this.constructor.name}`);
    // If the streams are locked, this means they are in-use
    // or they have been composed with `pipeThrough` or `pipeTo`.
    // At this point the management of their lifecycle is no longer
    // `QUICStream`'s responsibility.
    // If they not already closed, we cannot proceed with destroying
    // this `QUICStream`.
    if (
      (this.readable.locked && !this._recvClosed)
      ||
      (this.writable.locked && !this._sendClosed)
    ) {
      // The `QUICConnection` may then result in a partial destruction,
      // some of its `QUICStream` may be destroyed
      // This means the destruction may need to be retried.
      // However a proper destruction should destroy the users of
      // the `QUICStream` first before destroying the `QUICConnection`.
      throw new errors.ErrorQUICStreamLockedAndActive();
    }
    // If the streams are not locked, and they haven't been closed yet,
    // we can close them here.
    if (!this.readable.locked && !this._recvClosed) {
      this.readable.cancel();
    }
    if (!this.writable.locked && !this._sendClosed) {
      await this.writable.close();
    }
    this.streamMap.delete(this.streamId);
    this.dispatchEvent(new events.QUICStreamDestroyEvent());
    this.logger.info(`Destroyed ${this.constructor.name}`);
  }

Which means during the PK destruction it's important to ensure that the RPC client and RPC server is destroyed first before destroying the QUIC socket.

And also means NodeConnection would have to encompass the RPC destruction first.

Also the RPCServer must then attempt to destroy its streams if they are still there if RPCServer.destroy is called.

The only possibility is with a raw handler that takes the RWP and forgets to close things.

It does also appear that you must call releaseLock even after calling cancel or close. Otherwise the locks won't be released. Using raw stream handlers is therefore dangerous as it can leak to stream leaks.

CMCDragonkai commented 1 year ago

One other thing, the for await on the readable stream does not auto cancel the readable stream. This make sense, since there could be more data on the stream arriving later. It does however auto release the internal reader.

CMCDragonkai commented 1 year ago

Another interesting thing is that if you do controller.close() on a readable stream, the readable stream can no longer be cancelled because it is already closed.

tegefaulkes commented 1 year ago

Ah I see, so if a stream is locked because it is composed then we have no control over it anymore. We can't force close it in anyway. Not from the outside at least. That does mean it's up to the final part of the stream chain to handle the life-cycle. of the streams.

This is not ideal IMO since we do in the worst case need to force close the streams to avoid hanging the program. The handlers to take an abort signal but it's advisory, it can just ignore any signal and keep running. Or the handler can be badly implemented and never consume the reader or write thus just hanging.

It may still be possible to force close a stream by using a transformation stream. The transformation controller has the following interface

    interface TransformStreamDefaultController<O = any> {
        readonly desiredSize: number | null;
        enqueue(chunk?: O): void;
        error(reason?: any): void; // Signals to both the readable and writable side that an error has occurred while processing the transform data, causing both sides to be abruptly closed.
        terminate(): void; // Closes the readable side of the transport and causes the writable side to be abruptly closed with an error.
    }

Either the error() or terminate() method will cause the stream to end early. It can be called at any time. This may be what we need. I'll prototype it to see how it works.

tegefaulkes commented 1 year ago

OK I've tested it. Using a transform stream this way it is possible to force close the stream. If we use error(reason) it closes the stream and any attempt to write to or read the stream results in the reason being thrown. Using terminate() will cause the stream to close, the reading side to end early without an error and the writer to throw TypeError: Invalid state: TransformStream has been terminated. I think this is a reasonable way to force close the stream if needed.

CMCDragonkai commented 1 year ago

If it is possible to refer to the methods of the controller to force close the stream, then this can be done at the QUICStream level not at some arbitrary point in the transformation stack.

I can copy over the methods of the readable and writable controller to instance properties of QUICStream.

CMCDragonkai commented 1 year ago

I've managed to have a working QUICSocket that doesn't die upon HTTP3 queries.

Peek 2023-01-25 16-47

CMCDragonkai commented 1 year ago

If it is possible to refer to the methods of the controller to force close the stream, then this can be done at the QUICStream level not at some arbitrary point in the transformation stack.

I can copy over the methods of the readable and writable controller to instance properties of QUICStream.

@tegefaulkes to make this happen on QUICStream, there is difference between readable and writable.

On the readable stream we have controller.error and controller.close.

On the writable stream we only have controller.error.

I think if we are forcing a shutdown and thus breaking the downstream ownership, we have to use controller.error for both ends.

I have to extract the logic that shuts down the quic stream and then error out on the QUICStream readable and writable objects.

CMCDragonkai commented 1 year ago

Ok the QUICStream now has:

  1. read and write - the read converts an external push into an internal pull via boolean-skips, the write converts an internal push to an external pull via promise plugs
  2. streamRecv and streamSend this is the actual business logic to interact with the quic streams and web streams, both now handle RESET_STREAM and STOP_SENDING frame error codes
  3. closeRecv and closeSend this is where closing logic occurs, they can close by removing state, but if there is an "error", they will actually close by sending an error by doing a stream shutdown, so basically if a shutdown is needed, that means an error is required as this usually means a shutdown is occurring without completing regular stream request/response cycle, but if there's no error, then likely the stream is already closed and we just need to do QUICStream cleanup
  4. destroy - now has the force boolean, however we are unlikely to use this much, we would want to PK to gracefully close the RPC server and have properly implemented RPC handlers, then to destroy the QUICSocket, but I suppose if a regular SIGTERM doesn't complete, then we could do a little delay before finally sending a force signal, but that's a PK application design issue
CMCDragonkai commented 1 year ago

@tegefaulkes the QUICStream is pretty good structure right now.

When you integrate this into the RPC system, you need the rpc/utils to have reasonToCode and codeToReason mappers (for unexpected rpc handler and client errors).

The websocket/TCP socket will already have an event interface/node stream, that means its backpressure mechanism may be simpler than what we are doing with QUIC stream. You need to check the way they do backpressure.

Do note that there's no analogue for stream reset and stream stop error codes in TCP (and probably web socket), instead they would just have a regular RST packet with no further details.

Finally to get hole punching in, this could be at just the UDP level before QUICConnection, but it could also be done at the QUICConnection with dgram, but using useless random dcid and scid at the beginning.

CMCDragonkai commented 1 year ago

The QUICServer creates a new QUICConnection on a method newConnection.

The QUICClient by comparison would create a new QUICConnection at the beginning in its own creation.

The QUICServer returns this back to the caller, which in this case is the QUICSocket, it then decides to call recv and send on it.

When the QUICClient is creating, it would create a new QUICConnection, and upon creation, it would be set in the socket's connection map.

This technically means we have no need of using registerClient, since connections would be part of the connection map already.

However what about lifecycle issues?

Well a client or server could encapsulate the socket is the socket is not shared. Thus one only destroys the client/server.

However if the socket is shared, then this could mean that socket is what manages the lifecycle of clients and servers. However I think this should instead turn into a sort of cross reference. That is the socket cannot be destroyed if there are still clients and servers running.

It is the user's responsibility to then shutdown their clients and server before shutting down their shared socket. Destroying the client/server won't affect the shared socket. But destroying the shared socket is not allowed if there are still clients/servers registered with it.

Like a foreign key with restrict constraint.

CMCDragonkai commented 1 year ago

The QUICClient can also destroyed by a failure in the QUICConnection. Remember that recv, send and timeout can cause the connection to be destroyed. Unlike QUICServer, if the QUICConnection is destroyed, the client must also be destroyed. In that sense, the QUICClient seems mostly a factory for creating the QUICConnection, the QUICSocket primarily just keeps track of it in the connection map.

The QUICClient may then expose the ability to start streams, which is different from the QUICConnection usage on the server side, which is primarily receiving stream events.

However it should make sense that it is possible to start new streams even from the server side. So I imagine that each connection object must have the ability to start a stream.

CMCDragonkai commented 1 year ago

Considering that the server side, we expose the connection object through the connection event, it should mean that QUICClient should just expose the connection property directly to the end user so they can use it create streams if necessary. Like quicClient.connection.newStream().

We may want to hide the internal methods though. This could be done with the @internal doc flag.

I might want to preserve SOV, so quicClient.connection.streamNew() is better. Similar to quicServer.connectionNew().

CMCDragonkai commented 1 year ago

This would then enable on the RPC server to be able to call RPC methods back to the caller much more easily. The server is just starting a stream back to the client.

CMCDragonkai commented 1 year ago

For the QUICConnection.streamNew to be implemented we need a way of creating the new stream ID.

As we discovered before the only way to start a stream is to use stream_send but with a unique stream ID.

However I don't see anyway to prevent stream IDs from conflicting from streams being received.

I checked the example code, and I can see that for the HTTP3 example, it uses the src/h3/mod.rs and in the send_request function it uses:

let stream_id = self.next_request_stream_id;

It later does something like:

self.next_request_stream_id = self.next_request_stream_id.checked_add(4).ok_or(Error::IdError)?;

The send_request call ends up returning the new stream_id.

It adds 4 to it because of this commit message:

Instead of increasing the next available stream ID counters, wait until data (e.g. HEADERS or some control data) has been actually written to the stream, this way we avoid burning through a bunch of stream IDs in case of errors (e.g. stream blocked, or insufficient stream credits).

This also fixes a potential integer overflow when 4 is added to the current stream ID (the if would only check that the previous ID was below std::u64::MAX, but the result of the sum could go over the limit), though it's unlikely anyone can actually hit that limit.

So instead of just creating a new stream ID, a stream ID is used and some data is sent over (in the case of HTTP3, these are the headers) and only if this succeeds, do we end up incrementing the stream ID.

Now again there's no where in the code that indicates whether it's possible for the stream ID received may overlap with stream IDs that we may create here.

I think this is because from the client-perspective, there's no expectation of streams being created as an event, so any streams created would be of the client's volition.

But if we want to be able to create streams on the server side back to the client, then we would want to avoid to overlap with the stream IDs that may be received from the client.

CMCDragonkai commented 1 year ago

This makes me wonder if it is even possible for the QUICConnection when accepted is capable of sending on a stream ID that is not already existing.

CMCDragonkai commented 1 year ago

Turns out there's a pattern/rules to stream ID generation described here: https://datatracker.ietf.org/doc/html/draft-ietf-quic-transport-25#section-2.1

image

CMCDragonkai commented 1 year ago

Therefore in terms of bits:

00 - client initiated, bidirectional
01 - server initiated, bidirectional
10 - client initiated, unidirectional
11 - server initiated, unidirectional

Within each type, the streams are then just numerically increased. It has to be increased by 1, if you use a stream ID out of order, it results in all the lower numbered stream IDs also opened. But I think this explains why it adds by 4 on the client side.

This means there's no chance of overlap for the stream ID for server initiated streams.

However this also means when we create a new stream, we need to give it a type... well the type would depend on whether the QUICConnection was created via an accept (server-initiated) or via a connect (client-initiated).

Right now all of our streams will be bidirectional so we will ignore the unidirectional form.

CMCDragonkai commented 1 year ago

The initial client stream is supposed to start as 0. Then the next client stream is 0+4.

4 is 100. This preserves the 00 indicating a client initiated bidirectional stream.

We just keep adding 4 to get the next stream.

00 - 0
01
10
11
100 - 4
101
110
111
1000 - 8
1001
1010
1011
1100 - 12

The initial server bidirectional stream starts at 1 which is 01.

Then we add 4 as usual.

10 - 1
11
100
101
110 - 5
111
1000
1001
1010 - 9

So QUICStream.streamNew just needs to know how the stream was created and use 0 or 1 and then increment by 4 to get new stream IDs.

CMCDragonkai commented 1 year ago

Note that you must release the lock of the writer even if the writer has an error:

import { ReadableStream, WritableStream, TransformStream } from 'stream/web';

async function main () {
  const writableStream = new WritableStream({
    write: async (chunk: Uint8Array) => {
      throw new Error('fail');
    },
  });
  const writer = writableStream.getWriter();
  try {
    await writer.write(new Uint8Array(0));
  } catch (e) {
    console.log(writableStream.locked);
    writer.releaseLock();
    console.log(writableStream.locked);
  }
}

void main();

@tegefaulkes this will be important for any raw handlers and other places where you are acquiring the writer/readers. Always release the lock regardless of error.

CMCDragonkai commented 1 year ago

Due to the last condition that opening stream IDs out of order will result in lower streams also being opened it is necessary that streamNew calls are seralised. We need to have a lock here to ensure that each call to streamNew is blocking each other.

CMCDragonkai commented 1 year ago

Done with async-locks, adding in a streamIdLock. We don't have DB transactions here, so it's not attached to a transaction.

While bidi and uni can be in parallel, we don't support uni streams yet, since that would have to create a different kind of QUICStream and right now QUICStream is always ReadableWritablePair.

I'm not sure if we would ever support unidirectional streams yet..., every RPC call involve bidirectionality so we don't ever need unidirectional streams for our usecase in PK.

CMCDragonkai commented 1 year ago

We can now proceed with:

  1. [ ] Complete the QUICClient with the shared socket QUICSocket. - 3 day
  2. [ ] Test the multiplexing/demultipexing of the UDP socket with multiple QUICClient and a single QUICServer. - 1 day
  3. [ ] Test the error handling of the QUIC stream, and life cycle and destruction routines. - 1 day
CMCDragonkai commented 1 year ago

In the QUICClient.createQUICClient, we will also do QUICConnection.connectQUICConnection.

Now this doesn't immediately create a stream, because we don't need to.

But in order to actually "start" the connection, it's not enough to just create the connection object, because no data is actually being sent on the socket.

It also makes sense to actually start the packet/frame sending so that some handshake can actually be completed. This means triggering the QUICConnection.send.

I believe upon doing so, that "ignites" the interaction loop. So we just need to do at least one await connection.send(). It will trigger the socket to send UDP datagrams, and any received datagrams will then be routed to the connection.

We also need add event handlers to the connection on failure and correspondingly destroy the QUICClient too.

CMCDragonkai commented 1 year ago

In terms of actually interacting via streams, I imagine something like this:

const stream = await quicClient.connection.streamNew();
await quicClient.connection.dgramSend();

That way we don't need to replicate all the methods of QUICConnection to the QUICClient. Because on the server side, one can also create new streams and send dgrams.

CMCDragonkai commented 1 year ago

@tegefaulkes I've just assigned you to this issue which you can work on after the websocket transport has been merged.

There's no PR for this, as I've just been pushing WIP commits to master. We can continue to do this until we squash everything into 1 commit, or slice up several different sections of the repository.

CMCDragonkai commented 1 year ago

In terms of tasks to be done @tegefaulkes, the major ones are already documented above in the issue task list.

To be more precise, I was in the middle of working on QUICClient.

This class wraps around the QUICConnection. Take note that for P2P purposes, both the server and client can share the same QUICSocket instance. This is what enables us to do hole punching.

Thus these are the immediate todos: https://github.com/MatrixAI/js-quic/issues/1#issuecomment-1413231540 before going into the rest of the tasks in the issue.

So in the QUICClient it has to perform QUICConnection.connectQUICConnection, which is one of the smart constructors of QUICConnection. Which is opposed to QUICConnection.acceptQUICConnection.

Once a connection object is created, it doesn't mean that any data has actually been sent. Because it is UDP at the bottom, the thing is connection-less. There's no state maintained until you send some data. This is why I proposed https://github.com/MatrixAI/js-quic/issues/1#issuecomment-1414692561 where we have to do a quicConnection.send() in order to start the process. But I'm not sure if this is supposed to send an empty frame first or if that's meant to be done on the stream.

Another thing is that on the client-side, there's 3 layers of abstraction:

  1. QUICClient
  2. QUICConnection
  3. QUICStream

It's 1 to 1 for QUICClient and QUICConnection. This means we don't replicate all the methods from the QUICConnection to the QUICClient. Instead we can just expose the QUICConnection as a connection property. That basically means that the client is just a thin wrapper that deals with all the bootstrapping and teardown logic.

So you'd do quicClient.connection to deal with the connection directly. Subsequently when you actually want to communicate something, it all starts with streams. So you have to create a new stream from your connection.

The lifecycle of connections is managed by the socket. The lifecycle of streams is managed by the connection. So the same socket could have multiple client connections, and multiple server-side connections. While each connection may have multiple streams.

We still haven't implemented the dgram features of quic connections. This would be added as methods to the QUICConnection. This should be explored in the context of hole punching, but we need to experiment with this to see if it is relevant.

Important regarding errors: https://github.com/MatrixAI/js-quic/issues/1#issuecomment-1406002244

There are "connection errors" and there are "stream errors" these are independent of each other. Connection errors can be protocol-level errors, or application errors, it's possible to distinguish with a special code. I think that's the case with stream errors too.

To work on this you actually need a few pieces of documentation:

  1. The QUIC RFC spec is essential to understand the flow - https://datatracker.ietf.org/doc/rfc9000/
  2. The quiche rust library docs is necessary to understand what the underlying library is doing - https://docs.rs/quiche/latest/quiche/

To run and build this system. Use nix-shell, then npm run napi-build. This will build the rust library. Then we also have an example server in src/bin/server.ts. This is executed with npm run server. We don't have an example client yet, but we have been using the quiche's own client to test against the server. But now we are building our own client with QUICClient, so you shouldn't need to do that anymore.

The full process is:

# Build the the native binary
npm run napi-build

# Put certificates in `./tmp`
step certificate create \
  localhost localhost.crt localhost.key \
  --profile self-signed \
  --subtle \
  --no-password \
  --insecure \
  --force \
  --san 127.0.0.1 \
  --san ::1 \
  --not-after 31536000s

# Go inside the quiche directory (using the same nix-shell of js-quic)
# Make sure you have cloned the cloudflare quiche into your projects

cd ./apps

cargo run --bin quiche-client -- 'http://127.0.0.1:55555'

# Run without verifying TLS cause the certs are self-signed
cargo run --bin quiche-client -- --no-verify 'http://127.0.0.1:55555'

# Run quiche's own server
cargo run --bin quiche-server -- --listen 127.0.0.1:55555
cargo run --bin quiche-client -- --no-verify 'https://127.0.0.1:55555'

Yea and you also need some fake TLS certs, which you can generate using step-cli. PK isn't a PKI yet... as we don't have secret "generation" but we can bring this into the future, so we don't need to use step-cli.

CMCDragonkai commented 1 year ago

In order to make it easier for me to review and coordinate with you @tegefaulkes you should start a new PR for any changes on master, so we can see. I should be getting my new laptop on Tuesday (Sydney time), so I can doing some actual work on it.

CMCDragonkai commented 1 year ago

In order to test, we need to have a tests/utils.ts that exports:

export {
  generateKey,
  sign,
  verify,
  randomBytes
};

I'm using webcrypto as it is native to nodejs and portable to test which doesn't require external libraries.

It does end up being a bit inefficient due to the import/export between CryptoKey and ArrayBuffer.

But in production in PK we use libsodium so this is fine.

Note that js-db still uses node-forge in its testing which we shouldn't be using anymore since it is super slow.

CMCDragonkai commented 1 year ago

What is the purpose of this crypto object? It's used when bootstrapping a connection in QUIC. You have to do some exchanges of signed tokens and then verification.

This is done before TLS is involved. It's done at the UDP packet level. So there's connection bootstrap happening first, before TLS is involved.

The quiche library does not provide any cryptographic routines, this is what we have to supply ourselves.

At the same time, the quiche library does however does bundle the BoringSSL library during rust compilation. This is all handled by cargo though, so we aren't even specialising the library in anyway. This means this doesn't use node's builtin openssl system.

Now on the websocket transport layer, it also doesn't use node's builtin openssl system. @tegefaulkes can you shed light on what your websocket library is using? I think we had an issue on uws about being able to swap it out for node's openssl library instead.

This does mean an app built on node with js-quic and websocket transport ends up having node's builtin openssl, uws's openssl, and quic's boringssl all running at the same time. Note that this is somewhat due to openssl not merging support for QUIC. So node's openssl is actually been changed to the fork that does have support for QUIC. But boringssl may have different requirements. The is ok for now, but going ahead long term, we should be looking at merging it all into 1 tls library that can be used for all transport layers. And remember node runtime won't even exist for mobile OS, so really we would want an agnostic tls library. Also discussion on quiche: https://github.com/cloudflare/quiche/issues/877

tegefaulkes commented 1 year ago

I think uWS is using some version of openSSL or boring SSL. I think the response to the issue was that they won't support using node's openSSL implementation so If we wanted that we'd need to fork and do it ourselves.

CMCDragonkai commented 1 year ago

@tegefaulkes

I think in order to be able to support the method Config::with_boring_ssl_ctx you'd need to have the Cargo package boring = "2.0.0".

This is what allows you to access the qualified namespace of boring::....

Note that usually it's supposed to take the ssl context. But in our case we would be taking the entire certificate chain and key. Then perhaps we can just add some optional parameters into the new constructor instead of having an extra method.

CMCDragonkai commented 1 year ago

During my QUICClient testing I've discovered a few peculiar behaviour here.

In NodeJS, the :: means dual stack. Meaning a socket is then bound on all IPv4 and IPv6. This means the server can accept client packets sent to local IPv4 and IPv6 addresses. Basically a dual stack server that can server both IPv4 and IPv6 clients.

However in the UDP socket, there needs to be a type parameter. Right now, when using ::, the type must be udp6, or else the bind call fails. This is how it is currently implemented.

This turns out to be a problem when using the UDP socket as a client.

If you were to bind the UDP socket to :: and thus create a dual stack socket. You can no longer send packets to an IPv4 address, because the type of the socket was forced to udp6, and attempting to send to an IPv4 address, it ends up failing with EINVAL on the send command.

This technically means there's no such thing as a dual stack client socket. If you intend to use the socket to send data... then using :: as the local host is just confusing.

On the server it means this is a "dual stack" server.

But I guess there's no such support for "dual stack" client.

This means if you want to send IPv4 packets, the local host of the client, must also be IPv4.

Correspondingly if you want to send IPv6 packets, then the local host of the client must also be IPv6.

This means these are legitimate:

QUICClient.createQUICClient({
  host: '127.0.0.1' as Host,
  localHost: '0.0.0.0' as Host
})
QUICClient.createQUICClient({
  host: '::1' as Host,
  localHost: '::' as Host
})

But mixing up the remote host and local host is not allowed.

Furthermore the target host must not be a wildcard. It has to be a legitimate target. We're dealing with this with the resolvesZeroIP function.

CMCDragonkai commented 1 year ago

This inability to have a dual stack client creates a bit of difficulty when sharing a socket between the client and server, especially in a P2P scenario.

So ideally we would have dual stack servers, which uses a dual stack socket.

But then this limits it to UDP6 packets when packets are sent out...

I find this kind of strange because if I'm handling a packet on a dual stack server, then surely when responding back to an IPv4 address, I should be able to send back a IPv4 packet?

Perhaps it's actually doing a IPv4 mapped IPv6 address automatically. I haven't actually checked what happens when an IPv4 client connects to dual stack server and what the remote address is.

CMCDragonkai commented 1 year ago

Ok this makes sense now. On a dual stack socket, the received address is an IPv4 mapped IPv6 address. I can see it now:

WE GOT A PACKET { address: '::ffff:127.0.0.1', family: 'IPv6', port: 44377, size: 42 }

This should mean, we could use the same mapped address structure to send packets on a dual stack client socket.

CMCDragonkai commented 1 year ago

Ok that actually works.

It is possible to use a dual stack client socket, and send IPv4 addresses using the IPv4 mapped IPv6 address format.

That's actually what is necessary to ensure that we can use the same dual stack socket between client and server!!

CMCDragonkai commented 1 year ago

One issue is that ::ffff:127.0.0.1 is neither ipv4 nor ipv6, so currently it tries to resolve it as a hostname, but it's not a hostname.

I need to check if the ip-num library can recognise this.

CMCDragonkai commented 1 year ago

It has support for it https://github.com/ip-num/ip-num/blob/352c7b649eb5f771740f642dab4c33f947cd1da0/README.md#ipv4-mapped-ipv6-address-support.

But it's not part of the validator.

CMCDragonkai commented 1 year ago

Raised issue upstream https://github.com/ip-num/ip-num/issues/92 to request that the validator accepts ipv4 mapped ipv6 addresses are accepted as ipv6 address.

Ok so assuming this ends up working, we still need to be able to then take IPv4 target hosts, and convert them to the mapped version when the socket is a dual stack socket.

I might need to give a property to QUICSocket that indicates whether its dual stack or not.

If something is dual stack, we can apply automatic conversions to target host IPs in QUICClient.

Then we can full dual stack support between client and server. This would mean a dual stack client can send IPv4/IPv6 packets. And dual stack server can receive IPv4/IPv6 packets. Finally the same dual stack socket can be shared between client and server!

@tegefaulkes I think this is also relevant to the websocket layer too.. I'm pretty sure you didn't look into IPv6/IPv4 support there right?

CMCDragonkai commented 1 year ago

Now we have tests for IPv4 and IPv6.

 PASS  tests/QUICSocket.test.ts
  QUICSocket
    ✓ ipv4 to ipv4 succeeds (51 ms)
    ✓ ipv4 to ipv6 fails (3 ms)
    ✓ ipv4 to dual stack succeeds (3 ms)
    ✓ ipv6 to ipv6 succeeds (7 ms)
    ✓ ipv6 to ipv4 fails (3 ms)
    ✓ ipv6 to dual stack succeeds (5 ms)
    ✓ dual stack to ipv4 succeeds (3 ms)
    ✓ dual stack to ipv6 succeeds (3 ms)
    ✓ dual stack to dual stack succeeds (4 ms)

I wonder if it is possible to have a dual stack socket that listens on different IPv4 and IPv6 addresses and not just ::. But this is obviously not possible in NodeJS atm.

CMCDragonkai commented 1 year ago

The above tests imply that when sharing the quic socket between client and server... the most flexible will be the dual stack socket, since it can receive from IPv4 and IPv6, but also send to IPv4 and IPv6.

If the user only binds to an IPv6 address, then it can only talk to peers on IPv6 or dual stack.

If the user only binds to IPv4 address, then it can only talk to peers on IPv4 or dual stack.

This means in a P2P network. There is some reachability concerns. IPv4 peers and IPv6 peers are going to be segregated. While dual stack peers can talk to everybody.

From a NAT busting pov, this segregation is due to IP protocol layer.

Technically if our NAT busting has relay support, then it would actually be possible to relay between IPv4 and IPv6 peers over a dual stack peer. But I haven't explored this atm.

CMCDragonkai commented 1 year ago

Ok so the QUICSocket.send will now force the port and address as parameters because the underlying UDP socket is connection less. Which as per node's documentation means that the socket send must be passed the port and address.

Furthermore it will resolve any hostnames.

Then it checks the conditions under which it is not possible to connect (ipv4 to ipv6, ipv6 to ipv4, and using ipv4-mapped-ipv6 addresses in ipv6 or ipv4).

If it cannot pass, then it will throw a ErrorQUICSocketInvalidSendAddress.

CMCDragonkai commented 1 year ago

The fact that dual stack sockets will report IPv4 IPs as ::ffff:1.1.1.1 means that this has some implications for our Node Graph. @tegefaulkes would this be a problem? Especially since if these IPs are passed around in a decentralised way, IPv4 peers needs to be able to convert them to regular IPv4 addresses, while dual stack peers need to preserve them.

It's possible we force a canonical format so that mapped addresses are always stored as ipv4, but then this forces a conversion cost on every send.

If we allow both formats to exist, then it increases some complexity.