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

Propagate connection info along side stream creation #16

Closed
tegefaulkes opened this issue Apr 26, 2023 · 6 comments
Closed

Propagate connection info along side stream creation #16

tegefaulkes opened this issue Apr 26, 2023 · 6 comments
Assignees
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Apr 26, 2023

Specification

We need a way to provide the connection info and other metadata along side the stream when it is created. The stream itself should implement the RpcStream interface defined in the agnostic RPC system. This is defined below.

interface RPCStream<
  R,
  W,
  M extends Record<string, JSONValue> = Record<string, JSONValue>,
> extends ReadableWritablePair<R, W> {
  cancel: (reason?: any) => void;
  meta?: M;
}

This information should be stored within the QUICConnection and can be taken from there. Certain information such as src address can change, so we need to keep this data up to date.

This also means we need to be able to get the connection information from the underlying QUICConnection. Features may need to be added to support this.

Metadata we need to include are:

type Metadata = {
  remoteCertificates?: Certificates
  localHost: Host;
  localPort: Port;
  remoteHost: Host;
  remotePort: Port;
}

Additional context

Tasks

  1. Have the streams implement the RpcStream interface.
  2. Add any required methods to QUICConnection to provide any connection information an other metadata.
@tegefaulkes
Copy link
Contributor Author

The Quiche system returns the peer certificates as DER encoded Uint8Arrays. Do we want to convert this to any other form such as the X509 or the PEM format?

@tegefaulkes
Copy link
Contributor Author

I also need to note, Relating to this issue cloudflare/quiche#1489. The TLS handshake completes after the connection establishment handshake. I think this means that streams can be created before TLS is established and the peer certificates is obtained.

So it's possible for the peer certificates to exist, but the metadata will return null for them until the TLS handshake has completed. I currently don't have a good way of knowing when that has happened without polling.

@CMCDragonkai
Copy link
Member

Do you see that right now in the code?

You may wish to convert them to the expected format in PK. I think they are PEM strings...?

@tegefaulkes
Copy link
Contributor Author

I haven't seen in the test I just made. But I think it's a potential race condition if we don't keep it in mind.

I've converted the DER format certs to the PEM format.

@CMCDragonkai
Copy link
Member

New issue should be created for that. Can you also confirm upstream by raising an issue about this?

Wait... Why would streams and TLS certs have anything to do with each other? Streams in our case can only be created after connection is established and that should only occur after TLS handshake is done. But I think you said we have to potentially wait for it to happen because there's no event for it. I think a poll utility function can be copied over from PK.

@tegefaulkes
Copy link
Contributor Author

There is an upstream issue for this. cloudflare/quiche#1489. They said that connection establishment and TLS handshake are different with the connection handshake completing first then the TLS handshake after. The is_established being true refers to the first handshake. I don't know of a way to check if the TLS handshake has completed.

I'm just saying that a connection can be established and created before the TLS has finished. So if you try to get the connection info, the peer cert could be missing. But it could be missing due to the handshake hasn't completed yet or the peer hasn't sent a cert at all.

CMCDragonkai pushed a commit that referenced this issue May 17, 2023
…onnection info

* This is to match the stream interface for Polykey's agnostic RPC streams.
* Also including a cancel method to match that interface

* Fixes #16

[ci skip]
@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices label Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices
Development

No branches or pull requests

2 participants