Attestation libraries

[gmandyam]

Add attestation service. Initial cut.

[rusty1968]

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.

[gmandyam]

Changed accordingly. Exact pin removed.

[rusty1968]

Can we drop Cargo.toml? This is a Bazel project and the crate is already fully defined in BUILD.bazel — targets and deps included.

[gmandyam]

Dropped.

[rusty1968]

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.

[rusty1968]

Small naming nit: CaliptraSigner lives in the "platform-independent" API crate, but the name (and sign_es384/alias_cert_der) bakes in a specific vendor and algorithm.

[gmandyam]

Renamed to HWSigner

[rusty1968]

alias_cert is dead — I grepped the whole tree and its only references are the definition itself and the one unit test that exists solely to test it. This is just a pass-thru method invoked only by the test.

[gmandyam]

Removed function and unit test

[rusty1968]

As written, both attest crates are std-based and heap-allocating. Please make sure all crates are no_std.

[rusty1968]

Suggested change

	#![no_std]
	#![forbid(unsafe_code)]

[gmandyam]

Added. See lib.rs files.

[rusty1968]

The crate must be no_std. You are using O/S facilities in a crate that is going to be consumed in an embedded environment.

[gmandyam]

stdlib dependencies are removed.

[rusty1968]

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.

[rusty1968]

openprot-secure-coding — violation

zeroize is added as a dependency (producer/Cargo.toml:19, producer/BUILD.bazel:16) and
the README (producer/README.md:81) documents it as "Zero-on-drop for intermediate key
material and sensitive buffers" — but it's never imported or called anywhere in the source.
Dead dependency, and the security claim in the README is currently false.

[gmandyam]

As written, both attest crates are std-based and heap-allocating. Please make sure all crates are no_std.

Moved to heapless.

[gmandyam]

openprot-secure-coding — violation

zeroize is added as a dependency (producer/Cargo.toml:19, producer/BUILD.bazel:16) and the README (producer/README.md:81) documents it as "Zero-on-drop for intermediate key material and sensitive buffers" — but it's never imported or called anywhere in the source. Dead dependency, and the security claim in the README is currently false.

Removed and README updated.

[rusty1968]

Can this live under mod tests?

[rusty1968]

LGTM: [x]

Correctness

• producer/src/builder.rs build(): the ueid claim (key 256) is hard-coded to a constant placeholder (ueid[0] = 0x01, the remaining 28 bytes always zero) for every device, in the shared build() path used by both HwAttestProducer (production) and SoftwareAttestProducer. The README documents this claim as "Unique Entity Identifier from Caliptra CDI", but nothing derives it from device identity — every token from every device will carry the same UEID. This defeats the purpose of the claim and should either be wired to a real per-device source (e.g. via HwSigner/DICE identity) or clearly gated so it can't ship silently wrong (a // TODO alone is not enough given this is the production code path, not just the test stub).
• third_party/crates_io/Cargo.lock: diffed against current main tip (not just this PR's stale base), this PR's lockfile reintroduces pldm-common/pldm-interface (already removed on main), is missing statig (added on main since this branch's last lockfile rebuild), and downgrades ~15+ unrelated crates (bitflags, bytes, clap/clap_builder, hashbrown, foldhash, log, memchr, smallvec, uuid, ruzstd, syn, zerocopy, …) that have nothing to do with ciborium/minicbor. This needs a lockfile refresh/rebase before merge — merging as-is risks silently resurrecting removed dependencies and regressing pinned versions repo-wide.
• Both services/attest/api/README.md and services/attest/producer/README.md document an API that no longer matches the code: the README shows a CaliptraSigner trait with sign_es384/alias_cert_der/cert_chain_der(&self) -> Result<Vec<Vec<u8>>, _>, and an AttestProducer::generate_token(&self, ...) -> Result<Vec<u8>, _> / cert_chain(&self) -> Result<CertChain, _>. The actual code (traits.rs, types.rs) has been reworked to no_std/heapless out-parameter signatures (HwSigner, generate_token(..., iat, out: &mut Vec<u8, MAX_TOKEN_SIZE>), cert_chain(&self, buf: &mut Vec<...>)), and the producer README's usage examples (Arc::new(caliptra_driver), Box::new(UefiFirmwareMeasurements), let token: Vec<u8> = ...generate_token(...)) don't compile against the real, reference-based, no-alloc constructors. These docs will actively mislead the first integrator who copies them. The README's claim table is also missing the sw-name(14)/sw-version(15) claims that builder.rs actually emits.

No dynamic allocation (openprot-no-alloc)

No violations found. Verified across the full diff: both crates declare #![no_std] (api/src/lib.rs, producer/src/lib.rs), and every buffer in consts.rs, error.rs, traits.rs, types.rs, builder.rs, dice_identity.rs, measurements.rs, and signer.rs uses heapless::{Vec, String} with named fixed capacities from consts.rs. The only place std-only crates (ciborium) appear is producer/tests/producer_integration.rs, a host-side integration-test binary, which is the documented exemption.

Panic-free / explicit errors (openprot-no-panic)

• producer/src/measurements.rs::test_caliptra_measurements() is gated by #[cfg(feature = "test-support")], not #[cfg(test)] — it is reachable at runtime from SoftwareAttestProducer::generate_token, a build configuration the README documents as legitimate ("software stub enabled, no Caliptra hardware required"). It contains 4 .unwrap() calls (push_str, extend_from_slice, push). These are practically infallible given the hardcoded literal lengths, but the rule is unconditional outside #[cfg(test)]. Suggested fix (included in the diff): return Result<Vec<Measurement, MAX_MEASUREMENTS>, AttestError> and propagate with ?/map_err, matching the style already used everywhere else in this crate; update the one call site in signer.rs.
• Everywhere else (builder.rs production path, signer.rs non-test-support impls, dice_identity.rs production fn) consistently uses map_err(...)? — good, this is the only outlier.

Secrets, crypto, and register access (openprot-secure-coding)

• The prior finding (unused zeroize dependency) is resolved: zeroize no longer appears anywhere, and there is in fact no local sensitive/secret buffer in this diff to zeroize — the private Alias Key never leaves the (out-of-scope) Caliptra mailbox driver, and HwSigner::sign() only returns a public signature. Nothing to flag here.
• No unsafe anywhere (#![forbid(unsafe_code)] in both crate roots), no secret-byte comparisons, and no key material appears in Debug/error output.
• builder::build() stack-allocates two independent MAX_TOKEN_SIZE (8 KiB each) scratch buffers (payload_scratch, cose_scratch) simultaneously, plus a 128-byte header scratch and the caller-owned out buffer. Worth confirming this fits the target RoT's stack budget (~16-24 KiB of scratch alone) — not blocking, but should be measured/documented for an embedded target.

Verbosity / over-engineering (ponytail-review)

• builder.rs::build() has ~30 near-identical e.<method>(...).map_err(|_| AttestError::Cbor)?; lines. The previously-flagged shape (claims.push((Value::Integer(...), Value::...)) → a claim() helper) doesn't map cleanly onto the new streaming minicbor::Encoder API, but the repeated .map_err(|_| AttestError::Cbor)? boilerplate could still be collapsed with a small local macro or closure. Nit, not blocking.
• The stub DER-cert bytes ([0x30, 0x00]) are constructed inline and duplicated across signer.rs (StubSigner, SoftwareAttestProducer::cert_chain), dice_identity.rs tests, and builder.rs tests — a shared fn stub_cert() -> [u8; 2] test helper would remove the duplication. Low priority, test-only code.
• decode_outer/find_claim* helpers in builder.rs tests (via minicbor) and producer_integration.rs (via ciborium) look like duplicated test scaffolding, but they intentionally use two different CBOR libraries to cross-validate the encoder against an independent decoder — that's reasonable, not bloat. The comment in builder.rs's decode_outer ("Use ciborium only in tests") is simply stale/wrong now (the code uses minicbor::Decoder); fixed in the diff.

Readability & Maintainability

• HwAttestProducer.providers: Vec<&'a dyn MeasurementProvider, 8> uses a bare magic-number capacity where every other buffer in this crate uses a named MAX_* constant; fixed in the diff (MAX_PROVIDERS).
• third_party/crates_io/Cargo.toml: ciborium/minicbor were inserted directly under the # Host tool crates needed by pigweed pw_kernel/tooling comment even though minicbor is a production, no_std dependency of the producer crate (not host tooling). Fixed with its own comment block in the diff.

Commit Message

• This PR's commit history mixes Conventional-Commit-style prefixes (fix:, deps:, build:, style:) that don't match the plainer, component-first style used elsewhere in this repo's history (e.g. orchestrator: ..., Add X, Fix Y). Not blocking if this gets squash-merged, but worth a single descriptive commit message on merge rather than carrying the 20-commit fixup trail (several of which are literally "fix: resolve clippy warnings" / "fix: remove unused import" against this same PR's own earlier commits).

[chrysh]

@rusty1968 @gmandyam Are we adding AI skills to the repo now?

[chrysh]

Follow-up on the earlier review: 13 inline suggestions on style and API shape, all click-to-apply.

Three of them depend on another: comment on builder.rs:25 needs the builder.rs:82 slice change, builder.rs:95 needs the Cursor alias on builder.rs:51, and signer.rs:24 needs MAX_PROVIDERS in consts.rs. Each says so in its own text.

Commit history

20+ commits mixing fix:, deps:, build: and style: prefixes that do not
match this repo's plainer component-first style (orchestrator: ..., Add X).
Several are fixups against this PR's own earlier commits. Not blocking if this
is squash-merged, but the squash message should be a single descriptive one.

[chrysh]

The stub DER bytes [0x30, 0x00] are written out at twelve places across
signer.rs, builder.rs and dice_identity.rs. One constant here, used from
all of them, makes it obvious they are the same placeholder and gives it a name
to grep for when a real cert appears.

Suggested change

	struct StubSigner;
	/// Placeholder DER: an empty SEQUENCE. Not a parseable certificate.
	const STUB_CERT: [u8; 2] = [0x30, 0x00];
	struct StubSigner;

[chrysh]

Thanks, that covers signer.rs. The seven copies in the test modules are still
inline: builder.rs:236, 244, 246 and dice_identity.rs:46, 54, 70, 78. (The
0x30, 0x01 at dice_identity.rs:80 is a deliberately different cert, leave it.)

STUB_CERT is currently #[cfg(feature = "test-support")] and private, so those
modules cannot reach it. To share it, widen the gate and the visibility:

#[cfg(any(test, feature = "test-support"))]
pub(crate) const STUB_CERT: [u8; 2] = [0x30, 0x00];

then use crate::signer::STUB_CERT; in both test modules.

The [0x30, 0x00] in cert_ueid.rs are DER field placeholders, not stub certs, so
they stay as they are.

[gmandyam]

Please review.

STUB_CERT is now pub(crate) under #[cfg(any(test, feature = "test-support"))] in signer.rs, and both builder.rs and dice_identity.rs test modules import and use it — seven inline [0x30, 0x00] literals replaced.

[gmandyam]

producer/src/builder.rs build(): the ueid claim (key 256) is hard-coded to a constant placeholder (ueid[0] = 0x01, the remaining 28 bytes always zero) for every device, in the shared build() path used by both HwAttestProducer (production) and SoftwareAttestProducer. The README documents this claim as "Unique Entity Identifier from Caliptra CDI", but nothing derives it from device identity — every token from every device will carry the same UEID. This defeats the purpose of the claim and should either be wired to a real per-device source (e.g. via HwSigner/DICE identity) or clearly gated so it can't ship silently wrong (a // TODO alone is not enough given this is the production code path, not just the test stub).

ueid as I defined it in RFC 9711 (https://www.rfc-editor.org/info/rfc9711/#section-4.2.1) was meant to be determined directly from HW and not potentially overridden by a Caliptra cert ueid field (e.g. due to faulty implementation).

On target, I expect the ueid to be available in HW (e.g. via fuse sense register), but we don't have a specific HW target. That being said, I will modify the code to derive ueid from the cert chain, but I will also need to add a check to ensure that ueid is uniform across the entire cert chain. I can also add a note in the readme that the implementor will have to replace this part of the code if ueid is determined from a source other than the Caliptra cert chain.