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.

Sign up for GitHub

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

Jump to bottom

cleanups (mainly of mirage layer) #487

Merged
merged 9 commits into from Jan 4, 2024
Merged

cleanups (mainly of mirage layer) #487

merged 9 commits into from Jan 4, 2024

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Dec 22, 2023 •
edited

Best read commit-by-commit. This is still work-in-progress, there'll be support for shutdown (to conform to mirage-flow 4) in a later commit.

@hannesm hannesm changed the title cleanups (mainly of mirage layer) cleanups (mainly of mirage layer) - also shutdown Dec 22, 2023
@hannesm hannesm force-pushed the mirage-flow4 branch 2 times, most recently from aec2ef7 to 9df5c57 Compare January 3, 2024 19:02
dinosaure
dinosaure approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@dinosaure dinosaure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems right even if I think that the deletion of Flow‧close should be done into the shutdown (for a better clarity in my opinion but that's fine).

dinosaure
dinosaure reviewed Jan 4, 2024
View reviewed changes
mirage/tls_mirage.mli
@@ -20,6 +20,9 @@ module Make (F : Mirage_flow.S) : sig
with type error := error
and type write_error := write_error

(** [underlying t] returns the underlying flow. This is useful to extract
information such as [src] and [dst] of that flow. *)
val underlying : flow -> FLOW.flow
Copy link
Contributor

@dinosaure dinosaure Jan 4, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 it fixes #425

reynir
reynir approved these changes Jan 4, 2024
View reviewed changes
lib/packet.ml
and int_to_content_type = function
| 20 -> Some CHANGE_CIPHER_SPEC
| 21 -> Some ALERT
| 22 -> Some HANDSHAKE
| 23 -> Some APPLICATION_DATA
| 24 -> Some HEARTBEAT
| _ -> None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this is responsible for erroring out on HEARTBEAT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. The int_to_content_type is used by Reader.parse_record, which errors out with UnknownContent _ - which leads (in Engine.separate_records to Error (`Fatal (`UnknownContentType _)).

There is still an open question about error semantics -- which cases are fatal, which are not. For the heartbeat, this requires a negotiation in the client/server hello (which we do not support) -- thus anyone sending us a heartbeat packet violates the protocol.

@hannesm
engine/packet: remove HEARTBEAT decoding
d785a86 
Previously, we decoded the content type and errored when handling such a record
Now, we fail slightly earlier, upon receiving that content type
@hannesm
Engine: whitespace changes
f9b683b
@hannesm
Engine: separate_handshakes is not a result
da7d1cb
@hannesm
Copy link
Member Author

hannesm commented Jan 4, 2024

Seems right even if I think that the deletion of Flow‧close should be done into the shutdown (for a better clarity in my opinion but that's fine).

Ok, I removed that close commit from this PR.

@hannesm hannesm changed the title cleanups (mainly of mirage layer) - also shutdown cleanups (mainly of mirage layer) Jan 4, 2024
@hannesm hannesm merged commit 75d8c34 into mirleft:main Jan 4, 2024
1 check passed
@hannesm hannesm deleted the mirage-flow4 branch January 4, 2024 11:06
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 26, 2024
@hannesm
[new release] tls (5 packages) (0.17.4)
ea2b98b 
CHANGES:

* tls: handle half-closed connection properly: a received CLOSE_NOTIFY does not
  lead to a CLOSE_NOTIFY to be sent (a `send_close_notify` sends it explicitly)
  (mirleft/ocaml-tls#488 @hannesm)
* tls: modify return type of `handle_tls` - the Alert is now in the right hand
  side, and `` `Eof `` is explicit in the second part of the tuple
  (mirleft/ocaml-tls#488 @hannesm)
* tls: remove `can_handle_appdata`, the function `handshake_in_progress` is
  available (mirleft/ocaml-tls#488 @hannesm)
* tls-mirage: avoid exceptions in reneg and rekey (mirleft/ocaml-tls#487 @hannesm)
* tls: remove HEARTBEAT decoding - HEARTBEAT was never supported in this
  library, the decoder was superfluous (mirleft/ocaml-tls#487 @hannesm)
* tls-mirage: provide `underlying : flow -> FLOW.flow` (mirleft/ocaml-tls#487 @hannesm,
  fixes mirleft/ocaml-tls#425 @dinosaure)
* tls-mirage: implement mirage-flow 4 API (`val shutdown`) (mirleft/ocaml-tls#488 @hannesm)
* tls-eio: adapt to half-closed connections (mirleft/ocaml-tls#488 @talex5)
@hannesm hannesm mentioned this pull request Mar 26, 2024
Merged
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 26, 2024
@hannesm
[new release] tls (5 packages) (0.17.4)
c260053 
CHANGES:

* tls: handle half-closed connection properly: a received CLOSE_NOTIFY does not
  lead to a CLOSE_NOTIFY to be sent (a `send_close_notify` sends it explicitly)
  (mirleft/ocaml-tls#488 @hannesm)
* tls: modify return type of `handle_tls` - the Alert is now in the right hand
  side, and `` `Eof `` is explicit in the second part of the tuple
  (mirleft/ocaml-tls#488 @hannesm)
* tls: remove `can_handle_appdata`, the function `handshake_in_progress` is
  available (mirleft/ocaml-tls#488 @hannesm)
* tls-mirage: avoid exceptions in reneg and rekey (mirleft/ocaml-tls#487 @hannesm)
* tls: remove HEARTBEAT decoding - HEARTBEAT was never supported in this
  library, the decoder was superfluous (mirleft/ocaml-tls#487 @hannesm)
* tls-mirage: provide `underlying : flow -> FLOW.flow` (mirleft/ocaml-tls#487 @hannesm,
  fixes mirleft/ocaml-tls#425 @dinosaure)
* tls-mirage: implement mirage-flow 4 API (`val shutdown`) (mirleft/ocaml-tls#488 @hannesm)
* tls-eio: adapt to half-closed connections (mirleft/ocaml-tls#488 @talex5)
* tls-eio: implement Eio.Resource.Close (mirleft/ocaml-tls#489 @paurkedal, reviewed by @talex5)
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 27, 2024
@hannesm
[new release] tls (5 packages) (0.17.4)
7bc112a 
CHANGES:

* tls: handle half-closed connection properly: a received CLOSE_NOTIFY does not
  lead to a CLOSE_NOTIFY to be sent (a `send_close_notify` sends it explicitly)
  (mirleft/ocaml-tls#488 @hannesm)
* tls: modify return type of `handle_tls` - the Alert is now in the right hand
  side, and `` `Eof `` is explicit in the second part of the tuple
  (mirleft/ocaml-tls#488 @hannesm)
* tls: remove `can_handle_appdata`, the function `handshake_in_progress` is
  available (mirleft/ocaml-tls#488 @hannesm)
* tls-mirage: avoid exceptions in reneg and rekey (mirleft/ocaml-tls#487 @hannesm)
* tls: remove HEARTBEAT decoding - HEARTBEAT was never supported in this
  library, the decoder was superfluous (mirleft/ocaml-tls#487 @hannesm)
* tls-mirage: provide `underlying : flow -> FLOW.flow` (mirleft/ocaml-tls#487 @hannesm,
  fixes mirleft/ocaml-tls#425 @dinosaure)
* tls-mirage: implement mirage-flow 4 API (`val shutdown`) (mirleft/ocaml-tls#488 @hannesm)
* tls-eio: adapt to half-closed connections (mirleft/ocaml-tls#488 @talex5)
* tls-eio: implement Eio.Resource.Close (mirleft/ocaml-tls#489 @paurkedal, reviewed by @talex5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dinosaure dinosaure dinosaure approved these changes

@reynir reynir reynir approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hannesm @dinosaure @reynir