Skip to content

Commit 2c5149b

Browse files
JeromyStCopilot
andauthored
feat(native): signing, validation & DID layer with zero-copy architecture (#186)
Layer 3 staged merge: signing, validation, DID, factories, headers + FFI + C/C++ projections. Zero-copy architecture: Arc<[u8]>-backed message parsing, ArcSlice sub-views, LazyHeaderMap deferred parsing, consume-vs-set FFI pattern. New crates: cose_sign1_signing, cose_sign1_validation, cose_sign1_factories, cose_sign1_headers, cose_sign1_validation_primitives, did_x509, plus 7 FFI crates. 15 _to_message FFI functions, ByteView C++ accessors, PEM key support, validate_arc() zero-copy validation path, comprehensive Copilot instruction files. 5,756 tests, 0 failures. Line coverage: 90%+. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent dab33a8 commit 2c5149b

352 files changed

Lines changed: 116037 additions & 730 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
---
2+
applyTo: "native/**"
3+
---
4+
# Code Review Standards — Native CoseSignTool
5+
6+
> Criteria for reviewing PRs and evaluating code quality across the native stack.
7+
8+
## Review Dimensions
9+
10+
Every review of native code should evaluate these dimensions:
11+
12+
### 1. Zero-Copy / No-Allocation Architecture
13+
- Are unnecessary `.clone()`, `.to_vec()`, `.to_owned()`, `.to_string()` present?
14+
- Do FFI handle conversions use bounded lifetimes (`<'a>`) not `'static`?
15+
- Do C++ accessors return `ByteView` (borrowed) not `std::vector` (copied)?
16+
- Are `_consume` / `_to_message` zero-copy variants available where appropriate?
17+
- See `zero-copy-design.instructions.md` for the full checklist.
18+
19+
### 2. Safety & Correctness
20+
- **Null checks**: Every FFI pointer param checked before dereference.
21+
- **Panic safety**: Every `extern "C"` function wrapped in `catch_unwind`.
22+
- **SAFETY comments**: Every `unsafe` block has a `// SAFETY:` justification.
23+
- **Lifetimes**: No `'static` references to heap-allocated handles.
24+
- **Memory ownership**: `*const` for borrowed, `*mut` for owned/consumed. Documented in C header and C++ wrapper.
25+
26+
### 3. API Design & Ergonomics
27+
- Builder patterns are fluent (return `&mut self` or `Self`).
28+
- Error types use manual `Display` + `Error` impls (no `thiserror`).
29+
- Traits are `Send + Sync` when stored in `Arc`.
30+
- C++ classes are move-only (delete copy ctor/assign).
31+
- Destructors check `if (handle_)` before calling `*_free()`.
32+
33+
### 4. Test Quality
34+
- Tests follow Arrange-Act-Assert.
35+
- Both success and error paths are tested.
36+
- FFI null-pointer safety tests for every parameter.
37+
- Roundtrip tests: sign → parse → validate.
38+
- No shared mutable state between tests (parallel-safe).
39+
- Temp files use unique names (thread ID or nanos).
40+
41+
### 5. Documentation
42+
- Public APIs have `///` doc comments.
43+
- FFI functions have `# Safety` sections.
44+
- Module-level `//!` comments in every `lib.rs`.
45+
- C++ methods have `@see` cross-refs to zero-copy alternatives.
46+
47+
## Grading Scale
48+
49+
| Grade | Meaning |
50+
|-------|---------|
51+
| A+ | Exceptional. No issues. Exemplary patterns. |
52+
| A | Excellent. Minor style nits only. |
53+
| A- | Very good. 1-2 non-blocking improvements identified. |
54+
| B+ | Good. Several improvements needed but no bugs. |
55+
| B | Acceptable. Notable gaps in docs, tests, or design. |
56+
| C | Needs work. Missing safety checks, unsound lifetimes, or significant test gaps. |
57+
| F | Blocking issues. UB, memory leaks, or security vulnerabilities. |
58+
59+
## Common Anti-Patterns to Flag
60+
61+
### Rust
62+
```rust
63+
// ❌ Cloning when moving is possible (builder consumed via Box::from_raw)
64+
builder_inner.protected.clone()
65+
66+
// ❌ 'static lifetime on FFI handle reference
67+
fn handle_to_inner(h: *const H) -> Option<&'static Inner>
68+
69+
// ❌ .to_string() on string literals in error paths
70+
Err(MyError::InvalidFormat("expected:format".to_string()))
71+
// ✅ Use .into()
72+
Err(MyError::InvalidFormat("expected:format".into()))
73+
74+
// ❌ thiserror in production crates
75+
#[derive(thiserror::Error)] // Not allowed
76+
77+
// ❌ expect()/unwrap() in production code (tests are fine)
78+
let value = map.get("key").unwrap();
79+
```
80+
81+
### C++
82+
```cpp
83+
// ❌ const_cast to work around ownership semantics
84+
const char* s = const_cast<const char*>(raw_ptr);
85+
86+
// ❌ Forgetting to null-out source in move constructor
87+
HeaderMap(HeaderMap&& other) : handle_(other.handle_) { }
88+
// ✅ Must null the source
89+
HeaderMap(HeaderMap&& other) noexcept : handle_(other.handle_) {
90+
other.handle_ = nullptr;
91+
}
92+
93+
// ❌ Missing copy deletion
94+
class MyHandle { /* no copy ctor/assign declaration */ };
95+
// ✅ Explicitly delete
96+
MyHandle(const MyHandle&) = delete;
97+
MyHandle& operator=(const MyHandle&) = delete;
98+
```
99+
100+
### C Headers
101+
```c
102+
// ❌ Missing extern "C" guard
103+
// ✅ Every C header needs:
104+
#ifdef __cplusplus
105+
extern "C" {
106+
#endif
107+
108+
// ❌ Using const for output ownership-transfer pointers
109+
int func(const char** out_string); // implies borrowed
110+
// ✅ Non-const for caller-owned allocations
111+
int func(char** out_string); // caller must free
112+
```
113+
114+
## Coverage Exclusion Policy
115+
116+
Only FFI boundary stubs may use `#[cfg_attr(coverage_nightly, coverage(off))]`.
117+
**Never exclude**: business logic, validation, parsing, crypto, error handling.
118+
119+
Justified exclusions:
120+
- `handle_panic()` — unreachable without `catch_unwind` triggering
121+
- `write_signed_bytes/message()` — unreachable due to mandatory post-sign verification
122+
- `*_abi_version()` — compile-time constants
123+
- `*_free()` / `*_string_free()` — pointer deallocation stubs
124+
- `cose_last_error_*()` — thread-local error accessors
125+
126+
Flag any exclusion on non-FFI code as a review blocker.
Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
---
2+
applyTo: "native/**"
3+
---
4+
# Migration Playbook — V2 C# to Native Rust
5+
6+
> Step-by-step guidance for porting feature packs and capabilities from the
7+
> V2 C# branch (`users/jstatia/v2_clean_slate:V2/`) to native Rust.
8+
9+
## Migration Phases
10+
11+
The native port follows a layered merge strategy. Each phase is a staged PR
12+
from a working branch into `native_ports_final`.
13+
14+
| Phase | What | Status |
15+
|-------|------|--------|
16+
| 1 | Primitives (CBOR, Crypto, COSE types, Sign1 builder) | ✅ Complete |
17+
| 2 | OpenSSL crypto provider + PEM support | ✅ Complete |
18+
| 3 | Signing, Validation, DID, Factories, Headers + FFI + C/C++ | ✅ Complete (PR #186) |
19+
| 4 | Certificates extension pack + local cert utilities | 🔜 Next |
20+
| 5 | Azure Key Vault extension pack | Planned |
21+
| 6 | MST (Microsoft Transparency) extension pack | Planned |
22+
| 7 | CLI tool + packaging | Planned |
23+
24+
## Porting a New Extension Pack
25+
26+
### 1. Identify V2 C# Source
27+
```
28+
V2/CoseSign1.Certificates/ → native/rust/extension_packs/certificates/
29+
V2/CoseSign1.Transparent.MST/ → native/rust/extension_packs/mst/
30+
```
31+
32+
### 2. Create Rust Crate Structure
33+
```
34+
extension_packs/new_pack/
35+
├── Cargo.toml
36+
├── src/
37+
│ ├── lib.rs # Module-level docs, pub use re-exports
38+
│ ├── signing/ # If pack contributes to signing
39+
│ │ └── mod.rs
40+
│ └── validation/ # If pack contributes to validation
41+
│ ├── mod.rs
42+
│ ├── trust_pack.rs # impl CoseSign1TrustPack
43+
│ ├── fact_producer.rs
44+
│ └── key_resolver.rs
45+
├── tests/ # Integration tests (not in src/)
46+
└── ffi/
47+
├── Cargo.toml
48+
├── src/
49+
│ ├── lib.rs # FFI exports + catch_unwind
50+
│ ├── types.rs # Opaque handle types
51+
│ └── error.rs # Thread-local error storage
52+
└── tests/ # FFI smoke + null safety tests
53+
```
54+
55+
### 3. Implement Core Traits
56+
57+
Every extension pack implements some subset of:
58+
59+
```rust
60+
// Validation side
61+
impl CoseSign1TrustPack for MyPack {
62+
fn name(&self) -> &str;
63+
fn fact_producer(&self) -> Arc<dyn TrustFactProducer>;
64+
fn cose_key_resolvers(&self) -> Vec<Arc<dyn CoseKeyResolver>>;
65+
fn post_signature_validators(&self) -> Vec<Arc<dyn PostSignatureValidator>>;
66+
fn default_trust_plan(&self) -> Option<CompiledTrustPlan>;
67+
}
68+
69+
// Signing side (if applicable)
70+
impl TransparencyProvider for MyProvider {
71+
fn submit_and_poll(&self, message: &[u8]) -> Result<Vec<u8>, TransparencyError>;
72+
}
73+
74+
// Factory extension (if applicable)
75+
impl SignatureFactoryProvider for MyFactory {
76+
fn create_bytes(&self, ...) -> Result<Vec<u8>, FactoryError>;
77+
}
78+
```
79+
80+
### 4. Create FFI Projection
81+
82+
Follow these mandatory patterns (see `native-ffi.instructions.md` for full details):
83+
84+
- `#![deny(unsafe_op_in_unsafe_fn)]`
85+
- All functions: `catch_unwind(AssertUnwindSafe(|| { ... }))`
86+
- All pointer params: null-checked before dereference
87+
- Handle-to-inner: bounded `<'a>` lifetimes
88+
- Thread-local error storage: `LAST_ERROR: RefCell<Option<ErrorInner>>`
89+
- ABI version export: `cose_*_abi_version() -> u32`
90+
91+
### 5. Create C/C++ Headers
92+
93+
**C header**: `native/c/include/cose/sign1/extension_packs/new_pack.h`
94+
**C++ header**: `native/c_pp/include/cose/sign1/extension_packs/new_pack.hpp`
95+
96+
C++ must provide:
97+
- RAII wrapper class (move-only, no copy)
98+
- `release()` method for ownership transfer
99+
- `@see` cross-references for zero-copy alternatives
100+
- `ByteView` return types for byte accessors
101+
102+
### 6. Update Build Configuration
103+
104+
- Add to `Cargo.toml` workspace members
105+
- Add to `CMakeLists.txt` with `COSE_HAS_NEW_PACK` feature guard
106+
- Add to `cose.hpp` umbrella include
107+
- Update `vcpkg.json` if new features needed
108+
109+
### 7. Test Requirements
110+
111+
| Test Type | Location | Minimum |
112+
|-----------|----------|---------|
113+
| Rust unit tests | `extension_packs/new_pack/tests/` | All public APIs |
114+
| FFI smoke tests | `extension_packs/new_pack/ffi/tests/` | Null safety + lifecycle |
115+
| C smoke test | `native/c/tests/` | Roundtrip with feature guard |
116+
| C++ smoke test | `native/c_pp/tests/` | RAII lifecycle test |
117+
| Coverage | Via `collect-coverage.ps1` | ≥ 90% line coverage |
118+
119+
## V2 C# → Rust Translation Patterns
120+
121+
### Async
122+
```csharp
123+
// C#: Everything is async
124+
public async Task<T> DoSomethingAsync(CancellationToken ct) { ... }
125+
```
126+
```rust
127+
// Rust: Provide both sync and async variants
128+
pub fn do_something(&self) -> Result<T, Error> { ... }
129+
pub async fn do_something_async(&self) -> Result<T, Error> { ... }
130+
```
131+
132+
### Options / Configuration
133+
```csharp
134+
// C#: Options class with defaults
135+
public class MyOptions {
136+
public string Endpoint { get; set; } = "https://default";
137+
public TimeSpan Timeout { get; set; } = TimeSpan.FromSeconds(30);
138+
}
139+
```
140+
```rust
141+
// Rust: Struct with Default impl
142+
#[derive(Default)]
143+
pub struct MyOptions {
144+
pub endpoint: Option<String>,
145+
pub timeout: Duration,
146+
}
147+
148+
impl Default for MyOptions {
149+
fn default() -> Self {
150+
Self {
151+
endpoint: None,
152+
timeout: Duration::from_secs(30),
153+
}
154+
}
155+
}
156+
```
157+
158+
### Dependency Injection
159+
```csharp
160+
// C#: Constructor injection
161+
public MyService(IHttpClient client, ILogger logger) { ... }
162+
```
163+
```rust
164+
// Rust: Trait objects in Arc
165+
pub struct MyService {
166+
client: Arc<dyn HttpClient>,
167+
log_verbose: Option<fn(&str)>,
168+
}
169+
```
170+
171+
### Error Handling
172+
```csharp
173+
// C#: Exceptions
174+
throw new InvalidOperationException("message");
175+
```
176+
```rust
177+
// Rust: Result + manual error types (NO thiserror)
178+
#[derive(Debug)]
179+
pub enum MyError {
180+
InvalidOperation(String),
181+
}
182+
impl std::fmt::Display for MyError { ... }
183+
impl std::error::Error for MyError {}
184+
```
185+
186+
## Performance Evaluation Checklist
187+
188+
When porting Azure SDK interactions (e.g., MST client, AKV client), evaluate:
189+
190+
- [ ] Does the Azure Rust SDK cache HTTP connections? (V2 C# required explicit caching)
191+
- [ ] Does the SDK handle LRO polling efficiently? (V2 required custom poller)
192+
- [ ] Are retry policies configurable? (V2 needed custom retry wrapper)
193+
- [ ] Is the transaction log response cached? (V2 had a caching policy issue)
194+
- [ ] Can we avoid serialization copies in the SDK's request/response pipeline?
195+
196+
## CI Workflow
197+
198+
PRs to `native_ports_final` trigger:
199+
- `native-rust`: `cargo build + cargo test + collect-coverage.ps1` (90% gate)
200+
- `native-c-cpp`: CMake build + GTest + smoke tests
201+
- `CodeQL` (Rust + C/C++)
202+
- `dependency-review`
203+
204+
All must pass before merge.

0 commit comments

Comments
 (0)