ALPNs and NodeIDs should be infallible

[flub]

Because of our crypto setup we know we have some invariants on an accepted connection: we always have a NodeId. We (probably also) always have an ALPN. Similarly the underlying APIs of peer_identity and handshake_data are probably also infallible. But maybe we don't even want to provide the underlying function at all anymore.

These APIs need to be carefully checked and the implementations should be made infallible. Taking care to deal with the fact that we can not panic based on data accepted on the network since anyone can modify iroh or write a custom impl that would violate the constraints. Those should gracefully reject the connection.

---

Original issue text:

The Connection::remote_node_id method currently returns a Result. The kind of errors it throws are currently all essentially bugs in the code. I am not sure how future proof this is however:

• Maybe [WIP] feat: implement support for raw public keys in TLS #2937 which introduces raw public keys would change things?
• Maybe some other change will no longer mean the number of certificates will always be 1 and then having this check in old versions would at least mean they don't panic.

It is however an eyesore that this API is fallible right now. Is there a way we can safely ensure this API is actually infallible? Maybe it involves changing how the handshake data gets stored so we can directly store a NodeId? This would also involve making the peer_identity no longer optional.

[matheus23]

Maybe some other change will no longer mean the number of certificates will always be 1 and then having this check in old versions would at least mean they don't panic.

I still strongly believe Connection::remote_node_id should be infallible. At the point at which you have a Connection, you should know your peer's NodeId. Otherwise how would it be an end-to-end encrypted channel with specifically that NodeId.

Maybe raw public keys would change things?

No - we still have client authentication. And I don't think one should have access to a Connection without client auth. And if you have client auth, then the remote node ID is essentially what you "are authenticated to".

[flub]

I think you're right. But it'd be so much nicer if it didn't involve .unwrap(). This should be possible, should it not?

[matheus23]

I don't think so, because (1) the quinn::Connection is more general than our use case and includes use cases without client authentication and (2) the type of certificate is not a type parameter - so it's impossible to codify that fact at compile time.

[matheus23]

Okay, I looked at this again/thought about this a little more.
I think there's a way of doing this "without unwrap", and that is: In Connecting::poll, after we get Poll::Ready(Ok(connection)) where connection: quinn::Connection, we check that the quinn::Connection really is the type of connection we'd expect, by calling get_remote_node_id. If not, we error out. And if it succeeds, we can either store that NodeId directly on the Connection struct (no unwrap!), or we just repeat that call in Connection::remote_node_id, and use unwrap there.