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

Fix make on master #51

Merged
merged 3 commits into from
Jan 31, 2022
Merged

Fix make on master #51

merged 3 commits into from
Jan 31, 2022

Conversation

tbrk
Copy link
Contributor

@tbrk tbrk commented Jan 30, 2022

make on master fails under OCaml 4.12.1. Is this normal? Maybe I've misunderstood something. Anyway this patch fixes it.

@dinosaure
Copy link
Member

Yes, ocaml-base64 is pretty stable about its API and we did not upgraded the distribution with new version of some libraries which deprecated some functions. I think bench needs a lower-bounds on core_bench to take the last version of it but the rest of your PR seems fine. Waiting the CI and merge it - however, we don't need a release.

Thanks!

@tbrk
Copy link
Contributor Author

tbrk commented Jan 30, 2022

OK. Thanks @dinosaure.

I was having a look at ocaml-base64 because it is used by ocaml-rpc to decode return values from XMLRPC servers. It turns out that the Python xmlrpc library encodes with line breaks (and I need to communicate with a Python server), so I was going to propose a patch to ocaml-base64 to accept line breaks. Then I understood that there is already a Base64_rfc2045 module, so it may be better just to update ocaml-rpc to use it's decode function.

@dinosaure
Copy link
Member

so I was going to propose a patch to ocaml-base64 to accept line breaks.

Hmmhmm, be care about that (/cc @hannesm). From a security perspective, we should not accept some else than the given alphabet - and if we get a "bad" character, we should just fail instead of silently accepts some non-conform characters. Base64_rfc2045 is mostly a derivation of the base64 encoding for emails - I'm not sure that it's the right direction.

May be the best should to add a trim function (before the decoding) which erases unexpected characters (such as whitespace and newline).

@tbrk
Copy link
Contributor Author

tbrk commented Jan 30, 2022

I understand. Thanks for the remark. Looking at the RFCs and discussions here, I got the impression that there were security issues. The doc for the Python library says that it linebreaks every 76 characters and is based on the rfc2045 interpretation of base64. Unfortunately, the server I want to communicate with seems to be written in Python!
I've made a pull request for XMLRPC. (CC @mseri)

@hannesm
Copy link
Member

hannesm commented Jan 30, 2022

well, similar to ?pad we can have a ?linebreak optional argument for decode and encode. I've myself written code somewhere to add linebreaks (or spaces) after 56 characters (in the case of DNS zone files, this is what is standard). For PEM files, 64 characters is the usual place to put a line break.

So, eventually there are two dimensions here: after how many characters and which character to insert. Maybe two optional arguments would be good to have.

@dinosaure dinosaure merged commit dfc0dab into mirage:master Jan 31, 2022
@dinosaure
Copy link
Member

Thanks, in few days, I will try to propose something about ?linebreak and definitely fix the issue #6.

@tbrk
Copy link
Contributor Author

tbrk commented Jan 31, 2022

Thanks @dinosaure. For what it's worth, here was the function that I hacked together to make my client work (with a suggestion from @mseri for the comments):

let base64_2045_decode s =
  let open Base64_rfc2045 in
  let buf = Buffer.create 1024 in
  let d = decoder (`String s) in
  let rec go () =
    match decode d with
    | `Flush s -> (Buffer.add_string buf s; go ())
    | `End -> Buffer.contents buf
    (* best-effort *)
    | `Malformed _   (* ignore; usually missing '\r' before '\n' *)
    | `Wrong_padding (* ignore *)
    | `Await -> go ()
  in
  go ()

One idea for the ocaml-rpc library is simply to allow callers to specify their own base64 decode function, while defaulting to Base64.decode_exn. This may be a good way to decouple ocaml-rpc from ocaml-base64 as far as data formats go.

dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Jan 24, 2023
CHANGES:

- Few fixes about benchmarks and tests (mirage/ocaml-base64#51, @tbrk, @dinosaure)
- Add missing dependency about `fmt` and fix the compilation for OCaml 5.0 (mirage/ocaml-base64#52, @kit-ty-kate)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Jan 24, 2023
CHANGES:

- Few fixes about benchmarks and tests (mirage/ocaml-base64#51, @tbrk, @dinosaure)
- Add missing dependency about `fmt` and fix the compilation for OCaml 5.0 (mirage/ocaml-base64#52, @kit-ty-kate)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants