Skip to content

peer_name should be strongly typed#434

Merged
hannesm merged 1 commit into
mirleft:mainfrom
torinnd:main
Jul 24, 2021
Merged

peer_name should be strongly typed#434
hannesm merged 1 commit into
mirleft:mainfrom
torinnd:main

Conversation

@torinnd
Copy link
Copy Markdown
Contributor

@torinnd torinnd commented Jun 22, 2021

Change [peer_name] to a [[`host] Domain_name.t]. This passes the obligation to handle errors to the caller.

[peer_name] is defined as a [string option] and passed around as such until it arrives at the function [host_name_opt] in lib/handshake_common.ml, where it is converted into a [[`host] Domain_name.t option]. A malformed string that cannot be lifted into a [[`host] Domain_name.t] will return [None], which will disable hostname verification. This will cause, for example, connections to IP addresses like “1.2.3.4” over HTTPS to succeed where one might’ve expected them to fail.

I haven't updated the lwt/ code and expect that won't compile, so I'm tagging this as a draft. It's also an interface change and probably should be rolled into a major version bump, if the project is interested in this change.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jun 22, 2021

the API change looks good to me -- tls predates the domain-name library, that's the reason why peer_name is a string option at the moment. I'll need to refresh my brain about length restrictions in SNI and Domain names -- and whether there are use cases where the peer name is not a host name.

Comment thread lib/reader.ml Outdated
@torinnd
Copy link
Copy Markdown
Contributor Author

torinnd commented Jun 23, 2021

the API change looks good to me -- tls predates the domain-name library, that's the reason why peer_name is a string option at the moment. I'll need to refresh my brain about length restrictions in SNI and Domain names -- and whether there are use cases where the peer name is not a host name.

I think the only practical limit is enforced by DNS and limits [peer_name] to 253 bytes. The TLS RFC dances around the issue a bit (16 bit length for the "list" of server names, but there must only be one of each type, and DNS is the only type).

@torinnd torinnd marked this pull request as ready for review June 25, 2021 09:22
@torinnd
Copy link
Copy Markdown
Contributor Author

torinnd commented Jun 25, 2021

I think there are ocaml-ci problems, possibly with tls-async.opam? I think aside from that this PR is ready for review.

- cstruct-async -> (problem)
    tls-async dev requires >= v0.14
    Rejected candidates:
      cstruct-async.6.0.0: Incompatible with restriction: >= v0.14
      cstruct-async.5.2.0: Requires cstruct = 5.2.0
      cstruct-async.5.1.1: Requires cstruct = 5.1.1
      cstruct-async.5.0.0: Requires cstruct = 5.0.0
      cstruct-async.4.0.0: Requires cstruct = 4.0.0
      ...
- ocaml -> ocaml.4.12.0
    ocaml-base-compiler 4.12.0 requires = 4.12.0
- ocaml-base-compiler -> ocaml-base-compiler.4.12.0
    User requested = 4.12.0
2021-06-25 09:23.37 [INFO] = ubuntu-21.04-4.12 =
2021-06-25 09:23.37 [INFO] Can't find all required versions.

EDIT: Ah, I needed to merge the changes from main! CI is working now.

@torinnd torinnd force-pushed the main branch 2 times, most recently from 40a5777 to 629968b Compare June 25, 2021 10:30
@torinnd
Copy link
Copy Markdown
Contributor Author

torinnd commented Jul 6, 2021

I needed to make some adjustments to the test suite to get this to compile. The long strings like foofoofoofoo... now include '.' every 60 characters. The case in readertests.ml where the string "127.0.0.1" was passed as a valid SNI value is amended to "example.com" (and lengths are adjusted accordingly).

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jul 24, 2021

Thanks for your work, this looks good to go.

@hannesm hannesm merged commit 21bf613 into mirleft:main Jul 24, 2021
hannesm added a commit to hannesm/ocaml-tls that referenced this pull request Jul 28, 2021
The peer_name field is configured by the client (emitting a SNI (Server Name
Indication) extension in the ClientHello). The own_name is filled by the server
from the ClientHello SNI.

This extends mirleft#434.
hannesm added a commit to hannesm/ocaml-tls that referenced this pull request Jul 28, 2021
The peer_name field is configured by the client (emitting a SNI (Server Name
Indication) extension in the ClientHello). The own_name is filled by the server
from the ClientHello SNI.

This extends mirleft#434.
hannesm added a commit to hannesm/opam-repository that referenced this pull request Aug 2, 2021
CHANGES:

* Breaking: peer_name (in config and epoch data, also own_name) is now a
  [`host] Domain_name.t instead of a string. (mirleft/ocaml-tls#434 mirleft/ocaml-tls#438 @torinnd @hannesm)
* Add a X509_async module (mirleft/ocaml-tls#435 @torinnd)
* Client and server constructor log messages are on the debug level (mirleft/ocaml-tls#436
  reported by @talex5, fix by @hannesm)
* Adapt to cstruct 6.0.0 API (Cstruct.len is deprecated) mirleft/ocaml-tls#439 @hannesm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants