Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
126 changes: 126 additions & 0 deletions .github/instructions/code-review-standards.instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
---
applyTo: "native/**"
---
# Code Review Standards — Native CoseSignTool

> Criteria for reviewing PRs and evaluating code quality across the native stack.

## Review Dimensions

Every review of native code should evaluate these dimensions:

### 1. Zero-Copy / No-Allocation Architecture
- Are unnecessary `.clone()`, `.to_vec()`, `.to_owned()`, `.to_string()` present?
- Do FFI handle conversions use bounded lifetimes (`<'a>`) not `'static`?
- Do C++ accessors return `ByteView` (borrowed) not `std::vector` (copied)?
- Are `_consume` / `_to_message` zero-copy variants available where appropriate?
- See `zero-copy-design.instructions.md` for the full checklist.

### 2. Safety & Correctness
- **Null checks**: Every FFI pointer param checked before dereference.
- **Panic safety**: Every `extern "C"` function wrapped in `catch_unwind`.
- **SAFETY comments**: Every `unsafe` block has a `// SAFETY:` justification.
- **Lifetimes**: No `'static` references to heap-allocated handles.
- **Memory ownership**: `*const` for borrowed, `*mut` for owned/consumed. Documented in C header and C++ wrapper.

### 3. API Design & Ergonomics
- Builder patterns are fluent (return `&mut self` or `Self`).
- Error types use manual `Display` + `Error` impls (no `thiserror`).
- Traits are `Send + Sync` when stored in `Arc`.
- C++ classes are move-only (delete copy ctor/assign).
- Destructors check `if (handle_)` before calling `*_free()`.

### 4. Test Quality
- Tests follow Arrange-Act-Assert.
- Both success and error paths are tested.
- FFI null-pointer safety tests for every parameter.
- Roundtrip tests: sign → parse → validate.
- No shared mutable state between tests (parallel-safe).
- Temp files use unique names (thread ID or nanos).

### 5. Documentation
- Public APIs have `///` doc comments.
- FFI functions have `# Safety` sections.
- Module-level `//!` comments in every `lib.rs`.
- C++ methods have `@see` cross-refs to zero-copy alternatives.

## Grading Scale

| Grade | Meaning |
|-------|---------|
| A+ | Exceptional. No issues. Exemplary patterns. |
| A | Excellent. Minor style nits only. |
| A- | Very good. 1-2 non-blocking improvements identified. |
| B+ | Good. Several improvements needed but no bugs. |
| B | Acceptable. Notable gaps in docs, tests, or design. |
| C | Needs work. Missing safety checks, unsound lifetimes, or significant test gaps. |
| F | Blocking issues. UB, memory leaks, or security vulnerabilities. |

## Common Anti-Patterns to Flag

### Rust
```rust
// ❌ Cloning when moving is possible (builder consumed via Box::from_raw)
builder_inner.protected.clone()

// ❌ 'static lifetime on FFI handle reference
fn handle_to_inner(h: *const H) -> Option<&'static Inner>

// ❌ .to_string() on string literals in error paths
Err(MyError::InvalidFormat("expected:format".to_string()))
// ✅ Use .into()
Err(MyError::InvalidFormat("expected:format".into()))

// ❌ thiserror in production crates
#[derive(thiserror::Error)] // Not allowed

// ❌ expect()/unwrap() in production code (tests are fine)
let value = map.get("key").unwrap();
```

### C++
```cpp
// ❌ const_cast to work around ownership semantics
const char* s = const_cast<const char*>(raw_ptr);

// ❌ Forgetting to null-out source in move constructor
HeaderMap(HeaderMap&& other) : handle_(other.handle_) { }
// ✅ Must null the source
HeaderMap(HeaderMap&& other) noexcept : handle_(other.handle_) {
other.handle_ = nullptr;
}

// ❌ Missing copy deletion
class MyHandle { /* no copy ctor/assign declaration */ };
// ✅ Explicitly delete
MyHandle(const MyHandle&) = delete;
MyHandle& operator=(const MyHandle&) = delete;
```

### C Headers
```c
// ❌ Missing extern "C" guard
// ✅ Every C header needs:
#ifdef __cplusplus
extern "C" {
#endif

// ❌ Using const for output ownership-transfer pointers
int func(const char** out_string); // implies borrowed
// ✅ Non-const for caller-owned allocations
int func(char** out_string); // caller must free
```

## Coverage Exclusion Policy

Only FFI boundary stubs may use `#[cfg_attr(coverage_nightly, coverage(off))]`.
**Never exclude**: business logic, validation, parsing, crypto, error handling.

Justified exclusions:
- `handle_panic()` — unreachable without `catch_unwind` triggering
- `write_signed_bytes/message()` — unreachable due to mandatory post-sign verification
- `*_abi_version()` — compile-time constants
- `*_free()` / `*_string_free()` — pointer deallocation stubs
- `cose_last_error_*()` — thread-local error accessors

Flag any exclusion on non-FFI code as a review blocker.
204 changes: 204 additions & 0 deletions .github/instructions/migration-playbook.instructions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
---
applyTo: "native/**"
---
# Migration Playbook — V2 C# to Native Rust

> Step-by-step guidance for porting feature packs and capabilities from the
> V2 C# branch (`users/jstatia/v2_clean_slate:V2/`) to native Rust.

## Migration Phases

The native port follows a layered merge strategy. Each phase is a staged PR
from a working branch into `native_ports_final`.

| Phase | What | Status |
|-------|------|--------|
| 1 | Primitives (CBOR, Crypto, COSE types, Sign1 builder) | ✅ Complete |
| 2 | OpenSSL crypto provider + PEM support | ✅ Complete |
| 3 | Signing, Validation, DID, Factories, Headers + FFI + C/C++ | ✅ Complete (PR #186) |
| 4 | Certificates extension pack + local cert utilities | 🔜 Next |
| 5 | Azure Key Vault extension pack | Planned |
| 6 | MST (Microsoft Transparency) extension pack | Planned |
| 7 | CLI tool + packaging | Planned |

## Porting a New Extension Pack

### 1. Identify V2 C# Source
```
V2/CoseSign1.Certificates/ → native/rust/extension_packs/certificates/
V2/CoseSign1.Transparent.MST/ → native/rust/extension_packs/mst/
```

### 2. Create Rust Crate Structure
```
extension_packs/new_pack/
├── Cargo.toml
├── src/
│ ├── lib.rs # Module-level docs, pub use re-exports
│ ├── signing/ # If pack contributes to signing
│ │ └── mod.rs
│ └── validation/ # If pack contributes to validation
│ ├── mod.rs
│ ├── trust_pack.rs # impl CoseSign1TrustPack
│ ├── fact_producer.rs
│ └── key_resolver.rs
├── tests/ # Integration tests (not in src/)
└── ffi/
├── Cargo.toml
├── src/
│ ├── lib.rs # FFI exports + catch_unwind
│ ├── types.rs # Opaque handle types
│ └── error.rs # Thread-local error storage
└── tests/ # FFI smoke + null safety tests
```

### 3. Implement Core Traits

Every extension pack implements some subset of:

```rust
// Validation side
impl CoseSign1TrustPack for MyPack {
fn name(&self) -> &str;
fn fact_producer(&self) -> Arc<dyn TrustFactProducer>;
fn cose_key_resolvers(&self) -> Vec<Arc<dyn CoseKeyResolver>>;
fn post_signature_validators(&self) -> Vec<Arc<dyn PostSignatureValidator>>;
fn default_trust_plan(&self) -> Option<CompiledTrustPlan>;
}

// Signing side (if applicable)
impl TransparencyProvider for MyProvider {
fn submit_and_poll(&self, message: &[u8]) -> Result<Vec<u8>, TransparencyError>;
}

// Factory extension (if applicable)
impl SignatureFactoryProvider for MyFactory {
fn create_bytes(&self, ...) -> Result<Vec<u8>, FactoryError>;
}
```

### 4. Create FFI Projection

Follow these mandatory patterns (see `native-ffi.instructions.md` for full details):

- `#![deny(unsafe_op_in_unsafe_fn)]`
- All functions: `catch_unwind(AssertUnwindSafe(|| { ... }))`
- All pointer params: null-checked before dereference
- Handle-to-inner: bounded `<'a>` lifetimes
- Thread-local error storage: `LAST_ERROR: RefCell<Option<ErrorInner>>`
- ABI version export: `cose_*_abi_version() -> u32`

### 5. Create C/C++ Headers

**C header**: `native/c/include/cose/sign1/extension_packs/new_pack.h`
**C++ header**: `native/c_pp/include/cose/sign1/extension_packs/new_pack.hpp`

C++ must provide:
- RAII wrapper class (move-only, no copy)
- `release()` method for ownership transfer
- `@see` cross-references for zero-copy alternatives
- `ByteView` return types for byte accessors

### 6. Update Build Configuration

- Add to `Cargo.toml` workspace members
- Add to `CMakeLists.txt` with `COSE_HAS_NEW_PACK` feature guard
- Add to `cose.hpp` umbrella include
- Update `vcpkg.json` if new features needed

### 7. Test Requirements

| Test Type | Location | Minimum |
|-----------|----------|---------|
| Rust unit tests | `extension_packs/new_pack/tests/` | All public APIs |
| FFI smoke tests | `extension_packs/new_pack/ffi/tests/` | Null safety + lifecycle |
| C smoke test | `native/c/tests/` | Roundtrip with feature guard |
| C++ smoke test | `native/c_pp/tests/` | RAII lifecycle test |
| Coverage | Via `collect-coverage.ps1` | ≥ 90% line coverage |

## V2 C# → Rust Translation Patterns

### Async
```csharp
// C#: Everything is async
public async Task<T> DoSomethingAsync(CancellationToken ct) { ... }
```
```rust
// Rust: Provide both sync and async variants
pub fn do_something(&self) -> Result<T, Error> { ... }
pub async fn do_something_async(&self) -> Result<T, Error> { ... }
```

### Options / Configuration
```csharp
// C#: Options class with defaults
public class MyOptions {
public string Endpoint { get; set; } = "https://default";
public TimeSpan Timeout { get; set; } = TimeSpan.FromSeconds(30);
}
```
```rust
// Rust: Struct with Default impl
#[derive(Default)]
pub struct MyOptions {
pub endpoint: Option<String>,
pub timeout: Duration,
}

impl Default for MyOptions {
fn default() -> Self {
Self {
endpoint: None,
timeout: Duration::from_secs(30),
}
}
}
```

### Dependency Injection
```csharp
// C#: Constructor injection
public MyService(IHttpClient client, ILogger logger) { ... }
```
```rust
// Rust: Trait objects in Arc
pub struct MyService {
client: Arc<dyn HttpClient>,
log_verbose: Option<fn(&str)>,
}
```

### Error Handling
```csharp
// C#: Exceptions
throw new InvalidOperationException("message");
```
```rust
// Rust: Result + manual error types (NO thiserror)
#[derive(Debug)]
pub enum MyError {
InvalidOperation(String),
}
impl std::fmt::Display for MyError { ... }
impl std::error::Error for MyError {}
```

## Performance Evaluation Checklist

When porting Azure SDK interactions (e.g., MST client, AKV client), evaluate:

- [ ] Does the Azure Rust SDK cache HTTP connections? (V2 C# required explicit caching)
- [ ] Does the SDK handle LRO polling efficiently? (V2 required custom poller)
- [ ] Are retry policies configurable? (V2 needed custom retry wrapper)
- [ ] Is the transaction log response cached? (V2 had a caching policy issue)
- [ ] Can we avoid serialization copies in the SDK's request/response pipeline?

## CI Workflow

PRs to `native_ports_final` trigger:
- `native-rust`: `cargo build + cargo test + collect-coverage.ps1` (90% gate)
- `native-c-cpp`: CMake build + GTest + smoke tests
- `CodeQL` (Rust + C/C++)
- `dependency-review`

All must pass before merge.
Loading
Loading