Skip to content

Helpers for common Async functions#150

Closed
torinnd wants to merge 3 commits into
mirleft:mainfrom
torinnd:main
Closed

Helpers for common Async functions#150
torinnd wants to merge 3 commits into
mirleft:mainfrom
torinnd:main

Conversation

@torinnd
Copy link
Copy Markdown

@torinnd torinnd commented Jun 25, 2021

I propose introducing x509-async with the following general changes:

  • Async programs don't usually pass around file contents as [Cstruct.t]'s. They also prefer [Or_error.t]. To that end, various decode function inputs are lifted from [Cstruct.t -> ('a, [`Msg of string]) result] to [~contents:string -> 'a Or_error.t].
  • Another common pattern is "read file(s) into memory using [Async.Reader.file_contents], then apply a decode function". [of_pem_file] and [of_pem_dir] functions are provided to accommodate this.
  • Async programs tend to prefer working with [to_string] and [sexp_of_t] functions, so they're provided for the [verification_error], [ca_error], [validation_error] and [signature_error] types.

This could be used as a replacement for the [X509_async] module in tls-async's example test_server. It should also simplify any future projects that use tls-async.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jun 25, 2021

I'm not keen on these changes, it adds quite some duplication and complexity to X.509. Instead, my intuition is that

  • remove Cstruct from the interface (later remove cstruct from asn1-combinators & mirage-crypto) -- instead provide a bytes or string interface would benefit others as well (I'm not too convinced that Cstruct is valuable here (and in TLS)).
  • what is Or_error.t? can that be expressed as Stdlib type?
  • sexplib dependency: I'm not too keen on it. The pp_* functions for errors can be used to go from error to string, and you can easily pack that into a sexplib Atom.

@torinnd
Copy link
Copy Markdown
Author

torinnd commented Jun 25, 2021

['a Or_error.t] is type-equal to [('a, Error.t) result]. Error comes from Base so it can't easily be expressed as a Stdlib type, unfortunately. Roughly it's a lazily evaluated string and is semantically the same as [('a, [`Msg of string]) result], with the added benefit of excellent ppx support and other niceties one gets from using Base / Core / Async. Any code written with Async will want to convert [('a, [`Msg of string]) result] to ['a Or_error.t].

Dropping Cstruct sounds like a usability win to me! I'm not picky on how that happens; I've just found myself writing Cstruct.of_string far too many times in too many places against this library. Similarly it's not a big deal to call Fmt.to_to_string to convert from pp_* functions, it's just something that's come up a lot in my experience.

I do want to call out that the interface changes to the Authenticator module are going to be needed by any Async code running on Unix that wants to do chain-of-trust authentication (and therefore wants to read in a file with trust anchors). That and the of_pem_file functions are probably the pieces of this PR I'd be most sad to lose. They're pretty tightly coupled to IO so I don't immediately have an idea how to bridge the gap here. I'll think about it over the weekend.

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Jun 25, 2021

@torinnd as an intermediate solution -- similar to x509_lwt is present in tls.lwt -- we could have that I/O dependent functionality in tls-async. this is not the cleanest thing to do, but since 99% of users of x509 use it via tls, I'd be fine with that.

@torinnd
Copy link
Copy Markdown
Author

torinnd commented Jun 29, 2021

That works for me! I've rewritten this PR with this feedback in mind and will offer it to the ocaml-tls repository today or tomorrow.

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.

2 participants