# Feature Specification: SDK Review Hardening

**Feature Branch**: `002-sdk-review-hardening`
**Created**: 2026-01-17
**Status**: Draft
**Input**: Multi-perspective adversarial review of SDK before public release

## Overview

This specification captures findings from 8 adversarial reviewer personas simulating the harsh criticism the SDK would receive upon public release. Reviewers included: Open Source Advocate, Rust Purist, Quality Engineer, Linus Torvalds, Arrogant Junior Dev, Enterprise Security Architect, Network Architect, and Amazon WebDev integrator.

The goal is to address issues that would result in the SDK being "slayed on Reddit and everywhere else the neckbeards live" - achieving a 10/10 release quality.

---

## User Scenarios & Testing

### User Story 1 - Security-Critical Fixes (Priority: P0-BLOCKER)

As a security-conscious organization, I need cryptographic operations to be implemented correctly so that credentials cannot be compromised.

**Why this priority**: Security flaws are CVE-worthy and will destroy the SDK's reputation immediately upon discovery.

**Independent Test**: Security audit by external party would pass without critical findings.

**Acceptance Scenarios**:

1. **Given** a DPoP key is generated, **When** using cryptographic RNG, **Then** `OsRng` MUST be used, not `thread_rng()`
2. **Given** a signing key needs zeroization, **When** `Zeroize::zeroize()` is called, **Then** memory MUST be overwritten with zeros, not replaced with new random key material
3. **Given** an endpoint URL is configured, **When** validating the URL, **Then** `http://` MUST be rejected except for `localhost` (HTTPS-only in production)
4. **Given** a JWT has time claims, **When** validating `exp` and `nbf`, **Then** a configurable clock skew tolerance MUST be applied (default: 60 seconds)
5. **Given** a DPoP token is received, **When** validating token binding, **Then** `cnf.jkt` claim MUST be verified against SDK's key thumbprint

---

### User Story 2 - Community Standards Compliance (Priority: P1)

As an open source contributor, I need the repository to meet community expectations so that I can evaluate and contribute to the project.

**Why this priority**: Missing community files (CODE_OF_CONDUCT, SECURITY.md) are immediate red flags that will be called out on HN/Reddit within minutes of release.

**Independent Test**: GitHub Community Standards checklist shows 100% compliance.

**Acceptance Scenarios**:

1. **Given** a user visits the repository, **When** looking for conduct expectations, **Then** CODE_OF_CONDUCT.md MUST exist in repository root
2. **Given** a security researcher finds a vulnerability, **When** looking for disclosure process, **Then** SECURITY.md MUST exist with GPG key and 90-day timeline
3. **Given** a user reads the README, **When** evaluating project health, **Then** badges for CI, crates.io, docs.rs, and coverage MUST be visible
4. **Given** a user wants to try the npm package, **When** following README instructions, **Then** the package name MUST match actual published package

---

### User Story 3 - Rust Idiom Compliance (Priority: P1)

As a Rust developer, I need the SDK to follow Rust conventions so that I can integrate it without fighting the API.

**Why this priority**: r/rust will tear apart non-idiomatic code and word spreads fast.

**Independent Test**: `cargo clippy --all-features -- -D warnings` passes with zero warnings.

**Acceptance Scenarios**:

1. **Given** a domain primitive (DID, ClientId, URL), **When** represented in code, **Then** a newtype wrapper MUST be used (not raw `String`)
2. **Given** a magic number exists, **When** reading the code, **Then** it MUST be a named `const` with documentation
3. **Given** the WASM/native trait split, **When** defining async traits, **Then** a single `#[cfg_attr(...)]` pattern MUST be used, not duplicated traits
4. **Given** public enums exist, **When** designing for SemVer, **Then** `#[non_exhaustive]` MUST be applied

---

### User Story 4 - Test Coverage Reality (Priority: P1)

As a QA engineer, I need tests to actually verify behavior so that security claims are backed by evidence.

**Why this priority**: The `#[ignore]` on signature verification is a smoking gun that will be screenshot and posted everywhere.

**Independent Test**: Remove all `#[ignore]` attributes and all tests pass.

**Acceptance Scenarios**:

1. **Given** SD-JWT signature verification is claimed, **When** running tests, **Then** an end-to-end test with real P-256 key pair MUST exist and pass (no `#[ignore]`)
2. **Given** test coverage is reported, **When** comparing README claims to CI config, **Then** thresholds MUST match (95% or 80%, pick one)
3. **Given** examples exist, **When** CI runs, **Then** all examples MUST be compiled/run (not just tests)
4. **Given** WASM is supported, **When** testing WASM, **Then** at least one `#[wasm_bindgen_test]` per WASM-specific code path MUST exist

---

### User Story 5 - Enterprise Network Compatibility (Priority: P2)

As a network architect, I need the SDK to work in corporate networks so that it can be deployed in enterprise environments.

**Why this priority**: SDKs that don't work behind proxies are immediately unusable for enterprise customers.

**Independent Test**: SDK works through a corporate HTTP proxy.

**Acceptance Scenarios**:

1. **Given** a corporate network with mandatory proxy, **When** configuring the SDK, **Then** `proxy_url` and `proxy_auth` MUST be configurable
2. **Given** different timeout requirements, **When** configuring the SDK, **Then** connect, read, and request timeouts MUST be independently configurable
3. **Given** connection pool tuning is needed, **When** configuring the SDK, **Then** pool size and idle timeout MUST be configurable (not hardcoded)

---

### User Story 6 - Integration Pain Reduction (Priority: P2)

As an Amazon WebDev forced to integrate this SDK, I need clear error handling and observable behavior so that I can debug issues at 3am without calling the vendor.

**Why this priority**: Integration friction = abandoned SDK.

**Independent Test**: A developer can complete integration in one sprint without contacting support.

**Acceptance Scenarios**:

1. **Given** a timeout occurs during polling, **When** checking the result, **Then** it MUST return `Err(LastIDError::Timeout(...))`, not `Ok(RequestStatus::Timeout)`
2. **Given** HTTP requests are made, **When** debugging, **Then** correlation IDs MUST be attached for end-to-end tracing
3. **Given** token management is happening, **When** tokens fail, **Then** error messages MUST distinguish between client_id issues, DPoP issues, and IDP issues
4. **Given** config is loaded, **When** client_id is empty, **Then** validation MUST fail at build time, not at first request

---

### User Story 7 - Simplification (Priority: P2)

As a developer who just wants this to work, I need the SDK to not be over-engineered so that I can understand and use it.

**Why this priority**: Complexity without value is a maintenance burden and barrier to adoption.

**Independent Test**: A new developer can understand the core flow in 30 minutes.

**Acceptance Scenarios**:

1. **Given** 8 policy feature flags exist, **When** adding the SDK, **Then** `default` feature SHOULD include all policies (opt-out, not opt-in)
2. **Given** a simple policy with one field, **When** using type-state pattern, **Then** it SHOULD be reconsidered (phantom types for one field is over-engineering)
3. **Given** platform abstraction exists in shared.rs, **When** cfg blocks are still needed in client code, **Then** the abstraction SHOULD be removed (it's not abstracting anything)
4. **Given** a 60-second cache exists, **When** implementing, **Then** a simple HashMap with timestamps SHOULD suffice (no trait hierarchy needed)

---

### User Story 8 - Documentation Accuracy (Priority: P2)

As a user reading the documentation, I need it to match reality so that I can trust the project.

**Why this priority**: Documentation lies erode trust immediately.

**Independent Test**: Every code example in README compiles and runs.

**Acceptance Scenarios**:

1. **Given** default features are documented, **When** reading README, **Then** documented defaults MUST match Cargo.toml exactly
2. **Given** spec links exist in README, **When** clicking links, **Then** all links MUST resolve to existing files
3. **Given** npm package is documented, **When** reading README, **Then** package name MUST match actual published package
4. **Given** an example config file is referenced, **When** cloning the repo, **Then** `lastid.toml.example` MUST exist with documented options

---

### User Story 9 - Observability (Priority: P3)

As a network architect, I need metrics and tracing so that I can monitor SDK health in production.

**Why this priority**: Without observability, production debugging is guesswork.

**Independent Test**: Grafana dashboard can be populated from SDK metrics.

**Acceptance Scenarios**:

1. **Given** tracing feature is enabled, **When** requests are made, **Then** structured spans with request_id MUST be emitted
2. **Given** WebSocket fallback occurs, **When** falling back to polling, **Then** a warning log MUST be emitted (not silent)
3. **Given** trust registry cache is used, **When** monitoring, **Then** cache hit/miss metrics MUST be available

---

### User Story 10 - Serverless Deployment Support (Priority: P3)

As a developer deploying to Lambda, I need DPoP keys to be persistable so that cold starts don't break token binding.

**Why this priority**: Serverless is a common deployment pattern; fresh keys per instance breaks DPoP binding.

**Independent Test**: Lambda cold start maintains valid token binding.

**Acceptance Scenarios**:

1. **Given** DPoP keypair is generated, **When** persisting state, **Then** keypair serialization/deserialization MUST be supported
2. **Given** a new client instance is created, **When** providing a persisted keypair, **Then** the SDK MUST use it instead of generating fresh

---

### Edge Cases

- What happens when IDP is completely unavailable? (Need circuit breaker)
- What happens when clock skew exceeds tolerance? (Clear error message needed)
- What happens when trust registry cache is stale during issuer revocation? (60-second window of acceptance)
- What happens when proxy requires NTLM authentication? (Document limitations)
- What happens when WASM localStorage is unavailable? (Graceful fallback needed)

---

## Requirements

### Security Requirements

- **SEC-001**: All production endpoints MUST use HTTPS. HTTP is only permitted for `localhost` URLs.
- **SEC-002**: Cryptographic key generation MUST use `OsRng`, not `thread_rng()`.
- **SEC-003**: Secret zeroization MUST overwrite memory with zeros, not replace with new random data.
- **SEC-004**: DPoP token binding MUST be verified by checking `cnf.jkt` claim matches SDK key thumbprint.
- **SEC-005**: JWT time claims (`iat`, `exp`, `nbf`) MUST be validated with configurable clock skew (default: 60s).
- **SEC-006**: DPoP proofs MUST include `exp` claim with maximum lifetime of 300 seconds.
- **SEC-007**: Internal token cache MUST use `SecretBox<AccessToken>`, not plain `String`.
- **SEC-008**: Error messages exposed to callers MUST NOT include raw HTTP response bodies.

### Functional Requirements

- **FR-001**: SDK MUST provide CODE_OF_CONDUCT.md (Contributor Covenant 2.1 recommended).
- **FR-002**: SDK MUST provide SECURITY.md with vulnerability disclosure process.
- **FR-003**: README MUST include badges for CI status, crates.io version, docs.rs, coverage.
- **FR-004**: SDK MUST use newtypes for domain primitives: `Did`, `ClientId`, `EndpointUrl`, `RequestId`.
- **FR-005**: SDK MUST use `#[non_exhaustive]` on all public enums.
- **FR-006**: SDK MUST have zero clippy warnings on `cargo clippy --all-features -- -D warnings`.
- **FR-007**: SDK MUST provide proxy configuration: `proxy_url`, `proxy_auth`, `no_proxy`.
- **FR-008**: SDK MUST provide separate connect, read, and request timeout configuration.
- **FR-009**: SDK MUST validate `client_id` is non-empty at build time.
- **FR-010**: Timeout during polling MUST return `Err(LastIDError::Timeout)`, not `Ok(RequestStatus::Timeout)`.
- **FR-011**: HTTP requests MUST include correlation ID header for tracing.
- **FR-012**: WebSocket fallback to polling MUST emit warning log.
- **FR-013**: DPoP keypair MUST be serializable/deserializable for persistence.
- **FR-014**: All magic numbers MUST be named constants with documentation.
- **FR-015**: Default feature set SHOULD include all policy types (opt-out model).

### Test Requirements

- **TR-001**: Signature verification test MUST use real P-256 keypair (no `#[ignore]`).
- **TR-002**: Coverage threshold MUST be consistent between docs and CI.
- **TR-003**: All examples MUST be compiled/run in CI.
- **TR-004**: WASM-specific code paths MUST have `#[wasm_bindgen_test]` coverage.
- **TR-005**: Fuzz testing MUST cover SD-JWT parsing functions.
- **TR-006**: Property-based tests with proptest MUST cover parsing functions.

### Documentation Requirements

- **DR-001**: README default features MUST match Cargo.toml exactly.
- **DR-002**: All documentation links MUST resolve to existing files.
- **DR-003**: npm package name MUST match actual published package.
- **DR-004**: `lastid.toml.example` MUST exist with all documented options.
- **DR-005**: MSRV policy MUST be documented.
- **DR-006**: Credential claim schemas MUST be documented per credential type.

### Key Entities

- **Did**: Decentralized Identifier (e.g., `did:lastid:abc123`) - validated format on construction
- **ClientId**: OAuth client identifier - non-empty string validation
- **EndpointUrl**: IDP endpoint URL - HTTPS validation (localhost exception for dev)
- **RequestId**: Credential request identifier - UUID format
- **AccessToken**: OAuth access token - wrapped in `SecretBox` for protection
- **DPoPKeyPair**: Ephemeral key pair - serializable for persistence, zeroizable

---

## Success Criteria

### Measurable Outcomes

- **SC-001**: GitHub Community Standards shows 100% compliance (CODE_OF_CONDUCT, SECURITY.md, etc.)
- **SC-002**: `cargo clippy --all-features -- -D warnings` produces zero warnings
- **SC-003**: All tests pass with zero `#[ignore]` attributes on security-critical tests
- **SC-004**: External security review finds zero CRITICAL or HIGH severity issues
- **SC-005**: SDK successfully operates through HTTP proxy in enterprise test environment
- **SC-006**: New developer completes integration in under 4 hours following documentation
- **SC-007**: README code examples compile and run without modification
- **SC-008**: r/rust community post receives positive reception (upvote ratio > 80%)

---

## Reviewer Attribution

Issues identified by each reviewer persona:

| Persona | Critical Issues | User Stories |
|---------|-----------------|--------------|
| Open Source Advocate | No CODE_OF_CONDUCT, no SECURITY.md, broken doc links, npm name mismatch | 2, 8 |
| Rust Purist | String everywhere (no newtypes), duplicated WASM traits, magic numbers, clippy warnings | 3 |
| Quality Engineer | Ignored signature verification test, coverage mismatch, no fuzz testing | 4 |
| Linus Torvalds | Over-engineered type-state for simple structs, useless abstractions, 8 feature flags | 7 |
| Junior Dev (valid points) | Zeroize generates new keys (!), uses thread_rng for crypto, no #[non_exhaustive] | 1 |
| Security Architect | HTTP allowed, no clock skew, missing nbf check, tokens not SecretBox | 1 |
| Network Architect | No proxy support, hardcoded pool config, no circuit breaker | 5, 9 |
| Amazon WebDev | Timeout as Ok(), no correlation IDs, fresh DPoP keys per instance, opaque token errors | 6, 10 |
