Skip to content

Commit

Permalink
Split M4 pair setup step into stages
Browse files Browse the repository at this point in the history 
SRP PMS generation is very expensive on ESP8266 and takes about 5
seconds. With non-preemptive SDK this leads to device missing enough
beacons to get dropped by the AP and pairing process failing.
This happens often enough to be annoying

With this change SRP PMS generation happens in stages, allowing other
stuff to get done. The longest step is 2, taking 2.7 seconds, and there
are still occasional connection drops, but the rate of success overall
has improved a lot.

mongoose-os-apps/shelly-homekit#748
  • Loading branch information
@rojer
rojer committed Aug 8, 2021
1 parent d43ed85 commit 65021ee
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 23 deletions.
5 changes: 5 additions & 0 deletions HomeKitADK/HAP/HAPAccessoryServer+Internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ typedef struct {

bool flagsPresent : 1; /**< Whether Pairing Type flags were present in Pair Setup M1. */
bool keepSetupInfo : 1; /**< Whether setup info should be kept on disconnect. */

uint8_t srpPMSStage;
} pairSetup;

/**
Expand Down Expand Up @@ -189,6 +191,9 @@ typedef struct {
/** Timer that on expiry runs the garbage task. */
HAPPlatformTimerRef garbageCollectionTimer;

/** Timer that fires to move processing of an expensive request forward. */
HAPPlatformTimerRef processTimer;

/** Timer that on expiry schedules a maximum idle time check. */
HAPPlatformTimerRef maxIdleTimeTimer;

Expand Down
82 changes: 69 additions & 13 deletions HomeKitADK/HAP/HAPIPAccessoryServer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2147,15 +2147,20 @@ static void handle_pairing_data(
if (session->httpContentLength.isDefined) {
HAPAssert(session->httpContentLength.value <= session->inboundBuffer.position - session->httpReaderPosition);
if (session->httpContentLength.value <= maxScratchBufferBytes) {
HAPRawBufferCopyBytes(
scratchBuffer,
&session->inboundBuffer.data[session->httpReaderPosition],
session->httpContentLength.value);
tlv8_reader_init.bytes = scratchBuffer;
tlv8_reader_init.numBytes = session->httpContentLength.value;
tlv8_reader_init.maxBytes = maxScratchBufferBytes;
HAPTLVReaderCreateWithOptions(&tlv8_reader, &tlv8_reader_init);
r = write_hap_pairing_data(HAPNonnull(session->server), &session->securitySession._.hap, &tlv8_reader);
// Skip writing phase if we are still processing the previous request.
if (session->state != kHAPIPSessionState_Processing) {
HAPRawBufferCopyBytes(
scratchBuffer,
&session->inboundBuffer.data[session->httpReaderPosition],
session->httpContentLength.value);
tlv8_reader_init.bytes = scratchBuffer;
tlv8_reader_init.numBytes = session->httpContentLength.value;
tlv8_reader_init.maxBytes = maxScratchBufferBytes;
HAPTLVReaderCreateWithOptions(&tlv8_reader, &tlv8_reader_init);
r = write_hap_pairing_data(HAPNonnull(session->server), &session->securitySession._.hap, &tlv8_reader);
} else {
r = 0;
}
if (r == 0) {
HAPTLVWriterCreate(&tlv8_writer, scratchBuffer, maxScratchBufferBytes);
r = read_hap_pairing_data(HAPNonnull(session->server), &session->securitySession._.hap, &tlv8_writer);
Expand Down Expand Up @@ -2211,6 +2216,9 @@ static void handle_pairing_data(
session->outboundBuffer.position = mark;
write_msg(&session->outboundBuffer, kHAPIPAccessoryServerResponse_OutOfResources);
}
} else if (r == kHAPError_Busy) {
HAPLogDebug(&logObject, "processing...");
session->state = kHAPIPSessionState_Processing;
} else {
log_result(
kHAPLogType_Error,
Expand All @@ -2221,6 +2229,11 @@ static void handle_pairing_data(
__LINE__);
write_msg(&session->outboundBuffer, kHAPIPAccessoryServerResponse_InternalServerError);
}
if (r != kHAPError_Busy && session->state == kHAPIPSessionState_Processing) {
// Response is ready.
HAPLogDebug(&logObject, "done");
session->state = kHAPIPSessionState_Writing;
}
} else {
write_msg(&session->outboundBuffer, kHAPIPAccessoryServerResponse_BadRequest);
}
Expand Down Expand Up @@ -2902,14 +2915,17 @@ static void handle_http(HAPIPSessionDescriptor* session) {
(const void*) session,
(long) requestLen);
handle_http_request(session);
HAPIPByteBuffer* b = &session->inboundBuffer;
HAPIPByteBufferShiftLeft(b, requestLen);
if (session->state != kHAPIPSessionState_Processing) {
HAPIPByteBufferShiftLeft(&session->inboundBuffer, requestLen);
}
if (session->accessorySerializationIsInProgress) {
// Session is already prepared for writing
HAPAssert(session->outboundBuffer.data || session->outboundBuffer.isDynamic);
HAPAssert(session->outboundBuffer.position <= session->outboundBuffer.limit);
HAPAssert(session->outboundBuffer.limit <= session->outboundBuffer.capacity);
HAPAssert(session->state == kHAPIPSessionState_Writing);
} else if (session->state == kHAPIPSessionState_Processing) {
// Needs more time to prepare a response.
} else {
HAPAssert(session->outboundBuffer.data || session->outboundBuffer.isDynamic);
HAPAssert(session->outboundBuffer.position <= session->outboundBuffer.limit);
Expand Down Expand Up @@ -3241,8 +3257,11 @@ static void handle_input(HAPIPSessionDescriptor* session) {
handle_http(session);
}
session->inboundBufferMark = session->inboundBuffer.position;
session->inboundBuffer.position = session->inboundBuffer.limit;
session->inboundBuffer.limit = session->inboundBuffer.capacity;
if (session->state != kHAPIPSessionState_Processing) {
session->inboundBuffer.position = session->inboundBuffer.limit;
session->inboundBuffer.limit = session->inboundBuffer.capacity;
}

if ((session->state == kHAPIPSessionState_Reading) &&
(session->inboundBuffer.position == session->inboundBuffer.limit)) {
log_protocol_error(
Expand Down Expand Up @@ -3443,11 +3462,48 @@ static void write_event_notifications(HAPIPSessionDescriptor* session) {
}
}

static void handle_io_progression(HAPIPSessionDescriptor* session);

static void handle_server_process_timer(HAPPlatformTimerRef timer, void* _Nullable context) {
HAPPrecondition(context);
HAPAccessoryServerRef* server_ = context;
HAPAccessoryServer* server = (HAPAccessoryServer*) server_;
(void) server;
HAPPrecondition(timer == server->ip.processTimer);
server->ip.processTimer = 0;

if (server->ip.state != kHAPIPAccessoryServerState_Running) {
return;
}

for (size_t i = 0; i < server->ip.storage->numSessions; i++) {
HAPIPSession* ipSession = &server->ip.storage->sessions[i];
HAPIPSessionDescriptor* session = (HAPIPSessionDescriptor*) &ipSession->descriptor;
if (!session->server || session->state != kHAPIPSessionState_Processing) {
continue;
}

HAPLogDebug(&logObject, "Re-processing session %p...", session);
handle_input(session);
handle_io_progression(session);
}
}

static void handle_io_progression(HAPIPSessionDescriptor* session) {
HAPPrecondition(session);
HAPPrecondition(session->server);
HAPAccessoryServer* server = (HAPAccessoryServer*) session->server;

if (session->state == kHAPIPSessionState_Processing) {
if (server->ip.processTimer == 0) {
HAPError err = HAPPlatformTimerRegister(
&server->ip.processTimer, HAPPlatformClockGetCurrent() + 150, handle_server_process_timer, server);
if (err) {
HAPLog(&logObject, "Not enough resources to schedule processing timer!");
HAPFatalError();
}
}
}
if ((session->state == kHAPIPSessionState_Reading) && (session->inboundBuffer.position == 0)) {
if (server->ip.state == kHAPIPAccessoryServerState_Stopping) {
CloseSession(session);
Expand Down
3 changes: 3 additions & 0 deletions HomeKitADK/HAP/HAPIPAccessoryServer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ HAP_ENUM_BEGIN(uint8_t, HAPIPSessionState) { /** Accessory server session is idl
/** Accessory server session is reading. */
kHAPIPSessionState_Reading,

/** Accessory server session is processing. */
kHAPIPSessionState_Processing,

/** Accessory server session is writing. */
kHAPIPSessionState_Writing
} HAP_ENUM_END(uint8_t, HAPIPSessionState);
Expand Down
36 changes: 28 additions & 8 deletions HomeKitADK/HAP/HAPPairingPairSetup.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ static HAPError HAPPairingPairSetupProcessM3(
* @return kHAPError_Unknown If communication with Apple Auth Coprocessor or persistent store access failed.
* @return kHAPError_InvalidState If a different request is expected in the current state.
* @return kHAPError_OutOfResources If response writer does not have enough capacity.
* @return kHAPError_Busy If response is not yet ready, invoke again with the same arguments.
*/
HAP_RESULT_USE_CHECK
static HAPError HAPPairingPairSetupGetM4(
Expand All @@ -436,7 +437,7 @@ static HAPError HAPPairingPairSetupGetM4(
HAPPrecondition(server->pairSetup.sessionThatIsCurrentlyPairing == session_);
HAPPrecondition(session_);
HAPSession* session = (HAPSession*) session_;
HAPPrecondition(session->state.pairSetup.state == 4);
// HAPPrecondition(session->state.pairSetup.state == 4);
HAPPrecondition(!session->state.pairSetup.error);
HAPPrecondition(responseWriter);

Expand Down Expand Up @@ -471,10 +472,23 @@ static HAPError HAPPairingPairSetupGetM4(
}
HAPSetupInfo* _Nullable setupInfo = HAPAccessorySetupInfoGetSetupInfo(server_, restorePrevious);
HAPAssert(setupInfo);

int e = HAP_srp_premaster_secret(S, server->pairSetup.A, server->pairSetup.b, u, setupInfo->verifier);
/*
* S is used to hold SRP PMS generation state between stages it is located in scratch space
* and we make use of the fact that scratch buffer is not touched during pairing,
* there are no other requests and so exact same location is used during all PMS stages.
* This is a valid assumption as long as there is no other activity possible while pairing is ongoing,
* which is true, since there can be no other sessions until device is paired to a controller.
*/
int ss = server->pairSetup.srpPMSStage;
int e = HAP_srp_premaster_secret_stage(
S, server->pairSetup.A, server->pairSetup.b, u, setupInfo->verifier, &server->pairSetup.srpPMSStage);
if (e) {
HAPAssert(e == 1);
HAPAssert(e == 1 || e == 2 || e == HAP_SRP_PREMASTER_SECRET_NEED_MORE);
if (e == HAP_SRP_PREMASTER_SECRET_NEED_MORE) {
HAPLogDebug(&logObject, "SRP PMS stage %d", server->pairSetup.srpPMSStage);
(void) ss;
return kHAPError_Busy;
}
// Illegal key A.
HAPLog(&logObject, "Pair Setup M4: Illegal key A.");
session->state.pairSetup.error = kHAPPairingError_Authentication;
Expand Down Expand Up @@ -574,10 +588,10 @@ static HAPError HAPPairingPairSetupGetM4(
}

// kTLVType_State.
uint8_t four = 4;
err = HAPTLVWriterAppend(
responseWriter,
&(const HAPTLV) { .type = kHAPPairingTLVType_State,
.value = { .bytes = &session->state.pairSetup.state, .numBytes = 1 } });
&(const HAPTLV) { .type = kHAPPairingTLVType_State, .value = { .bytes = &four, .numBytes = 1 } });
if (err) {
HAPAssert(err == kHAPError_OutOfResources);
return err;
Expand Down Expand Up @@ -1412,10 +1426,16 @@ HAPError HAPPairingPairSetupHandleRead(
}
} break;
case 3: {
session->state.pairSetup.state++;
err = HAPPairingPairSetupGetM4(server, session_, responseWriter);
HAPLogDebug(&logObject, "err = %d", err);
if (err) {
HAPAssert(err == kHAPError_Unknown || err == kHAPError_InvalidState || err == kHAPError_OutOfResources);
HAPAssert(
err == kHAPError_Unknown || err == kHAPError_InvalidState || err == kHAPError_OutOfResources ||
kHAPError_Busy);
if (err == kHAPError_Busy)
return err;
} else {
session->state.pairSetup.state++;
}
} break;
case 5: {
Expand Down
4 changes: 3 additions & 1 deletion HomeKitADK/HAP/HAPSession.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,9 @@ HAPError HAPSessionHandlePairSetupRead(
bool wasPaired = HAPAccessoryServerIsPaired(server_);
err = HAPPairingPairSetupHandleRead(server_, session_, responseWriter);
if (err) {
HAPAssert(err == kHAPError_InvalidState || err == kHAPError_Unknown || err == kHAPError_OutOfResources);
HAPAssert(
err == kHAPError_InvalidState || err == kHAPError_Unknown || err == kHAPError_OutOfResources ||
err == kHAPError_Busy);
return err;
}
bool isPaired = HAPAccessoryServerIsPaired(server_);
Expand Down
3 changes: 2 additions & 1 deletion HomeKitADK/PAL/Crypto/MbedTLS/HAPMbedTLS.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,8 @@ static void
#define ALIGN_BN_LIMB __attribute__((aligned(sizeof(mbedtls_mpi_uint))))

// SRP 3072-bit prime number, little-endian.
static const uint8_t N_3072_data_le[] ALIGN_BN_LIMB = {
// Note: non-const to speed up access on ESP8266 (I-Bus is slower tha D-Bus, 300 ms difference for SRP PMS).
static uint8_t N_3072_data_le[] ALIGN_BN_LIMB = {
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xca, 0xd2, 0x3a, 0xa9, 0x20, 0xd1, 0x82, 0x4b, 0x8e, 0x10, 0xfd,
0xe0, 0xfc, 0x5b, 0xdb, 0x43, 0x31, 0xab, 0xe5, 0x74, 0xa0, 0x4f, 0xe2, 0x08, 0xe2, 0x46, 0xd9, 0xba, 0xc0, 0x88,
0x09, 0x77, 0x6c, 0x5d, 0x61, 0x7a, 0x57, 0x17, 0xe1, 0xbb, 0x0c, 0x20, 0x7b, 0x17, 0x18, 0x2b, 0x1f, 0x52, 0x64,
Expand Down

0 comments on commit 65021ee

Please sign in to comment.