Make public API more stable

[robin-nitrokey]

This PR contains two changes that make the public API more robust:

• Marking enums like Error, Response, Reply and Mechanism as non-exhaustive makes it possible to add features without breaking compatibility.
• Removing re-exports of the serialization functions from cbor_smol and postcard gives us more flexibility to change these implementation details.

Fixes https://github.com/trussed-dev/trussed/issues/62

[robin-nitrokey]

Previously we talked about making types::ui::Status non-exhaustive. I don’t think this would be appropriate – how should runners handle unknown states?

[daringer]

I would vote for types::ui::Status handling inside the runners should have a fallback _ => foo() otherwise runners would have to be adapted for any types::ui::Status updates, which as of today is LED only, but sooner or later might also include other interactions with the user/hardware...

[robin-nitrokey]

Added the CBOR re-exports again because they are used in the serialize_key syscalls. Still not sure if we should really have them, but dropping postcard is more important.

[robin-nitrokey]

I would vote for types::ui::Status handling inside the runners should have a fallback _ => foo() otherwise runners would have to be adapted for any types::ui::Status updates, which as of today is LED only, but sooner or later might also include other interactions with the user/hardware...

But can we really model these more complex states inside Trussed? Or would we need a way to give the runner more control over the UI state machine?

[daringer]

Good point, but in consequence this would mean we've to come up with a extension mechanism? I would prefer a simple solution, which would work for simple cases now (fallback?) and once the need arises we would look at a more flexible solution. Anyways not a high prio topic, so I would add it to the discussion list for the next steering meeting...

[robin-nitrokey]

It might be a much bigger change but I think we should also consider switching to a builder pattern for making requests. It could easily be generated by the macros. This would allow adding fields to any Request without any breakage.

Hmm, and how would the syscall interface look like? My best guess would be something like this:

• no arguments

  syscall!(Uptime);

• only required arguments:

  syscall!(ReadFile, (Location::Internal, path));

• required and optional arguments:

  syscall!(WriteFile, (Location::Internal, path, data), |r| r.user_attribute(UserAttribute));

  We could also discuss grouping the requests by topic as we do for the Client traits, and enforcing the mapping between requests and replies in the type system.

Anyway, I think we should discuss this separately as it is a much more invasive and fundamental change. What do you think about moving this topic into a discussion?

[sosthene-nitrokey]

No, I meant generating the builder types from the impl_reply and impl_request macros. Then we would keep the current syscall macro and have something like:

syscall!(client.request(Request::read_file(mandatory, arguments).optional_arg(smth).build()));

Other variants:

syscall!(client.read_file(mandatory, arguments).optional_arg(smth))

syscall!(Request::read_file(mandatory, arguments).optional_arg(smth).build(client))

[sosthene-nitrokey]

Or getting rid of the syscall macro, even currently I don't see why it need to be a macro and not a function:

Request::read_file(mandatory, arguments).optional_arg(smth).syscall(client);
Request::read_file(mandatory, arguments).optional_arg(smth).try_syscall(client);

This could exist in parallel to the current way to build syscalls to migrate progressively.

[daringer]

I am a big fan of this:

Request::read_file(mandatory, arguments).optional_arg(smth).syscall(client);

but I would also be in favor of having this separated from this PR as:

• this could be purely optional (as @sosthene-nitrokey suggests)
• this could also be a trigger to talk about client.rs which is also highly repetitive & redundant

[nickray]

You guys are on a roll, I love it <3

For the file operations, my ideal would be for the clients to have something closely resembling std::fs/std::path as API, appropriately channeled over the generic Response/Request mechanism. Essentially the adaptation that littlefs2 offers on a Filesystem (perhaps a trait outside littlefs2, higher level than embedded-storage, that then both littlefs2::Filesystem and Client implement). The backend would namespace these into the actual filesystems. Also regarding the static/ephemeral/external filesystems we have, my hope was to model these as "mounts" on a single file system, so e.g. ephemeral/RAM storage would be mapped into /tmp from a client perspective, an external flash chip to /mnt, etc. Configurable at runner level (i.e. inspired by man 7 hier).

The syscall! was an early hack that never got revisited. At first glance, I feel like the client.<method>(args){,.optional(args)}.call() seems a bit more idiomatic and discoverable than the Request::<method>(args){,optional(args}.call(client) approach (although the former is probably syntactic sugar for the latter in reality). As then Client can be bound on traits requiring the subset of methods a Trussed app requires, and documentation more clearly splits up the methods. The main goal is that writing app code feels "normal" to a non-embedded developer, even maybe a bit like Python. They want to (I think) do "simple things" like let key = client.generate_p256_key();, but have the power to do more complex things if needed.

Challenge: Could we in some way replace let x = client.method().optional().do() get rid of the final do? As in some clever trick involving ephemeral objects? It would be annoying for the above keygen to turn into let key = client.generate_key().<some_name>().

Regarding UI, general thoughts are that the apps should rely on semantic signals (such as the user is present, or the user is authenticated... extendable once you have touch screens or whatnot), whereas the runner should be able to fully inject the UI implementation. So might make sense to make a list of envisioned UI elements (LED, button, screen, pin pad) and figure out the semantic methods and states from an app perspective, and how to wire this up into Trussed without Trussed service being aware of the implementations.

[sosthene-nitrokey]

The syscall! was an early hack that never got revisited. At first glance, I feel like the client.<method>(args){,.optional(args)}.call() seems a bit more idiomatic and discoverable than the Request::<method>(args){,optional(args}.call(client) approach (although the former is probably syntactic sugar for the latter in reality). As then Client can be bound on traits requiring the subset of methods a Trussed app requires, and documentation more clearly splits up the methods. The main goal is that writing app code feels "normal" to a non-embedded developer, even maybe a bit like Python. They want to (I think) do "simple things" like let key = client.generate_p256_key();, but have the power to do more complex things if needed.

Challenge: Could we in some way replace let x = client.method().optional().do() get rid of the final do? As in some clever trick involving ephemeral objects? It would be annoying for the above keygen to turn into let key = client.generate_key().<some_name>().

Currently I don't think so. If we end up going the async way once async traits land (It looks like RTIC is going this way) we use the IntoFuture trait to have something like:

let x = client.method().optional().await;

I think removing any call method would be confusing though.

[nickray]

Can't we already add some sort of trivial runtime/waker and have actual async methods? The syscall! macro is essentially {smol,tokio}::block_on with spinning (except we don't actually spin, as the service runs on higher RTIC priority). Cf. https://github.com/paulkernfeld/spin_on.

This should let us use standard async-y types, and perhaps enable PC runner to use real smol/tokio.

We wouldn't need to wrap each syscall in block_on, but could do

trussed::block_on(async {
  let key = client.generate_key().await?;
  // other stuff
  let sig = client.sign(key, data).await?;
  Ok(sig)
})

at various granularities (to be chosen appropriately - probably at a high level if we want to swap out trussed::block_on with some_other::block_on easily... or we could just select an appropriate block_on via --cfg flags).

[robin-nitrokey]

Rebased onto 9fc19904b5970c421d81fbf28f63c13943a445b6 and made Context and CoreContext non-exhaustive.

[daringer]

ui:Status as non-exhaustive is also to be added

[robin-nitrokey]

Updated:

• Made ui::Status non-exhaustive.
• Added changelog entries.
• Replaced oath-authenticator patch with upstream.