diff --git a/APIConfiguration.swift b/APIConfiguration.swift index 60dac32..ef4e395 100644 --- a/APIConfiguration.swift +++ b/APIConfiguration.swift @@ -37,9 +37,8 @@ struct APIConfiguration: Equatable, Codable { var sessionCookieName: String = "sessionid" var csrfCookieName: String = "csrftoken" - /// How to treat the server's TLS certificate. Defaults to system trust; the - /// app has no self-signed bypass, so a self-signed box needs its CA installed - /// on the device (MDM/profile). See `ServerTrustEvaluator`. + /// How to treat the server's TLS certificate. These boxes typically present + /// a self-signed / internal-CA cert. See `ServerTrustEvaluator`. var trustPolicy: ServerTrustPolicy = .system /// Request timeout (seconds). diff --git a/CHANGELOG.md b/CHANGELOG.md deleted file mode 100644 index 821d7fe..0000000 --- a/CHANGELOG.md +++ /dev/null @@ -1,100 +0,0 @@ -# Changelog - -## 2026-05-31 — Code-review pass + TLS parity - -A multi-agent code review of the iOS app, the confirmed fixes applied, and the -self-signed TLS option removed for parity with the Android client. This entry is -written for cross-pollination: the two apps share REST behaviour and field paths -(see `MCMS_API.md`), so items below are grouped by how likely they apply to Android. - -### 🔁 Check on Android — shared REST logic / behaviour - -- **Pagination cursor must be the real `_id`, not a synthesized id.** - `getAll` followed the `next` cursor using each model's `id`, which fell back to a - random value when `_id` was absent → the cursor became meaningless and could - re-serve or skip pages (silently wrong fleet list). Fixed to drive the cursor off - the actual `_id` and stop paging when it's absent. - → *Android:* if the paginator derives `next` from a model `id` that has any - non-`_id` fallback, it has the same bug. Use the document `_id` (or stop). - -- **Firmware image with empty compatibility metadata was shown as "compatible for - every ONU."** `isCompatible` returned true when the image declared no - `Compatible Manufacturer` / `Compatible Model`. For a destructive flash that - invites pushing the wrong image. Now requires a positive model match; un-typed - images appear only under the "Show all" escape hatch. - → *Android:* mirror the same rule in the firmware picker's compatibility filter. - -- **Per-ONU alarm filter may return the WHOLE fleet's alarms.** The ONU-detail - Alarms tab filters the fleet-wide alarm-history endpoint with - `query = {"_id": onuId}`, but that key is unverified — if it's wrong the server - returns 200 with every ONU's alarms (no error). Left as-is (couldn't verify the - key off a live box) with a loud in-code warning. - → *Android:* same endpoint + key risk. Verify the real filter key against - `openapi.json` / a live response, or use the dedicated per-id alarm endpoint. - -- **Optical RX thresholds were inconsistent between screens.** Dashboard flagged - abnormal at `< -28 || > -10` dBm; the detail row tinted green at `>= -28` but its - footer legend said "green ≥ -30". Unified to one source of truth - (`OpticalThreshold`: green floor -28, red floor -30, high -10) and corrected the - legend. - → *Android:* if it shows optical levels with color thresholds, align dashboard - and detail to one constant and check the legend text matches the actual rule. - -- **Dashboard best-effort sections went stale silently on a transient failure.** - During the 30s auto-refresh, if the alarm-histories fetch failed, the - alarm-by-severity counts kept the previous cycle's values while the dashboard - still showed "loaded" (and controller count masked a failed fetch as `0`). Now - keeps last-good explicitly and doesn't report a failed count as zero. - → *Android:* if the dashboard auto-refreshes with best-effort sections, decide - explicitly between keep-last-good and show-empty rather than whatever the control - flow produces. - -- **JSON number → Int coercion could crash on a malformed/huge value.** Swift's - `Int(double)` traps on NaN/±∞/out-of-range; a JSON literal like `1e400` decodes - to `∞` and crashed an accessor. Guarded to return nil. - → *Android:* Kotlin's `Double.toInt()` won't trap the same way, but confirm - out-of-range / NaN counters (FW Bank Ptr, DHCP lease seconds, util %, temps) - don't yield garbage Ints in the equivalent accessors. - -### 🔒 Security parity (the point of this round) - -- **Removed the "Allow self-signed certificate" option entirely** — the - `ServerTrustPolicy.allowSelfSignedForHost` case, the Settings toggle, and its - wiring. The app now uses **system TLS trust only** (a code-only public-key pinning - option remains, unused by the UI). Consequence: a self-signed appliance (e.g. the - lab box `mcms.lab.svorka.net`) is unreachable until its CA is installed on the - device (MDM/configuration profile) or the box gets a CA-trusted cert. This matches - the Android client. Old persisted configs that stored the self-signed policy will - fail to decode and reset to defaults (one-time; just re-enter the server URL). - -- **Public-key pin was hashed over the raw key, not the SubjectPublicKeyInfo** — - so a standard `openssl … pkey -pubin -outform der | dgst -sha256` pin never - matched (pinning failed closed). Now prepends the algorithm-specific ASN.1 SPKI - header and hashes the real SPKI. - → *Android:* if it pins, OkHttp `CertificatePinner` already uses SPKI - `sha256/…` — no change needed; just don't reintroduce a raw-key hash. - -### ❓ Analogous patterns worth a look (not certain to apply) - -- **Overlapping load / stale-commit races.** iOS view models let a pull-to-refresh - overlap a tab-switch / auto-refresh load and commit stale or wrong-tab data; fixed - with a per-load generation guard + cancellation check before committing. - → *Android:* if `load()` / refresh use coroutines without single-flight or - job-cancel, the same stale-commit / wrong-tab race can occur. - -- **List-row identity fallback.** iOS list rows fell back to a random id when `_id` - was missing, breaking diffing/scroll/animation. - → *Android:* if DiffUtil keys fall back similarly, use a stable/content-derived - key. - -- **Networking client lifecycle.** iOS leaked a `URLSession` per connection/probe - (created with a delegate, never invalidated). - → *Android:* ensure OkHttp clients are shared/closed rather than created and - dropped per connection test. - -### 📱 iOS-only (listed for completeness; not relevant to Android) - -- Swift closure type-inference build fix (annotate list-VM merge locals). -- `MongoDocument.id` SwiftUI identity specifics; SwiftUI `ForEach` keying. -- `Format` byte/bit-rate rounding rollover at the unit boundary. -- Dead-code, stale-docstring, and helper-dedup cleanups. diff --git a/GUI-NOTES.md b/GUI-NOTES.md index da605ef..f87e501 100644 --- a/GUI-NOTES.md +++ b/GUI-NOTES.md @@ -9,7 +9,7 @@ a working skeleton you run against a live MCMS box, then refine. App/ MCMSMobileApp (@main), RootView routing, AppEnvironment UI/ Shared components (badges, count tiles, load/error states, LoadPhase) Features/ - Settings/ Connection config + test (direct or proxy; system TLS trust only) + Settings/ Connection config + test (direct or proxy, self-signed toggle) Login/ Email/password, Keychain prefill Dashboard/ Device + alarm counts (VM + View) ONUList/ Searchable ONU list (VM + View) @@ -39,12 +39,11 @@ reinstate a `ContentUnavailableView` shim. ## App Transport Security (the gotcha) -TLS uses **system trust only** — there is no self-signed bypass. (Historical note: -an earlier build had a `ServerTrustEvaluator` self-signed toggle; it was removed for -parity with Android.) ATS is separate and can still block the connection: +TLS *trust* for self-signed certs is handled in code by `ServerTrustEvaluator`, +but **ATS is separate** and can still block the connection: -- **HTTPS to a self-signed host:** the connection FAILS unless the host's CA is - installed on the device (MDM/configuration profile). Keep TLS 1.2+. +- **HTTPS to a self-signed host:** the trust delegate covers it; no Info.plist + change needed for cert validation itself. (Keep TLS 1.2+.) - **Plain HTTP** (no TLS) or weak TLS: ATS blocks it by default. Add a *scoped* exception for your host in Info.plist rather than a blanket allow: diff --git a/HANDOFF.md b/HANDOFF.md index 1345d76..0e74333 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -3,25 +3,6 @@ Context for the next session/developer. Pairs with [`README.md`](README.md) and [`MCMS_API.md`](MCMS_API.md). -## Last session (2026-05-31) - -Landed on `main`: a multi-agent code-review pass (correctness / concurrency / -security / cleanup — see [`CHANGELOG.md`](CHANGELOG.md)), then removal of the -self-signed TLS option (system trust only, parity with the Android client). - -> ⚠️ **Verify on the next Mac build.** The code-review fixes were confirmed -> building on the Mac, but the **self-signed-TLS removal was merged to `main` -> un-compiled** — the Linux dev box that made the change has no Swift toolchain. -> First thing next session: build `main` and confirm it compiles. It's a small -> change (removed `ServerTrustPolicy.allowSelfSignedForHost` + the Settings -> toggle; the trust `switch` stays exhaustive), so it should be clean. - -TLS consequence: **system trust only** now — a self-signed appliance (the lab box -`mcms.lab.svorka.net`) is unreachable until its CA is installed on the device -(MDM/configuration profile), or it gets a CA-trusted cert. `CHANGELOG.md` is -written to port these changes to the Android side. Merged branches -`code-review-fixes` and `remove-self-signed-tls` can be deleted. - ## Status Feature-complete and (per off-device typecheck + the user's Xcode builds) diff --git a/MCMSConnection.swift b/MCMSConnection.swift index 76e859f..081921c 100644 --- a/MCMSConnection.swift +++ b/MCMSConnection.swift @@ -37,14 +37,18 @@ final class MCMSConnection { /* USAGE SKETCH (delete once real views exist) - // The appliance must present a CA-trusted cert (real cert, or an internal CA - // installed on the device via MDM/profile) — system trust only, no self-signed bypass: - let url = URL(string: "https://mcms.example.com/api")! + // Direct access to the appliance with a self-signed cert on a controlled net: + let url = URL(string: "https://10.2.10.29/api")! let config = APIConfiguration( baseURL: url, apiVersion: "v1", // confirm /api/v1 vs /v1 from openapi.json - databaseId: nil // or a specific DB id - ) // trustPolicy defaults to .system + databaseId: nil, // or a specific DB id + trustPolicy: .allowSelfSignedForHost("10.2.10.29") + ) + + // Behind a reverse proxy with a real cert — change ONLY the URL + trust: + // let config = APIConfiguration(baseURL: URL(string: "https://mcms.example.com")!, + // trustPolicy: .system) let connection = MCMSConnection(config: config) diff --git a/MCMS_API.md b/MCMS_API.md index 9415de8..0836031 100644 --- a/MCMS_API.md +++ b/MCMS_API.md @@ -652,10 +652,10 @@ is between −28 and −30 dBm. HTTPCookieStorage.shared.cookies(for: baseURL)? .first { $0.name == "__Host-csrftoken" || $0.name == "csrftoken" }?.value ``` -- **Self-signed certs**: NOT accepted in-app — both clients use system trust only, - with no "accept self-signed" bypass. A self-signed appliance must have its CA - installed on the device's OS trust store (MDM/configuration profile). Do not - add a trust-bypass toggle. +- **Self-signed certs**: Implement + `URLSessionDelegate.urlSession(_:didReceive:completionHandler:)` + and accept `serverTrust` only when the user has explicitly toggled + "accept self-signed" for the configured host. Never default-on. - **JSON**: `JSONSerialization` is simpler than `Codable` for the envelope pattern (`{status, data, details}`) because the `data` payload is heterogeneous. `Codable` works if you model each @@ -700,14 +700,11 @@ MCMS APIs are typically reachable only from the operator's internal network. Test on cellular: many internal hosts won't be reachable, and your error UI needs to communicate that clearly. -### 8.7 Self-signed certs → install the CA +### 8.7 Self-signed cert prompt -There is no in-app "accept self-signed" prompt (removed for iOS/Android parity). -The lab MCMS at `https://mcms.lab.svorka.net/` uses a self-signed cert, so it is -unreachable until that CA is installed on the device's OS trust store -(MDM/configuration profile) — or the box is given a CA-trusted cert. Surface the -resulting trust failure clearly in the error UI so it doesn't look like a silent -login break. +Make this a first-class onboarding step. The lab MCMS at +`https://mcms.lab.svorka.net/` uses a self-signed cert, and any +auto-rejection will silently break login. --- diff --git a/README.md b/README.md index 1d81aba..6dca8b0 100644 --- a/README.md +++ b/README.md @@ -33,12 +33,11 @@ Until then, this folder is the source of truth — edit here, copy into Xcode. - **App name / icon:** set the target's **Display Name → "PON Go"**, and drag `Assets.xcassets/AppIcon.appiconset` into the project's asset catalog (replacing the existing AppIcon). -- **App Transport Security:** the app uses **system TLS trust only** — there is no - self-signed bypass (parity with the Android client). A self-signed appliance must - have its CA installed on the device (MDM/configuration profile); otherwise the - connection fails with `APIError.serverTrust`. For plain **HTTP** hosts, add a - *scoped* `NSAppTransportSecurity` → `NSExceptionDomains` entry for that host in - Info.plist — never `NSAllowsArbitraryLoads`. +- **App Transport Security:** self-signed TLS is handled in code by + `ServerTrustEvaluator` (no Info.plist change for HTTPS cert validation). For + plain **HTTP** hosts, add a *scoped* `NSAppTransportSecurity` → + `NSExceptionDomains` entry for that host in Info.plist — never + `NSAllowsArbitraryLoads`. ## Verify off-device (no iOS SDK needed) diff --git a/ServerTrustEvaluator.swift b/ServerTrustEvaluator.swift index 92eeb54..1f19c6b 100644 --- a/ServerTrustEvaluator.swift +++ b/ServerTrustEvaluator.swift @@ -4,18 +4,22 @@ import CryptoKit /// How to evaluate the server's TLS certificate. /// -/// The appliance must present a CA-trusted certificate — a real cert, or an -/// internal CA installed on the device via MDM/configuration profile. There is -/// deliberately NO "accept self-signed" escape hatch: the app never bypasses -/// trust evaluation (parity with the Android client). +/// MCMS appliances usually present a self-signed or internal-CA certificate. +/// Pick the *narrowest* policy that works for your deployment. Never ship +/// `.allowSelfSignedForHost` against a host you don't control. enum ServerTrustPolicy: Equatable, Codable { - /// Default OS trust evaluation. The default policy and the only one the UI - /// produces. + /// Default OS trust evaluation (use this if the box has a real cert or a + /// CA your devices already trust via MDM/profile — strongly preferred). case system + /// Accept the server's certificate ONLY for this exact host, after a normal + /// trust evaluation that ignores the "untrusted root" failure. Scoped escape + /// hatch for self-signed boxes on a controlled mgmt network/VPN. + case allowSelfSignedForHost(String) + /// Certificate/public-key pinning: trust only if the leaf's SHA-256 - /// SubjectPublicKeyInfo hash (base64) matches one of these. Code-only (no UI); - /// the strictest option for a fixed appliance — capture the hash once and pin it. + /// public-key hash (base64) matches one of these. The safest option for a + /// fixed appliance — capture the hash once and pin it. case pinPublicKeySHA256([String], host: String) } @@ -45,6 +49,19 @@ final class ServerTrustEvaluator: NSObject, URLSessionDelegate { case .system: completionHandler(.performDefaultHandling, nil) + case .allowSelfSignedForHost(let allowedHost): + // Scoped escape hatch for self-signed appliances on a controlled + // network. Because a self-signed root fails normal evaluation, this + // accepts ANY certificate the allowed host presents — including + // expired or name-mismatched ones. The only gate is the hostname, so + // use this only on hosts you control; prefer `.pinPublicKeySHA256` + // once you can capture the appliance's key hash. + guard host == allowedHost else { + completionHandler(.cancelAuthenticationChallenge, nil) + return + } + completionHandler(.useCredential, URLCredential(trust: trust)) + case .pinPublicKeySHA256(let pins, let pinnedHost): guard host == pinnedHost, let leaf = Self.leafCertificate(from: trust), diff --git a/SettingsView.swift b/SettingsView.swift index 136a15d..b84b4af 100644 --- a/SettingsView.swift +++ b/SettingsView.swift @@ -10,6 +10,7 @@ struct SettingsView: View { @State private var urlString = "https://" @State private var apiVersion = "v1" @State private var databaseId = "" + @State private var allowSelfSigned = false @State private var testPhase: LoadPhase = .idle @State private var testResult: String? @@ -37,6 +38,16 @@ struct SettingsView: View { .autocorrectionDisabled() } + Section { + Toggle("Allow self-signed certificate", isOn: $allowSelfSigned) + } header: { + Text("Security") + } footer: { + Text(allowSelfSigned + ? "Trusts the certificate for this host only. Use only on a controlled management network/VPN. For production, install the CA via MDM and leave this off, or add public-key pinning in code." + : "Uses standard system trust evaluation (recommended).") + } + Section { Button { Task { await runTest() } @@ -93,15 +104,17 @@ struct SettingsView: View { urlString = config.baseURL.absoluteString apiVersion = config.apiVersion databaseId = config.databaseId ?? "" + if case .allowSelfSignedForHost = config.trustPolicy { allowSelfSigned = true } } private func makeConfig() -> APIConfiguration? { - // TLS uses system trust only (no self-signed bypass); trustPolicy defaults to .system. - guard let url = Self.normalizedBaseURL(urlString), url.host != nil else { return nil } + guard let url = Self.normalizedBaseURL(urlString), let host = url.host else { return nil } + let policy: ServerTrustPolicy = allowSelfSigned ? .allowSelfSignedForHost(host) : .system return APIConfiguration( baseURL: url, apiVersion: apiVersion.trimmingCharacters(in: .whitespaces), - databaseId: databaseId.isEmpty ? nil : databaseId + databaseId: databaseId.isEmpty ? nil : databaseId, + trustPolicy: policy ) }