oxidecomputer / idolatry

An experimental IPC interface definition language for Hubris.
Mozilla Public License 2.0
17 stars 11 forks source link

Add server::build_restricted_server_support() #22

Closed jgallagher closed 2 years ago

jgallagher commented 2 years ago

build_restricted_server_support() takes an additional arg compared to build_server_support(): a map of operation names to task names. For any operation present in the map, additional code is generated in the server that checks that the calling task is one of the specified task names (with error checking for operation names or task names that don't exist).

If the map of allowed callers is nonempty, server code generation is tightly tied to the hubris build system: it will read $HUBRIS_TASKS both to know what task names are legal and to map them to indices for the checks performed.

This PR also adds a ClientError::AccessViolation variant (which maps to the already-existing ReplyFaultReason::AccessViolation), which is the error we return if a restricted operation is called by a different task.

mkeeter commented 2 years ago

I don't love extracting the names from $HUBRIS_TASKS – even though Hubris is the only Idolatry user, it seems a little too magical.

What about adding an argument, e.g.

pub fn build_restricted_server_support(
    source: &str,
    stub_name: &str,
    style: ServerStyle,
    allowed_callers: &BTreeMap<String, Vec<String>>,
    task_ids: &BTreeMap<String, usize>,
)

Then, we could put a helper function to go from $HUBRIS_TASKS -> BTreeMap<String, usize> in hubris/build/util, and call it in the individual build.rs files.

jgallagher commented 2 years ago

I think I like that less than what I did originally, which was to not use $HUBRIS_TASKS at all and use ::hubris_num_tasks::Task::{task_name} as usize instead of task ID literal values in the generated code. This has the downside of requiring clients to depend on sys/num-tasks with the task-enum feature enabled. Cliff suggested making idolatry depend on $HUBRIS_TASKS directly.

If we want to put more work on the callers and provide utilities to help them, could we squash your two args into allowed_callers: &BTreeMap<String, Vec<usize>> and expect them to have already done the conversion?

mkeeter commented 2 years ago

I'm on the fence – looking more at the code, this is already pretty tightly coupled to Hubris (e.g. using userlib::TaskId). On the other hand, passing arguments via environmental variables just rubs me the wrong way...

Switching to allowed_callers: &BTreeMap<String, Vec<usize>> with a helper function in hubris/build/util seems like a reasonable option!

jgallagher commented 2 years ago

Updated to remove $HUBRIS_TASKS in favor of BTreeMap<String, Vec<usize>> in e10a2ca.