From 6bb65d2aa31bc0a7cdf22acc4285350e60a87eac Mon Sep 17 00:00:00 2001 From: Jon Vanvik Date: Sun, 31 May 2026 17:15:38 +0200 Subject: [PATCH] Remove self-signed TLS option (system trust only); add session changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parity with the Android client: drop the "Allow self-signed certificate" escape hatch. The app now performs system TLS trust only — a self-signed appliance must have its CA installed on the device (MDM/profile). A code-only public-key pinning policy remains (no UI). - ServerTrustEvaluator: remove ServerTrustPolicy.allowSelfSignedForHost and its challenge handler; keep .system (default) and .pinPublicKeySHA256. - SettingsView: remove the self-signed toggle/state/prefill; makeConfig uses the default .system policy. - MCMSConnection / APIConfiguration: update usage-sketch + doc comments. - README / GUI-NOTES / MCMS_API.md: document system-trust-only; self-signed boxes need their CA installed on the device. - Add CHANGELOG.md for this session, flagged for the Android port. Note: configs previously persisted with the self-signed policy fail to decode and reset to defaults (one-time re-entry of the server URL). Co-Authored-By: Claude Opus 4.8 (1M context) --- APIConfiguration.swift | 5 +- CHANGELOG.md | 100 +++++++++++++++++++++++++++++++++++++ GUI-NOTES.md | 11 ++-- MCMSConnection.swift | 14 ++---- MCMS_API.md | 19 ++++--- README.md | 11 ++-- ServerTrustEvaluator.swift | 33 +++--------- SettingsView.swift | 19 ++----- 8 files changed, 142 insertions(+), 70 deletions(-) create mode 100644 CHANGELOG.md diff --git a/APIConfiguration.swift b/APIConfiguration.swift index ef4e395..60dac32 100644 --- a/APIConfiguration.swift +++ b/APIConfiguration.swift @@ -37,8 +37,9 @@ struct APIConfiguration: Equatable, Codable { var sessionCookieName: String = "sessionid" var csrfCookieName: String = "csrftoken" - /// How to treat the server's TLS certificate. These boxes typically present - /// a self-signed / internal-CA cert. See `ServerTrustEvaluator`. + /// 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`. var trustPolicy: ServerTrustPolicy = .system /// Request timeout (seconds). diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..821d7fe --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,100 @@ +# 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 f87e501..da605ef 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, self-signed toggle) + Settings/ Connection config + test (direct or proxy; system TLS trust only) Login/ Email/password, Keychain prefill Dashboard/ Device + alarm counts (VM + View) ONUList/ Searchable ONU list (VM + View) @@ -39,11 +39,12 @@ reinstate a `ContentUnavailableView` shim. ## App Transport Security (the gotcha) -TLS *trust* for self-signed certs is handled in code by `ServerTrustEvaluator`, -but **ATS is separate** and can still block the connection: +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: -- **HTTPS to a self-signed host:** the trust delegate covers it; no Info.plist - change needed for cert validation itself. (Keep TLS 1.2+.) +- **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+. - **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/MCMSConnection.swift b/MCMSConnection.swift index 081921c..76e859f 100644 --- a/MCMSConnection.swift +++ b/MCMSConnection.swift @@ -37,18 +37,14 @@ final class MCMSConnection { /* USAGE SKETCH (delete once real views exist) - // Direct access to the appliance with a self-signed cert on a controlled net: - let url = URL(string: "https://10.2.10.29/api")! + // 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")! let config = APIConfiguration( baseURL: url, apiVersion: "v1", // confirm /api/v1 vs /v1 from openapi.json - 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) + databaseId: nil // or a specific DB id + ) // trustPolicy defaults to .system let connection = MCMSConnection(config: config) diff --git a/MCMS_API.md b/MCMS_API.md index 0836031..9415de8 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**: 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. +- **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. - **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,11 +700,14 @@ 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 cert prompt +### 8.7 Self-signed certs → install the CA -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. +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. --- diff --git a/README.md b/README.md index 6dca8b0..1d81aba 100644 --- a/README.md +++ b/README.md @@ -33,11 +33,12 @@ 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:** 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`. +- **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`. ## Verify off-device (no iOS SDK needed) diff --git a/ServerTrustEvaluator.swift b/ServerTrustEvaluator.swift index 1f19c6b..92eeb54 100644 --- a/ServerTrustEvaluator.swift +++ b/ServerTrustEvaluator.swift @@ -4,22 +4,18 @@ import CryptoKit /// How to evaluate the server's TLS certificate. /// -/// 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. +/// 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). enum ServerTrustPolicy: Equatable, Codable { - /// 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). + /// Default OS trust evaluation. The default policy and the only one the UI + /// produces. 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 - /// public-key hash (base64) matches one of these. The safest option for a - /// fixed appliance — capture the hash once and pin it. + /// 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. case pinPublicKeySHA256([String], host: String) } @@ -49,19 +45,6 @@ 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 b84b4af..136a15d 100644 --- a/SettingsView.swift +++ b/SettingsView.swift @@ -10,7 +10,6 @@ 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? @@ -38,16 +37,6 @@ 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() } @@ -104,17 +93,15 @@ struct SettingsView: View { urlString = config.baseURL.absoluteString apiVersion = config.apiVersion databaseId = config.databaseId ?? "" - if case .allowSelfSignedForHost = config.trustPolicy { allowSelfSigned = true } } private func makeConfig() -> APIConfiguration? { - guard let url = Self.normalizedBaseURL(urlString), let host = url.host else { return nil } - let policy: ServerTrustPolicy = allowSelfSigned ? .allowSelfSignedForHost(host) : .system + // 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 } return APIConfiguration( baseURL: url, apiVersion: apiVersion.trimmingCharacters(in: .whitespaces), - databaseId: databaseId.isEmpty ? nil : databaseId, - trustPolicy: policy + databaseId: databaseId.isEmpty ? nil : databaseId ) }