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

close and shutdown semantics #452

Closed
hannesm opened this issue Sep 27, 2022 · 5 comments
Closed

close and shutdown semantics #452

hannesm opened this issue Sep 27, 2022 · 5 comments

Comments

@hannesm
Copy link
Member

hannesm commented Sep 27, 2022

As discussed in #451:

  • TLS 1.0: only properly closed connections could be used for resumption
  • TLS 1.0 - 1.2: a peer receiving a close_notify immediately has to drop everything and reply with a close_notify
  • TLS 1.3: close_notify is single direction only.

The implementation does not track close_notify being sent or received, but it replies to a close_notify (and any other alert) with a close_notify itself. To support shutdown Write (and return `Eof early if the other peer already sent close_notify) we need to track this in the engine state.

@bikallem
Copy link
Contributor

bikallem commented Dec 15, 2022

@hannesm @talex5

The implementation does not track close_notify being sent or received, but it replies to a close_notify (and any other alert) with a close_notify itself. To support shutdown Write (and return `Eof early if the other peer already sent close_notify) we need to track this in the engine state.

As I understand it, in TLS 1.3, close_notify is not really an EOF - the end of the file or full shutdown. It only means that the party sending close_notify will only close the write side of the connection. It should still be able to read from the same connection. Additionally, we no longer need to send close_notify in response to a receipt of close_notify nor should we expect it. Here is the paragraph from the spec which led me to come to this understanding:

Each party MUST send a "close_notify" alert before closing its write
side of the connection, unless it has already sent some error alert.
This does not have any effect on its read side of the connection.
  1. It would be preferable for the Tls.Engine.state to track receipt/send of 'close_notify' so as to reduce/eliminate duplication of logic from effectful implementations such as eio, lwt, async, unix etc. In my PR Dns client eio #458, I attempt to implement such a tracking. I imagine other effectul implementations will have to track similarly if they want to implement precise 'close_notify' semantics. Therefore, Tls.Engine.state/handle_tls could ease effectful implementations by incorporating this logic in the Tls.Engine.state itself.

  2. Tls.Engine.send_close_notify tls preferably return Some close_notify_bytes if 'close_notify' hasn't yet been sent and None if it has already. Again, this is to ease effectful implementations and guide them towards correct implementations. As an anecdote, while working on Dns client eio #458, at one point I incorrectly sent two consecutive 'close_notify' and it resulted in TLS MACMismatch error (not sure how two close_notify could lead to the error but just pointing out that it did result in an error). The error doesn't exist in current eio implementation. However, as I understand we don't fully implement TLS 1.3 'close_notify' semantics(as enumerated above) as yet.

  3. Tls.Engine.handle_tls data not return Response close_notify if it encounters 'close_notify' in data. TLS 1.3 (https://www.rfc-editor.org/rfc/rfc8446#section-6.1) says this is no longer required and advises against doing so. Currently, we immediately send 'close_notify' in response to a 'close_notify'. If we are to implement the TLS 1.3 close_notify spec correctly, then this behavior leads to a full shutdown of the socket, i.e. both sides have indicated that they won't be writing to the connection any more therefore it is a full shutdown. I haven't verified fully but I suspect currently both lwt and eio still write to the connection even after sending 'close_notify' message to the other party. I'd like to point out this scenario here (https://github.com/bikallem/ocaml-tls/blob/5580b365f8ced48bf9722826839f6febd66f9c74/eio/tls_eio.ml#L48-L57).

  4. Tls.Engine.send_application_data return None if 'close_notify' is already sent. It already uses option as its return type so it can send None in the latter case too.

@hannesm
Copy link
Member Author

hannesm commented Dec 15, 2022

Thanks for your huge comment, just to be clear:

As I understand it, in TLS 1.3, close_notify is not really an EOF - the end of the file or full shutdown. It only means that the party sending close_notify will only close the write side of the connection. It should still be able to read from the same connection.

Yes, for TLS 1.3 this is single direction close only - similar to a shutdown (Write|Read - depending whether you send or received it) on Unix.

Additionally, we no longer need to send close_notify in response to a receipt of close_notify nor should we expect it.

Yes, that is also what I have in mind.

Now, this library implements also earlier versions of TLS, so the API should cope with these earlier versions as well. In earlier TLS versions, a close_notify has different semantics, as I mentioned in this issue.

I'm not sure I understand your 4 points. The reason for this issue to exist is that there should be some API changes, and all supported TLS protocol versions need to be respected.

@bikallem
Copy link
Contributor

bikallem commented Dec 16, 2022

I'm not sure I understand your 4 points. The reason for this issue to exist is that there should be some API changes, and all supported TLS protocol versions need to be respected.

Apologies for the noise. I will keep the discussion only to close_notify and shutdown semantics.

Now, this library implements also earlier versions of TLS, so the API should cope with these earlier versions as well. In earlier TLS versions, a close_notify has different semantics, as I mentioned in this issue.

TLS 1.0 and TLS 1.1 has been deprecated since sometime in 2021. Do you still intend to provide support, i.e. updates for them?

With regards to TLS 1.2 and 1.3, as I understand it, at the moment eio effectual layer is in the half-way house with regards to close_notify spec. What I mean is that we send 'close_notify' in response to a 'close_notify' as required by TLS 1.2; however, we don't close the write part of the connection, as required by it, i.e. we still write to the connection after sending 'close_notify'. This is in effect a full shutdown since by now both parties have indicated that they will not be writing to the connection any more. However, the same behavior - not closing write part of the connection when receiving 'close_notify' - is conformant to the TLS 1.3, however, tls 1.3 doesn't require to send 'close_notify' is response to 'close_notify'. Should this be rectified in tls-eio?

@hannesm
Copy link
Member Author

hannesm commented Dec 16, 2022

TLS 1.0 and TLS 1.1 has been deprecated since sometime in 2021

Ok. What can I say, apart from: this library supports TLS 1.0 and TLS 1.1. I've not looked into recent deployment statistics how widely TLS 1.0 and TLS 1.1 are still used. As far as I understand, the semantics of close_notify in TLS 1.2 and TLS 1.1 and TLS 1.0 are all the same.

With regards to TLS 1.2 and 1.3, as I understand it, at the moment eio effectual layer is in the half-way house with regards to close_notify spec.

I'm sorry to hear. From what I understand, the lwt and mirage layers (and async as well) do implement the TLS 1.2 semantics: close is always a full close, a close_notify is replied to with a close_notify. I lack understanding of eio flow semantics, and won't have time this year to figure the details out.

@hannesm
Copy link
Member Author

hannesm commented Mar 21, 2024

the way to go is #488. I'll close this issue in the meantime.

@hannesm hannesm closed this as completed Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants