Add task named 'attest' to acts as the root of trust for reporting. #1378
Conversation
flihp
force-pushed
the
rotr
branch
2 times, most recently
from
7b2f8b6
to
78fed1f
Compare
|
|
||
| Interface( | ||
| name: "Attest", | ||
| ops: { |
There was a problem hiding this comment.
I should get around to making this the default, but could I encourage you to apply
encoding: Hubpack,
to each operation in any new IDL interface? It will let you use more flexible argument and error types.
|
|
||
| #[repr(u32)] | ||
| #[derive(Copy, Clone, Debug, FromPrimitive, Eq, PartialEq, IdolError)] | ||
| pub enum AttestError { |
There was a problem hiding this comment.
FWIW, should you elect to turn on encoding: Hubpack, this type will need to change in the following ways:
- add
derive(Serialize, Deserialize, hubpack::SerializedSize) - remove
IdolError,FromPrimitive,#[repr(u32)] - no longer needs explicit discriminator values.
task/attest/src/main.rs
Outdated
|
|
||
| //! Root of trust for reporting (RoT-R) task. | ||
| //! | ||
| //! Use the rotr-api crate to interact with this task. |
There was a problem hiding this comment.
nit: you wound up naming that crate attest-api.
task/attest/src/main.rs
Outdated
| let addr = unsafe { CERTS.assume_init_ref() }; | ||
| let cert_data = match CertData::load_from_addr(addr) { | ||
| Ok(a) => a, | ||
| Err(_) => panic!("CertData"), |
There was a problem hiding this comment.
If the cert data cannot be loaded, this will cause your task to restart forever, starving all tasks at lower priorities. That's not good.
I recommend instead changing the content of the Server to Option and providing a way of indicating "could not load certs" in the IPC API.
There was a problem hiding this comment.
oof that's bad test coverage on my part.
There was a problem hiding this comment.
ran across a small bug in lib-lpc55-rot-startup dependencies while testing this: #1402
There was a problem hiding this comment.
Implementing this was mostly straight forward. The only bit I'm not wild about is the repetitive code required to pull data out of the handoff ram. I took a swing at implementing a generic function but I couldn't convince the compiler that a ref to the contents of the MaybeUninit was a byte slice.
task/attest/src/main.rs
Outdated
|
|
||
| // Map the memory used to pass the segment of the identity cert chain common | ||
| // to all tasks to a variable. | ||
| #[used] |
There was a problem hiding this comment.
You're doing a somewhat subtle thing here that I think should get added to the comment (because I had to cursor around and convince myself it was correct).
By assigning a link_section you're saying you want this placed in an area of memory -- not at the base of that memory. However, the app config (and thus the linker script) sizes these two sections of memory such that they're exactly the size of these statics. This should ensure that they're placed at the base of the section.
So, I'd mention that intent in the comment, lest someone adjust it later and cause this to become invalid.
This would probably be better served by the extern-regions facility, which helps to avoid this class of thing.
There was a problem hiding this comment.
I've added this comment and I'll admit I didn't fully understand the subtlety of the behavior. I'll start looking into replacing this with the extern-regions mechanism as you suggest. If that comes together before this PR gets approved I'll add another commit for it.
There was a problem hiding this comment.
Spent some time reading up on the extern-regions stuff over the last few days. This PR has been updated to replace the existing code using link_sections with extern-regions. This took a bit more stack space but there's probably ways to reduce that if necessary (mut statics?).
|
CI isn't running on this PR, probably caused by the conflicts noted. Rebasing during a review is often frowned upon so I'll hold off fixing these conflicts unless requested. |
|
gah formatting |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 487 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 486 | 1 | 0 | 0 |
Click to see the invalid file list
- task/attest/src/config.rs
flihp
force-pushed
the
rotr
branch
2 times, most recently
from
187d2f9
to
4b91faa
Compare
|
and again with my bad license text fixed |
|
oxide-rot-1/app-dev.toml builds fine for me locally but fails in CI needing more flash. Not sure I have any option but to YOLO larger flash values at CI till it works. |
This task is currently limited to producing a cert chain from the
Aliascert back to either the last intermediate before the root (dice-mfg) or a self-signed identity cert (dice-self). A matching PR to humility will provide an interface to extract these certs & provide a nice interface (PEM encoding, combined cert chain etc).