# Research: SDK Review Hardening

**Branch**: `002-sdk-review-hardening` | **Date**: 2026-01-17

## Executive Summary

Research validates that all 8 adversarial reviewer concerns are **real and actionable**. Critical security issues (zeroization, RNG, HTTPS enforcement) require immediate fixes. Code idiom violations (no newtypes, no `#[non_exhaustive]`, magic numbers) and test coverage gaps (ignored signature test, zero WASM tests) need systematic remediation.

---

## Security Findings (SEC-001 to SEC-008)

### SEC-001: HTTP URLs Allowed in Production

**Decision**: VIOLATION CONFIRMED - Must enforce HTTPS-only (localhost exception)

**Evidence**:
- `src/config/mod.rs:163-167` - Validation accepts `http://` URLs
- `src/policies/base.rs:81-85` - Callback URL validation accepts HTTP
- All 8 policy files have identical HTTP-accepting validation

**Rationale**: HTTP is vulnerable to MITM attacks. Identity verification must use HTTPS.

**Alternatives Considered**:
- Allow HTTP everywhere (rejected: security risk)
- Warn but allow HTTP (rejected: still exploitable)
- **Chosen**: Block HTTP except `http://localhost:*` for dev

---

### SEC-002: Weak RNG in Default Implementation

**Decision**: VIOLATION CONFIRMED - Must use `OsRng` exclusively

**Evidence**:
- `src/crypto/dpop.rs:54` - `Default` impl uses `thread_rng()`
- `src/crypto/dpop.rs:62` - `Zeroize` impl uses `thread_rng()`
- `src/crypto/dpop.rs:109` - `generate()` correctly uses `OsRng`

**Rationale**: `thread_rng()` is not cryptographically secure for key generation.

**Alternatives Considered**:
- Keep `thread_rng()` for non-crypto paths (rejected: confusing, risky)
- **Chosen**: Remove `Default` impl entirely, require explicit `generate()`

---

### SEC-003: Zeroization Generates New Keys

**Decision**: VIOLATION CONFIRMED - Critical security bug

**Evidence**:
- `src/crypto/dpop.rs:58-64` - `Zeroize` creates new random key instead of zeroing

```rust
impl Zeroize for SigningKeyWrapper {
    fn zeroize(&mut self) {
        self.0 = SigningKey::random(&mut rand::thread_rng()); // WRONG!
    }
}
```

**Rationale**: This doesn't zero memory; old key bytes remain until overwritten.

**Alternatives Considered**:
- Use `MaybeUninit` with manual zeroing (rejected: complex, unsafe-adjacent)
- **Chosen**: Use `p256`'s built-in key handling + explicit byte zeroing via `zeroize` crate on the key bytes

---

### SEC-004: No DPoP Token Binding Verification

**Decision**: VIOLATION CONFIRMED - Must verify `cnf.jkt` claim

**Evidence**:
- `src/auth/token_manager.rs:168-176` - No `cnf` claim in token request
- No verification of `cnf.jkt` when receiving tokens

**Rationale**: Without token binding, stolen tokens can be used with different DPoP keys.

**Alternatives Considered**:
- Skip binding for simplicity (rejected: defeats DPoP purpose)
- **Chosen**: Implement RFC 9449 Section 6 token binding verification

---

### SEC-005: No Clock Skew Tolerance

**Decision**: VIOLATION CONFIRMED - Must add configurable skew

**Evidence**:
- `src/client/lastid_client.rs:393-401` - Exact comparison with `>=`
- No `nbf` claim validation at all

**Rationale**: Clock drift between systems causes valid credentials to be rejected.

**Alternatives Considered**:
- Fixed 30-second skew (rejected: not configurable)
- **Chosen**: Configurable `clock_skew_seconds` with 60s default

---

## Code Idiom Findings (FR-004, FR-005, FR-014)

### FR-004: Newtypes for Domain Primitives

**Decision**: PARTIAL IMPLEMENTATION - Must add newtypes

**Evidence**:
- `src/types/request_id.rs` - `RequestId` IS a proper newtype (good)
- `src/config/mod.rs:79,83,87` - `idp_endpoint`, `client_id`, `trust_registry_endpoint` are raw `String`
- `src/trust_registry/types.rs:36,42` - `enterprise_did`, `endpoint_url` are raw `String`

**Chosen**: Add newtypes: `Did`, `ClientId`, `EndpointUrl` with validation on construction

---

### FR-005: Non-Exhaustive Enums

**Decision**: VIOLATION CONFIRMED - No `#[non_exhaustive]` anywhere

**Evidence**: All public enums lack the attribute:
- `LastIDError`, `HttpError`, `TrustRegistryError`, `WebSocketError`
- `RequestStatus`, `IssuerStatus`, `TrustLevel`
- `PersonaField`, `EmploymentStatus`, `AgeComparison`, etc.

**Chosen**: Add `#[non_exhaustive]` to all public enums for SemVer safety

---

### FR-014: Magic Numbers

**Decision**: INCONSISTENT - Must extract to constants

**Evidence**:
- Hardcoded `30` in `src/http/client.rs:39,68,78` (timeout, pool)
- Hardcoded `60` in `src/http/idp_client.rs:164`, `src/trust_registry/client.rs:100` (retry-after)
- Named constants exist in `src/config/cache.rs:30-31`, `src/http/websocket/subscription.rs:27-29`

**Chosen**: Extract all magic numbers to named constants with doc comments

---

## Test Findings (TR-001 to TR-006)

### TR-001: Ignored Signature Verification Test

**Decision**: VIOLATION CONFIRMED - Critical test is `#[ignore]`

**Evidence**:
- `tests/poll_and_verify.rs:244` - `#[ignore = "Requires properly signed SD-JWT..."]`
- Test uses `create_mock_sdjwt()` with fake `"mock-signature"`

**Chosen**: Implement real P-256 key pair signing + verification test

---

### TR-002: Coverage Threshold Mismatch

**Decision**: MISMATCH CONFIRMED

**Evidence**:
- README claims "100% test coverage requirement"
- `ci.yml:76` enforces `80%` threshold

**Chosen**: Update README to claim 80% (more realistic for SDK)

---

### TR-003: Examples Not Compiled in CI

**Decision**: NOT VERIFIED - No explicit example compilation

**Evidence**:
- 8 example files exist in `/examples/`
- No `cargo run --example` or `cargo test --examples` in CI

**Chosen**: Add CI step to compile and check examples

---

### TR-004: Zero WASM Tests

**Decision**: VIOLATION CONFIRMED - Zero `#[wasm_bindgen_test]`

**Evidence**:
- Grep found zero `#[wasm_bindgen_test]` in entire codebase
- WASM-specific code exists in `src/crypto/dpop.rs:280-288`, `src/trust_registry/cache.rs`

**Chosen**: Add WASM tests for timestamp, cache, and WebSocket code paths

---

## Networking Findings (FR-007, FR-008, FR-010, FR-011)

### FR-007: No Proxy Support

**Decision**: NOT IMPLEMENTED - Must add proxy configuration

**Evidence**:
- `src/http/client.rs` - No `.proxy()` calls
- No `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` handling

**Chosen**: Add `NetworkConfig` with `proxy_url`, `proxy_auth`, `no_proxy` fields

---

### FR-008: Single Unified Timeout

**Decision**: PARTIAL - Only one timeout exists

**Evidence**:
- `src/config/mod.rs:107` - Single `timeout_seconds` field
- No separate connect, read, request timeouts

**Chosen**: Add `connect_timeout_seconds`, `read_timeout_seconds` to config

---

### FR-010: Timeout Returns Ok() Instead of Err()

**Decision**: VIOLATION CONFIRMED - Wrong return type

**Evidence**:
- `src/client/lastid_client.rs:195-201` - Returns `Ok(RequestStatus::Timeout {...})`
- Spec requires `Err(LastIDError::Timeout(...))`

**Chosen**: Change polling timeout to return `Err(LastIDError::Timeout(elapsed))`

---

### FR-011: No Correlation ID Headers

**Decision**: NOT IMPLEMENTED

**Evidence**:
- Only `Authorization` and `DPoP` headers set
- No `X-Request-ID` or `X-Correlation-ID`

**Chosen**: Add correlation ID header to all HTTP requests

---

### Connection Pool Hardcoded

**Decision**: HARDCODED - Must make configurable

**Evidence**:
- `src/http/client.rs:39-40` - `pool_idle_timeout(30)`, `pool_max_idle_per_host(5)`

**Chosen**: Add to `NetworkConfig` or `SDKConfig`

---

## Summary: Violations by Priority

| Priority | Count | Requirements |
|----------|-------|--------------|
| P0 (BLOCKER) | 5 | SEC-001, SEC-002, SEC-003, SEC-004, SEC-005 |
| P1 | 5 | FR-005, FR-006, TR-001, TR-002, TR-004 |
| P2 | 7 | FR-004, FR-007, FR-008, FR-010, FR-011, FR-014, TR-003 |

All spec requirements have been researched and validated. Proceed to Phase 1 design.
