Rexec client and server #39

reynir · 2019-09-25T22:54:39Z

This PR adds functionality to the qrexec implementation such that receiving and sending qrexec rpc calls concurrently is possible.

I will need to review this myself again, but it seems to work provided mirage/ocaml-vchan#132 is merged.

reynir · 2019-09-26T10:45:13Z

Added @yomimono as co-author since a lot of the code was based on theirs (thanks!!)

yomimono · 2019-09-26T16:48:32Z

please add @linse also :)

reynir · 2019-09-26T18:59:48Z

Done! :-)

reynir · 2019-09-28T16:17:16Z

I had a discussion with @cfcs regarding RExec.Client_flow.writef. It differs from RExec.Flow.writef in that it doesn't add a newline. We discussed renaming it RExec.Client_flow.writefmt and adding a RExec.Flow.writefmt with equivalent semantics.

cfcs · 2019-09-29T22:51:18Z

leaving a note here about concatenating printf format6 values to avoid first evaluating, then string concatenating (for the \n thing)

# open CamlinternalFormatBasics ;;
# let f (Format (fmt, n)) =
  let joined = Format (concat_fmt fmt (Char_literal ('z', End_of_format)), "") in
  Printf.sprintf joined
in f "a %s " "b";;
- : string = "a b z"

So @hannesm suggested this instead:

let f fmt = Printf.sprintf (fmt ^^ "z") in f "a %s " "b";;
- : string = "a b z"

cfcs · 2019-09-29T22:58:06Z

lib/rExec.ml

+  let next_msg t =
+    recv t.dstream >|= function
+    | `Ok (`Data_stdout, data) ->
+      t.stdout_buf <- Cstruct.append t.stdout_buf data;


I think the Cstruct.append here is superfluous since next_msg is only called in read when Cstruct.len t.stdout_buf = 0 && Cstruct.len t.stderr_buf = 0 ?

Is the append here to prevent making a reference to the data buffer or can we do t.stdout_buf <- data ;?

(ping @reynir @yomimono @linse )

I've reviewed it, and it seems to be a leftover from #36 which has read_line that would require buffering. I opted to not implement read_line when writing Client_flow. It should be safe to return directly from next_msg. I can also implement read_line if it's desired.

I think read_line might make sense as a utility function for the line-based protocols, but let's not let that block this PR :)

lib/rExec.ml

cfcs · 2019-09-29T23:14:38Z

lib/rExec.ml

+    | `Ok (ty, _) ->
+      Log.err Formats.Qrexec.(fun f -> f "unexpected message of type %ld (%s) received; \
+                                          ignoring it" (int_of_type ty) (string_of_type ty));
+      `Ok t


Maybe we should err on the side of caution here and terminate? Do we anticipate any unhandled messages, or would this only happen in qrexec upgrades?

Qrexec version is negotiated when connecting, so I think this would only happen if the other end is misbehaving. How do you suggest we terminate?

I would kill the flow from here if that makes sense?

See reynir#1 for a suggested implementation

Looks good to me. Do we need to QV.disconnect t.dstream on Eof?

lib/rExec.ml

lib/rExec.mli

reynir · 2019-09-30T09:50:33Z

The CI fails at OCaml 4.04.0. I use Hashtbl.find_opt which is from OCaml 4.05.0. Mirage requires OCaml >= 4.05.0, so I think the CI should be updated to reflect this. :-)

cfcs

Had some more uncommitted comments

lib/rExec.ml

hannesm · 2019-10-10T10:18:37Z

from what I understand (please correct me if I'm wrong)

this superseeds implement an rexec_client; move previous rexec code to rexec_server #36 (@yomimono @linse ?)
there are some outstanding comments by @cfcs in this PR (which could be addressed in here or in a subsequent PR)
- read_line Rexec client and server #39 (comment)
- kill the flow Rexec client and server #39 (comment)
- surround if/then/else with begin .. end Rexec client and server #39 (comment)
- handle dom0 closing the control channel properly (remove stuff from hashtable) Rexec client and server #39 (comment)
- Logs.warn for unsoiicited answers Rexec client and server #39 (comment)
CI fails with older compilers (but we can and should safely update to 4.05+)
- this can easily be done by updating .travis.yml

thanks for reading, maybe we can converge to get this merged (and a mirage-qubes release)!?

reynir · 2019-10-21T11:26:29Z

@hannesm sorry, I have been travelling with little opportunity for computer time. Yes, that is correct. I have adapted some of the suggestions made by @cfcs, and #44 ups the OCaml compiler version. What is still outstanding is:

read_line Rexec client and server #39 (comment)
kill the flow Rexec client and server #39 (comment)
surround if/then/else with begin .. end Rexec client and server #39 (comment)

I don't find the latter an issue personally, but I'm open for adding begin .. end. I am personally not interested in adding read_line, so I think it would be better as a separate PR. Regarding killing the flow I've made a PR to my own PR with an implementation that I'm considering reynir#1.

I would love to see this merged and released soon.

cfcs · 2019-10-28T08:19:14Z

@reynir I'm happy with your kill flow implementation. I haven't tested it, but I think it looks reasonable, and I think we should get this merged.
My comments aside, all of which I consider addressed, are we ready to merge?

hannesm · 2019-10-28T09:30:48Z

a rebase onto master would be great (than CI should pass AFAICT)!

Co-authored-by: linse <sschirme@gmail.com>

Co-authored-by: Mindy <meetup@yomimono.org> Co-authored-by: linse <sschirme@gmail.com>

Instead of as debug messages. We would not want to hide these.

Co-Authored-By: C For C's Sake <cfcs@users.noreply.github.com>

We raise an exception if the arguments do not fit in the cstruct. Also some whitespace changes.

Try to reflect that it's the remote service exit code

cfcs · 2019-10-28T10:34:22Z

Looks like CI is happy :)

hannesm · 2019-10-28T12:41:31Z

for me, the remaining question is about kill flow implementation -- reynir#1 (on unexpected message?, and EOF?) -- which is not part of this PR!?

reynir · 2019-10-28T14:39:43Z

I was considering doing kill flow in a separate PR. I noticed the existing server implementation did not close the flow either, but rereading the code now I noticed that it raises an exception (by Lwt.fail) on unexpected message types. It seems to me that a misbehaving client can then take down the whole unikernel?!

hannesm · 2019-11-01T12:35:17Z

I agree with @reynir that the kill behaviour can be improved in a future PR (there are other components, such as qubesdb, which lead to Lwt.fail if they receive unexpected messages). Merging after review and ok from @cfcs to include into the next release. Thanks for your patience and submitting this PR!

reynir force-pushed the rexec_client_server-wip branch from d794ca0 to 8f9a254 Compare September 26, 2019 10:38

reynir force-pushed the rexec_client_server-wip branch from 8f9a254 to edf90d6 Compare September 26, 2019 18:59

talex5 mentioned this pull request Sep 27, 2019

since we require dune, refer to it in makefile #40

Merged

cfcs requested changes Sep 29, 2019

View reviewed changes

reynir mentioned this pull request Sep 30, 2019

implement an rexec_client; move previous rexec code to rexec_server #36

Closed

cfcs requested changes Oct 5, 2019

View reviewed changes

lib/rExec.ml Show resolved Hide resolved

lib/rExec.ml Outdated Show resolved Hide resolved

lib/rExec.ml Outdated Show resolved Hide resolved

talex5 mentioned this pull request Oct 11, 2019

Update formats #43

Merged

yomimono and others added 11 commits October 28, 2019 11:08

qrexec: reword error message for unexpected recv msg

5b0e3dc

Co-authored-by: linse <sschirme@gmail.com>

Implement initiating qrexec calls

cb2951b

Co-authored-by: Mindy <meetup@yomimono.org> Co-authored-by: linse <sschirme@gmail.com>

Clarify comment

82f84ca

Log unexpected messages as errors

f15d430

Instead of as debug messages. We would not want to hide these.

Apply suggestions from code review

c404dec

Co-Authored-By: C For C's Sake <cfcs@users.noreply.github.com>

lib/rExec.mli: Break long lines

8bb90ad

Rename data to params

ebfa546

Handle client cleanup a little better

d8e2fe1

RExec.qrexec: check lengths of arguments

8ba639c

We raise an exception if the arguments do not fit in the cstruct. Also some whitespace changes.

RExec: Use [Exit_code] instead of [Done]

71b0724

Try to reflect that it's the remote service exit code

Add changes suggested by @cfcs

8c96707

reynir force-pushed the rexec_client_server-wip branch from a3b09be to 8c96707 Compare October 28, 2019 10:08

hannesm merged commit e9f455c into mirage:master Nov 1, 2019

This was referenced Nov 1, 2019

Implement doing qrexec calls #23

Closed

release mirage-qubes 0.8.0 ocaml/opam-repository#15189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rexec client and server #39

Rexec client and server #39

reynir commented Sep 25, 2019

reynir commented Sep 26, 2019

yomimono commented Sep 26, 2019

reynir commented Sep 26, 2019

reynir commented Sep 28, 2019

cfcs commented Sep 29, 2019 •

edited

Loading

cfcs Sep 29, 2019

reynir Sep 30, 2019

cfcs Sep 30, 2019

cfcs Sep 29, 2019

reynir Sep 30, 2019

cfcs Sep 30, 2019

reynir Oct 21, 2019

cfcs Oct 28, 2019

reynir commented Sep 30, 2019

cfcs left a comment

hannesm commented Oct 10, 2019

reynir commented Oct 21, 2019

cfcs commented Oct 28, 2019

hannesm commented Oct 28, 2019

cfcs commented Oct 28, 2019

hannesm commented Oct 28, 2019

reynir commented Oct 28, 2019

hannesm commented Nov 1, 2019

Rexec client and server #39

Rexec client and server #39

Conversation

reynir commented Sep 25, 2019

reynir commented Sep 26, 2019

yomimono commented Sep 26, 2019

reynir commented Sep 26, 2019

reynir commented Sep 28, 2019

cfcs commented Sep 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reynir commented Sep 30, 2019

cfcs left a comment

Choose a reason for hiding this comment

hannesm commented Oct 10, 2019

reynir commented Oct 21, 2019

cfcs commented Oct 28, 2019

hannesm commented Oct 28, 2019

cfcs commented Oct 28, 2019

hannesm commented Oct 28, 2019

reynir commented Oct 28, 2019

hannesm commented Nov 1, 2019

cfcs commented Sep 29, 2019 •

edited

Loading