-
Notifications
You must be signed in to change notification settings - Fork 73
feat: add cargo-optee and update the docs of quick start #263
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces cargo-optee, a new Rust-based build tool for OP-TEE Trusted Applications (TAs) and Client Applications (CAs), along with comprehensive documentation updates. The tool aims to replace complex Makefile-based workflows with a unified, type-safe command-line interface.
Key changes:
- Implementation of cargo-optee with support for TA/CA/Plugin builds across aarch64/arm architectures and std/no-std modes
- Fixed manifest path resolution issues for single-package and workspace projects
- Improved --std/--no-std argument semantics with explicit mutually exclusive flags
- Updated documentation to recommend cargo-optee as the primary workflow while maintaining backward compatibility with Makefile-based builds
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cargo-optee/src/main.rs | CLI interface with subcommands for build/install/clean operations |
| cargo-optee/src/ta_builder.rs | TA build pipeline: clippy, build, strip, and sign |
| cargo-optee/src/ca_builder.rs | CA/plugin build pipeline: clippy, build, and strip/copy |
| cargo-optee/src/config.rs | Configuration resolution with priority: CLI > Cargo.toml metadata > defaults |
| cargo-optee/src/common.rs | Shared utilities for architecture handling, command execution, and file operations |
| cargo-optee/Cargo.toml | Dependencies for the cargo-optee tool |
| cargo-optee/README.md | Comprehensive documentation for cargo-optee usage and design |
| cargo-optee/*.json | Target specification files for custom aarch64/arm-unknown-optee targets |
| ci/build.sh | New CI build script using cargo-optee to build all examples |
| .github/workflows/ci.yml | Added CI jobs for testing cargo-optee builds in std/no-std and aarch64/arm32 configurations |
| docs/emulate-and-dev-in-docker.md | Updated to document both cargo-optee and Makefile workflows |
| docs/emulate-and-dev-in-docker-std.md | Added cargo-optee workflow alongside legacy switch_config approach |
| docs/ta-development-modes.md | Corrected list of examples excluded in no-std mode |
| optee-utee-build/src/linker.rs | Fixed environment variable name from TARGET_TA to TARGET |
| examples/metadata.json | Newly added metadata file mapping examples to their TA/CA/plugin paths |
| .licenserc.yaml | Updated to exclude all JSON files from license checking |
| README.md | Added reference to cargo-optee documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "===========================================" | ||
| echo "" | ||
| echo "Mode: ${STD:-no-std}" | ||
| echo "Architecture: TA=$ARCH_TA, CA=$ARCH_CA" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable ARCH_CA is referenced but never defined. Based on the context, this should be ARCH_HOST instead, which is defined at line 52.
| echo "Architecture: TA=$ARCH_TA, CA=$ARCH_CA" | |
| echo "Architecture: TA=$ARCH_TA, CA=$ARCH_HOST" |
| echo "Mode: ${STD:-no-std}" | ||
| echo "Architecture: TA=$ARCH_TA, CA=$ARCH_CA" | ||
| echo "Examples: $CURRENT installed" | ||
| echo "Target dir: $SHARED_DIR" |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable SHARED_DIR is referenced but never defined. Based on the installation logic earlier in the script (lines 164-184), this should likely be one of TA_INSTALL_DIR, CA_INSTALL_DIR, or PLUGIN_INSTALL_DIR, or perhaps just removed from the summary output.
| echo "Target dir: $SHARED_DIR" | |
| echo "TA install dir: $TA_INSTALL_DIR" | |
| echo "CA install dir: $CA_INSTALL_DIR" | |
| echo "Plugin install dir: $PLUGIN_INSTALL_DIR" |
|
Please also fix the CI, thanks! |
Sorry, it's my fault. I will fix the CI and doc. thank you. |
5204281 to
2da26cd
Compare
|
@DemesneGH Hi , I’ve updated the relevant files according to your suggestions and fixed the CI issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cargo-optee/src/ca_builder.rs
Outdated
| let cargo_toml = std::fs::read_to_string("Cargo.toml")?; | ||
| let lib_name = cargo_toml | ||
| .lines() | ||
| .find(|line| line.trim().starts_with("name")) | ||
| .and_then(|line| line.split('=').nth(1)) | ||
| .map(|s| s.trim().trim_matches('"')) | ||
| .ok_or_else(|| anyhow::anyhow!("Could not find package name in Cargo.toml"))?; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cargo_toml parsing is fragile and may fail if the Cargo.toml has comments, whitespace variations, or if the name field appears in other sections. Consider using the toml crate to properly parse the Cargo.toml file, or use cargo metadata command which provides this information reliably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please help to fix this and the next two Copilot comments @asir66
cargo-optee/src/ca_builder.rs
Outdated
| let cargo_toml = std::fs::read_to_string("Cargo.toml")?; | ||
| let binary_name = cargo_toml | ||
| .lines() | ||
| .find(|line| line.trim().starts_with("name")) | ||
| .and_then(|line| line.split('=').nth(1)) | ||
| .map(|s| s.trim().trim_matches('"')) | ||
| .ok_or_else(|| anyhow::anyhow!("Could not find package name in Cargo.toml"))?; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same fragile Cargo.toml parsing pattern is used here. Consider using the toml crate to properly parse the Cargo.toml file, or use cargo metadata command which provides this information reliably.
cargo-optee/src/ta_builder.rs
Outdated
| if !stripped_path.exists() { | ||
| bail!("Stripped binary path does not exist: {:?}", stripped_path); | ||
| } |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code checks if the stripped binary path exists before creating it, which will always fail. The stripped binary should be created by the objcopy command on line 448-452. This check should be removed, or it should verify that objcopy succeeded by checking if the file exists after the command runs.
2da26cd to
1f884b3
Compare
cargo-optee/src/main.rs
Outdated
| // Validate that ta_dev_kit_dir exists (print absolute path) | ||
| if !ta_dev_kit_dir.exists() { | ||
| bail!( | ||
| "TA development kit directory does not exist: {:?}", | ||
| ta_dev_kit_dir | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this check into resolve_path_relative_to_project() to reduce the duplicate code, the same case is in line 115-118, please also check and fix other similar cases
| // Resolve project path from manifest or current directory | ||
| let project_path = if let Some(manifest) = common.manifest_path { | ||
| let parent = manifest | ||
| .parent() | ||
| .ok_or_else(|| anyhow::anyhow!("Invalid manifest path"))?; | ||
|
|
||
| // Normalize: if parent is empty (e.g., manifest is just "Cargo.toml"), | ||
| // use current directory instead | ||
| if parent.as_os_str().is_empty() { | ||
| std::env::current_dir()? | ||
| } else { | ||
| // Canonicalize must succeed, otherwise treat as invalid manifest path | ||
| parent | ||
| .canonicalize() | ||
| .map_err(|_| anyhow::anyhow!("Invalid manifest path"))? | ||
| } | ||
| } else { | ||
| std::env::current_dir()? | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this into a fn for code reuse, same code in line158-175
cargo-optee/src/main.rs
Outdated
| let project_path = if let Some(manifest) = clean_cmd.manifest_path { | ||
| manifest | ||
| .parent() | ||
| .ok_or_else(|| anyhow::anyhow!("Invalid manifest path"))? | ||
| .to_path_buf() | ||
| } else { | ||
| std::env::current_dir()? | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can reuse some fn like resolve_project_path()
cargo-optee/src/ta_builder.rs
Outdated
| let binary_path = profile_dir.join(&package_name); | ||
|
|
||
| if !binary_path.exists() { | ||
| bail!("Binary not found at {:?}", binary_path); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the same pattern can move into a fn like join_and_check(), using that fn avoids we forget check the path
|
For 5.1, "cargo-optee" is not recognized as a cargo subcommand, it is because cargo invokes external binaries in an unexpected way. For example, One workaround is to add a parsing rule or configuration option that discards the first (duplicate) argument: #[derive(Parser)]
#[command(version, about, long_about = None)]
struct Cli {
// By stripping the duplicate "optee" argument during subcommand invocation,
// this solution maintains compatibility between standalone and cargo-integrated execution modes.
#[arg(name = "tool_name", hide = true)]
_tool_name: String,
#[command(subcommand)]
commands: Commands,
} |
That makes sense—thank you for the suggestion. I also noticed that with the previous implementation, running cargo-optee in standalone mode results in the following error: For this reason, I believe the following approach is more robust: making _tool_name an optional argument. #[derive(Debug, Parser)]
#[command(version, about)]
pub struct Cli {
// By stripping the duplicate "optee" argument during subcommand invocation,
// this solution maintains compatibility between standalone and cargo-integrated execution modes.
// When invoked as `cargo optee build`, cargo passes "optee" as the first argument.
// When invoked as `cargo-optee build`, no extra argument is passed.
#[arg(name = "tool_name", hide = true)]
_tool_name: Option<String>,
#[command(subcommand)]
pub cmd: Command,
}This allows the CLI to work correctly in both invocation scenarios without requiring additional conditional logic. |
|
@asir66 Thanks for your effort! It seems Could you help with a brief study of how other cargo subcommands handle this case? To achieve the goal of "cargo-optee acting as a cargo subcommand," it's good to follow the established principles used by other subcommands. If there is a standard convention, we can implement it in the same way; otherwise, the current implementation is fine. |
|
After reviewing a more canonical Cargo subcommand implementation, I noticed that cargo-fmt handles the Cargo-injected subcommand argument in a different and more robust way. In cargo-fmt, the implementation explicitly drops the first occurrence of the subcommand name (fmt) at program entry, rather than relying on a positional placeholder in the argument parser. This behavior is intentional and aligns well with Cargo’s external subcommand invocation model. When Cargo invokes an external subcommand, it follows the convention: As a result, a well-behaved Cargo subcommand should ideally support all of the following invocation forms:
I verified that cargo-fmt conforms to this convention, while cargo-sgx currently does not handle all of these cases consistently. For reference, cargo-fmt implements this logic by filtering out the first occurrence of the injected subcommand name at the very beginning of execution: fn execute() -> i32 {
// Drop extra `fmt` argument provided by `cargo`.
let mut found_fmt = false;
let args = env::args().filter(|x| {
if found_fmt {
true
} else {
found_fmt = x == "fmt";
x != "fmt"
}
});
let opts = Opts::parse_from(args);
}Following the same rationale, I propose adopting an equivalent approach for cargo-optee, by removing the first occurrence of "optee" from the argument list at program entry: // Drop extra `optee` argument provided by `cargo`.
let mut found_optee = false;
let filtered_args: Vec<String> = env::args()
.filter(|x| {
if found_optee {
true
} else {
found_optee = x == "optee";
x != "optee"
}
})
.collect();
let cli = Cli::parse_from(filtered_args);This approach avoids introducing Cargo-specific positional arguments into the CLI definition, keeps the argument parsing logic explicit and localized, and ensures consistent behavior across all supported invocation forms. I believe this pattern is both more idiomatic and better aligned with existing Cargo tooling practices, as demonstrated by cargo-fmt. |
1f884b3 to
e7fc767
Compare
|
This commit performs a structural refactor of cargo-optee to improve Key changes include:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 28 out of 33 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "===========================================" | ||
| echo "" | ||
| echo "Mode: ${STD:-no-std}" | ||
| echo "Architecture: TA=$ARCH_TA, CA=$ARCH_CA" |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable ARCH_CA is used in the summary output but is never defined in the script. The script uses ARCH_HOST for the CA/Plugin architecture. This will result in an empty or undefined variable being printed in the summary.
| echo "Architecture: TA=$ARCH_TA, CA=$ARCH_CA" | |
| echo "Architecture: TA=$ARCH_TA, CA=$ARCH_HOST" |
| } | ||
|
No newline at end of file |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is trailing whitespace at the end of this JSON file. While this doesn't affect functionality, it's inconsistent with the ARM target JSON file which has no trailing whitespace.
| } | |
| } |
| The **dev-env with std support** provides a complete setup for building TAs that can use Rust's standard library | ||
| features like collections, networking, etc. |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent line breaking pattern. Lines 10-11 split a sentence across lines while line 38 keeps "no-std-only" intact. Consider consistent line breaking: either break after complete phrases or keep related terms together.
| The **dev-env with std support** provides a complete setup for building TAs that can use Rust's standard library | |
| features like collections, networking, etc. | |
| The **dev-env with std support** provides a complete setup for building TAs that can use Rust's standard library features like collections, networking, etc. |
| .find(|pkg| { | ||
| pkg.manifest_path | ||
| .to_string() | ||
| .contains(&*cargo_toml_path_str) | ||
| }) |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package lookup logic for workspace projects uses a substring match (contains) which could incorrectly match packages with similar path names. For example, if you have paths like "/project/my_ta/Cargo.toml" and "/project/my_ta_test/Cargo.toml", searching for "my_ta/Cargo.toml" could incorrectly match "my_ta_test/Cargo.toml". Consider using exact path comparison or comparing canonicalized paths instead.
| .find(|pkg| { | |
| pkg.manifest_path | |
| .to_string() | |
| .contains(&*cargo_toml_path_str) | |
| }) | |
| .find(|pkg| pkg.manifest_path.to_string() == cargo_toml_path_str) |
| if std::path::Path::new(env_path).exists() { | ||
| std::process::Command::new("bash") | ||
| .arg("-c") | ||
| .arg(format!("source {}", env_path)) |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source_cargo_env function has a command injection vulnerability. The env_path parameter is directly interpolated into a bash command without proper escaping. If a user's HOME or CARGO_HOME environment variable contains special shell characters (e.g., ;, $(), backticks), this could lead to arbitrary command execution. Use proper command argument passing or escape the path before using it in a shell command.
| .arg(format!("source {}", env_path)) | |
| .arg("source \"$1\"") | |
| .arg("bash") | |
| .arg(env_path) |
| ta-dev-kit-dir = { aarch64 = "/opt/teaclave/optee/optee_os/out/arm-plat-vexpress/export-ta_arm64", arm = "/opt/teaclave/optee/optee_os/out/arm-plat-vexpress/export-ta_arm32" } | ||
| signing-key = "/opt/teaclave/optee/optee_os/out/arm-plat-vexpress/export-ta_arm64/keys/default_ta.pem" |
Copilot
AI
Jan 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded absolute paths in the metadata configuration may not be portable across different development environments. While this is intended as example configuration, consider documenting that these paths should be adjusted for each developer's environment, or consider using environment variables like ${OPTEE_OS_DIR} if supported by the configuration system.
Overview
This update is based on PR #245 and focuses on improving the robustness of cargo-optee in several areas, including manifest path resolution, std/no-std argument semantics, and consistency across documentation and CI.
Specifically, this PR includes the following changes:
--std/--no-stdRelated background discussion: #245 (comment)
1. Manifest Path Resolution Fix
This issue is fundamentally caused by insufficiently robust manifest path resolution. It manifests differently depending on the project structure:
Single-package projects
When
--manifest-path Cargo.toml(without./) is explicitly provided, cargo-optee does not correctly handle this edge case when deriving the project path, which may result in failing to locate the project directory.Workspace projects
When a relative path containing
./is used as the manifest path, cargo metadata returns canonicalized manifest paths, while cargo-optee attempts to match them using the original, non-normalized path string. This mismatch causes package discovery to fail.Fix and Rationale
To address this issue at its root, manifest paths are now canonicalized at the entry point using:
This standard library function normalizes paths into absolute form and automatically eliminates:
./../2.
--std/--no-stdArgument SemanticsPrevious Behavior
cargo-optee previously used a boolean flag to represent whether std was enabled.
When the flag was not explicitly provided, it defaulted to Some(false), which
caused cargo-optee to always fall back to the no-std build mode.
Updated Design
This update introduces an explicit
--no-stdflag and refactors the argumentparsing logic accordingly:
--stdand--no-stdare mutually exclusiveNoneThis clarifies user intent and improves extensibility for future configuration
sources.
3. JSON File Normalization
Relevant JSON files have been updated to ensure that fields are ordered in
ascending ASCII order.
alignment with common tooling and formatting conventions
4. Documentation and CI Updates
During this process, an additional documentation inconsistency was identified and addressed as part of this PR.
5. Investigation: Differences Between cargo, cargo-sgx, and cargo-optee
The following section summarizes findings from an investigation into the design and usage differences between cargo, cargo-sgx, and cargo-optee.
5.1 cargo-optee Is Not a cargo Subcommand
Currently, cargo-optee cannot be invoked as a cargo subcommand (i.e. cargo optee).
One possible reason is that the customized cargo toolchain used in the environment does not include subcommand extension support.
From a pragmatic perspective, the current form is acceptable. However, from a consistency and extensibility standpoint, retaining this capability may be worth considering in the future.
5.2 Rationale for
cargo-optee build ta/caUnlike cargo-sgx, which does not require explicit sub-target selection, cargo-optee requires specifying either
taorca.This difference stems from fundamental architectural distinctions:
As a result, this design choice is both necessary and appropriate.
5.3 Default Build Mode Differences
Tool | Default Build Mode -- | -- cargo | `--debug` cargo-sgx | `--release` cargo-optee | `--release`cargo-optee aligns with cargo-sgx in this respect, which matches the performance and determinism requirements of TEE environments.
5.4
runvssyncBoth cargo and cargo-sgx support running applications directly via
run. In contrast, cargo-optee requires an explicitsyncstep before execution.This difference is driven by runtime environment constraints:
Therefore, this behavior reflects architectural constraints rather than a tooling deficiency.
That said, it may be worth exploring whether the
syncstep could be optionally integrated into cargo-optee (e.g. via a flag or default behavior) to improve usability.5.5 Features, Targets, and Workflow Overlap
In cargo:
In cargo-optee, these dimensions partially overlap at the workflow level.
This has also been a key point in previous discussions(see: #245 (comment)).
At present, the main source of complexity comes from the overlap between features and targets in the build workflow. This overlap makes certain parts of the model harder to reason about and evolve.
It is possible that this issue could be addressed more cleanly in the future if the toolchain converges on a cargo-only workflow (for example, if xargo is eventually phased out). Under such conditions, the separation of concerns between features and targets may become clearer.
For now, however, cargo-optee remains a well-suited tool for this project: it effectively reduces configuration burden, integrates smoothly with the TrustZone SDK, and provides a simple and efficient developer experience.
6. Future Directions (Design Considerations)
In the long term, cargo-optee could consider adopting a unified and configurable parameter resolution model, for example:
(Environment variables may also be viewed as overridable defaults.)
Such a model could:
This would allow for unified configurability without sacrificing explicitness.
Thanks the help of @DemesneGH @m4sterchain