Conversation
Version 0.7.6 deprecated the pre_init! macro as a hard error, breaking //target/ast10x0:entry. Pin to the same version as upstream main. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
|
|
||
| cortex-m = { version = "0.7.7", features = ["critical-section-single-core"] } | ||
| cortex-m-rt = "0.7.5" | ||
| cortex-m-rt = "=0.7.5" |
There was a problem hiding this comment.
We don't need the exact = pin here. The committed Cargo.lock already resolves cortex-m-rt (and cortex-m-rt-macros) to exactly 0.7.5 with a checksum, so every build that respects the lockfile is already reproducible — the = on the requirement doesn't add any determinism on top of that.
There was a problem hiding this comment.
Changed accordingly. Exact pin removed.
|
Quick note on Cargo.toml files: this is a Bazel project, so the only manifest we actually need is Cargo.toml — that's what drives crate_universe / external crate resolution for Bazel. All the per-crate Cargo.toml files scattered under services (attest, spdm, mctp, etc.) aren't used to build or test anything; the real target and dependency definitions live in each crate's BUILD.bazel. |
|
As written, both attest crates are std-based and heap-allocating. Please make sure all crates are no_std. |
| //! ``` | ||
| //! | ||
| //! Claim key numbers follow RFC 9711 and the OCP-EAT profile. | ||
|
|
There was a problem hiding this comment.
| #![no_std] | |
| #![forbid(unsafe_code)] |
There was a problem hiding this comment.
Added. See lib.rs files.
|
One thing I noticed: dice_identity.rs and measurements.rs are each basically a whole module wrapping a single pub fn that's only ~1-3 lines — collect is a loop + extend, cert_chain is one call + a length check, and alias_cert is a straight pass-through (and dead). It's a bit of a "one file per function" thing, where the module boundary costs more than the logic it's hiding. These would be better as private helpers/methods behind HwAttestProducer. |
- Remove exact-pin from cortex-m-rt (lockfile already pins the checksum) - Drop services/attest/api/Cargo.toml and services/attest/producer/Cargo.toml (Bazel project; crates fully defined in BUILD.bazel) - Rename CaliptraSigner → HwSigner, sign_es384 → sign, alias_cert_der → leaf_cert_der to remove vendor/algorithm bake-in from platform-independent API - Remove dead alias_cert() function and its unit test from dice_identity.rs - Make api and producer crates no_std + alloc; replace std::time::Duration with core::time::Duration in AttestConfig - Remove std::time::SystemTime from builder::build; caller now supplies iat: u64 Unix timestamp — no OS clock in embedded context
…ration - Add minicbor 0.21 to third_party/crates_io/Cargo.toml - Add services/attest/api/src/consts.rs with all fixed-capacity constants (MAX_CHAIN_LEN=5, MAX_CERT_SIZE=2048, MAX_MEASUREMENTS=16, MAX_TOKEN_SIZE=8192, etc.) - api/src/types.rs: replace Vec<u8>/String with heapless equivalents throughout; HwSigner::cert_chain_der and leaf_cert_der now write into caller-supplied bufs; MeasurementProvider::measurements appends into caller-supplied buf - api/src/traits.rs: AttestProducer::generate_token writes into caller-supplied Vec<u8, MAX_TOKEN_SIZE>; cert_chain writes into caller-supplied buf - api/src/error.rs: drop String payloads (no alloc); add BufferFull variant - producer/src/builder.rs: replace ciborium with minicbor + stack BufWriter; no heap allocation in hot path - producer/src/signer.rs: HwAttestProducer holds lifetime-tied refs instead of Arc/Box; providers stored in heapless::Vec<&dyn, 8> - producer/src/measurements.rs / dice_identity.rs: updated to heapless types - producer/BUILD.bazel: swap ciborium→minicbor in library deps; ciborium kept as test-only dep for integration test decode - producer/tests/producer_integration.rs: updated for new API signatures
openprot-secure-coding — violation
|
Moved to heapless. |
Neither crate is referenced in the producer source; removing them from BUILD.bazel deps and the README dependency table.
Removed and README updated. |
api/README.md: - Remove deleted CertChain type from types table - Remove CaliptraSigner (replaced by HwSigner) from traits section - Update AttestProducer signature to 4-arg generate_token with out buffer - Update HwSigner trait (was CaliptraSigner): add caliptra_measurements, use &mut buf output pattern throughout - Update MeasurementProvider::measurements to &mut out pattern - Note no_std + heapless in Cargo section producer/README.md: - HwAttestProducer example: reference not Arc, &dyn not Box, 4-arg generate_token - SoftwareAttestProducer example: 4-arg generate_token with out buffer - Dependencies: minicbor (not ciborium), HwSigner (not CaliptraSigner) Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Widen STUB_CERT gate to #[cfg(any(test, feature = "test-support"))] and make it pub(crate) so the builder.rs and dice_identity.rs test modules can reference it via use crate::signer::STUB_CERT, replacing seven inline [0x30, 0x00] literals. The [0x30, 0x01] in dice_identity.rs is intentionally distinct and is unchanged. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
|
@gmandyam Are the tests already running in the CI when you push to the PR? |
| buf: &mut Vec<Vec<u8, MAX_CERT_SIZE>, MAX_CHAIN_LEN>, | ||
| ) -> Result<(), AttestError>; | ||
| /// Return Caliptra-internal firmware measurements (ROM, FMC, runtime, etc.). | ||
| fn caliptra_measurements( |
There was a problem hiding this comment.
Is this trait always going to be calling into caliptra? Otherwise, should we choose a more generic name? E.g. rot_measurements?
There was a problem hiding this comment.
This trait leverages Caliptra. Making the name space generic is not going to change the dependencies on Caliptra.
There was a problem hiding this comment.
Can we guarantee that in 5 years caliptra will still be our only measurement source? If yes, then keep the name. If we might have a different measurement device, the name should be more generic.
Note the api crate itself already has no Caliptra dependency (BUILD deps are heapless and thiserror; the mailbox code is in the producer). So the name is the only Caliptra thing in the seam, and renaming it costs nothing.
There was a problem hiding this comment.
Can we guarantee that in 5 years caliptra will still be our only measurement source? If yes, then keep the name. If we might have a different measurement device, the name should be more generic.
Note the api crate itself already has no Caliptra dependency (BUILD deps are heapless and thiserror; the mailbox code is in the producer). So the name is the only Caliptra thing in the seam, and renaming it costs nothing.
OpenPRoT is intended to be an OCP standard (see https://www.amd.com/en/blogs/2025/openprot--building-a-secure-and-transparent-foundation-for-platf.html). OCP created Caliptra and still refers normatively to Caliptra in relevant specifications (e.g. see https://opencomputeproject.github.io/Security/device-identity-provisioning/HEAD/). The RoTs are not interchangeable from that perspective. At very least any RoT must support TCG DICE and be OCP SAFE certifiable (https://github.com/opencomputeproject/OCP-Security-SAFE/tree/main/Reports/CHIPS_Alliance).
There was a problem hiding this comment.
The SAFE and DICE requirement is the part that argues for the generic name. "Any
RoT must support TCG DICE and be OCP SAFE certifiable" describes a class of
devices, so the trait method should be named after the role in that class, not
after one product in it. A normative reference to Caliptra says Caliptra is a
conforming RoT, not that a portable trait method has to carry its name.
This tree already ships more than one RoT. target/earlgrey is OpenTitan Earl
Grey, target/veer is the Caliptra emulator, and both run in ./pw ci. On the
Earl Grey port the firmware measurements come from OpenTitan, not from a
Caliptra mailbox, so HwSigner::caliptra_measurements is already the wrong name
for one of the two platforms we build today.
To be clear about scope, I am not asking to scrub Caliptra from the crate.
MeasurementAuthority::Caliptra should stay: it tells the verifier the digest
came from Caliptra rather than from a platform provider asserting about itself,
which is exactly the distinction that decides how much the digest is worth.
AttestError::Caliptra for mailbox errors is fine too. Those name a real thing.
The trait method names a role, and that is the only line I want changed:
fn caliptra_measurements(...) -> fn rot_measurements(...)
One rename plus its call sites, cheap now and awkward once anyone outside this
repo implements HwSigner.
One small ask on process: could you leave threads open until whoever raised
them has had a look? I resolve mine as soon as I have checked the fix, so
nothing sits around, and it keeps the open threads as an accurate list of what
is still being discussed.
There was a problem hiding this comment.
Agree with Chrsina on the naming - +1
The SAFE and DICE requirement is the part that argues for the generic name. "Any RoT must support TCG DICE and be OCP SAFE certifiable" describes a class of devices, so the trait method should be named after the role in that class, not after one product in it. A normative reference to Caliptra says Caliptra is a conforming RoT, not that a portable trait method has to carry its name.
This tree already ships more than one RoT.
target/earlgreyis OpenTitan Earl Grey,target/veeris the Caliptra emulator, and both run in./pw ci. On the Earl Grey port the firmware measurements come from OpenTitan, not from a Caliptra mailbox, soHwSigner::caliptra_measurementsis already the wrong name for one of the two platforms we build today.To be clear about scope, I am not asking to scrub Caliptra from the crate.
MeasurementAuthority::Caliptrashould stay: it tells the verifier the digest came from Caliptra rather than from a platform provider asserting about itself, which is exactly the distinction that decides how much the digest is worth.AttestError::Caliptrafor mailbox errors is fine too. Those name a real thing. The trait method names a role, and that is the only line I want changed:fn caliptra_measurements(...) -> fn rot_measurements(...)One rename plus its call sites, cheap now and awkward once anyone outside this repo implements
HwSigner.One small ask on process: could you leave threads open until whoever raised them has had a look? I resolve mine as soon as I have checked the fix, so nothing sits around, and it keeps the open threads as an accurate list of what is still being discussed.
I agree the naming should reflect the vocabulary of the domain. There was another instance where Caliptra was part of the name of the type and we got rid of that.
There was a problem hiding this comment.
You both are missing the main point: the checks in the code (e.g. DICE identity) are specific to Caliptra. That is by design. Changing the function naming is syntactic sugar - nothing more. OpenTitan will fail as a DICE identity provider because it follows the OpenDICE standard. If an IHV wants to use a different RoT then that is up to them to customize the code base.
I will genericize the names but that won't affect the checks in the code.
There was a problem hiding this comment.
If it is caliptra specific, then it should not be in something called services/attest/api/src/types.rs. A programmer that looks at this directory expects something generic. Can you make sure that any code that is
caliptra specific resides in a directory or filename that contains caliptra, e.g. services/attest/implementations/caliptra.rs? Any code that can also be reused for other attestation devices can be in the types.rs.
If we don't design our software this genric way, we will accumulate technical debt that we don't want for the future, which will make it harder to extend the software.
Does that make sense to you?
There was a problem hiding this comment.
Can you make sure that any code that is caliptra specific resides in a directory or filename that contains caliptra, e.g. services/attest/implementations/caliptra.rs?
Yes - this suggestion makes sense.
If we don't design our software this genric way, we will accumulate technical debt that we don't want for the future, which will make it harder to extend the software.
Agree.
There was a problem hiding this comment.
Change has been made by adding caliptra.rs. Please verify.
| out: &mut Vec<Measurement, MAX_MEASUREMENTS>, | ||
| ) -> Result<(), AttestError> { | ||
| for m in caliptra { | ||
| out.push(m.clone()).map_err(|_| AttestError::BufferFull)?; |
There was a problem hiding this comment.
why do we use different conventions here? out.push vs p.measurements(out)?
Furthermore, "append" is a convention, not a contract. Nothing in the
trait doc at types.rs:77-81 says a provider must only append. A provider calling
out.clear() silently drops the RoT's own measurements, and the token still signs and verifies fine. That is a platform component erasing the root of trust's measurements from an attestation token.
Possible but maybe not the best fix in collect, but no extra buffer and no extra stack:
for p in providers {
let start = out.len();
p.measurements(out)?;
if out.len() < start {
return Err(AttestError::Provider("provider shrank the measurement list"));
}
}
plus one line on the trait: implementations append, they never remove or reorder existing entries.
There was a problem hiding this comment.
why do we use different conventions here? out.push vs p.measurements(out)?
Agentic inefficiency. Change made.
Furthermore, "append" is a convention, not a contract. Nothing in the
trait doc at types.rs:77-81 says a provider must only append. A provider calling
out.clear() silently drops the RoT's own measurements, and the token still signs and verifies fine. That is a platform component erasing the root of trust's measurements from an attestation token.
This is a reasonable suggestion, but I won't incorporate it for now.
OCP-EAT ((https://opencomputeproject.github.io/Security/ietf-eat-profile/HEAD/) is a profile of the EAT specification (RFC 9711). I deliberately did not specify where profile compliance is enforced in RFC 9711 (https://www.rfc-editor.org/info/rfc9711/#section-6), so the enforcement point could be at the attester or verifier.
OCP-EAT requires certain mandatory claims including measurements. However, the front matter of OCP-EAT states the following regarding mandatory claims: "These claims are REQUIRED for all attestations and provide the minimum necessary information for verifier appraisal policies. The verifier can expect at a minimum these claims in a compliant attestation ...".
It is up to the remote verifier to evaluate attestation tokens and make appraisal decisions based on the received evidence (including the profile claim), and this involves more than just verifying the token signature ("silently drops the RoT's own measurements, and the token still signs and verifies fine"). If the provider attempts to remove measurements that are expected in the token, the verification will fail because expected measurement values are missing.
CI tests are always re-executed upon a new push (as far as I can tell). Is this not your experience? |
Send + Sync removed from AttestProducer, HwSigner, MeasurementProvider: bare-metal target has no threading model that requires these bounds. Unused MAX_* constants removed from consts.rs: MAX_NONCE_LEN, MAX_EVIDENCE_LEN. Nonce and evidence are passed as &[u8] slices with no capacity enforcement. Uncalled functions removed: - HwSigner::leaf_cert_der (only cert_chain_der is called by production code) - MeasurementProvider::component_name (no caller in the codebase) - All leaf_cert_der stub impls in signer.rs, builder.rs, dice_identity.rs cert_ueid::extract demoted from pub to private: only called within the same file by extract_and_verify. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
|
@gmandyam remove the SKILLS from this PR. I created them for our internal use, it seems you added to your PR by accident. |
Claim key 260 (hwversion) is not defined in the OCP-EAT profile. Version information is already carried per-component in the measurements array. Removes: CLAIM_HWVER constant, encoding block in builder.rs, hw_version field from AttestConfig, MAX_HW_VERSION_LEN from consts.rs, and all test/README references. FIXED_CLAIMS updated 11 → 10. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
|
Is the name of this service attest or verifier? |
These are local development aids and should not be upstreamed. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
- measurements::collect() is now provider-only; caliptra measurements are written directly by HwSigner::caliptra_measurements() in signer.rs before collect() is called, eliminating the asymmetric push loop - Add test covering provider failure after partial write - Update api and producer READMEs to match current trait signatures, AttestConfig fields, and token claim table (add sw-name/sw-version) Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Removed the skills. |
Done. |
Attest. Verifier (local verifier) is part of the attestation service per design specification - see https://github.com/OpenPRoT/openprot/blob/main/docs/src/specification/services/attestation.md. |
| rust_test( | ||
| name = "attest_producer_unit_test", | ||
| crate = ":attest_producer", | ||
| features = ["test-support"], | ||
| deps = [ | ||
| "//services/attest/api:attest_api", | ||
| ], | ||
| ) | ||
|
|
||
| rust_test( | ||
| name = "attest_producer_integration_test", | ||
| srcs = ["tests/producer_integration.rs"], | ||
| edition = "2021", | ||
| features = ["test-support"], | ||
| deps = [ | ||
| ":attest_producer", | ||
| "//services/attest/api:attest_api", | ||
| "@rust_crates//:ciborium", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
features is Bazel's own attribute, not the rules_rust one. The Rust feature
never gets set, so #![cfg(feature = "test-support")] at
tests/producer_integration.rs:6 strips the whole file and the target compiles to
an empty crate. Bazel then reports it as PASSED with zero tests, locally and in
./pw ci, so all nine tests in there have never run.
Three changes to make them run. The attribute rename alone is not enough,
because SoftwareAttestProducer is itself behind the feature, so the test needs
a library built with it:
| rust_test( | |
| name = "attest_producer_unit_test", | |
| crate = ":attest_producer", | |
| features = ["test-support"], | |
| deps = [ | |
| "//services/attest/api:attest_api", | |
| ], | |
| ) | |
| rust_test( | |
| name = "attest_producer_integration_test", | |
| srcs = ["tests/producer_integration.rs"], | |
| edition = "2021", | |
| features = ["test-support"], | |
| deps = [ | |
| ":attest_producer", | |
| "//services/attest/api:attest_api", | |
| "@rust_crates//:ciborium", | |
| ], | |
| ) | |
| rust_library( | |
| name = "attest_producer_test_support", | |
| srcs = glob(["src/**/*.rs"]), | |
| crate_features = ["test-support"], | |
| crate_name = "openprot_attest_producer", | |
| edition = "2021", | |
| deps = [ | |
| "//services/attest/api:attest_api", | |
| "@rust_crates//:heapless", | |
| "@rust_crates//:minicbor", | |
| ], | |
| ) | |
| rust_test( | |
| name = "attest_producer_unit_test", | |
| crate = ":attest_producer", | |
| crate_features = ["test-support"], | |
| deps = [ | |
| "//services/attest/api:attest_api", | |
| ], | |
| ) | |
| rust_test( | |
| name = "attest_producer_integration_test", | |
| srcs = ["tests/producer_integration.rs"], | |
| crate_features = ["test-support"], | |
| edition = "2021", | |
| deps = [ | |
| ":attest_producer_test_support", | |
| "//services/attest/api:attest_api", | |
| "@rust_crates//:ciborium", | |
| "@rust_crates//:heapless", | |
| ], | |
| ) |
heapless is new in the test deps: it is used at producer_integration.rs:8 and
only shows up as an unresolved import once the file actually compiles.
With this on 73f549d the integration test runs 9 tests and all pass, the unit
test stays at 16. Checked with
bazel test //services/attest/producer:attest_producer_integration_test \
--test_output=all --nocache_test_results
There was a problem hiding this comment.
Filed the CI side as #462: a rust_test whose sources are all stripped by cfg reports PASSED with zero tests, so this target was green from the day it was added.
There was a problem hiding this comment.
Thank you for pointing this out. I will make the changes and verify the CI picks them up.
|
Looking at the CI log, those tests don't show up: The second one is not |
HwSigner is Caliptra-specific; isolate it in api/src/caliptra.rs so the module boundary is visible at a glance. The concise-evidence claim (-70001) is removed: generate_token now takes only (nonce, out). READMEs updated throughout. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Resolved Cargo.lock conflict from upstream pigweed roll. Upstream Cargo.toml already carries ciborium and minicbor; added the corresponding [[package]] entries to the merged lockfile. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
- cert_ueid: map_or(false, …) → is_some_and (clippy::unnecessary_map_or) - rustfmt: reflow long lines in builder, cert_ueid, dice_identity, measurements, signer; fix module declaration order in lib.rs Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Looks like the tests are showing up in the log now. |
- Rename caliptra.rs → hw_abstraction.rs; update module declaration and re-export in lib.rs - Rename HwSigner::caliptra_measurements → measurements - Rename test_caliptra_measurements → test_measurements - Rename AttestError::Caliptra → AttestError::Mailbox; update error message to "Mailbox error: ..." - Update all call sites, trait impls, and README files Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Adds a software-key attestation path that requires no Caliptra hardware: - AttestError::InvalidKey — new error variant for key material failures - SwSignerConfig — carries a 48-byte P-384 scalar and DER cert chain - SignerKind — Hardware | Software discriminant added to AttestConfig - SwSigner — validates scalar (non-zero, < P-384 order) and certs (non-empty, each a DER SEQUENCE) at construction time; implements HwSigner - SwAttestProducer — uses SwSigner, skips DICE chain validation, falls back to a zeroed UEID placeholder if leaf cert carries no TCG UEID extension All 33 existing tests continue to pass. AttestConfig construction sites updated to include signer_kind: SignerKind::Hardware. READMEs updated. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
…DMEs hw_abstraction.rs now hosts both HwSigner (hardware path) and SwSigner (software path), so the name no longer reflects its contents. Renamed to signing_abstraction.rs. Also rewrites services/attest/README.md to reflect the current API: correct AttestProducer and HwSigner signatures, add SwSigner description, add SignerKind/SwSignerConfig to the Key types table, remove stale fields (evidence, iat, leaf_cert_der, caliptra_measurements, component_name). Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Add attestation service. Initial cut.