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

yomimono · 2019-05-13T17:02:31Z

Here's another attempt at #35, which tries to implement a more sensible API.

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

yomimono · 2019-05-13T17:03:06Z

/cc @linse

talex5

Thanks for doing this! It looks good to me.

There's one bit I'm not sure about though: I see that the client and server both end up running a vchan service on the same port and waiting for dom0 to connect. Does that mean that a unikernel can either be a client or be a server, but not both? If outgoing client and incoming service requests go over the same vchan (I hadn't thought of that before), maybe there should be a single vchan server with a single listen loop, and RExec_client.connect ... should just be RExec.request_service t ..., returning an RExec_client.Flow.t? Apart from the that, the new API looks perfect to me :-)

cfcs

I think it would be super nice to get this merged, thanks for writing this!
I left some comments, but none that I feel should block the merging of this.

@yomimono do you have an opinion about @talex5's question?

cfcs · 2019-07-14T01:24:36Z

lib/rExec_client.mli

+
+  val write : t -> Cstruct.t -> [ `Ok of unit | `Eof ] Lwt.t
+
+  val writef : t -> ('a, unit, string, unit S.or_eof Lwt.t) format4 -> 'a


Seems like documenting the trailing \n being added here would make sense, if we want to keep it.

It seems confusing to me that write would not add a newline, while writef does.
This behavior is in line with the [current implementation of RExec.writef]https://github.com/mirage/mirage-qubes/blob/master/lib/rExec.ml#L66-L83) which is documented in the mli interface (which I also find confusing). :)

cfcs · 2019-07-14T01:36:28Z

lib/rExec_common.ml

+
+let rec send t ~ty data =
+  let data, data' = Cstruct.split data (min max_data_chunk (Cstruct.len data)) in
+  let hdr = Cstruct.create sizeof_msg_header in


if we wrapped the body in a recursive function we could avoid an allocation of Cstruct.create sizeof_msg_header by reusing it (necessitating only the set_msg_header_len hdr (Cstruct.len data |> Int32.of_int); in the loop).
Since max_data_chunk = 4096 this might make sense for large sends like file transfers?

cfcs · 2019-07-14T01:52:48Z

lib/rExec_client.ml

+  Log.debug (fun f -> f "connection with dom0 established (version %ld)" version);
+  RExec_common.send server ~ty:`Trigger_service tsp >>= function
+  | `Eof -> Lwt.return @@ Error `Closed
+  | `Ok () ->


XXX Not related to this PR, but I went looking for the receiving side of this, and fell over qubes-core-agent-linux/qrexec/qrexec-agent.c:handle_server_exec_request which seems to assume that buf is 0-terminated:

if ((hdr->type == MSG_EXEC_CMDLINE || hdr->type == MSG_JUST_EXEC) && !strstr(buf, ":nogui:")) { /* seems to me this strstr() should probably be a memmem() bounded to buf_len */

cfcs · 2019-07-14T02:08:06Z

lib/rExec_client.ml

+    in
+    write_string service ~dst_offset:0 ~max_len:64;
+    write_string vm ~dst_offset:64 ~max_len:32;
+    write_string identifier ~dst_offset:(64+32) ~max_len:32;


Any reason this doesn't use the [%struct] accessors functions like set_trigger_service_parameters_service_name? (Well that is a ridiculously long function name, but it does spare us the trouble of having to be careful to get the offsets and lengths right)

In the definition they seem to suggest these should be 0-terminated, so maybe we should pass max_len:whatever-1, or validate + fail if the application decides to pass us garbage input?

The code in QubesOS does add a zero byte at the very end of the char array in any case. But I agree that we should ensure it's also zero-terminated on our end because otherwise we potentially lose one byte of the identifier. https://github.com/QubesOS/qubes-core-qrexec/blob/master/daemon/qrexec-daemon.c#L817

I noticed in qrexec-client-vm that they just pass SOCKET every time as the identifier. Maybe we should do the same and remove identifier from the arguments making it simpler.

cfcs · 2019-07-14T02:17:46Z

lib/rExec_client.ml

+
+  let rec read_line t =
+    let stdout = Cstruct.to_string t.stdout_buf
+    and stderr = Cstruct.to_string t.stderr_buf


I was about to pose the cheeky question of whether converting both to string on each call was necessary, but to my surprise I found that Cstruct doesn't seem to have a index function. That sucks.

Perhaps we can do something like storing the length of the previously examined stdout_buf and stderr_buf so we can avoid checking the same bytes on long lines over and over?

I'd still prefer if we could avoid converting both buffers if we know we are always going to go with the first if there's a match; would it be OK with you to serialize these so the second conversion only happens when newline stdout is None?

cfcs · 2019-07-14T02:19:34Z

lib/rExec_client.ml

+      let retval = String.sub stdout 0 i in
+      t.stdout_buf <- Cstruct.shift t.stdout_buf (i + 1);
+      Lwt.return (`Stdout retval)
+    | _, Some i ->


Not sure this is a problem, but as long as there's stdout_buf newlines we will never return anything from `Stderr.
It doesn't seem entirely fair, but I guess not an issue in real life.

This is similar to how read works FWIW. Stdout has priority over stderr.

cfcs · 2019-07-14T02:22:03Z

lib/formats.ml

+      service_name : uint8_t [@len 64];
+      target_domain : uint8_t [@len 32];
+      request_id : uint8_t [@len 32]
+    } [@@little_endian]


I think it would make sense to preserve the these should be \0-terminated comments from the original header file?

cfcs · 2019-07-14T02:25:54Z

lib/rExec_common.ml

+    let i = String.index s chr in
+    Some (String.sub s 0 i, String.sub s (i + 1) (String.length s - i - 1))
+  with Not_found ->
+    None


I think we could avoid exceptions with something like:

let split chr s = match String.index_opt s chr with | None -> None | Some i -> Some (String.sub s 0 i, String.sub s (i + 1) (String.length s - i - 1))

String.index_opt was added in 4.05.0 and that's our new minimum, so I concur.

reynir · 2019-08-19T11:54:55Z

I think this doesn't take account for qrexec calls can be to and from the VM during the same session - so the server part will eat client messages, and/or the client part will eat server messages. I may be wrong, but I remember thinking of how to solve this when I wanted to implement this myself. I will dig some further...

reynir · 2019-08-04T20:08:08Z

lib/rExec_common.ml

+    let i = String.index s chr in
+    Some (String.sub s 0 i, String.sub s (i + 1) (String.length s - i - 1))
+  with Not_found ->
+    None


String.index_opt was added in 4.05.0 and that's our new minimum, so I concur.

reynir · 2019-08-04T20:11:09Z

lib/rExec_common.ml

+let disconnect = QV.disconnect
+
+let vchan_base_port =
+  match Vchan.Port.of_string "512" with


Where does this constant come from?

reynir · 2019-08-04T20:16:53Z

lib/rExec_common.ml

+  if Cstruct.len data' = 0
+  then QV.send t [hdr; data]
+  else QV.send t [hdr; data] >>= function
+    | `Eof -> return `Eof


Do we really want to return `Eof when we haven't sent all data?

reynir · 2019-08-04T20:51:05Z

lib/rExec_client.ml

+      let retval = String.sub stdout 0 i in
+      t.stdout_buf <- Cstruct.shift t.stdout_buf (i + 1);
+      Lwt.return (`Stdout retval)
+    | _, Some i ->


This is similar to how read works FWIW. Stdout has priority over stderr.

reynir · 2019-08-14T12:33:23Z

lib/rExec_client.ml

+    in
+    write_string service ~dst_offset:0 ~max_len:64;
+    write_string vm ~dst_offset:64 ~max_len:32;
+    write_string identifier ~dst_offset:(64+32) ~max_len:32;


The code in QubesOS does add a zero byte at the very end of the char array in any case. But I agree that we should ensure it's also zero-terminated on our end because otherwise we potentially lose one byte of the identifier. https://github.com/QubesOS/qubes-core-qrexec/blob/master/daemon/qrexec-daemon.c#L817

reynir · 2019-08-14T12:47:34Z

lib/rExec_client.ml

+    in
+    write_string service ~dst_offset:0 ~max_len:64;
+    write_string vm ~dst_offset:64 ~max_len:32;
+    write_string identifier ~dst_offset:(64+32) ~max_len:32;


I noticed in qrexec-client-vm that they just pass SOCKET every time as the identifier. Maybe we should do the same and remove identifier from the arguments making it simpler.

reynir · 2019-09-30T09:11:45Z

Oh, sorry. I meant to submit pending review in #39 and not these old comments

talex5 · 2019-10-11T12:49:43Z

Closing this as it appears to be superseded by #39.

yomimono and others added 8 commits May 1, 2019 19:06

since we require dune, refer to it in makefile

1cab7b2

formats: add trigger_service_params & service_refused

cf58575

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

formats: add string_of_type for message types

e853ade

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

qrexec: reword error message for unexpected recv msg

b4ea728

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

move previous rexec to rexec_server

c48f093

require ocaml 4.05, and test compilers 4.05 and 4.07

7c0d9c7

move common stuff to rExec_common

c6e66cc

implement rExec_client

d241564

talex5 approved these changes May 14, 2019

View reviewed changes

cfcs reviewed Jul 14, 2019

View reviewed changes

talex5 mentioned this pull request Sep 27, 2019

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

Merged

reynir reviewed Sep 30, 2019

View reviewed changes

reynir mentioned this pull request Sep 30, 2019

Rexec client and server #39

Merged

talex5 closed this Oct 11, 2019

hannesm mentioned this pull request Nov 2, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yomimono commented May 13, 2019

yomimono commented May 13, 2019

talex5 left a comment •

edited

cfcs left a comment

cfcs Jul 14, 2019

cfcs Jul 14, 2019

cfcs Jul 14, 2019

cfcs Jul 14, 2019

reynir Aug 14, 2019

reynir Aug 14, 2019

cfcs Jul 14, 2019

cfcs Jul 14, 2019

reynir Aug 4, 2019

cfcs Jul 14, 2019

cfcs Jul 14, 2019

reynir Aug 4, 2019

reynir commented Aug 19, 2019

reynir Aug 4, 2019

reynir Aug 4, 2019

reynir Aug 4, 2019

reynir Aug 4, 2019

reynir Aug 14, 2019

reynir Aug 14, 2019

reynir commented Sep 30, 2019

talex5 commented Oct 11, 2019


		val write : t -> Cstruct.t -> [ `Ok of unit \| `Eof ] Lwt.t

		val writef : t -> ('a, unit, string, unit S.or_eof Lwt.t) format4 -> 'a

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

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

Conversation

yomimono commented May 13, 2019

yomimono commented May 13, 2019

talex5 left a comment • edited

Choose a reason for hiding this comment

cfcs left a 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

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 Aug 19, 2019

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

talex5 commented Oct 11, 2019

talex5 left a comment •

edited