Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Add ExternalDependencies.md #3312

Merged
merged 9 commits into from
May 12, 2023
Merged

Conversation

alistair23
Copy link
Contributor

Pull Request Overview

This adds a document that describes some of the requirements of adding external crates.

Once this is accepted the idea is then to add an external crate with #3092

Testing Strategy

TODO or Help Wanted

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
twilfredo
twilfredo previously approved these changes Nov 4, 2022
@hudson-ayers
Copy link
Contributor

I added this as an agenda item for tomorrow's call

@hudson-ayers
Copy link
Contributor

We ended up not having much time to discuss this on the call.

I made one comment on the call which I wanted to try to summarize here:

Having dependencies which use unsafe internally directly in capsules violates Tock's written threat model for capsules. I am of the opinion that even relatively high quality dependencies are more likely to have soundness bugs than Rust's core library -- especially once we are using a stable compiler -- and these are not equivalent cases. That said, I realize it is important that some capsules be able to call code (e.g. crypto code) that will be defined in external dependencies.

I proposed that all dependencies should be hidden in a top-level directory named external. This top-level directory would not itself be a crate (like boards/, for instance), but would hold a collection of crates, with each crate having a single external dependency. For the ghash example, there would be a ghash-tock crate in the external folder. This crate would implement a trait which should be defined in capsules. For the ghash example, this trait might look like:

trait Hash {
    fn update_padded(&self, &'static mut [u8]);
    fn finalize(&self); 
    // etc. 
}

and the ghash-tock crate would define a struct which would implement this trait using the external dependency. The capsule (e.g. aes_gcm) would be generic over this trait, and accept an instance of it when created. Accordingly, the board using the capsule would be responsible for passing a GhashTock struct into the aes_gcm capsule in main.rs. Thus it is the board author explicitly choosing to use the ghash-tock crate, a crate clearly marked as containing an external dependency, and doing this mapping itself.

The upsides of this approach:

  1. capsules does not itself gain any external dependencies, and all dependencies on potentially unsafe crates are expressed directly from the board repo, as is currently the case.
  2. This works well to express situations where a given interface might be implemented by a trusted, high quality software library or by an accelerator, since the trait could be implemented for either and used equivalently (with some potential problems dealing with async vs. sync interfaces...)
  3. The way that code can interact with the dependency is very clearly defined in this external interface crate, as opposed to the alternative approach where auditing the use of the external dependency requires tracking down every place that ghash::x is called throughout all of capsules.
  4. Boards not using the individual capsules which require external dependencies do not have to compile those dependencies. This is nice because of a problem with external dependencies directly in capsules: even boards that do not need the dependency at all will be forced to compile them. Compiling external crates with large dependency trees can be quite slow, and compiling external crates could lead to the execution of build.rs files or procedural macros that themselves represent potential security vulnerabilities.

I also realize that this approach has a size and ergonomics overhead relative to direct use of the dependency. It also may be difficult to incorporate certain dependencies behind traits (procedural macro support traits being one such example). If this turns out to be impractical, I am sympathetic to that. Regardless, I think we should try to find a solution that addresses my final point above. So if this trait-based separation approach is impractical, I think that we should still create a seperate crate or crates for capsules with dependencies, we could just call it external-capsules or something, and that way boards that do not want to compile dependencies are not forced to.

@alistair23
Copy link
Contributor Author

So if this trait-based separation approach is impractical, I think that we should still create a seperate crate or crates for capsules with dependencies, we could just call it external-capsules or something, and that way boards that do not want to compile dependencies are not forced to.

I agree with your 4 advantages, and I like the idea of trying to isolate external dependencies as much as possible. I do worry that it will add a lot of overhead. In terms of possible code size, but mostly development time, re-writing another level of abstraction. The ghash is a simple case, but for more complex functionality I think this will balloon into a lot of code overhead.

I do like the external-capsules idea a lot though. We could basically just move this capsule into an external-capsules directory (not itself a crate) and then allow board authors to add that. That means we will still expose HILs, so the implementations can be easily swapped out with others if desired.

@bradjc
Copy link
Contributor

bradjc commented Nov 15, 2022

I somehow missed Hudson's comment before doing this, but I did a pass updating the doc. My main changes:

  1. I specified we only do external deps for crypto from https://github.com/RustCrypto.
  2. I specified one approach Hudson mentioned, specifically that all dependencies are included from a specific crate.
  3. I wrote down what we talked about before with board crates having more latitude to use external dependencies.

I'm not overly attached to item 2, but I do like the idea that it would be easily grep-able.

@bradjc
Copy link
Contributor

bradjc commented Nov 15, 2022

Having dependencies which use unsafe internally directly in capsules violates Tock's written threat model for capsules.

Right, do we treat external dependencies like unaudited capsule code, or like highly managed code (like the kernel crate)? I think I end up on the side of like highly managed code. This is more consistent with the ethos that we should be very selective of our external dependencies, and really we are just adding a convenience layer versus creating a vendor-specific copy in the tock repo itself.

Thus it is the board author explicitly choosing to use the ghash-tock crate, a crate clearly marked as containing an external dependency, and doing this mapping itself.

In my view, this is how board-specific dependencies should (and would be forced to) work. For core dependencies like cryptographic libraries, I find it dissonant that we are including external crates to promote secure code, but then falling back to trusting board authors to do the right thing. I view the external dependency inclusion process as the audit that the dependency is ok, and we do not want board authors overriding that choice.

Now, I do understand the SE challenge of allowing SW or HW implementations. But I think that comes at a different layer, using HILs, and not with a new collection of capsule interfaces.

Comment on lines 54 to 59
There are two general methods to for including an external dependency in the
Tock kernel: core external dependencies, and board-specific external
dependencies. Core external dependencies may be used in "core" Tock crates, such
as the `kernel`, `chips`, and `capsules` crates. Board-specific external
dependencies may _only_ be used by crates in the `board/` folder. The processes
for inclusion between these two methods are different.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For crypto crates that will ultimately need to be used by the core kernel, this makes sense to me -- it is inevitable that those crates will need to be compiled by all boards unless we want to rely on some hacky conditional includes that will be a mess to maintain.

For external dependencies which are only needed by individual capsules, I think things look a little different -- we really don't want to force all boards to compile those external crates if they are not using those capsules, so I think any such capsules should exist in stand-alone crates rather than inside the capsules crate.

ghash is an interesting example because currently it is only needed in capsules, but my understanding is that the plan is to eventually use it as part of the kernel app verification workflow (is this correct, @phil-levis ?), which would require the kernel to depend on it anyway. So I don't know that we need to move Alistair's capsule into a standalone crate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the right place to put the separation? I worry that we could be including fine-grained slicing before it's clear we need to. Does anyone care if the first build takes a couple seconds longer? I agree that once it's minutes then that is a lot of overhead, but perhaps we cross that bridge when we get there.

We also could look at separating capsules in to multiple crates, which could address this issue as well. Or put all capsules-with-dependencies in their own crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ghash isn't specifically for app signing. The idea with the ghash crate is that we can expose AES-GCM on all platforms that have any AES hardware. The goal here would be to support all AES block ciphers (GCM, CCM, CTR, CBC and ECB) on any platform that supports AES. That way we can rely on all implementations existing and expose them all to userspace and the kernel.

Basically CTR, CBC and ECB are commonly supported in hardware, and the other two can be built with software on top of the hardware.

In the longer run though, I think it would be worth supporting crypto operations (AES, RSA, ECC) on all platforms with software fallbacks. The goal there is to allow loading signed/encrypted applications on all Tock platforms. As a secure operating system, it seems very useful for Tock to be able to do that.

I do agree with Brad. It does seem like we are worrying about something that isn't a problem yet. In terms of crypto crates we only ever expect to include a handful. We can also do the external-capsules direction, but I feel like the crypto ones might end up being included all the time anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, ghash isn't needed for application credentials. However, public key signatures (RSA, ECDSA, DH, etc.) will need to use tested-and-true software implementations (existing crates).

@hudson-ayers
Copy link
Contributor

I think that I am happy with the current text of the document, modulo my one comment

doc/Design.md Outdated Show resolved Hide resolved
@hudson-ayers
Copy link
Contributor

We talked about this on the core call last week, some people read the updated version of this for the first time. The conclusion was that if we are gonna do this, we should go ahead and restructure the capsules directory to not be a crate, and instead have a few crates contained within it. I opened #3346 to propose a division, and will implement whatever consensus is reached there

@hudson-ayers
Copy link
Contributor

Update: we reached a consensus in #3346, but I have not had a chance to implement the division (it is mostly just a large mechanical refactor). If someone else has a chance to implement it before me please feel free.

bors bot added a commit that referenced this pull request Mar 1, 2023
3396: capsules: split into `core/` and `extra/` (implementing #3346) r=lschuermann a=lschuermann

### Pull Request Overview

This pull request splits capsules into `core/` and `extra/` as proposed and discussed in #3346 for #3312.

### Testing Strategy

This pull request was tested by `cargo check`.

### TODO or Help Wanted

This PR is still in development. Specifically, I'd like to check whether it actually moves capsules into the correct crates, and update documentation pointing to capsules, as well as updating the capsules (now `core/`) README.

### Documentation Updated

- [ ] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make prepush`.


Co-authored-by: Leon Schuermann <leon@is.currently.online>
@alistair23
Copy link
Contributor Author

Ok! Now that #3346 is fixed and #3396 is merged we can get back to this.

Is the plan to have a extern crate that contains all of the capsules that have external dependencies? Or do we want a individual crate per external capsule?

@bradjc
Copy link
Contributor

bradjc commented Apr 26, 2023

I updated the document with a first pass based on the call. @lschuermann please revise as needed.

Copy link
Member

@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bradjc! I restructured those sections a little, to reflect the proposed policy around the various capsule-crates.

While our previous discussions really only focused on adding dependencies to the capsule-crates, they did not explicitly consider board-specific external dependencies. However, those seem even less controversial, given a board can simply choose to depend on a capsule-crate which has external dependencies.

Finally, I'd appreciate some input on my comment regarding core external dependencies. I recall that there was a lot of pushback against that, which is why we converged on the capsule-crate based approach in the first place. The document may look inconsistent as of now, given that it establishes a policy around core external dependencies (which judging from the discussions we might not even want right now, at least not without concrete motivating examples), but not a process for including these.

It should be noted that the motivating example for this PR (#3092) could be resolved by, for instance, a crypto capsule crate.

capsules use HILs, and a wrapper would use the external dependency to implement
the trait) to avoid the overhead of implementing and maintaining a wrapper to
implement the trait. While architecturally this has advantages, the overhead was
deemed too burdensome for the expected benefit.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to emphasize that, while we don't force external crates to be hidden behind traits to make them accessible to Tock, this approach may still be preferred for additional flexibility. If possible (for a specific dependency), this can be a low-effort way to promote code-reuse and a cohesive crate structure. If not, splitting it out into an external crate is a good escape hatch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a paragraph about this to the board-specific external dependencies section.

### Including Core External Dependencies

As of now, no process for including core external dependencies has been
established.
Copy link
Member

@lschuermann lschuermann May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall from our discussions that we did deliberately punt on deciding over "core external dependencies". We converged on the capsule-crates restructuring to allow for using external dependencies where (arguably) they'd be needed the most right now, and to avoid forcing all Tock boards to use an external dependency.

In those discussions, there was significant pushback against such "core" external dependencies, specifically ones in kernel/ and arch/. I didn't want to outright remove it from the document with my edit. Nonetheless, given the current use-cases of external dependencies, IIRC adding them to a capsules-crate was deemed sufficient for now.

To reflect that, I added that we simply do not have an established process for introducing core external dependencies yet. This might be weird, given the document does specify a policy around them.

doc/ExternalDependencies.md Outdated Show resolved Hide resolved
In accordance with many discussions on the Tock core team calls, in
addition to discussions on this document's PR (tock#3312, [1]), the notion
of "core external dependencies" shall be removed for now. If required
in the future, this original text can be restored.

[1]: https://github.com/tock/tock/pull/3312/files/8f19da6979b7eb28ecd3172583f65c06f9388d92#r1181726351
doc/ExternalDependencies.md Outdated Show resolved Hide resolved
doc/ExternalDependencies.md Outdated Show resolved Hide resolved
@hudson-ayers
Copy link
Contributor

Items from the call:

  • We should clarify that this document only applies to things that will end up in the kernel binary
  • Would be good to lighten the language about cargo in the intro
  • Move the "specific approach motivation" down to the end of the doc, and possibly rename that section

@bradjc
Copy link
Contributor

bradjc commented May 10, 2023

I updated based on Amit's comment of the discussion.

Is this ready to merge?

Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready!

@lschuermann lschuermann added the last-call Final review period for a pull request. label May 12, 2023
@hudson-ayers
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented May 12, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 4a86e48 into tock:master May 12, 2023
@alistair23 alistair23 deleted the alistair/external-deps branch May 12, 2023 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation last-call Final review period for a pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants