KM-17448 Integrate Kape PlatformSDK - #350
Conversation
a357eec to
922d176
Compare
3f28eba to
50a6ffa
Compare
![kpkb-1f8e9813897fe9831983e89f7143[bot]](https://avatars.githubusercontent.com/u/75775826?s=60&v=4){kind=small}
…opting an already-running tunnel
…ansport, and port
|
Claude finished @kp-diego-trevisan's task in 2m 15s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🔴 Still outstanding — must-fix before productionBoth release blockers are unchanged on this HEAD (neither of the new commits touched them). 1. Forced feature-flag override ( if flag == .usePlatformSDKVPN {
#if DEBUG || DEVELOPMENT || STAGING
return true
#endif
}2. VPN credentials persisted as plaintext JSON ( Mitigations still worth crediting: the file is cleared on logout ( 🟢 New commits — reviewed, look correct
🟡 Smaller items
🟢 Confirmed still resolved (worth resolving the KB threads)
🧪 Test coverageStill worth adding, per CLAUDE.md (PIALibraryTests / PIA VPNTests with mocks): On-device flows to verify (physical device required for NE)Connect/disconnect on OpenVPN, WireGuard, Automatic (exercise the new pecking order); in-place region switch on a live tunnel ( Summary: the new commits (data-usage IPC, pecking order, fallback-timer suppression, · branch |
|
Claude finished @kp-diego-trevisan's task in 1m 29s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🟢 New commit — reviewed, looks correct
🔴 Still outstanding — must-fix before productionBoth release blockers are unchanged on this HEAD (the new commit didn't touch either). 1. Forced feature-flag override ( if flag == .usePlatformSDKVPN {
return true
}Since the subscript is the single read path, CSI has no way to turn the PlatformSDK tunnel off — there is no remote kill switch if a bad SDK build ships. The if flag == .usePlatformSDKVPN {
#if DEBUG || DEVELOPMENT || STAGING
return true
#endif
}2. VPN credentials persisted as plaintext JSON ( Mitigations still worth crediting: the file is cleared on logout ( 🟢 Confirmed still resolved (worth resolving the KB threads)
🟡 Smaller items (unchanged, still open)
🧪 Test coverageStill worth adding, per CLAUDE.md (PIALibraryTests / PIA VPNTests with mocks): On-device flows to verify (physical device required for NE)Connect/disconnect on OpenVPN, WireGuard, Automatic; in-place region switch on a live tunnel ( Summary: the new commit is a clean docs + tvOS bundle-id fix — the bundle-id change is correct and the ADR renumbering closes my earlier filename note. The two release blockers — the forced flag override and plaintext-credential storage — remain open on this HEAD and should be resolved (or the flag fenced out of Release) before shipping to production. · branch |
|
Claude finished @kp-diego-trevisan's task in 1m 11s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🟢 New commit — reviewed, looks correct
🔴 Still outstanding — must-fix before productionBoth release blockers are unchanged on this HEAD (the new commit didn't touch either). 1. Forced feature-flag override ( if flag == .usePlatformSDKVPN {
return true
}Since the subscript is the single read path, CSI has no way to turn the PlatformSDK tunnel off — there is no remote kill switch if a bad SDK build ships. The if flag == .usePlatformSDKVPN {
#if DEBUG || DEVELOPMENT || STAGING
return true
#endif
}2. VPN credentials persisted as plaintext JSON ( Mitigations still worth crediting: the file is cleared on logout ( 🟢 Confirmed still resolved (worth resolving the KB threads)
🟡 Smaller items (unchanged, still open)
🧪 Test coverageStill worth adding, per CLAUDE.md (PIALibraryTests / PIA VPNTests with mocks): On-device flows to verify (physical device required for NE)Connect/disconnect on OpenVPN, WireGuard, Automatic; in-place region switch on a live tunnel ( Summary: the new commit is a correct, minimal Mac Catalyst archive fix ( · branch |
| } | ||
|
|
||
| private enum CodingKeys: String, CodingKey { | ||
| case `protocol`, serverId, resolvedTransport, updatedAt |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — VPN credentials stored as plaintext JSON in App Group container (PlatformSDK tunnel path)
The new KapePlatformSDKTunnelProfile path stores full VPN credentials — OpenVPN username and password (vpnTokenUsername/vpnTokenPassword) and the WireGuard authentication token (vpnToken or dipUsername) — as plaintext fields inside PIATunnelSharedState, a JSON file (pia_platformsdk_state.json) written atomically to the App Group container at every VPN connect: ```swift // KapePlatformSDKTunnelProfile.doSave() — runs at every connect PIATunnelSharedState.write(.init( openVPN: o…
kb · F-1XFZ6TVJKDY0F9R7
| } | ||
|
|
||
| let cipher = sharedDefaults.string(forKey: AppConstants.UserDefaultsKeys.OpenVPN.cipher) ?? AppConstants.OpenVPNCrypto.default.rawValue | ||
| let auth = sharedDefaults.string(forKey: AppConstants.UserDefaultsKeys.OpenVPN.auth) ?? AppConstants.OpenVPNCrypto.defaultAuth |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK shared state stores VPN credentials as plaintext in App Group container JSON file
KapePlatformSDKTunnelProfile.writeSharedState() serialises both OpenVPN credentials (username, password) and the WireGuard authentication token (token) as plain JSON strings into PIATunnelSharedState.State, which is then written to pia_platformsdk_state.json in the App Group container: ```swift // KapePlatformSDKTunnelProfile+OpenVPN.swift return PIATunnelSharedState.OpenVPNSettings( caCertificate: caCertificate, username: username, // vpnTokenUsername — plaintext VPN cred…
kb · F-1YVSXRM49TCCV5CS
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK VPN credentials stored as plaintext in App Group container file
KapePlatformSDKTunnelProfile.writeSharedState() writes VPN credentials — OpenVPN username and password (vpnTokenUsername / vpnTokenPassword) and WireGuard token (vpnToken) — as plain UTF-8 strings into PIATunnelSharedState.State, which is serialised as a JSON file (pia_platformsdk_state.json) in the App Group container. This file is written on every connect and every in-place server switch via switchLocation. App Group container files are included in **unencrypted local (iTunes/Fi…
kb · F-2KQH614QJ8ZFT02S
|
|
||
| /// Returns `true` if the feature flag is set. | ||
| public subscript(_ flag: FeatureFlag) -> Bool { | ||
| // TODO: [PlatformSDK] Temporary — force the PlatformSDK tunnel on regardless |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — VPN credentials (OpenVPN password + WireGuard token) stored in plaintext App Group JSON file (PlatformSDK path)
KapePlatformSDKTunnelProfile.doSave() serializes the full OpenVPN username, OpenVPN password, and WireGuard token into PIATunnelSharedState.State, which is persisted as a JSON file (pia_platformsdk_state.json) in the App Group container directory. Unlike the legacy OpenVPN (PIATunnelProfile) and IKEv2 paths — which store only an opaque Keychain passwordReference in NEVPNProtocol and never write the credential value to a file — this new path writes the actual VPN credential strings t…
kb · F-3KJB6A1X12FG7HV8
|
|
||
| - name: Run iOS unit tests | ||
| uses: nick-fields/retry@v4 | ||
| with: |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — New PR CI workflows use external GitHub Actions pinned to mutable version tags
The newly added .github/workflows/ios_pull_request.yml and .github/workflows/tvos_pull_request.yml use external GitHub Actions pinned to mutable major-version tags rather than immutable commit SHAs: - maxim-lobanov/setup-xcode@v1 — third-party, not GitHub-maintained - nick-fields/retry@v4 — third-party, not GitHub-maintained - ruby/setup-ruby@v1 — Ruby organization - actions/cache@v4, actions/checkout@v6, actions/upload-artifact@v6 — GitHub-maintained but not SHA-pinned Per the…
kb · F-WK64N10YSZ6K7VVN
| /// session (disconnected, or not running through the PlatformSDK tunnel). Individual fields are | ||
| /// `nil` when that dimension did not resolve (e.g. the protocol under "Automatic", or a server id | ||
| /// the app can't match). Callers fall back to the user's selection per field. | ||
| public struct ActualConnection { |
There was a problem hiding this comment.
idea: Should we explicitly mark data types as Sendable? I know structs that can automatically be sendable are automatically marked as sendable, but marking them explicitly would help us move to swift 6 eventually.
There was a problem hiding this comment.
The blocker here is that ActualConnection has a Server property, which is a class, so neither are actually Sendable. We can work on making Server sendable (I predict it's a big work) or drop Server in ActualConnection and store only the serverID instead (but the consumers will have to do some filtering).
kp-mario-nachbaur
left a comment
kp-mario-nachbaur
left a comment
There was a problem hiding this comment.
Overall looks good. I left some comments with open questions, feel free to ignore them.
- VPNDaemon: revert the scheduleFallbackTimerIfNeeded refactor to match master (hotfix revert); drop the .disconnected no-internet retry loop, keeping only the PlatformSDK connect-time fallback-timer suppression - KapePlatformSDKTunnelProfile+OpenVPN: replace NSError throws with a typed throws(OpenVPNSettingsError) Swift error enum - ActualConnection: type vpnType as KapePlatformSDKVPNType instead of a raw String; bridge to the String preference at the ConnectionTile edge - PIATunnelProfile: log the OpenVPN.ProviderConfiguration cast failure in requestLog/requestDataUsage; return .vpnProfileUnavailable from the latter for consistency with its other failure path - AppConstants: make UserDefaultsKeys/OpenVPN/WireGuard caseless enum namespaces instead of instantiable structs - DefaultVPNProvider: remove the redundant #if os(iOS) || os(tvOS) guard in seedInitialVPNStatus
|
Claude finished @kp-diego-trevisan's task in 1m 57s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🟢 New commit — reviewed, looks correctThe commit cleanly addresses the open reviewer threads from @kp-mario-nachbaur:
🔴 Still outstanding — must-fix before productionNeither release blocker was touched by this commit. 1. Forced feature-flag override ( if flag == .usePlatformSDKVPN {
return true
}Since the subscript is the single read path, CSI has no remote kill switch if a bad SDK build ships. The if flag == .usePlatformSDKVPN {
#if DEBUG || DEVELOPMENT || STAGING
return true
#endif
}2. VPN credentials persisted as plaintext JSON ( Mitigations still worth crediting: the file is cleared on logout ( 🟢 Confirmed still resolved (worth resolving the KB threads)
🟡 Smaller items (unchanged, still open)
🧪 Test coverageStill worth adding, per CLAUDE.md (PIALibraryTests / PIA VPNTests with mocks): On-device flows to verify (physical device required for NE)Connect/disconnect on OpenVPN, WireGuard, Automatic; in-place region switch on a live tunnel ( Summary: the new commit is a clean, well-targeted response to the review threads — typed error, enum-typed · branch |
|
Claude finished @kp-diego-trevisan's task in 2m 50s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🟢 New commit — reviewed, all changes look correctEach item maps cleanly to a reviewer comment and is implemented well:
One residual note on the enum-conversion thread: the 🔴 Still outstanding — must-fix before productionBoth release blockers are unchanged on this HEAD (the new commit didn't touch either). These remain acceptable while the engine is under active development given the 1. Forced feature-flag override ( if flag == .usePlatformSDKVPN {
return true
}Since the subscript is the single read path, CSI has no remote kill switch if a bad SDK build ships. At minimum, fence it out of Release: if flag == .usePlatformSDKVPN {
#if DEBUG || DEVELOPMENT || STAGING
return true
#endif
}2. VPN credentials persisted as plaintext JSON ( Mitigations worth crediting: the file is cleared on logout ( 🟢 Confirmed still resolved (worth resolving the KB threads)
🟡 Smaller items (unchanged, still open)
🧪 Test coverageStill worth adding, per CLAUDE.md (PIALibraryTests / PIA VPNTests with mocks): On-device flows to verify (physical device required for NE)Connect/disconnect on OpenVPN, WireGuard, Automatic; in-place region switch on a live tunnel ( Summary: the new commit is a clean, correct response to the human-review feedback — typed error, enum-typed · branch |
|
Claude finished @kp-diego-trevisan's task in 5m 38s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🟢 The merge itself looks clean
🟠 New this push — the placeholder path now runs through the PlatformSDK profileBecause 1. In practice the window is narrow — login guards 2. Placeholder identity leaks into the shared state. Consequences are bounded — 🟡 Also new to me this pass —
|
| let encodedToken = token.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed), | ||
| let url = URL(string: "https://\(host):\(config.authPort)/addKey?pubkey=\(encodedPubkey)&pt=\(encodedToken)") | ||
| else { | ||
| logger.error("Failed to build key-exchange URL for \(host):\(config.authPort)") |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK WireGuard key exchange sends VPN token to server with no TLS certificate validation
In PIAWireguardAuthenticator.authenticate(config:), the WireGuard key-exchange HTTPS request is made through a URLSession backed by TrustAllCertsDelegate, which accepts any TLS certificate presented by the server: swift let delegate = TrustAllCertsDelegate() let session = URLSession(configuration: .ephemeral, delegate: delegate, delegateQueue: nil) TrustAllCertsDelegate.urlSession(_:didReceive:completionHandler:) unconditionally calls `completionHandler(.useCredential, URLCreden…
kb · F-0NT3A776DA24F1QB
| } | ||
|
|
||
| private enum CodingKeys: String, CodingKey { | ||
| case `protocol`, serverId, resolvedTransport, updatedAt |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — VPN credentials stored as plaintext JSON in App Group container (PlatformSDK tunnel path)
The new KapePlatformSDKTunnelProfile path stores full VPN credentials — OpenVPN username and password (vpnTokenUsername/vpnTokenPassword) and the WireGuard authentication token (vpnToken or dipUsername) — as plaintext fields inside PIATunnelSharedState, a JSON file (pia_platformsdk_state.json) written atomically to the App Group container at every VPN connect: ```swift // KapePlatformSDKTunnelProfile.doSave() — runs at every connect PIATunnelSharedState.write(.init( openVPN: o…
kb · F-1XFZ6TVJKDY0F9R7
| } | ||
| username = dipUsername | ||
| password = dipIp | ||
| } else { |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK shared state stores VPN credentials as plaintext in App Group container JSON file
KapePlatformSDKTunnelProfile.writeSharedState() serialises both OpenVPN credentials (username, password) and the WireGuard authentication token (token) as plain JSON strings into PIATunnelSharedState.State, which is then written to pia_platformsdk_state.json in the App Group container: ```swift // KapePlatformSDKTunnelProfile+OpenVPN.swift return PIATunnelSharedState.OpenVPNSettings( caCertificate: caCertificate, username: username, // vpnTokenUsername — plaintext VPN cred…
kb · F-1YVSXRM49TCCV5CS
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK VPN credentials stored as plaintext in App Group container file
KapePlatformSDKTunnelProfile.writeSharedState() writes VPN credentials — OpenVPN username and password (vpnTokenUsername / vpnTokenPassword) and WireGuard token (vpnToken) — as plain UTF-8 strings into PIATunnelSharedState.State, which is serialised as a JSON file (pia_platformsdk_state.json) in the App Group container. This file is written on every connect and every in-place server switch via switchLocation. App Group container files are included in **unencrypted local (iTunes/Fi…
kb · F-2KQH614QJ8ZFT02S
|
|
||
| /// Returns `true` if the feature flag is set. | ||
| public subscript(_ flag: FeatureFlag) -> Bool { | ||
| // TODO: [PlatformSDK] Temporary — force the PlatformSDK tunnel on regardless |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — VPN credentials (OpenVPN password + WireGuard token) stored in plaintext App Group JSON file (PlatformSDK path)
KapePlatformSDKTunnelProfile.doSave() serializes the full OpenVPN username, OpenVPN password, and WireGuard token into PIATunnelSharedState.State, which is persisted as a JSON file (pia_platformsdk_state.json) in the App Group container directory. Unlike the legacy OpenVPN (PIATunnelProfile) and IKEv2 paths — which store only an opaque Keychain passwordReference in NEVPNProtocol and never write the credential value to a file — this new path writes the actual VPN credential strings t…
kb · F-3KJB6A1X12FG7HV8
| key: spm-${{ runner.os }}-${{ hashFiles('**/Package.resolved') }} | ||
| restore-keys: spm-${{ runner.os }}- | ||
|
|
||
| - name: Set up Ruby |
There was a problem hiding this comment.
🟡 MEDIUM · SECURITY_REVIEW — New PR CI workflows use external GitHub Actions pinned to mutable version tags with access to org credentials
The new ios_pull_request.yml and tvos_pull_request.yml CI workflows use three external GitHub Actions pinned to mutable version tags instead of full commit SHAs: - maxim-lobanov/setup-xcode@v1 - nick-fields/retry@v4 - ruby/setup-ruby@v1 Per the repository advisory guidance, only actions from the xvpn, xvpn-meta, and xvpn-sec GitHub organisations are permitted to use mutable labels; all other external actions remain untrusted. None of these three actions come from those organisat…
kb · SEC-M-4
| case invalidURL | ||
| case serverError(String) | ||
| case missingAnchorCertificate | ||
| } |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — PinnedCertificateDelegate does not call SecTrustSetAnchorCertificatesOnly, allowing system-trusted CAs to satisfy the WireGuard key-exchange TLS check
PIAWireguardAuthenticator.PinnedCertificateDelegate.urlSession(_:didReceive:completionHandler:) sets the PIA root CA as a trust anchor with SecTrustSetAnchorCertificates(trust, [anchorCertificate] as CFArray) but does not call SecTrustSetAnchorCertificatesOnly(trust, true). Without the exclusivity flag, Apple's trust evaluation treats the custom anchor as additive: the system's built-in CA store remains valid alongside the PIA root CA. A certificate signed by any system-trusted CA that pre…
kb · F-5XSNY1058469A8SY
| logger.error("Pinning failed: no server trust on key-exchange challenge") | ||
| completionHandler(.cancelAuthenticationChallenge, nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — WireGuard key-exchange TLS pinning allows system-trusted CAs because SecTrustSetAnchorCertificatesOnly is not called
PinnedCertificateDelegate.urlSession(_:didReceive:completionHandler:) pins the WireGuard key-exchange TLS connection against the bundled PIA root CA by calling SecTrustSetAnchorCertificates(trust, [anchorCertificate] as CFArray). However, it does not follow this with SecTrustSetAnchorCertificatesOnly(trust, true). Without the exclusivity flag, Apple's documentation states that the system's built-in anchor certificates remain eligible for validation in addition to the explicitly supplied a…
kb · F-J0TH0QY0N6TS0XSA
|
|
||
| - name: Run iOS unit tests | ||
| uses: nick-fields/retry@v4 | ||
| with: |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — New PR CI workflows use external GitHub Actions pinned to mutable version tags
The newly added .github/workflows/ios_pull_request.yml and .github/workflows/tvos_pull_request.yml use external GitHub Actions pinned to mutable major-version tags rather than immutable commit SHAs: - maxim-lobanov/setup-xcode@v1 — third-party, not GitHub-maintained - nick-fields/retry@v4 — third-party, not GitHub-maintained - ruby/setup-ruby@v1 — Ruby organization - actions/cache@v4, actions/checkout@v6, actions/upload-artifact@v6 — GitHub-maintained but not SHA-pinned Per the…
kb · F-WK64N10YSZ6K7VVN
| authenticated.serverPublicKey = response.server_key | ||
| authenticated.clientPrivateKey = privateKeyBase64 | ||
| authenticated.internalIp = response.peer_ip | ||
| authenticated.dnsServers = dnsServers |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — WireGuard key-exchange TLS pinning allows system-trusted CAs alongside pinned PIA CA
In PIAWireguardAuthenticator.PinnedCertificateDelegate.urlSession(_:didReceive:completionHandler:), the certificate pinning implementation calls SecTrustSetAnchorCertificates(trust, [anchorCertificate]) but does NOT call SecTrustSetAnchorCertificatesOnly(trust, true) afterwards. Per Apple's documentation, omitting SecTrustSetAnchorCertificatesOnly means the system also trusts all built-in system anchor certificates in addition to the explicitly set PIA CA. A certificate chain that termin…
kb · SEC-L-4
# Conflicts: # LocalPackages/PIALibrary/Sources/PIALibrary/VPN/NetworkExtensionProfile.swift
|
Claude finished @kp-diego-trevisan's task in 4m 10s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🟢 The merge itself is clean
🟠 New this push — the PlatformSDK profile misses master's
|
|
Claude finished @kp-diego-trevisan's task in 2m 7s —— View job Claude PR Review — KapePlatformSDK integration (re-review on latest push)Re-reviewed the current HEAD (
🟢 The merge is trivial and clean
.package(url: "https://github.com/apple/swift-log", exact: "1.13.1")
…
.product(name: "Logging", package: "swift-log")Checks that matter here:
Nothing new to review beyond that — so the rest of this comment is a status roll-up of what's still open. No new issues introduced. 🔴 Release blockers — both unchanged1. Forced feature-flag override ( if flag == .usePlatformSDKVPN {
#if DEBUG || DEVELOPMENT || STAGING
return true
#endif
}2. VPN credentials persisted as plaintext JSON ( 🟠 Still open from my last two reviews (all re-verified on this HEAD)
🟢 Confirmed still resolved (worth resolving the KB threads)
🟡 Smaller items (unchanged)
🧪 Test coveragePer CLAUDE.md (PIALibraryTests / PIA VPNTests with mocks), still worth adding: On-device flows to verify (physical device required for NE)Connect/disconnect on OpenVPN, WireGuard, Automatic; reconnect immediately after a disconnect / while still Summary: this push is a one-line dependency merge from master ( |
| let encodedToken = token.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed), | ||
| let url = URL(string: "https://\(host):\(config.authPort)/addKey?pubkey=\(encodedPubkey)&pt=\(encodedToken)") | ||
| else { | ||
| logger.error("Failed to build key-exchange URL for \(host):\(config.authPort)") |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK WireGuard key exchange sends VPN token to server with no TLS certificate validation
In PIAWireguardAuthenticator.authenticate(config:), the WireGuard key-exchange HTTPS request is made through a URLSession backed by TrustAllCertsDelegate, which accepts any TLS certificate presented by the server: swift let delegate = TrustAllCertsDelegate() let session = URLSession(configuration: .ephemeral, delegate: delegate, delegateQueue: nil) TrustAllCertsDelegate.urlSession(_:didReceive:completionHandler:) unconditionally calls `completionHandler(.useCredential, URLCreden…
kb · F-0NT3A776DA24F1QB
| } | ||
|
|
||
| private enum CodingKeys: String, CodingKey { | ||
| case `protocol`, serverId, resolvedTransport, updatedAt |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — VPN credentials stored as plaintext JSON in App Group container (PlatformSDK tunnel path)
The new KapePlatformSDKTunnelProfile path stores full VPN credentials — OpenVPN username and password (vpnTokenUsername/vpnTokenPassword) and the WireGuard authentication token (vpnToken or dipUsername) — as plaintext fields inside PIATunnelSharedState, a JSON file (pia_platformsdk_state.json) written atomically to the App Group container at every VPN connect: ```swift // KapePlatformSDKTunnelProfile.doSave() — runs at every connect PIATunnelSharedState.write(.init( openVPN: o…
kb · F-1XFZ6TVJKDY0F9R7
| } | ||
| username = dipUsername | ||
| password = dipIp | ||
| } else { |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK shared state stores VPN credentials as plaintext in App Group container JSON file
KapePlatformSDKTunnelProfile.writeSharedState() serialises both OpenVPN credentials (username, password) and the WireGuard authentication token (token) as plain JSON strings into PIATunnelSharedState.State, which is then written to pia_platformsdk_state.json in the App Group container: ```swift // KapePlatformSDKTunnelProfile+OpenVPN.swift return PIATunnelSharedState.OpenVPNSettings( caCertificate: caCertificate, username: username, // vpnTokenUsername — plaintext VPN cred…
kb · F-1YVSXRM49TCCV5CS
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — PlatformSDK VPN credentials stored as plaintext in App Group container file
KapePlatformSDKTunnelProfile.writeSharedState() writes VPN credentials — OpenVPN username and password (vpnTokenUsername / vpnTokenPassword) and WireGuard token (vpnToken) — as plain UTF-8 strings into PIATunnelSharedState.State, which is serialised as a JSON file (pia_platformsdk_state.json) in the App Group container. This file is written on every connect and every in-place server switch via switchLocation. App Group container files are included in **unencrypted local (iTunes/Fi…
kb · F-2KQH614QJ8ZFT02S
|
|
||
| /// Returns `true` if the feature flag is set. | ||
| public subscript(_ flag: FeatureFlag) -> Bool { | ||
| // TODO: [PlatformSDK] Temporary — force the PlatformSDK tunnel on regardless |
There was a problem hiding this comment.
🟠 HIGH · SECURITY_REVIEW — VPN credentials (OpenVPN password + WireGuard token) stored in plaintext App Group JSON file (PlatformSDK path)
KapePlatformSDKTunnelProfile.doSave() serializes the full OpenVPN username, OpenVPN password, and WireGuard token into PIATunnelSharedState.State, which is persisted as a JSON file (pia_platformsdk_state.json) in the App Group container directory. Unlike the legacy OpenVPN (PIATunnelProfile) and IKEv2 paths — which store only an opaque Keychain passwordReference in NEVPNProtocol and never write the credential value to a file — this new path writes the actual VPN credential strings t…
kb · F-3KJB6A1X12FG7HV8
| key: spm-${{ runner.os }}-${{ hashFiles('**/Package.resolved') }} | ||
| restore-keys: spm-${{ runner.os }}- | ||
|
|
||
| - name: Set up Ruby |
There was a problem hiding this comment.
🟡 MEDIUM · SECURITY_REVIEW — New PR CI workflows use external GitHub Actions pinned to mutable version tags with access to org credentials
The new ios_pull_request.yml and tvos_pull_request.yml CI workflows use three external GitHub Actions pinned to mutable version tags instead of full commit SHAs: - maxim-lobanov/setup-xcode@v1 - nick-fields/retry@v4 - ruby/setup-ruby@v1 Per the repository advisory guidance, only actions from the xvpn, xvpn-meta, and xvpn-sec GitHub organisations are permitted to use mutable labels; all other external actions remain untrusted. None of these three actions come from those organisat…
kb · SEC-M-4
| case invalidURL | ||
| case serverError(String) | ||
| case missingAnchorCertificate | ||
| } |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — PinnedCertificateDelegate does not call SecTrustSetAnchorCertificatesOnly, allowing system-trusted CAs to satisfy the WireGuard key-exchange TLS check
PIAWireguardAuthenticator.PinnedCertificateDelegate.urlSession(_:didReceive:completionHandler:) sets the PIA root CA as a trust anchor with SecTrustSetAnchorCertificates(trust, [anchorCertificate] as CFArray) but does not call SecTrustSetAnchorCertificatesOnly(trust, true). Without the exclusivity flag, Apple's trust evaluation treats the custom anchor as additive: the system's built-in CA store remains valid alongside the PIA root CA. A certificate signed by any system-trusted CA that pre…
kb · F-5XSNY1058469A8SY
| logger.error("Pinning failed: no server trust on key-exchange challenge") | ||
| completionHandler(.cancelAuthenticationChallenge, nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — WireGuard key-exchange TLS pinning allows system-trusted CAs because SecTrustSetAnchorCertificatesOnly is not called
PinnedCertificateDelegate.urlSession(_:didReceive:completionHandler:) pins the WireGuard key-exchange TLS connection against the bundled PIA root CA by calling SecTrustSetAnchorCertificates(trust, [anchorCertificate] as CFArray). However, it does not follow this with SecTrustSetAnchorCertificatesOnly(trust, true). Without the exclusivity flag, Apple's documentation states that the system's built-in anchor certificates remain eligible for validation in addition to the explicitly supplied a…
kb · F-J0TH0QY0N6TS0XSA
|
|
||
| - name: Run iOS unit tests | ||
| uses: nick-fields/retry@v4 | ||
| with: |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — New PR CI workflows use external GitHub Actions pinned to mutable version tags
The newly added .github/workflows/ios_pull_request.yml and .github/workflows/tvos_pull_request.yml use external GitHub Actions pinned to mutable major-version tags rather than immutable commit SHAs: - maxim-lobanov/setup-xcode@v1 — third-party, not GitHub-maintained - nick-fields/retry@v4 — third-party, not GitHub-maintained - ruby/setup-ruby@v1 — Ruby organization - actions/cache@v4, actions/checkout@v6, actions/upload-artifact@v6 — GitHub-maintained but not SHA-pinned Per the…
kb · F-WK64N10YSZ6K7VVN
| authenticated.serverPublicKey = response.server_key | ||
| authenticated.clientPrivateKey = privateKeyBase64 | ||
| authenticated.internalIp = response.peer_ip | ||
| authenticated.dnsServers = dnsServers |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — WireGuard key-exchange TLS pinning allows system-trusted CAs alongside pinned PIA CA
In PIAWireguardAuthenticator.PinnedCertificateDelegate.urlSession(_:didReceive:completionHandler:), the certificate pinning implementation calls SecTrustSetAnchorCertificates(trust, [anchorCertificate]) but does NOT call SecTrustSetAnchorCertificatesOnly(trust, true) afterwards. Per Apple's documentation, omitting SecTrustSetAnchorCertificatesOnly means the system also trusts all built-in system anchor certificates in addition to the explicitly set PIA CA. A certificate chain that termin…
kb · SEC-L-4
2 participants
Summary
Consolidate PIA's VPN protocol stacks (OpenVPN, WireGuard) into a single PlatformSDK-Tunnel Network Extension powered by Kape's Rust-based VPN engine, gated behind the
usePlatformSDKVPNfeature flag.1. SDK Dependency Pipeline
scripts/pull-kape-platform-sdk.sh+scripts/kape-platform-sdk.version— pulls a pinned, vendored Kape Platform SDK from Cloudsmith intoLocalPackages/KapePlatformSDK/(gitignored)ci_scripts/ci_post_clone.sh+ CI workflows — CI runs the pull with caching before SPM resolvesPIAVPN/Package.swift— declares deps onKapeVPN-PacketTunnelandKapeVPN-OpenVPNPIALibrary/Package.swift— depends onTunnelKitPackage(the Kape TunnelKit fork,../KapePlatformSDK/TunnelKitPackage) for OpenVPN config typesREADME.md— documents the required local setup: obtaining a Cloudsmith token and running the pull before building2. Architecture Decision Records
ADRs/0008-integrate-kape-platform-sdk-vpn-engine.md— full design rationale: one shared engine replacing three divergent protocol stacks, gated behind a CSI-controlled feature flagADRs/0007-ios-coordinator-navigation-pattern.md— coordinator navigation pattern ADR3. New
PIAVPNLocal Package (Extension-Side Adapters)LocalPackages/PIAVPN/— thin adapters bridging PIA's model to the Kape SDK inside the extension:PIAPacketTunnelProviderNEPacketTunnelProviderentry point; wires SDK session/connection controllersPIAEndpointRepositoryPIAWireguardAuthenticatorPIATunnelLoggeros.Logger4. App ↔ Extension IPC via Bidirectional Shared State
PIALibrary/.../KapePlatformSDK/SharedState/:PIATunnelSharedState— persists state aspia_platformsdk_state.jsonin the shared app group (on tvOS underLibrary/Caches); every write posts a Darwin notification so the other side observes changes rather than pollingserverscache is written by both sidesActualConnection/activeConnection), a livetunnelStatus, live data usage (PIADataUsage— bytes sent/received, so the connection-stats UI reflects real tunnel throughput), and the connection timestamp (surfaced viaVPNProfile.connectionDate); the app foldstunnelStatusintoVPNStatus.resolve(system:tunnel:)to drive the "Connecting" UI even whileNEVPNStatusstays.connectedPIAPacketTunnelRequest.switchLocation(viasendProviderMessage()) switches region on a live tunnel in place, without tearing down the extension processServersPingerwrites latency rankings so the extension can fan out fastest-first (online non-DIP servers) for the Automatic region5. App-Side Tunnel Profile
PIALibrary/.../KapePlatformSDK/TunnelProfile/:KapePlatformSDKTunnelProfile— theNetworkExtensionProfilethat configures the PlatformSDK tunnelKapePlatformSDKVPNType— centralized protocol type seam (PIA/PIAWG/PIAAutomatic, plus a non-connectableIKEv2case for recognising/migrating legacy installs)ActualConnection— resolved protocol/server/transport reported back to the appPIAPacketTunnelRequest— IPC request model (switchLocation)OpenVPNProvider+Compat— compat bridge to legacy TunnelKit types6. New
PlatformSDK-TunnelExtensionPlatformSDK-Tunnel/), built by two platform targets —PlatformSDK-Tunnel-iOSandPlatformSDK-Tunnel-tvOS— replacing the per-protocol extensionsPIA tvOS Tunnel/7. Core Library (PIALibrary) Changes
VPNDaemon— suppresses app-side reconnect/fallback-timer/disconnect-error handling when the flag is on (the SDK owns reconnection)DefaultVPNProvider/VPNProvider— addschangeServerAPI; surfaces "Connecting" UI from extension write-backFeatureFlags— addsusePlatformSDKVPNCSI-controlled flag (ios_platform_sdk_vpn)Server— model updates for DIP servers and resolved endpoint dataAppConstants,Client+Configuration,NetworkExtensionProfile— new constants and protocol changes8. App Layer (iOS)
Bootstrapper— migrates legacy protocol to automatic, runscleanupLegacyVPNProfilesIfNeededAppPreferences/AppConfiguration— new preferences keysDashboardViewController/ConnectionTile/RegionTile— UI for new "Connecting" stateProtocolSettingsViewController/RegionsViewController— updated forKapePlatformSDKVPNTypeand DIPPIAConnectionLiveActivityManager/PIAWidgetAttributes— live activity updates9. tvOS Support
BootstraperFactory— flag setup, restores connected state on relaunchProtocolSelectionView/ViewModel/UseCase— brand new protocol selection UI (was unavailable)PIA-RSA-4096.pem— root CA for TLS pinningProtocolSelectionViewModelTests,AvailableSettingsViewModelTests10. Mac Catalyst (KM-16341)
AdaptiveSplitViewController— iPad/Catalyst sidebarAppDelegate,PIALibrary/Package.swift,UserInterface,CAGradientLayer+Image— Catalyst platform support and adjustments11. Subscriptions & Pricing
PIAAccountClient/EndpointManager/RequestBuilder/PIAWebServicesupdatesPurchasePlan,SubscriptionOptionViewModelMapper)12. Build Config, Entitlements & Misc
Development.xcconfig/Staging.xcconfig/Production.xcconfig— extension bundle IDsPackage.resolved— updated dependency graph