opened
DaanDeMeyer
username_0
While further integrating BoringSSL, I came upon another issue.

When `conn_recv_crypto` is called by the server in response to receiving the CRYPTO frame containing the client's Finished TLS handshake message, the following check fails:

```
if (NGTCP2_MAX_VARINT < fr_end_offset ||
    (max_rx_offset && max_rx_offset < fr_end_offset)) {
  return NGTCP2_ERR_PROTO;
}
```

More specifically, `max_rx_offset < fr_end_offset` fails (`512 < 548`). I think this also happens because the application level encryption keys are installed earlier when using BoringSSL compared to when using OpenSSL (because `max_rx_offset` is set to `conn->pktns.crypto.rx.offset_base` in `conn_recv_handshake_pkt` and `conn->pktns.crypto.rx.offset_base` is initialized in `ngtcp2_conn_install_rx_keys` to `conn->crypto.strm.rx.last_offset`. 

I'm thinking the problem is the current design doesn't handle receiving handshake data after the application level encryption keys are installed. If I increase the value of `conn->pktns.crypto.rx.offset_base` by 36 (the size of the Finished CRYPTO frame), the handshake completes successfully.

A possible solution might be to calculate the application `offset_base` by calculating the total size of all handshake messages. This does require all handshake data to be received before receiving application data which I'm not sure is guaranteed by QUIC.
conn_recv_crypto: NGTCP2_ERR_PROTO
issue
created
tatsuhiro-t
username_1
A possible solution is let application tell libngtcp2 when all handshake data is received.
I'm not sure it is possible with BoringSSL.
comment
created
DaanDeMeyer
username_0
BoringSSL doesn't seem to provide any way of notifying the user when all handshake data has been received.

Switching to separate buffers could also solve the issue but that might require larger changes.
comment
created
tatsuhiro-t
username_1
SSL_provide_quic_data has encryption_level parameter.  Assuming that TLS stack can validate that crypto data is received in the correct offset, I think I can remove the offset validation from ngtcp2 and leave it to TLS stack.
comment
created
tatsuhiro-t
username_1
In https://github.com/ngtcp2/ngtcp2/tree/rework-crypto-stream branch, I switched to separate buffer per encryption level approach, removed max offset check from ngtcp2, and add crypto_level to ngtcp2_acked_crypto_offset.   Could you check that it works with BoringSSL?
comment
created
DaanDeMeyer
username_0
Yup, those changes fix the problem. Thanks for all the quick responses!
comment
closed
DaanDeMeyer
username_0
issue