fix: critical security issues from code review #25
13 changed files with 522 additions and 46 deletions
|
|
@ -0,0 +1,53 @@
|
||||||
|
<h2>Voorgestelde Flow: De "Server-as-Broker" methode</h2>
|
||||||
|
<p class="subtitle">Gebruik de SylDa_Server als ontmoetingsplaats voor versie-informatie</p>
|
||||||
|
|
||||||
|
<div class="mockup">
|
||||||
|
<div class="mockup-header">Interactie Diagram</div>
|
||||||
|
<div class="mockup-body" style="background: #1e1e1e; color: #d4d4d4; padding: 20px; font-family: monospace; line-height: 1.4;">
|
||||||
|
<div style="margin-bottom: 10px; color: #569cd6;">// 1. Client meldt zich aan</div>
|
||||||
|
[Client] --(Write version: "5")--> [Server: ClientStatusChar]
|
||||||
|
[Client] --(Subscribe)------------> [Server: ClientControlChar]
|
||||||
|
|
||||||
|
<div style="margin-top: 20px; margin-bottom: 10px; color: #569cd6;">// 2. App checkt status</div>
|
||||||
|
[App] --(Read ClientStatus)----> [Server]
|
||||||
|
<span style="color: #ce9178;">App denkt: "Client heeft v5, ik heb v6. Update nodig!"</span>
|
||||||
|
|
||||||
|
<div style="margin-top: 20px; margin-bottom: 10px; color: #569cd6;">// 3. App start de dans</div>
|
||||||
|
[App] --(Write: "START_OTA")---> [Server: ClientControlChar]
|
||||||
|
[Server] --(Notify)---------------> [Client]
|
||||||
|
|
||||||
|
<div style="margin-top: 20px; margin-bottom: 10px; color: #569cd6;">// 4. De wissel</div>
|
||||||
|
<span style="color: #4ec9b0;">Client disconnect van Server -> Start Advertising</span>
|
||||||
|
<span style="color: #4ec9b0;">App disconnect van Server -> Scant naar Client OTA</span>
|
||||||
|
|
||||||
|
<div style="margin-top: 20px; margin-bottom: 10px; color: #569cd6;">// 5. De eigenlijke OTA</div>
|
||||||
|
[App] --(Direct BLE OTA)-------> [Client (nu in Server mode)]
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
|
||||||
|
<div class="section">
|
||||||
|
<h3>Technische details om te bespreken:</h3>
|
||||||
|
<div class="options" data-multiselect>
|
||||||
|
<div class="option" data-choice="uuid-tracking" onclick="toggleSelect(this)">
|
||||||
|
<div class="letter">1</div>
|
||||||
|
<div class="content">
|
||||||
|
<h3>UUID Tracking</h3>
|
||||||
|
<p>Aangezien de Server 2 clients ondersteunt, moet de Server de versie per UUID opslaan zodat de App weet welke client welke versie heeft.</p>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
<div class="option" data-choice="auth" onclick="toggleSelect(this)">
|
||||||
|
<div class="letter">2</div>
|
||||||
|
<div class="content">
|
||||||
|
<h3>Beveiliging (HMAC/AES)</h3>
|
||||||
|
<p>Moet de "START_OTA" opdracht ook versleuteld zijn? (Waarschijnlijk wel, om te voorkomen dat een vreemde de clients platlegt).</p>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
<div class="option" data-choice="reconnect" onclick="toggleSelect(this)">
|
||||||
|
<div class="letter">3</div>
|
||||||
|
<div class="content">
|
||||||
|
<h3>Auto-reconnect</h3>
|
||||||
|
<p>Na de update (of bij falen/timeout) moet de Client weer terug in zijn normale rol vallen en de Server zoeken.</p>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
|
@ -0,0 +1,48 @@
|
||||||
|
<h2>Ontwerpkeuzes BLE OTA voor de Client</h2>
|
||||||
|
<p class="subtitle">Hoe krijgt de Client (ESP32) de nieuwe firmware?</p>
|
||||||
|
|
||||||
|
<div class="options">
|
||||||
|
<div class="option" data-choice="android-direct" onclick="toggleSelect(this)">
|
||||||
|
<div class="letter">A</div>
|
||||||
|
<div class="content">
|
||||||
|
<h3>Android App Direct</h3>
|
||||||
|
<p>De Client ESP32 start een tijdelijke BLE Server (OTA Service). De Android app verbindt direct met de Client om de firmware te pushen.</p>
|
||||||
|
<div class="label">Voordeel: Simpel, hergebruikt Android OTA code.</div>
|
||||||
|
<div class="label">Nadeel: Handmatige actie nodig op Client (bijv. knop indrukken).</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
|
||||||
|
<div class="option" data-choice="server-proxy" onclick="toggleSelect(this)">
|
||||||
|
<div class="letter">B</div>
|
||||||
|
<div class="content">
|
||||||
|
<h3>Server Proxy / Relay</h3>
|
||||||
|
<p>De Android app stuurt de firmware naar de SylDa_Server. De Server stuurt het door naar de Client via de bestaande BLE verbinding.</p>
|
||||||
|
<div class="label">Voordeel: Geen directe verbinding Android-Client nodig.</div>
|
||||||
|
<div class="label">Nadeel: Complexer, Server moet grote buffers hebben of flow control.</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
|
||||||
|
<div class="option" data-choice="client-pull" onclick="toggleSelect(this)">
|
||||||
|
<div class="letter">C</div>
|
||||||
|
<div class="content">
|
||||||
|
<h3>Client Pull (via Server)</h3>
|
||||||
|
<p>De Server kondigt een update aan. De Client haalt de firmware "chunks" op bij de Server via een nieuwe OTA service op de Server.</p>
|
||||||
|
<div class="label">Voordeel: Client heeft controle over tempo.</div>
|
||||||
|
<div class="label">Nadeel: Server moet de firmware ergens opslaan (Flash/NVS/SPIFFS?) voordat hij het kan doorgeven.</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
|
||||||
|
<div class="section">
|
||||||
|
<h3>Huidige Situatie</h3>
|
||||||
|
<div class="mockup">
|
||||||
|
<div class="mockup-header">Architectuur</div>
|
||||||
|
<div class="mockup-body">
|
||||||
|
<pre>
|
||||||
|
[Android App] --(BLE OTA)--> [SylDa_Server (ESP32)]
|
||||||
|
|
|
||||||
|
? --(BLE OTA?)--> [SylDa_Client (ESP32)]
|
||||||
|
</pre>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
2
.superpowers/brainstorm/124687-1785870282/state/events
Normal file
2
.superpowers/brainstorm/124687-1785870282/state/events
Normal file
|
|
@ -0,0 +1,2 @@
|
||||||
|
{"type":"click","text":"3\n \n Auto-reconnect\n Na de update (of bij falen/timeout) moet de Client weer terug in zijn normale rol vallen en de Server zoeken.","choice":"reconnect","id":null,"timestamp":1785870849065}
|
||||||
|
{"type":"click","text":"3\n \n Auto-reconnect\n Na de update (of bij falen/timeout) moet de Client weer terug in zijn normale rol vallen en de Server zoeken.","choice":"reconnect","id":null,"timestamp":1785870849785}
|
||||||
|
|
@ -0,0 +1 @@
|
||||||
|
{"reason":"idle timeout","timestamp":1785872682901}
|
||||||
|
|
@ -0,0 +1,7 @@
|
||||||
|
{"type":"server-started","port":56787,"host":"0.0.0.0","url_host":"100.97.199.84","url":"http://100.97.199.84:56787","screen_dir":"/home/eddy/src/Project_SylDa/.superpowers/brainstorm/124687-1785870282/content","state_dir":"/home/eddy/src/Project_SylDa/.superpowers/brainstorm/124687-1785870282/state"}
|
||||||
|
{"type":"screen-added","file":"/home/eddy/src/Project_SylDa/.superpowers/brainstorm/124687-1785870282/content/ota_options.html"}
|
||||||
|
{"source":"user-event","type":"click","text":"A\n \n Android App Direct\n De Client ESP32 start een tijdelijke BLE Server (OTA Service). De Android app verbindt direct met de Client om de firmware te pushen.\n Voordeel: Simpel, hergebruikt Android OTA code.\n Nadeel: Handmatige actie nodig op Client (bijv. knop indrukken).","choice":"android-direct","id":null,"timestamp":1785870382036}
|
||||||
|
{"type":"screen-added","file":"/home/eddy/src/Project_SylDa/.superpowers/brainstorm/124687-1785870282/content/broker_flow.html"}
|
||||||
|
{"source":"user-event","type":"click","text":"3\n \n Auto-reconnect\n Na de update (of bij falen/timeout) moet de Client weer terug in zijn normale rol vallen en de Server zoeken.","choice":"reconnect","id":null,"timestamp":1785870849065}
|
||||||
|
{"source":"user-event","type":"click","text":"3\n \n Auto-reconnect\n Na de update (of bij falen/timeout) moet de Client weer terug in zijn normale rol vallen en de Server zoeken.","choice":"reconnect","id":null,"timestamp":1785870849785}
|
||||||
|
{"type":"server-stopped","reason":"idle timeout"}
|
||||||
|
|
@ -0,0 +1 @@
|
||||||
|
124706
|
||||||
|
|
@ -22,8 +22,10 @@ import androidx.core.app.ActivityCompat
|
||||||
import java.nio.ByteBuffer
|
import java.nio.ByteBuffer
|
||||||
import java.nio.ByteOrder
|
import java.nio.ByteOrder
|
||||||
import java.security.KeyPairGenerator
|
import java.security.KeyPairGenerator
|
||||||
|
import java.security.KeyFactory
|
||||||
import java.security.SecureRandom
|
import java.security.SecureRandom
|
||||||
import java.security.spec.ECGenParameterSpec
|
import java.security.spec.ECGenParameterSpec
|
||||||
|
import java.security.spec.X509EncodedKeySpec
|
||||||
import java.security.interfaces.ECPublicKey
|
import java.security.interfaces.ECPublicKey
|
||||||
import java.util.UUID
|
import java.util.UUID
|
||||||
import javax.crypto.Cipher
|
import javax.crypto.Cipher
|
||||||
|
|
@ -61,6 +63,25 @@ class BleRelayManager(
|
||||||
private const val ECDH_PKT_SIZE = 97
|
private const val ECDH_PKT_SIZE = 97
|
||||||
private const val BLE_MTU = 512
|
private const val BLE_MTU = 512
|
||||||
private const val ECDH_TIMEOUT_MS = 5000L
|
private const val ECDH_TIMEOUT_MS = 5000L
|
||||||
|
|
||||||
|
private val SECP256R1_SPKI_HEADER = byteArrayOf(
|
||||||
|
0x30.toByte(), 0x59.toByte(), 0x30.toByte(), 0x13.toByte(),
|
||||||
|
0x06.toByte(), 0x07.toByte(), 0x2A.toByte(), 0x86.toByte(),
|
||||||
|
0x48.toByte(), 0xCE.toByte(), 0x3D.toByte(), 0x02.toByte(),
|
||||||
|
0x01.toByte(), 0x06.toByte(), 0x08.toByte(), 0x2A.toByte(),
|
||||||
|
0x86.toByte(), 0x48.toByte(), 0xCE.toByte(), 0x3D.toByte(),
|
||||||
|
0x03.toByte(), 0x01.toByte(), 0x07.toByte(), 0x03.toByte(),
|
||||||
|
0x42.toByte(), 0x00.toByte()
|
||||||
|
)
|
||||||
|
|
||||||
|
private fun decodeEcPublicKey(uncompressedKey: ByteArray): ECPublicKey {
|
||||||
|
require(uncompressedKey.size == ECDH_PUBKEY_LEN && uncompressedKey[0] == 0x04.toByte()) {
|
||||||
|
"Invalid uncompressed EC key format"
|
||||||
|
}
|
||||||
|
val spkiEncoded = SECP256R1_SPKI_HEADER + uncompressedKey
|
||||||
|
return KeyFactory.getInstance("EC")
|
||||||
|
.generatePublic(X509EncodedKeySpec(spkiEncoded)) as ECPublicKey
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private var bluetoothGatt: BluetoothGatt? = null
|
private var bluetoothGatt: BluetoothGatt? = null
|
||||||
|
|
@ -84,6 +105,16 @@ class BleRelayManager(
|
||||||
private var ecdhNotifyBuf: ByteArray = ByteArray(0)
|
private var ecdhNotifyBuf: ByteArray = ByteArray(0)
|
||||||
private var ecdhCccdPending = false
|
private var ecdhCccdPending = false
|
||||||
|
|
||||||
|
@Synchronized
|
||||||
|
private fun setEcdhNotifyBuf(value: ByteArray) {
|
||||||
|
ecdhNotifyBuf = value.copyOf()
|
||||||
|
}
|
||||||
|
|
||||||
|
@Synchronized
|
||||||
|
private fun getEcdhNotifyBuf(): ByteArray {
|
||||||
|
return ecdhNotifyBuf.copyOf()
|
||||||
|
}
|
||||||
|
|
||||||
interface OtaNotifyCallback {
|
interface OtaNotifyCallback {
|
||||||
fun onOtaNotify(status: Int, seq: Int)
|
fun onOtaNotify(status: Int, seq: Int)
|
||||||
}
|
}
|
||||||
|
|
@ -404,7 +435,7 @@ class BleRelayManager(
|
||||||
Log.d(TAG, "OTA notify: status=$statusName seq=$seqVal raw=${value.joinToString("") { "%02X".format(it) }}")
|
Log.d(TAG, "OTA notify: status=$statusName seq=$seqVal raw=${value.joinToString("") { "%02X".format(it) }}")
|
||||||
handler.post { otaNotifyCallback?.onOtaNotify(statusVal, seqVal) }
|
handler.post { otaNotifyCallback?.onOtaNotify(statusVal, seqVal) }
|
||||||
} else if (characteristic.uuid == ECDH_UUID) {
|
} else if (characteristic.uuid == ECDH_UUID) {
|
||||||
ecdhNotifyBuf = value.copyOf()
|
setEcdhNotifyBuf(value)
|
||||||
ecdhDeferred?.complete(true)
|
ecdhDeferred?.complete(true)
|
||||||
Log.d(TAG, "ECDH notify received (${value.size} bytes)")
|
Log.d(TAG, "ECDH notify received (${value.size} bytes)")
|
||||||
}
|
}
|
||||||
|
|
@ -506,7 +537,7 @@ class BleRelayManager(
|
||||||
return@Thread
|
return@Thread
|
||||||
}
|
}
|
||||||
|
|
||||||
val resp = ecdhNotifyBuf
|
val resp = getEcdhNotifyBuf()
|
||||||
if (resp.size != ECDH_PKT_SIZE) {
|
if (resp.size != ECDH_PKT_SIZE) {
|
||||||
Log.w(TAG, "ECDH: bad response size ${resp.size}")
|
Log.w(TAG, "ECDH: bad response size ${resp.size}")
|
||||||
handler.post {
|
handler.post {
|
||||||
|
|
@ -530,28 +561,7 @@ class BleRelayManager(
|
||||||
|
|
||||||
val ka = KeyAgreement.getInstance("ECDH")
|
val ka = KeyAgreement.getInstance("ECDH")
|
||||||
ka.init(keyPair.private)
|
ka.init(keyPair.private)
|
||||||
val spec = java.security.spec.ECPublicKeySpec(
|
val serverKey = decodeEcPublicKey(serverPub)
|
||||||
java.security.spec.ECPoint(
|
|
||||||
java.math.BigInteger(1, serverPub.copyOfRange(1, 33)),
|
|
||||||
java.math.BigInteger(1, serverPub.copyOfRange(33, 65))
|
|
||||||
),
|
|
||||||
java.security.spec.ECParameterSpec(
|
|
||||||
java.security.spec.EllipticCurve(
|
|
||||||
java.security.spec.ECFieldFp(
|
|
||||||
java.math.BigInteger("FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF", 16)
|
|
||||||
),
|
|
||||||
java.math.BigInteger("FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFC", 16),
|
|
||||||
java.math.BigInteger("5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B", 16)
|
|
||||||
),
|
|
||||||
java.security.spec.ECPoint(
|
|
||||||
java.math.BigInteger("6B17D1F2E12C4247F8BCE6E563A440F277037D812DEB33A0F4A13945D898C296", 16),
|
|
||||||
java.math.BigInteger("4FE342E2FE1A7F9B8EE7EB4A7C0F9E162BCE33576B315ECECBB6406837BF51F5", 16)
|
|
||||||
),
|
|
||||||
java.math.BigInteger("FFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551", 16),
|
|
||||||
1
|
|
||||||
)
|
|
||||||
)
|
|
||||||
val serverKey = java.security.KeyFactory.getInstance("EC").generatePublic(spec)
|
|
||||||
ka.doPhase(serverKey, true)
|
ka.doPhase(serverKey, true)
|
||||||
val shared = ka.generateSecret()
|
val shared = ka.generateSecret()
|
||||||
|
|
||||||
|
|
@ -606,13 +616,13 @@ class BleRelayManager(
|
||||||
}
|
}
|
||||||
|
|
||||||
private fun loadState() {
|
private fun loadState() {
|
||||||
val prefs = PrefsHelper.getPrefs(context)
|
PrefsHelper.migrateCredentialsIfNeeded(context)
|
||||||
val hexKey = prefs.getString(PrefsHelper.PREF_KEY, null)
|
val hexKey = PrefsHelper.getPsk(context)
|
||||||
psk = hexKey?.let { hexStringToBytes(it) }
|
psk = hexKey?.let { hexStringToBytes(it) }
|
||||||
val hexUuid = PrefsHelper.getServerUuid(context)
|
val hexUuid = PrefsHelper.getServerUuid(context)
|
||||||
?: prefs.getString(PrefsHelper.PREF_UUID, null)
|
?: PrefsHelper.getDeviceCredentialUuid(context)
|
||||||
deviceUuid = hexUuid?.let { hexStringToBytes(it) }
|
deviceUuid = hexUuid?.let { hexStringToBytes(it) }
|
||||||
seq = prefs.getInt(PrefsHelper.PREF_SEQ, 1).coerceAtMost(PrefsHelper.SEQ_MAX)
|
seq = PrefsHelper.getPrefs(context).getInt(PrefsHelper.PREF_SEQ, 1).coerceAtMost(PrefsHelper.SEQ_MAX)
|
||||||
Log.d(TAG, "PSK loaded: ${psk?.size ?: "none"} bytes, UUID: ${deviceUuid?.size ?: "none"} bytes, seq=$seq")
|
Log.d(TAG, "PSK loaded: ${psk?.size ?: "none"} bytes, UUID: ${deviceUuid?.size ?: "none"} bytes, seq=$seq")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -3,12 +3,16 @@ package nl.eddydevink.sylda
|
||||||
|
|
||||||
import android.content.Context
|
import android.content.Context
|
||||||
import android.content.SharedPreferences
|
import android.content.SharedPreferences
|
||||||
|
import android.util.Log
|
||||||
import androidx.appcompat.app.AppCompatDelegate
|
import androidx.appcompat.app.AppCompatDelegate
|
||||||
|
import androidx.security.crypto.EncryptedSharedPreferences
|
||||||
|
import androidx.security.crypto.MasterKey
|
||||||
|
|
||||||
object PrefsHelper {
|
object PrefsHelper {
|
||||||
|
|
||||||
private const val TAG = "PrefsHelper"
|
private const val TAG = "PrefsHelper"
|
||||||
const val PREFS_NAME = "relay_prefs"
|
const val PREFS_NAME = "relay_prefs"
|
||||||
|
private const val ENCRYPTED_PREFS_NAME = "encrypted_prefs"
|
||||||
const val PREF_KEY = "psk"
|
const val PREF_KEY = "psk"
|
||||||
const val PREF_UUID = "device_uuid"
|
const val PREF_UUID = "device_uuid"
|
||||||
const val PREF_SEQ = "seq_counter"
|
const val PREF_SEQ = "seq_counter"
|
||||||
|
|
@ -20,6 +24,49 @@ object PrefsHelper {
|
||||||
const val KEY_CLIENT_UUID = "clientUuid"
|
const val KEY_CLIENT_UUID = "clientUuid"
|
||||||
const val SEQ_MAX = 1_000_000
|
const val SEQ_MAX = 1_000_000
|
||||||
|
|
||||||
|
private var masterKey: MasterKey? = null
|
||||||
|
|
||||||
|
private fun getMasterKey(context: Context): MasterKey {
|
||||||
|
return masterKey ?: run {
|
||||||
|
MasterKey.Builder(context)
|
||||||
|
.setKeyScheme(MasterKey.KeyScheme.AES256_GCM)
|
||||||
|
.build().also { masterKey = it }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fun getEncryptedPrefs(context: Context): SharedPreferences {
|
||||||
|
return EncryptedSharedPreferences.create(
|
||||||
|
context,
|
||||||
|
ENCRYPTED_PREFS_NAME,
|
||||||
|
getMasterKey(context),
|
||||||
|
EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
|
||||||
|
EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
fun migrateCredentialsIfNeeded(context: Context) {
|
||||||
|
val plainPrefs = getPrefs(context)
|
||||||
|
val encPrefs = getEncryptedPrefs(context)
|
||||||
|
|
||||||
|
val legacyKey = plainPrefs.getString(PREF_KEY, null)
|
||||||
|
val legacyUuid = plainPrefs.getString(PREF_UUID, null)
|
||||||
|
|
||||||
|
if (legacyKey != null && !encPrefs.contains(PREF_KEY)) {
|
||||||
|
Log.i(TAG, "Migrating credentials to encrypted storage")
|
||||||
|
encPrefs.edit().apply {
|
||||||
|
putString(PREF_KEY, legacyKey)
|
||||||
|
if (legacyUuid != null) putString(PREF_UUID, legacyUuid)
|
||||||
|
apply()
|
||||||
|
}
|
||||||
|
plainPrefs.edit().apply {
|
||||||
|
remove(PREF_KEY)
|
||||||
|
remove(PREF_UUID)
|
||||||
|
apply()
|
||||||
|
}
|
||||||
|
Log.i(TAG, "Credentials migration complete")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
fun getDeviceMode(context: Context): String = getPrefs(context).getString(KEY_DEVICE_MODE, "server") ?: "server"
|
fun getDeviceMode(context: Context): String = getPrefs(context).getString(KEY_DEVICE_MODE, "server") ?: "server"
|
||||||
fun setDeviceMode(context: Context, mode: String) = getPrefs(context).edit().putString(KEY_DEVICE_MODE, mode).apply()
|
fun setDeviceMode(context: Context, mode: String) = getPrefs(context).edit().putString(KEY_DEVICE_MODE, mode).apply()
|
||||||
fun getTransportMode(context: Context): String = getPrefs(context).getString("transportMode", "ble") ?: "ble"
|
fun getTransportMode(context: Context): String = getPrefs(context).getString("transportMode", "ble") ?: "ble"
|
||||||
|
|
@ -29,6 +76,11 @@ object PrefsHelper {
|
||||||
fun getClientUuid(context: Context): String? = getPrefs(context).getString(KEY_CLIENT_UUID, null)
|
fun getClientUuid(context: Context): String? = getPrefs(context).getString(KEY_CLIENT_UUID, null)
|
||||||
fun setClientUuid(context: Context, uuid: String) = getPrefs(context).edit().putString(KEY_CLIENT_UUID, uuid).apply()
|
fun setClientUuid(context: Context, uuid: String) = getPrefs(context).edit().putString(KEY_CLIENT_UUID, uuid).apply()
|
||||||
|
|
||||||
|
fun getPsk(context: Context): String? = getEncryptedPrefs(context).getString(PREF_KEY, null)
|
||||||
|
fun setPsk(context: Context, psk: String) = getEncryptedPrefs(context).edit().putString(PREF_KEY, psk).apply()
|
||||||
|
fun getDeviceCredentialUuid(context: Context): String? = getEncryptedPrefs(context).getString(PREF_UUID, null)
|
||||||
|
fun setDeviceCredentialUuid(context: Context, uuid: String) = getEncryptedPrefs(context).edit().putString(PREF_UUID, uuid).apply()
|
||||||
|
|
||||||
fun applyTheme(context: Context) {
|
fun applyTheme(context: Context) {
|
||||||
val prefs = getPrefs(context)
|
val prefs = getPrefs(context)
|
||||||
val mode = when (prefs.getString(PREF_THEME, "system")) {
|
val mode = when (prefs.getString(PREF_THEME, "system")) {
|
||||||
|
|
|
||||||
|
|
@ -372,9 +372,8 @@ class SettingsActivity : AppCompatActivity() {
|
||||||
private fun doKeyExchange(usbManager: UsbManager, device: UsbDevice) {
|
private fun doKeyExchange(usbManager: UsbManager, device: UsbDevice) {
|
||||||
showProgressDialog(getString(R.string.title_key_exchange), getString(R.string.msg_reading_key), STYLE_SPINNER)
|
showProgressDialog(getString(R.string.title_key_exchange), getString(R.string.msg_reading_key), STYLE_SPINNER)
|
||||||
|
|
||||||
val prefs = PrefsHelper.getPrefs(this)
|
val appUuid = PrefsHelper.getDeviceCredentialUuid(this)
|
||||||
val appUuid = prefs.getString(PrefsHelper.PREF_UUID, null)
|
val appKey = PrefsHelper.getPsk(this)
|
||||||
val appKey = prefs.getString(PrefsHelper.PREF_KEY, null)
|
|
||||||
|
|
||||||
lifecycleScope.launch(Dispatchers.IO) {
|
lifecycleScope.launch(Dispatchers.IO) {
|
||||||
val result = readKeyFromEsp(usbManager, device, appUuid, appKey)
|
val result = readKeyFromEsp(usbManager, device, appUuid, appKey)
|
||||||
|
|
@ -384,10 +383,8 @@ class SettingsActivity : AppCompatActivity() {
|
||||||
progressDialog = null
|
progressDialog = null
|
||||||
when (result) {
|
when (result) {
|
||||||
is KeyResult.Success -> {
|
is KeyResult.Success -> {
|
||||||
prefs.edit()
|
PrefsHelper.setDeviceCredentialUuid(this@SettingsActivity, result.deviceUuid)
|
||||||
.putString(PrefsHelper.PREF_UUID, result.deviceUuid)
|
PrefsHelper.setPsk(this@SettingsActivity, result.hexKey)
|
||||||
.putString(PrefsHelper.PREF_KEY, result.hexKey)
|
|
||||||
.apply()
|
|
||||||
if (PrefsHelper.getDeviceMode(this@SettingsActivity) == "server") {
|
if (PrefsHelper.getDeviceMode(this@SettingsActivity) == "server") {
|
||||||
PrefsHelper.setServerUuid(this@SettingsActivity, result.deviceUuid)
|
PrefsHelper.setServerUuid(this@SettingsActivity, result.deviceUuid)
|
||||||
} else {
|
} else {
|
||||||
|
|
@ -838,8 +835,7 @@ class SettingsActivity : AppCompatActivity() {
|
||||||
|
|
||||||
private fun showConflictDialog(usbManager: UsbManager, device: UsbDevice,
|
private fun showConflictDialog(usbManager: UsbManager, device: UsbDevice,
|
||||||
espUuid: String, espKey: String, appKey: String) {
|
espUuid: String, espKey: String, appKey: String) {
|
||||||
val prefs = PrefsHelper.getPrefs(this)
|
val appUuid = PrefsHelper.getDeviceCredentialUuid(this)
|
||||||
val appUuid = prefs.getString(PrefsHelper.PREF_UUID, null)
|
|
||||||
val newAppKey = appKey.ifEmpty { generateRandomHexKey() }
|
val newAppKey = appKey.ifEmpty { generateRandomHexKey() }
|
||||||
val newAppUuid = appUuid ?: generateRandomHex(UUID_BYTE_COUNT)
|
val newAppUuid = appUuid ?: generateRandomHex(UUID_BYTE_COUNT)
|
||||||
|
|
||||||
|
|
@ -946,11 +942,8 @@ class SettingsActivity : AppCompatActivity() {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
val prefs = PrefsHelper.getPrefs(this)
|
PrefsHelper.setDeviceCredentialUuid(this, newUuid)
|
||||||
prefs.edit()
|
PrefsHelper.setPsk(this, appKey)
|
||||||
.putString(PrefsHelper.PREF_UUID, newUuid)
|
|
||||||
.putString(PrefsHelper.PREF_KEY, appKey)
|
|
||||||
.apply()
|
|
||||||
if (PrefsHelper.getDeviceMode(this) == "server") {
|
if (PrefsHelper.getDeviceMode(this) == "server") {
|
||||||
PrefsHelper.setServerUuid(this, newUuid)
|
PrefsHelper.setServerUuid(this, newUuid)
|
||||||
} else {
|
} else {
|
||||||
|
|
|
||||||
305
docs/superpowers/plans/2026-08-04-code-review-fixes-v2.md
Normal file
305
docs/superpowers/plans/2026-08-04-code-review-fixes-v2.md
Normal file
|
|
@ -0,0 +1,305 @@
|
||||||
|
# Code Review Fixes (Batch 2) Implementation Plan
|
||||||
|
|
||||||
|
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development
|
||||||
|
> (recommended) or superpowers:executing-plans to implement this plan task-by-task.
|
||||||
|
> Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||||
|
|
||||||
|
**Goal:** Fix 6 issues found in code review: broken client OTA UI flow (SettingsActivity button), missing timeout in OTA polling, recomputeClientOta race condition, committed upload_port, wrong LED startup sequence, and noisy ESP_LOGI diagnostics.
|
||||||
|
|
||||||
|
**Architecture:** All fixes are isolated single-file edits. No new files. No architecture changes. Version bumps: server 21→22, client 11→12.
|
||||||
|
|
||||||
|
**Tech Stack:** C++ (Arduino/ESP-IDF), Kotlin (Android), PlatformIO
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 1: Fix SettingsActivity button — client+BLE now routes to startClientOtaUpdate
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt:167-174`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Replace button click handler**
|
||||||
|
|
||||||
|
In `setupButtons()` (or wherever the click listener is set), replace lines 167-174 with the corrected logic:
|
||||||
|
|
||||||
|
```kotlin
|
||||||
|
findViewById<Button>(R.id.updateModuleButton).setOnClickListener {
|
||||||
|
val mode = PrefsHelper.getDeviceMode(this)
|
||||||
|
val transportId = findViewById<RadioGroup>(R.id.radioTransport).checkedRadioButtonId
|
||||||
|
if (transportId == R.id.radioUsb) {
|
||||||
|
startFlashing()
|
||||||
|
} else if (mode == "client") {
|
||||||
|
startClientOtaUpdate(BleRelayManager.instance)
|
||||||
|
} else {
|
||||||
|
startServerOtaUpdate(BleRelayManager.instance)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Verify the change compiles**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd android && ./gradlew compileDebugKotlin 2>&1 | tail -5
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 3: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt
|
||||||
|
git commit -m "fix(android): client+BLE button now routes to client OTA instead of USB flash"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 2: Add timeout to waitForClientOtaReady polling loop
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt:393-405`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Add `import kotlinx.coroutines.withTimeoutOrNull`**
|
||||||
|
|
||||||
|
The import `kotlinx.coroutines.isActive` already exists at line 35. Just add the timeout:
|
||||||
|
|
||||||
|
```kotlin
|
||||||
|
import kotlinx.coroutines.withTimeoutOrNull
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Wrap polling loop in withTimeoutOrNull**
|
||||||
|
|
||||||
|
Replace `waitForClientOtaReady` (lines 393-405) with:
|
||||||
|
|
||||||
|
```kotlin
|
||||||
|
private suspend fun waitForClientOtaReady(serverManager: BleRelayManager): Boolean {
|
||||||
|
return withTimeoutOrNull(30_000L) {
|
||||||
|
var status = -1
|
||||||
|
serverManager.setClientOtaStatusListener { s -> status = s }
|
||||||
|
try {
|
||||||
|
while (status != 0x01) {
|
||||||
|
serverManager.readClientOtaStatus()
|
||||||
|
delay(2000)
|
||||||
|
}
|
||||||
|
} finally {
|
||||||
|
serverManager.setClientOtaStatusListener(null)
|
||||||
|
}
|
||||||
|
} ?: false
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 3: Verify the change compiles**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cd android && ./gradlew compileDebugKotlin 2>&1 | tail -5
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 4: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add android/app/src/main/java/nl/eddydevink/sylda/SettingsActivity.kt
|
||||||
|
git commit -m "fix(android): add 30s timeout to client OTA ready polling"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 3: Fix recomputeClientOta() race — preserve 0x01 status
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `src/main.cpp:251`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Add 0x01 exclusion to recomputeClientOta**
|
||||||
|
|
||||||
|
Change line 251 from:
|
||||||
|
```cpp
|
||||||
|
if (!clientOtaAvailable && clientOtaStatus != 0x00 && clientOtaStatus != 0x02) {
|
||||||
|
```
|
||||||
|
to:
|
||||||
|
```cpp
|
||||||
|
if (!clientOtaAvailable && clientOtaStatus != 0x00 && clientOtaStatus != 0x01 && clientOtaStatus != 0x02) {
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add src/main.cpp
|
||||||
|
git commit -m "fix(server): avoid resetting CLIENT_OTA_STATUS 0x01 in recomputeClientOta"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 4: Remove committed upload_port from platformio.ini
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `platformio.ini:16,38`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Remove both upload_port lines**
|
||||||
|
|
||||||
|
Delete line 16:
|
||||||
|
```
|
||||||
|
upload_port = /dev/serial/by-id/usb-Espressif_USB_JTAG_serial_debug_unit_44:1B:F6:F4:CF:14-if00
|
||||||
|
```
|
||||||
|
Delete line 38:
|
||||||
|
```
|
||||||
|
upload_port = /dev/serial/by-id/usb-Espressif_USB_JTAG_serial_debug_unit_44:1B:F6:F5:D1:E0-if00
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Verify platformio.ini is still valid**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
grep -n "upload_port" platformio.ini
|
||||||
|
```
|
||||||
|
Expected: no output.
|
||||||
|
|
||||||
|
- [ ] **Step 3: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add platformio.ini
|
||||||
|
git commit -m "fix: remove committed upload_port from platformio.ini"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 5: Restore LED startup sequence to Blue-Red-Blue
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `src/client.cpp:603-605`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Change LED startup sequence**
|
||||||
|
|
||||||
|
Replace lines 603-605:
|
||||||
|
```cpp
|
||||||
|
leds[0] = CRGB::Red; FastLED.show(); delay(500);
|
||||||
|
leds[0] = CRGB::Green; FastLED.show(); delay(500);
|
||||||
|
leds[0] = CRGB::Red; FastLED.show(); delay(500);
|
||||||
|
leds[0] = CRGB::Black; FastLED.show();
|
||||||
|
```
|
||||||
|
with:
|
||||||
|
```cpp
|
||||||
|
leds[0] = CRGB::Blue; FastLED.show(); delay(500);
|
||||||
|
leds[0] = CRGB::Red; FastLED.show(); delay(500);
|
||||||
|
leds[0] = CRGB::Blue; FastLED.show(); delay(500);
|
||||||
|
leds[0] = CRGB::Black; FastLED.show();
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add src/client.cpp
|
||||||
|
git commit -m "fix(client): restore LED startup sequence to blue-red-blue"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 6: Downgrade diagnostic ESP_LOGI to ESP_LOGD
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `src/client.cpp:907`
|
||||||
|
- Modify: `src/main.cpp:474`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Change client.cpp diagnostic log level**
|
||||||
|
|
||||||
|
Change line 907 from:
|
||||||
|
```cpp
|
||||||
|
ESP_LOGI(TAG, "OTA: status read value=%d bytes empty=%d firstByte=0x%02X",
|
||||||
|
```
|
||||||
|
to:
|
||||||
|
```cpp
|
||||||
|
ESP_LOGD(TAG, "OTA: status read value=%d bytes empty=%d firstByte=0x%02X",
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Change server.cpp onRead diagnostic log level**
|
||||||
|
|
||||||
|
Change line 474 from:
|
||||||
|
```cpp
|
||||||
|
ESP_LOGI(TAG, "CLIENT_OTA_STATUS: read by conn=%d", connInfo.getConnHandle());
|
||||||
|
```
|
||||||
|
to:
|
||||||
|
```cpp
|
||||||
|
ESP_LOGD(TAG, "CLIENT_OTA_STATUS: read by conn=%d", connInfo.getConnHandle());
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 3: Commit**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add src/client.cpp src/main.cpp
|
||||||
|
git commit -m "chore: downgrade diagnostic OTA logs from INFO to DEBUG"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Task 7: Bump versions and rebuild
|
||||||
|
|
||||||
|
**Files:**
|
||||||
|
- Modify: `version.txt`
|
||||||
|
|
||||||
|
- [ ] **Step 1: Bump versions**
|
||||||
|
|
||||||
|
```
|
||||||
|
server=22
|
||||||
|
client=12
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 2: Commit version bump**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add version.txt
|
||||||
|
git commit -m "chore: bump versions server 21->22, client 11->12"
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 3: Rebuild both firmware variants**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
pio run
|
||||||
|
```
|
||||||
|
|
||||||
|
Verify both variants compile: `[env:s3lite-server] SUCCESS` and `[env:s3lite-client] SUCCESS`.
|
||||||
|
|
||||||
|
- [ ] **Step 4: Copy built firmware to Android assets**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
cp .pio/build/s3lite-server/firmware.bin android/app/src/main/assets/firmware/server/firmware.bin
|
||||||
|
cp .pio/build/s3lite-client/firmware.bin android/app/src/main/assets/firmware/client/firmware.bin
|
||||||
|
```
|
||||||
|
|
||||||
|
- [ ] **Step 5: Commit firmware binaries**
|
||||||
|
|
||||||
|
```bash
|
||||||
|
git add android/app/src/main/assets/firmware/
|
||||||
|
git commit -m "chore: bundle server v22 and client v12 firmware in APK"
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### Verification
|
||||||
|
|
||||||
|
After all tasks are complete:
|
||||||
|
|
||||||
|
1. **Check git log:**
|
||||||
|
```bash
|
||||||
|
git log --oneline -8
|
||||||
|
```
|
||||||
|
Expected: 7 new commits on top of b8facac.
|
||||||
|
|
||||||
|
2. **Flash server v22:**
|
||||||
|
```bash
|
||||||
|
# Find ESP32 server port
|
||||||
|
ls /dev/serial/by-id/ | grep -i espressif
|
||||||
|
# Update upload_port in platformio.ini (or use override), then:
|
||||||
|
pio run -t upload -e s3lite-server
|
||||||
|
```
|
||||||
|
|
||||||
|
3. **Flash client v12:**
|
||||||
|
```bash
|
||||||
|
pio run -t upload -e s3lite-client
|
||||||
|
```
|
||||||
|
|
||||||
|
4. **Verification test — Server serial:**
|
||||||
|
Connect to server serial. Confirm startup message shows v22. Confirm no "CLIENT_OTA_STATUS: read by conn=..." spam (only on CORE_DEBUG_LEVEL=4).
|
||||||
|
|
||||||
|
5. **Verification test — Client LED startup:**
|
||||||
|
Power cycle client. Confirm LED shows Blue → Red → Blue → off.
|
||||||
|
|
||||||
|
6. **Verification test — Client OTA flow via BLE:**
|
||||||
|
- Build and install APK: `cd android && ./gradlew assembleDebug`
|
||||||
|
- Deploy APK to phone via ADB
|
||||||
|
- In MainActivity, verify client OTA available notification
|
||||||
|
- Tap notification → SettingsActivity opens with client+BLE pre-selected
|
||||||
|
- Tap "Update module" → must show "Wachten op client..." progress dialog (NOT USB dialog)
|
||||||
|
- If client has older version, OTA should proceed; if same version, toast "Firmware is up-to-date"
|
||||||
|
|
@ -260,7 +260,7 @@ static bool performEcdhHandshake(NimBLERemoteService *pSvc) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
memcpy(aesKey, ecdh.aes_key, AES_KEY_LEN);
|
memcpy(aesKey, ecdh.getAesKey(), AES_KEY_LEN);
|
||||||
ecdh.free();
|
ecdh.free();
|
||||||
ecdhReady = true;
|
ecdhReady = true;
|
||||||
ESP_LOGI(TAG, "ECDH: handshake complete");
|
ESP_LOGI(TAG, "ECDH: handshake complete");
|
||||||
|
|
|
||||||
|
|
@ -21,6 +21,9 @@ private:
|
||||||
mbedtls_ctr_drbg_context ctr_drbg;
|
mbedtls_ctr_drbg_context ctr_drbg;
|
||||||
bool initialized = false;
|
bool initialized = false;
|
||||||
|
|
||||||
|
uint8_t shared_secret[32];
|
||||||
|
uint8_t aes_key[AES_KEY_LEN];
|
||||||
|
|
||||||
CryptoECDH(const CryptoECDH&) = delete;
|
CryptoECDH(const CryptoECDH&) = delete;
|
||||||
CryptoECDH& operator=(const CryptoECDH&) = delete;
|
CryptoECDH& operator=(const CryptoECDH&) = delete;
|
||||||
|
|
||||||
|
|
@ -29,8 +32,9 @@ public:
|
||||||
|
|
||||||
uint8_t public_key[ECDH_PUBKEY_LEN];
|
uint8_t public_key[ECDH_PUBKEY_LEN];
|
||||||
size_t pub_len = 0;
|
size_t pub_len = 0;
|
||||||
uint8_t shared_secret[32];
|
|
||||||
uint8_t aes_key[AES_KEY_LEN];
|
const uint8_t* getAesKey() const { return aes_key; }
|
||||||
|
size_t getAesKeyLen() const { return AES_KEY_LEN; }
|
||||||
|
|
||||||
bool init() {
|
bool init() {
|
||||||
mbedtls_ecdh_init(&ecdh);
|
mbedtls_ecdh_init(&ecdh);
|
||||||
|
|
|
||||||
|
|
@ -406,7 +406,7 @@ class EcdhCharacteristicCallbacks : public NimBLECharacteristicCallbacks {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
memcpy(clients[idx].aesKey, clients[idx].ecdh.aes_key, AES_KEY_LEN);
|
memcpy(clients[idx].aesKey, clients[idx].ecdh.getAesKey(), AES_KEY_LEN);
|
||||||
|
|
||||||
uint8_t resp[ECDH_PKT_SIZE];
|
uint8_t resp[ECDH_PKT_SIZE];
|
||||||
memcpy(resp, clients[idx].ecdh.public_key, ECDH_PUBKEY_LEN);
|
memcpy(resp, clients[idx].ecdh.public_key, ECDH_PUBKEY_LEN);
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue