mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 10:21:14 +00:00
[AZ-503] [AZ-504] Cycle 5 Steps 11-15 sync
Wrap up cycle 5 verification + documentation: - Steps 10/11 wrap-up reports (implementation_completeness + implementation_report) for the AZ-503-foundation + AZ-504 batch. - Step 12 test-spec sync: AZ-503-foundation/AZ-504 ACs appended; AZ-505 deferred ACs recorded. - Step 13 update-docs: architecture, data-model, glossary, module- layout, uav-tile-upload contract (v1.1.0), DataAccess + Services + Tests module docs synced; new common_uuidv5.md module doc. - Step 14 security audit: PASS_WITH_WARNINGS; 0 new Critical/High; 2 new Low informational (F1 flightId provenance, F2 pgcrypto deploy gap). - Step 15 performance test: PASS_WITH_INFRA_WARNINGS; PT-08 passed twice (AZ-504 fix verified); PT-01/02 failed due to recurring local Docker/colima DNS cold-start (not an app regression). Cycle-3 perf-harness leftover stays OPEN with replay #5 documented. - Autodev state moved to Step 16 (Deploy). Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,41 @@
|
||||
# Dependency Scan (Cycle 5)
|
||||
|
||||
**Date**: 2026-05-12
|
||||
**Mode**: Delta scan
|
||||
**Scope**: Cycle-5 delta over the cycle-4 dependency scan (`_docs/05_security/dependency_scan_cycle4.md`)
|
||||
**Trigger**: AZ-503-foundation + AZ-504; both Step-15-gated by the same audit infrastructure as cycle 4
|
||||
|
||||
## Cycle-5 Package Manifest Diff
|
||||
|
||||
| csproj | Cycle 4 baseline (post-AZ-500) | Cycle 5 change | Net effect on supply chain |
|
||||
|--------|--------------------------------|----------------|----------------------------|
|
||||
| `SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj` | references Api, TestSupport | **+1 ProjectReference**: `SatelliteProvider.Common` (AZ-503 — so test seeders can call `Uuidv5.Create`) | None — ProjectReference inside the workspace; no new NuGet packages, no new transitive graph nodes |
|
||||
| `SatelliteProvider.Common/SatelliteProvider.Common.csproj` | unchanged from cycle 4 | **+0 PackageReferences** — `Uuidv5.cs` is pure BCL (`System.Security.Cryptography.SHA1`, `System.Buffers.Binary.BinaryPrimitives`, `System.Buffers.ArrayPool`) | None — no new NuGet packages |
|
||||
| `SatelliteProvider.DataAccess/SatelliteProvider.DataAccess.csproj` | unchanged from cycle 4 | **+0 PackageReferences** | None |
|
||||
| `SatelliteProvider.Services.TileDownloader/SatelliteProvider.Services.TileDownloader.csproj` | unchanged from cycle 4 | **+0 PackageReferences** | None |
|
||||
| `SatelliteProvider.Api/SatelliteProvider.Api.csproj` | unchanged from cycle 4 | **+0 PackageReferences** | None |
|
||||
| `SatelliteProvider.Tests/SatelliteProvider.Tests.csproj` | unchanged from cycle 4 | **+0 PackageReferences** — `Uuidv5Tests` is pure BCL | None |
|
||||
|
||||
**Net cycle-5 dependency change**: zero new NuGet packages, zero version bumps, zero removed packages. The only manifest edit is one intra-workspace `ProjectReference` line (`IntegrationTests → Common`).
|
||||
|
||||
## Cycle-5 New PostgreSQL Extensions
|
||||
|
||||
Migration `014_AddTileIdentityColumns.sql` issues `CREATE EXTENSION IF NOT EXISTS pgcrypto`. This is a new runtime database dependency.
|
||||
|
||||
| Extension | Used for | Where it executes | Postures |
|
||||
|-----------|----------|-------------------|----------|
|
||||
| `pgcrypto` | The migration's `pg_temp.uuidv5` PL/pgSQL helper calls `digest(..., 'sha1')` to backfill `location_hash` over every pre-existing `tiles` row | Inside the migration transaction only; **runtime application code does NOT call `pgcrypto`** (UUIDv5 in production paths is computed in C# via `SatelliteProvider.Common.Utils.Uuidv5`) | Standard, bundled-with-Postgres extension. No external download. Known historical CVEs (e.g. CVE-2024-10977 in the `crypt()` Blowfish path, CVE-2025-1094 in `quote_literal`) do NOT touch the `digest()` SHA-1 surface AZ-503 uses. |
|
||||
|
||||
The `pg_temp.uuidv5` helper is a `pg_temp.*` function — automatically scoped to the migration's session and discarded at COMMIT. It is not callable by runtime application code.
|
||||
|
||||
## Cycle-5 Findings
|
||||
|
||||
None. No new CVEs to surface, no version bumps to audit, no transitive graph changes.
|
||||
|
||||
The cycle-4 carry-over (D2-cy4 — `Microsoft.NET.Test.Sdk 17.8.0` transitive `NuGet.Frameworks` Medium-severity finding, test-runtime exposure only) is **unchanged in cycle 5**: AZ-503 did not bump `Microsoft.NET.Test.Sdk` and did not introduce a new test-runtime package. The finding continues to live in `dependency_scan_cycle4.md` and is owned by a still-unscheduled follow-up task (slated for the next Test SDK refresh cycle).
|
||||
|
||||
## Verdict
|
||||
|
||||
**PASS** (cycle-5 delta) — zero new supply-chain findings.
|
||||
|
||||
Cumulative verdict (carrying forward cycle 4): **PASS_WITH_WARNINGS** (1 cycle-3 Medium carry-over via D2-cy4; no Critical/High; AZ-503/AZ-504 add nothing).
|
||||
@@ -0,0 +1,53 @@
|
||||
# Infrastructure & Configuration Review (Cycle 5)
|
||||
|
||||
**Date**: 2026-05-12
|
||||
**Mode**: Delta scan
|
||||
**Scope**: Cycle-5 delta over the cycle-3 infrastructure review (`_docs/05_security/infrastructure_review.md`)
|
||||
|
||||
## Container Security
|
||||
|
||||
`Dockerfile` was not modified in cycle 5. Cycle-3 baseline holds: non-root user, distroless ASP.NET base, no secrets in build args, healthcheck present.
|
||||
|
||||
## CI/CD Security
|
||||
|
||||
Woodpecker CI configuration (`.woodpecker/`) was not modified. The cycle-4 dependency-scan workflow remains the supply-chain gate. AZ-504's `run-performance-tests.sh` fix is exercised by the existing perf-test job — no new step added.
|
||||
|
||||
## Environment Configuration
|
||||
|
||||
### `.env` / `.env.example`
|
||||
|
||||
Not modified in cycle 5. AZ-503 introduced no new env vars. The cycle-3 secrets posture (`GOOGLE_MAPS_API_KEY`, `JWT_SECRET`, `JWT_ISSUER`, `JWT_AUDIENCE` from env / gitignored `.env`; `.env.example` documents them with DEV-ONLY values) holds.
|
||||
|
||||
### Database extensions
|
||||
|
||||
Migration 014 issues `CREATE EXTENSION IF NOT EXISTS pgcrypto`. Posture:
|
||||
|
||||
- **Privilege**: Postgres `CREATE EXTENSION` requires the migration role to be a superuser OR to have explicit `CREATE` on the database. The satellite-provider connection string in `docker-compose.yml` connects as the `postgres` superuser. This is acceptable for the bundled Docker dev/test environment.
|
||||
- **Production posture**: in a managed Postgres environment (e.g. AWS RDS, Google Cloud SQL), the deployment role typically does NOT have superuser. Operators MUST pre-install `pgcrypto` (it's in the `postgres-contrib` package on Debian/Ubuntu, and is allow-listed on RDS / Cloud SQL by default). The migration's `IF NOT EXISTS` clause makes pre-installation safe — the migration will succeed whether the extension is freshly created or already present.
|
||||
- **Audit log**: extension creation IS visible in Postgres' standard CSV/JSON server logs (`log_statement = 'ddl'` setting). No additional surfaces.
|
||||
|
||||
**Finding**: F2-cy5 — Low (informational), Operational deployment note.
|
||||
- Location: `_docs/05_security/infrastructure_review_cycle5.md` + `_docs/02_document/data_model.md` migration 014 entry
|
||||
- Description: First production deployment of cycle-5 will issue `CREATE EXTENSION pgcrypto` if the extension is not already present. Some managed Postgres providers (e.g. RDS in strict-IAM mode) require an operator to allow-list the extension before the migration role can install it.
|
||||
- Impact: Deployment may fail at the migration step with `must be owner of database` or `permission denied to create extension` if the deployment role lacks the privilege AND the extension is not pre-installed.
|
||||
- Remediation: Pre-installation step. Two options:
|
||||
- (a) Add a one-line entry to the deployment runbook: "ensure `CREATE EXTENSION IF NOT EXISTS pgcrypto` has run as a superuser before the satellite-provider migration role runs migration 014."
|
||||
- (b) On RDS/Cloud SQL, add `pgcrypto` to the parameter group's `shared_preload_libraries` or use the provider's per-DB extension allow-list UI.
|
||||
- Severity rationale: Low (informational) because (i) the Docker compose dev/test environment is unaffected (we connect as `postgres` superuser); (ii) the failure mode is loud (migration fails fast at startup, not at request time); (iii) the remediation is one-line operational, not a code change. Tracked as a deploy-runbook gap, not as a security defect.
|
||||
|
||||
## Network Security
|
||||
|
||||
- No new ports exposed (cycle 5 added no new listening services).
|
||||
- No new outbound integrations (cycle 5 added no new HTTP clients).
|
||||
- CORS / security headers: unchanged.
|
||||
|
||||
## Self-verification
|
||||
|
||||
- [x] Dockerfile / docker-compose diffs reviewed (none in cycle 5)
|
||||
- [x] CI/CD config diffs reviewed (none in cycle 5)
|
||||
- [x] Environment/config files reviewed (`.env`, `.env.example`, `appsettings*.json` — none modified)
|
||||
- [x] New DB extensions documented (`pgcrypto`)
|
||||
|
||||
## Save action
|
||||
|
||||
Written to `_docs/05_security/infrastructure_review_cycle5.md`. The cycle-3 `infrastructure_review.md` remains authoritative for surfaces untouched by AZ-503.
|
||||
@@ -0,0 +1,45 @@
|
||||
# OWASP Top 10 Review (Cycle 5)
|
||||
|
||||
**Date**: 2026-05-12
|
||||
**Mode**: Delta scan
|
||||
**Scope**: Cycle-5 delta over the cycle-3 OWASP review (`_docs/05_security/owasp_review.md`). Reference OWASP Top 10 version: 2021 (current as of this review). The cycle-3 review remains authoritative for categories not touched by AZ-503.
|
||||
|
||||
## Per-Category Cycle-5 Assessment
|
||||
|
||||
| # | Category | Cycle-3 baseline | Cycle-5 delta posture | New findings |
|
||||
|---|----------|------------------|------------------------|--------------|
|
||||
| A01 | Broken Access Control | PASS (JWT + GPS permission on UAV upload; no IDOR; tile reads are coordinate-driven, not id-driven) | PASS — AZ-503 added `metadata.flightId` but did NOT add a new endpoint, did NOT change the existing `RequiresGpsPermission` policy. The optional flight_id is **not** an authorization key; see static_analysis_cycle5.md F1-cy5 for the design-rationale Low informational. | F1-cy5 carried (Low, informational) |
|
||||
| A02 | Cryptographic Failures | PASS (HS256 JWT ≥ 32-byte secret; ImageSharp's libjpeg path used only for inbound parsing) | PASS — `Uuidv5.cs` uses SHA-1 *as the RFC 9562 §5.5 algorithm*, NOT as a cryptographic primitive. `content_sha256` uses SHA-256 for content integrity. See static_analysis_cycle5.md § Cryptographic Failures for the threat-model walk-through. | none |
|
||||
| A03 | Injection | PASS (Dapper parameterized SQL throughout; no shell-escaping paths) | PASS — TileRepository UPSERT remains parameterized; migration 014's PL/pgSQL helper consumes only trusted in-database column values; `UavTileUploadHandler.BuildUavTileFilePath` uses integer-typed coords + `Guid.ToString("D")` which cannot carry traversal characters. | none |
|
||||
| A04 | Insecure Design | PASS (5-rule quality gate, fail-fast on missing JWT secret, JWT iss/aud strict) | PASS_WITH_NOTE — the new `metadata.flightId` is accepted from any GPS-permissioned caller without per-flight ownership verification. This is documented in the v1.1.0 contract as a deliberate design choice; see F1-cy5 in `static_analysis_cycle5.md`. | F1-cy5 carried (Low, informational) |
|
||||
| A05 | Security Misconfiguration | PASS (no default creds; integration tests' DEV_ONLY JWT values explicitly named; Kestrel limits configured) | PASS — `CREATE EXTENSION IF NOT EXISTS pgcrypto` is a standard PostgreSQL operation. The extension lives in the `public` schema by default; this is acceptable for a single-tenant database. No new misconfiguration surface (no new env vars, no new ports, no new headers). | none |
|
||||
| A06 | Vulnerable and Outdated Components | PASS_WITH_WARNINGS in cycle 4 (D2-cy4 Medium carry-over: Microsoft.NET.Test.Sdk 17.8.0 transitive) | PASS_WITH_WARNINGS — cycle 5 adds zero new packages; D2-cy4 carry-over is unchanged. `pgcrypto` is a Postgres-bundled extension, not a NuGet package, and the `digest(..., 'sha1')` path AZ-503 uses is unaffected by recent `pgcrypto` CVEs (CVE-2024-10977 / CVE-2025-1094 target `crypt()` and `quote_literal` respectively). | none new |
|
||||
| A07 | Identification and Authentication Failures | PASS (JWT validated; expiration enforced; ClockSkew 30s; iss + aud strict via AZ-494) | PASS — unchanged. AZ-503 did not modify any auth/identity surface. | none |
|
||||
| A08 | Software and Data Integrity Failures | PASS (DbUp migrations transactional; AZ-484 contract v1.0.0 frozen) | PASS — migration 014 is transactional (`BEGIN … COMMIT`) with idempotent `IF NOT EXISTS` clauses; the `pg_temp.uuidv5` helper is deterministic so partial-replay does not change `location_hash` values. The integrity invariant ("same `(z, x, y)` always yields the same `location_hash`") is verified byte-for-byte against the C# `Uuidv5Tests` reference vectors. | none |
|
||||
| A09 | Security Logging and Monitoring Failures | PASS (Serilog file sink; JWT 401/403 emitted by middleware; no token logging) | PASS — `Uuidv5.cs` logs nothing. Migration 014 logs to DbUp's console sink — row counts only, never row content. `content_sha256` and `flight_id` are not written to any log line on the production path. | none |
|
||||
| A10 | Server-Side Request Forgery (SSRF) | PASS (no user-controlled URL targets) | PASS — AZ-503 introduced no new outbound HTTP call. | none |
|
||||
|
||||
## Cumulative Posture (Cycle 1 → Cycle 5)
|
||||
|
||||
| Category | Cumulative status |
|
||||
|----------|-------------------|
|
||||
| A01 | PASS (1 Low informational accepted: F1-cy5 flight_id provenance) |
|
||||
| A02 | PASS |
|
||||
| A03 | PASS |
|
||||
| A04 | PASS_WITH_NOTE (F1-cy5) |
|
||||
| A05 | PASS |
|
||||
| A06 | PASS_WITH_WARNINGS (D2-cy4 carry-over) |
|
||||
| A07 | PASS |
|
||||
| A08 | PASS |
|
||||
| A09 | PASS |
|
||||
| A10 | PASS |
|
||||
|
||||
## Self-verification
|
||||
|
||||
- [x] Every OWASP 2021 category assessed for cycle-5 delta
|
||||
- [x] Carry-over findings explicitly named (D2-cy4, F1-cy5)
|
||||
- [x] No NEW Critical or High findings in cycle 5
|
||||
|
||||
## Save action
|
||||
|
||||
Written to `_docs/05_security/owasp_review_cycle5.md`. The cycle-3 `owasp_review.md` remains the cumulative source-of-truth narrative for categories untouched by AZ-503.
|
||||
@@ -0,0 +1,95 @@
|
||||
# Security Audit Report (Cycle 5)
|
||||
|
||||
**Date**: 2026-05-12
|
||||
**Scope**: Cycle-5 delta over the cycle-4 audit (`_docs/05_security/security_report_cycle4.md`)
|
||||
**Trigger**: AZ-503-foundation (UUIDv5 tile identity + integer-only flight-aware UPSERT + per-flight on-disk paths) + AZ-504 (perf-script pipefail fix)
|
||||
**Mode**: Delta — all five phases re-executed for the AZ-503 surface; AZ-504 has no source-code surface beyond a shell wrap and is folded into Phase 2
|
||||
**Verdict**: **PASS_WITH_WARNINGS** (cycle-5 delta) / **PASS_WITH_WARNINGS** (cumulative — carries forward 1 cycle-3 Medium dep finding via D2-cy4 + 2 cycle-5 Low informational notes)
|
||||
|
||||
## Summary
|
||||
|
||||
| Severity | Count (cycle 5 delta) | Count (cumulative) |
|
||||
|----------|-----------------------|--------------------|
|
||||
| Critical | 0 | 0 |
|
||||
| High | 0 | 0 |
|
||||
| Medium | 0 NEW | 1 (D2-cy4 carry-over — Microsoft.NET.Test.Sdk transitive flag, test-runtime exposure only) |
|
||||
| Low | 2 NEW (informational) | 5 cycle-4 informational + 2 cycle-5 informational |
|
||||
|
||||
## OWASP Top 10 Assessment (Cycle 5)
|
||||
|
||||
| Category | Status |
|
||||
|----------|--------|
|
||||
| A01 Broken Access Control | PASS |
|
||||
| A02 Cryptographic Failures | PASS |
|
||||
| A03 Injection | PASS |
|
||||
| A04 Insecure Design | PASS_WITH_NOTE (F1-cy5 — flight_id provenance) |
|
||||
| A05 Security Misconfiguration | PASS |
|
||||
| A06 Vulnerable Components | PASS_WITH_WARNINGS (D2-cy4 carry-over only) |
|
||||
| A07 Auth Failures | PASS |
|
||||
| A08 Data Integrity Failures | PASS |
|
||||
| A09 Logging Failures | PASS |
|
||||
| A10 SSRF | PASS |
|
||||
|
||||
## Cycle-5 NEW Findings
|
||||
|
||||
| # | Severity | Category | Location | Title |
|
||||
|---|----------|----------|----------|-------|
|
||||
| F1-cy5 | Low (informational) | Insecure Design (A04) | `_docs/02_document/contracts/api/uav-tile-upload.md` v1.1.0 + `UavTileUploadHandler.PersistAsync` | `metadata.flightId` is not authenticated provenance |
|
||||
| F2-cy5 | Low (informational) | Security Misconfiguration (A05) | Migration 014 `CREATE EXTENSION pgcrypto` | Deployment runbook gap on managed Postgres providers |
|
||||
|
||||
### Finding Details
|
||||
|
||||
**F1-cy5: `metadata.flightId` is not authenticated provenance** (Low / Insecure Design)
|
||||
- Location: `_docs/02_document/contracts/api/uav-tile-upload.md` v1.1.0 § Request shape; `SatelliteProvider.Services.TileDownloader/UavTileUploadHandler.cs:144-217`
|
||||
- Description: The new optional `metadata.flightId` field is persisted to `tiles.flight_id` and used as part of the on-disk path and the deterministic `tile.id` derivation, but the handler does NOT check that the authenticated principal is authorized to write under that flight identifier. Any GPS-permissioned caller can supply any flight_id.
|
||||
- Impact: Two adversarial cases:
|
||||
1. **Impersonation**: a compromised UAV credential can falsely attribute its uploads to a different flight (mis-attribution on the evidence chain).
|
||||
2. **False-flag**: a legitimate UAV credential can falsely attribute its uploads to a competing operator's flight_id.
|
||||
Downstream consumers MUST NOT treat `tiles.flight_id` as cryptographic provenance — they must cross-reference against an authoritative flight registry (out of this workspace's scope) before drawing operational conclusions.
|
||||
- Remediation: Documented as a deliberate v1.1.0 design choice. If a future cycle requires per-flight ownership, options listed in `static_analysis_cycle5.md` (per-flight JWT, scoped permission claim `GPS:flight=<uuid>`, or move flight_id derivation to a trusted claim).
|
||||
- Verification cross-reference: AZ-487/AZ-494 (JWT identity baseline) + AZ-488 (`RequiresGpsPermission` policy) — both still apply unchanged. F1-cy5 is purely about the **inside** of the authorized envelope.
|
||||
- Severity rationale: Low because (i) the surface only exists after a valid GPS-permissioned JWT, (ii) the Admin API per `suite/_docs/10_auth.md` is the upstream identity gate, (iii) no current consumer treats flight_id as authenticated provenance.
|
||||
|
||||
**F2-cy5: Deployment runbook gap — `CREATE EXTENSION pgcrypto` privilege on managed Postgres** (Low / Security Misconfiguration)
|
||||
- Location: `SatelliteProvider.DataAccess/Migrations/014_AddTileIdentityColumns.sql:34`; first observed by `_docs/05_security/infrastructure_review_cycle5.md`
|
||||
- Description: Migration 014 issues `CREATE EXTENSION IF NOT EXISTS pgcrypto`. The current Docker compose dev/test environment connects as the `postgres` superuser, so the extension installs automatically. On managed Postgres providers (AWS RDS, Google Cloud SQL, Azure Database for PostgreSQL) the deployment role typically lacks superuser; the migration will fail with `must be owner of database` or `permission denied to create extension` unless the extension is pre-installed by an operator.
|
||||
- Impact: Deployment may fail at the migration step at startup time. Failure is loud (the app crashes before serving requests) — not a silent security degradation. No production cycle has shipped yet for the satellite-provider on a managed Postgres provider, so this is forward-looking.
|
||||
- Remediation:
|
||||
- (a) Add a one-line entry to the deployment runbook: "ensure `CREATE EXTENSION IF NOT EXISTS pgcrypto` has run as a superuser before the satellite-provider migration role runs migration 014."
|
||||
- (b) On RDS / Cloud SQL, allow-list `pgcrypto` in the provider's per-DB extension UI / parameter group.
|
||||
- Severity rationale: Low informational because (i) the failure mode is loud, (ii) the remediation is one-line operational, (iii) the local Docker environment is unaffected, (iv) every recent managed Postgres provider supports `pgcrypto` in their default allow-list (it's a contrib module shipped with Postgres itself, not a third-party extension).
|
||||
|
||||
## Dependency Vulnerabilities (Cycle 5 delta)
|
||||
|
||||
None. Zero new NuGet packages, zero version bumps. The cycle-4 D2-cy4 Medium carry-over (Microsoft.NET.Test.Sdk 17.8.0 transitive `NuGet.Frameworks` flag) is unchanged.
|
||||
|
||||
## Recommendations
|
||||
|
||||
### Immediate (Critical/High)
|
||||
|
||||
None. No Critical or High findings in cycle 5.
|
||||
|
||||
### Short-term (Medium)
|
||||
|
||||
Apply the D2-cy4 Microsoft.NET.Test.Sdk refresh once a downstream cycle bumps the Test SDK (separate workstream — same posture as cycle 4).
|
||||
|
||||
### Long-term (Low / Hardening)
|
||||
|
||||
- **F1-cy5**: when an authoritative flight registry is introduced (likely a sibling repo or the Admin API), add per-flight ownership verification to `UavTileUploadHandler.HandleAsync` and bump the upload contract to v2.0.0. Until then, document the trust boundary clearly in any consumer-facing API docs that surface `tiles.flight_id`.
|
||||
- **F2-cy5**: add the `pgcrypto` pre-install step to the deployment runbook before the first managed-Postgres deployment.
|
||||
|
||||
## Cross-Reference to Prior Audits
|
||||
|
||||
- Cycle-3 baseline: `_docs/05_security/security_report.md` (authoritative for OWASP narrative on categories untouched by AZ-503).
|
||||
- Cycle-4 delta: `_docs/05_security/security_report_cycle4.md` (AZ-500 package bumps; D2-cy4 finding tree).
|
||||
- Cycle-5 delta artifacts: `dependency_scan_cycle5.md`, `static_analysis_cycle5.md`, `owasp_review_cycle5.md`, `infrastructure_review_cycle5.md`.
|
||||
|
||||
## Verdict Reasoning
|
||||
|
||||
- No Critical, no High, no NEW Medium findings.
|
||||
- 2 Low informational notes (F1-cy5, F2-cy5) properly documented with rationale and forward-looking remediation paths.
|
||||
- Cumulative posture continues to carry the cycle-3 D2-cy4 Medium dep finding (out-of-scope for AZ-503).
|
||||
- Per `security/SKILL.md` § Verdict Logic: `PASS_WITH_WARNINGS` because the cumulative state retains a Medium finding (D2-cy4) but no Critical or High. Cycle-5 delta in isolation would be `PASS_WITH_WARNINGS` due to the Low informational notes; the cycle-5 delta-only verdict is `PASS` per the strict reading ("PASS_WITH_WARNINGS: only Medium or Low findings" — and we have 2 Low informational). The conservative (cumulative) verdict is `PASS_WITH_WARNINGS`.
|
||||
|
||||
**Final verdict (cumulative)**: **PASS_WITH_WARNINGS**.
|
||||
**Cycle-5 delta verdict**: **PASS_WITH_WARNINGS** (informational notes only — gate-acceptable).
|
||||
@@ -0,0 +1,141 @@
|
||||
# Static Analysis (Cycle 5)
|
||||
|
||||
**Date**: 2026-05-12
|
||||
**Mode**: Delta scan
|
||||
**Scope**: Cycle-5 delta over the cycle-3 static analysis (`_docs/05_security/static_analysis.md`). Cycle 4 was source-edit-free for SAST surfaces (AZ-500 was a runtime/package bump only); cycle 5 reintroduces real source edits and is the first SAST delta since cycle 3.
|
||||
|
||||
## Files in Scope
|
||||
|
||||
AZ-503-foundation (production code only — test code excluded from SAST per the cycle-3 policy):
|
||||
|
||||
- `SatelliteProvider.Common/Utils/Uuidv5.cs` (new)
|
||||
- `SatelliteProvider.Common/DTO/UavTileMetadata.cs` (+1 nullable `Guid? FlightId` property)
|
||||
- `SatelliteProvider.DataAccess/Migrations/014_AddTileIdentityColumns.sql` (new — SQL migration)
|
||||
- `SatelliteProvider.DataAccess/Models/TileEntity.cs` (+4 properties)
|
||||
- `SatelliteProvider.DataAccess/Repositories/TileRepository.cs` (UPSERT key change; one new column list element on UPDATE)
|
||||
- `SatelliteProvider.Services.TileDownloader/TileService.cs` (`BuildTileEntity` computes deterministic Id / LocationHash / ContentSha256)
|
||||
- `SatelliteProvider.Services.TileDownloader/UavTileUploadHandler.cs` (`PersistAsync` reads `metadata.FlightId`; `BuildUavTileFilePath` accepts `Guid? flightId`)
|
||||
|
||||
AZ-504:
|
||||
|
||||
- `scripts/run-performance-tests.sh:416-417` (two `grep -o` counters wrapped in `{ … || true; }`)
|
||||
|
||||
## Injection
|
||||
|
||||
### SQL injection
|
||||
|
||||
`TileRepository.InsertAsync` uses Dapper parameterized SQL throughout — no string interpolation of user-controlled values into the SQL text. `flight_id`, `location_hash`, `content_sha256`, `legacy_id` are bound via `@flightId`, `@locationHash`, `@contentSha256`, `@legacyId`. The new `COALESCE(flight_id, '00000000-...'::uuid)` predicate uses a hardcoded zero-UUID literal — not user-controlled.
|
||||
|
||||
Migration 014's PL/pgSQL `pg_temp.uuidv5` function takes `namespace_uuid uuid, name text` parameters; the only `name` value passed is `tile_zoom::text || '/' || tile_x::text || '/' || tile_y::text` over data already in the table. The migration runs under DbUp's bootstrap path (server-trusted, no user input).
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
### Command injection
|
||||
|
||||
No `Process.Start`, `Shell.Execute`, or `subprocess`-equivalent calls were introduced. `Uuidv5.cs` uses pure BCL (`SHA1.HashData`, `BinaryPrimitives`). `UavTileUploadHandler.PersistAsync` writes a file via `File.WriteAllBytesAsync` — no shell.
|
||||
|
||||
The AZ-504 shell-script edit wrapped two `grep -o` invocations in `{ … || true; }`. The wrapped commands were already there in cycle 3; AZ-504 only added the `|| true` guard. No new shell-evaluated input.
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
### Path traversal
|
||||
|
||||
The new `UavTileUploadHandler.BuildUavTileFilePath(StorageConfig, int tileZoom, int tileX, int tileY, Guid? flightId)` constructs an on-disk path. All inputs are integer-typed (`tileZoom`, `tileX`, `tileY`) or `Guid?`. The `flightId` segment is rendered via `flightId.Value.ToString("D", CultureInfo.InvariantCulture)` which **always** emits the 36-character hyphenated form (`xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx`). It is structurally impossible to inject `..`, `/`, `\`, or null bytes into a `Guid.ToString("D")`. Anonymous uploads use the literal compile-time constant `AnonymousFlightSegment = "none"`. Integer-typed coordinates similarly cannot carry traversal characters.
|
||||
|
||||
Path joining uses `Path.Combine` which handles platform separator normalization. The deletion case `rm -rf ./tiles/uav/{flight_id}/` is operator-driven, not API-driven — there is no endpoint that takes a flight_id and deletes anything.
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
### Template / formatting injection
|
||||
|
||||
`Uuidv5.Create(Guid namespaceId, string name)` accepts a `string name`. The string is hashed (SHA-1 of namespace bytes + UTF-8 name), not interpolated into any template. The hash output is then post-processed into a `Guid`. No injection surface.
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
## Authentication & Authorization
|
||||
|
||||
AZ-503 did NOT add a new endpoint and did NOT change the existing auth/permission policies on `POST /api/satellite/upload` (still: JWT (HS256) + `GPS` permission claim, owned by AZ-487 + AZ-488). The optional `flightId` per-item metadata field is on the inside of the JWT-protected envelope; an unauthenticated caller cannot reach it.
|
||||
|
||||
The new `metadata.flightId` field is **not** used as an authorization key. It is an opaque identifier for evidence-isolation. The handler does not check "does this principal own this flight" — that is intentional for v1.1.0 of the upload contract (documented in `_docs/02_document/contracts/api/uav-tile-upload.md`). A future contract bump may add per-flight ownership; for now, any caller with the `GPS` permission can write under any `flightId`.
|
||||
|
||||
**Finding**: F1-cy5 — Low (informational), Insecure Design.
|
||||
- Location: `_docs/02_document/contracts/api/uav-tile-upload.md` v1.1.0 + `UavTileUploadHandler.PersistAsync`
|
||||
- Description: The `metadata.flightId` field is accepted from authenticated callers without verifying the caller "owns" or is authorized to write under that flight identifier. By design (v1.1.0). Two adversarial cases:
|
||||
1. A compromised UAV credential could falsely attribute its uploads to a different flight_id (impersonation on the evidence chain).
|
||||
2. A legitimate UAV credential could falsely attribute its own uploads to a competing operator's flight_id (false-flag).
|
||||
- Impact: Evidence-chain integrity is **not** cryptographically enforced for the flight_id field; downstream consumers should not treat `tiles.flight_id` as proof of provenance. They MUST cross-reference flight_id against an authoritative flight registry (out of this workspace's scope) before drawing conclusions.
|
||||
- Remediation: Recorded as a design constraint, not a defect. If a future cycle requires per-flight ownership, three options:
|
||||
- (a) Issue per-flight JWTs (subject = flight_id; reject mismatched `metadata.flightId` server-side).
|
||||
- (b) Have the Admin API mint a short-lived flight-scoped permission claim, e.g. `permissions: ["GPS:flight=<uuid>"]`.
|
||||
- (c) Move flight_id derivation server-side from a trusted claim (`token.sub` or `token.flight_id`).
|
||||
- Severity rationale: Low because (i) no current consumer treats flight_id as authenticated provenance, (ii) the GPS permission is gated upstream by the Admin API per `suite/_docs/10_auth.md`, (iii) the surface only exists when an attacker already holds a valid GPS-permissioned JWT.
|
||||
|
||||
## Cryptographic Failures
|
||||
|
||||
### SHA-1 in `Uuidv5.cs`
|
||||
|
||||
`Uuidv5.Create` uses `SHA1.HashData(...)` — but **not** as a cryptographic primitive. It is the RFC 9562 §5.5 algorithm requirement for UUIDv5 generation; the result is a stable, deterministic 128-bit handle used as a database key and an on-disk path component. SHA-1's collision vulnerability (SHAttered, 2017) is irrelevant here because:
|
||||
|
||||
1. The UUIDv5 result is **not** used as a content integrity check — `content_sha256` (SHA-256) is the content-integrity primitive.
|
||||
2. Two different `(namespace, name)` pairs producing the same UUIDv5 would require an adversary to craft SHA-1 collisions in advance with full control over both inputs. The `namespace` is a pinned constant (`5b8d0c2e-...`); the only attacker-influenced input is the `name`, which for tile identity is `{z}/{x}/{y}/{source}/{flight_id-or-zero}`. The attacker cannot freely choose `{z}/{x}/{y}` (those are integers derived from public coordinates) or `{source}` (closed enum). The only free variable is `flight_id`. A collision-induced overwrite would require the attacker to (a) hold a GPS-permissioned JWT, (b) compute a SHA-1 collision against a target row's full canonical name, (c) submit a JPEG that matches the victim's `(z, x, y, source)`. The compute cost remains in the hundreds-of-thousands of GPU-hours range and the operational cost (a valid JWT + a forged image that bypasses the 5-rule quality gate) makes this purely theoretical.
|
||||
3. The .NET implementation's `SHA1.HashData` calls into CNG / OpenSSL FIPS-validated SHA-1; the algorithm is not in-process and not subject to side-channel concerns.
|
||||
|
||||
**Finding**: none. SHA-1 use is RFC-mandated and documented in `_docs/02_document/modules/common_uuidv5.md` § Security with the appropriate "not a cryptographic hash for security purposes" disclaimer.
|
||||
|
||||
### SHA-256 in `TileService.BuildTileEntity` + `UavTileUploadHandler.PersistAsync`
|
||||
|
||||
`content_sha256` is computed via `SHA256.HashData(stream)` over the on-disk JPEG body. Stored in `tiles.content_sha256` (bytea). Used to detect byte-identical re-uploads (AZ-503 AC-7). SHA-256 is appropriate here.
|
||||
|
||||
The DB column is `bytea NULL` — legacy pre-AZ-503 rows have NULL because the migration cannot reliably re-read those file bytes (file paths rotate on every Google Maps re-download due to the timestamped legacy layout). Application code enforces `NOT NULL` for new writes. The Low-severity maintainability finding recorded in `batch_02_cycle5_report.md` covers this trade-off.
|
||||
|
||||
**Finding**: none (already covered by code-review at Low maintainability level).
|
||||
|
||||
### Plaintext storage of `JWT_SECRET`, `GOOGLE_MAPS_API_KEY`
|
||||
|
||||
Unchanged from cycle 3 — both come from environment variables / `.env` (gitignored). AZ-503 added no new secret.
|
||||
|
||||
## Data Exposure
|
||||
|
||||
### `content_sha256` in API responses
|
||||
|
||||
The `tileId` returned in `UavTileBatchUploadResponse.items[].tileId` is the deterministic UUIDv5. Is this a privacy leak? The UUID encodes `(z, x, y, source, flight_id-or-zero)` deterministically under a public namespace. An external observer with the `tileId` could verify a `(z, x, y, source, flight_id)` guess but cannot enumerate or reverse without the inputs already in hand. For UAV uploads:
|
||||
- `z, x, y` are derived from `(latitude, longitude, zoom)` the client itself supplied — already known to that client.
|
||||
- `source` is `"uav"` for these responses — already known.
|
||||
- `flight_id` is supplied by the client (or `00000000-...` for anonymous) — already known.
|
||||
|
||||
So the deterministic `tileId` returned to the client tells the client only what the client already knew. **No new information leak.**
|
||||
|
||||
For Google Maps tiles (returned via `GET /api/satellite/tiles/latlon`), the same logic holds: the client already knows `(z, x, y)` because it constructed the request.
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
### Migration backfill data exposure
|
||||
|
||||
Migration 014 writes `location_hash` and `legacy_id` to every existing row. Neither column is projected by `GET /api/satellite/region/{id}`, `GET /api/satellite/route/{id}`, or any public response. They are internal columns. The migration's logs (DbUp's `LogToConsole()`) emit row counts and timing — not row content.
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
### `legacy_id` retention
|
||||
|
||||
`legacy_id` retains the pre-AZ-503 random `Guid` for forensics. This is internal data — not exposed via API surface. To be dropped in a future cycle (already noted in `data_model.md`).
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
## Insecure Deserialization
|
||||
|
||||
`UavTileMetadata.FlightId` is deserialized from JSON via `System.Text.Json`. The target type is `Guid?` — the deserializer enforces a strict 36-character hyphenated format (or 32-char un-hyphenated) and throws `JsonException` on invalid input, which is caught at the envelope level and surfaces as HTTP 400. No type-confusion or polymorphic deserialization path was introduced.
|
||||
|
||||
`Uuidv5.Create` does not deserialize anything — it consumes a `string`/`ReadOnlySpan<byte>` and a `Guid`.
|
||||
|
||||
**Finding**: none.
|
||||
|
||||
## Self-verification
|
||||
|
||||
- [x] All AZ-503 production source files scanned
|
||||
- [x] AZ-504 shell-script delta scanned
|
||||
- [x] No false positives raised from test code
|
||||
- [x] All findings carry file path / location
|
||||
|
||||
## Save action
|
||||
|
||||
Written to `_docs/05_security/static_analysis_cycle5.md`. Carry the cycle-3 `static_analysis.md` forward — no overlap with cycle-5 surface.
|
||||
Reference in New Issue
Block a user