Problem: Nitro helper requires a separate enclave launching(#93) #109

linfeng-crypto · 2021-06-24T06:25:08Z

add launch-all and enclave subcommand:

SUBCOMMANDS:
    enclave       enclave sub-commands
    help          Prints this message or the help of the given subcommand(s)
    init          Create config + keygen
    launch-all    launch all
    start         start tmkms process

launch-all will start nitro enclave / enclave vsock-proxy / enclave helper all in one.

the enclave submand include the following:

SUBCOMMANDS:
    help           Prints this message or the help of the given subcommand(s)
    info           get tmkms info
    run            run enclave
    stop           stop enclave
    vsock-proxy    launch vsock proxy

move the enclave log server to command tmkms-nitro-helper enclave run command.

…m#93)

tomtau

given nitro-cli isn't yet exposed as a library properly, wouldn't be easier just call the shell command?
also: https://github.com/crypto-com/tmkms-light/pull/109/checks?check_run_id=2902100730#step:5:22

tomtau

looks good -- it seems much simpler with just Commands.
left a few suggestions and questions

tomtau · 2021-06-25T07:17:54Z

providers/nitro/nitro-helper/src/command.rs

+    let proxy_opt = VSockProxyOpt {
+        remote_addr: format!("kms.{}.amazonaws.com", aws_region),
+        ..Default::default()
+    };
+    let all_config = Config {
+        sign_opt: nitro_sign_opt,
+        enclave: enclave_opt,
+        vsock_proxy: proxy_opt,
+    };


given the "launch all" command is optional, I'm not sure if it makes sense to bundle the extra config to the global config?

#109 (comment)

tomtau · 2021-06-25T07:20:20Z

providers/nitro/nitro-helper/src/command.rs

+        cmd.contains(&"tmkms-nitro-helper".to_string())
+            && cmd.contains(&"enclave".to_string())


won't this always fail? it'll only match a command that has all those 3 names in its name?
also the functional is called check_vsock_proxy, why does it check for tmkms-nitro-helper...?

tomtau · 2021-06-25T07:25:09Z

providers/nitro/nitro-helper/src/command/launch_all.rs

+use crossbeam_channel::{bounded, Receiver, Sender};
+use std::thread::{self, sleep};
+use std::time::Duration;
+


for pub functions or struct, it'd be good to have some doc explaining what it does -- e.g. stop_enclave_sender isn't entirely obvious from the type

tomtau · 2021-06-25T07:27:09Z

providers/nitro/nitro-helper/src/command/launch_all.rs

+use crate::command::nitro_enclave::run_vsock_proxy;
+use crate::command::nitro_enclave::{describe_enclave, run_enclave};
+use crate::config::Config;
+use crossbeam_channel::{bounded, Receiver, Sender};


why is crossbeam used instead of std::sync::mpsc::channel? is it because the std one isn't deriving Sync or something like that?

later, we can use crossbeam::select in a loop, process data from TCP stream or a stop signal.

tomtau · 2021-06-25T07:27:45Z

providers/nitro/nitro-helper/Cargo.toml

@@ -11,7 +11,9 @@ main = ["sysinfo", "reqwest"]
 [dependencies]
 anomaly = "0.2"
 bytes = "= 0.5"
+ctrlc = "3.1"


Suggested change

ctrlc = "3.1"

ctrlc = "3"

tomtau · 2021-06-25T07:38:46Z

providers/nitro/nitro-helper/src/command/vsock_proxy.rs

@@ -0,0 +1,29 @@
+use crate::config::VSockProxyOpt;


it seems this module is unused? (only launch_all and nitro_enclave are denoted "mod" in command?)

tomtau · 2021-06-25T07:41:34Z

providers/nitro/nitro-helper/src/config.rs

+
+#[derive(Serialize, Deserialize, Default, Debug)]
+pub struct Config {
+    pub sign_opt: NitroSignOpt,


given only NitroSignOpt is used by the normal launch, perhaps separating the other two in a different struct? so it's still backwards-compatible and for "launch-all", one can have an extra config?

tomtau · 2021-06-25T08:10:41Z

providers/nitro/nitro-helper/src/main.rs

+    #[structopt(name = "info", about = "get tmkms info")]
+    Info,
+    #[structopt(name = "run", about = "run enclave")]
+    RunEnclave {


quite useful to have these "shorthand" commands, so one doesn't need to repeat the args (it'd just be automatically read or taken from a config file) 👍

tomtau · 2021-06-25T08:11:52Z

providers/nitro/nitro-helper/src/main.rs

@@ -37,49 +82,104 @@ enum TmkmsLight {
    #[structopt(name = "start", about = "start tmkms process")]
    /// start tmkms process (push config + start up proxy and state persistence)
    Start {
-        #[structopt(short)]
-        config_path: Option<PathBuf>,
+        #[structopt(short, default_value = "tmkms.toml")]


this could potentially be done for Init too?

tomtau · 2021-06-25T08:23:16Z

providers/nitro/nitro-helper/src/command/nitro_enclave.rs

+pub fn run_vsock_proxy(opt: &VSockProxyOpt) -> Result<(), String> {
+    tracing::debug!("run vsock proxy with config: {:?}", opt);
+    let _ = Command::new("vsock-proxy")
+        .args(&["--num_workers", &format!("{}", opt.num_workers)])
+        .args(&["--config", &opt.config_file])
+        .arg(opt.local_port.to_string())
+        .arg(&opt.remote_addr)
+        .arg(opt.remote_port.to_string())
+        .output()
+        .map_err(|e| format!("execute nitro-cli error: {:?}", e))?;
+    Ok(())
+}


two questions or possible suggestions:

should this also check if the vsock-proxy isn't already running in the background?

what happens to this child process when the main process is ended? Given the output() isn't used, maybe this could return .spawn() instead, so that one can have Child and pass the signal to it when exiting?

In my test, there is no output.

I'm fixing this...

providers/nitro/nitro-helper/src/command.rs

tomtau

changes in workflows to be documented in a separate PR #11

linfeng-crypto requested a review from tomtau as a code owner June 24, 2021 06:25

Problem: Nitro helper requires a separate enclave launching(crypto-co…

d1d197b

…m#93)

linfeng-crypto force-pushed the issue-93 branch from c2a8e66 to d1d197b Compare June 24, 2021 06:32

tomtau reviewed Jun 24, 2021

View reviewed changes

linfeng-crypto added 2 commits June 25, 2021 10:15

use command to run nitro-cli

dbc1064

fix command dir

d88fdea

tomtau suggested changes Jun 25, 2021

View reviewed changes

fix

23189f0

linfeng-crypto force-pushed the issue-93 branch from 56d7bb3 to 23189f0 Compare June 28, 2021 03:02

remove useless file

ab3100b

linfeng-crypto force-pushed the issue-93 branch from 30369ab to ab3100b Compare June 28, 2021 03:53

add check proxy in run_vsock_proxy

1a991e9

linfeng-crypto force-pushed the issue-93 branch from 41bbccd to 1a991e9 Compare June 28, 2021 03:58

fix vsock proxy stop

8cc19af

linfeng-crypto force-pushed the issue-93 branch from c8b5381 to 8cc19af Compare June 28, 2021 06:52

linfeng-crypto added 2 commits June 28, 2021 15:12

can set cid when start enclave

568bc46

use std channel

399c475

tomtau reviewed Jun 29, 2021

View reviewed changes

providers/nitro/nitro-helper/src/command.rs Outdated Show resolved Hide resolved

split the launch-all config file

7f95358

linfeng-crypto force-pushed the issue-93 branch from a068fc7 to 7f95358 Compare June 29, 2021 07:14

tomtau approved these changes Jun 30, 2021

View reviewed changes

tomtau merged commit b1a50dc into crypto-com:main Jun 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: Nitro helper requires a separate enclave launching(#93) #109

Problem: Nitro helper requires a separate enclave launching(#93) #109

linfeng-crypto commented Jun 24, 2021

tomtau left a comment

tomtau left a comment

tomtau Jun 25, 2021

tomtau Jun 25, 2021

tomtau Jun 25, 2021

tomtau Jun 25, 2021

tomtau Jun 25, 2021

linfeng-crypto Jun 28, 2021

tomtau Jun 25, 2021

tomtau Jun 25, 2021

linfeng-crypto Jun 29, 2021

tomtau Jun 25, 2021

tomtau Jun 25, 2021

tomtau Jun 25, 2021

tomtau Jun 25, 2021 •

edited

Loading

linfeng-crypto Jun 28, 2021

linfeng-crypto Jun 28, 2021

tomtau left a comment •

edited

Loading

		cmd.contains(&"tmkms-nitro-helper".to_string())
		&& cmd.contains(&"enclave".to_string())

Problem: Nitro helper requires a separate enclave launching(#93) #109

Problem: Nitro helper requires a separate enclave launching(#93) #109

Conversation

linfeng-crypto commented Jun 24, 2021

tomtau left a comment

Choose a reason for hiding this comment

tomtau 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

tomtau Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau left a comment • edited Loading

Choose a reason for hiding this comment

tomtau Jun 25, 2021 •

edited

Loading

tomtau left a comment •

edited

Loading