Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restores public server key and server address if missing #1300

Merged
merged 6 commits into from May 9, 2017

Conversation

@technobly
Copy link
Member

commented Apr 16, 2017

Problem

There have been a number of community reports of devices losing their Public Server Key and/or Server Address. Typically this is due to a power loss or reset during a critical flash write operation.

Solution

  • Public Server Key and Server Address are baked into System Firmware with this PR, and will be restored in RAM before a connection is attempted if either is found to be missing.
  • This data will not be automatically written to FLASH when found missing to prevent further corruption.
  • A cloud event will generated by the device alerting the cloud to which error is currently being patched by system firmware. (currently this does not hit the user's event stream)
  • A subscription to a cloud event has been added to system firmware that will allow the user to restore the data to FLASH remotely when deemed appropriate. (currently there is no mechanism for the user to do this)
  • The TCP server address in PSK is NULL terminated (backup copy in system firmware matches)
  • The UDP server address in PSK is NOT NULL terminated (backup copy in system firmware will now include a NULL terminated server address).
  • pubkey and private_key arrays in system_cloud_internal.cpp were not initialized and would contain garbage wherever key data was not written to, they are not initiallized to 0xFF to mimick erased flash memory.
  • PR #1295 offers hardening and error checking to the FLASH read/write routines to help prevent this PR from being necessary, but just in case we will still be able to recover.

Steps to Test

  • All features have been manually tested on Photon/Electron/Core.
  • Use dfu-util to erase [D] and verify [U] data at the appropriate address
    • Photon - dfu-util -d 2b04:d006 -a 1 -s 2082:768 -[D|U] blank.der
    • Electron - dfu-util -d 2b04:d00a -a 1 -s 3298:320 -[D|U] blank.der
    • Core - dfu-util -d 1d50:607f -a 1 -s 4096:768 -[D|U] blank.der
  • Change system subscription/publish event name from spark to spork and publish PUBLIC event spork/device/key/restore to restore PSK and Server Address.

Example App

This is an internal system change. No example application.

References

  • This will likely have a conflict with PR #1288 due to logging code where the public server address is read (prefer #1288's logging changes)
  • Related to PR #1295

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation (N/A)
  • Added to CHANGELOG.md after merging (add links to docs and issues)

Enhancement

  • [PR #1300] Restores public server key and server address if missing

@technobly technobly added this to the 0.7.0 milestone Apr 16, 2017

@technobly technobly requested a review from m-mcgowan Apr 16, 2017

@@ -272,6 +272,16 @@ void HAL_FLASH_Read_ServerAddress(ServerAddress* server_addr)
parseServerAddressData(server_addr, (const uint8_t*)data, DCT_SERVER_ADDRESS_SIZE);
}

void HAL_FLASH_Write_ServerAddress(uint8_t *buf)

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Apr 18, 2017

Contributor

I suggest that the protocol be part of the call so that the definition is self-defined and unambiguous, rather than defaulting to the current config. It will make application code simpler too, since the app also knows what kind of key is being used.

/* Length in bytes of DER-encoded 1024-bit RSA private key */
#define EXTERNAL_FLASH_CORE_PRIVATE_KEY_LENGTH (612)

void HAL_FLASH_Read_ServerAddress(ServerAddress *server_addr);
void HAL_FLASH_Write_ServerAddress(uint8_t *buf);

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Apr 18, 2017

Contributor

could you make the pointer const please :-)

@@ -475,6 +478,24 @@ void SystemEvents(const char* name, const char* data)
System.reset();
}
}
if (!strncmp(name, KEY_RESTORE_EVENT, strlen(KEY_RESTORE_EVENT))) {

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 4, 2017

Member

What's the usecase for this? If your key/server address information was corrupted/removed then you won't be able to connect to server at all. And if you are already connected and you are able to receive this event from the server, it means that you probably have valid public key and server address 😅

This comment has been minimized.

Copy link
@technobly

technobly May 4, 2017

Author Member

The PSK and server address can be restored in RAM from system flash... so you can connect again. We decided not to automatically restore to DCT/DCD in case of conditions still present that may have caused the first corruption; perhaps they may further corrupt something else during the next flash write. We will track metrics on units reporting corruption, and in the future enable a way for users to send the restore event.

@@ -179,6 +197,58 @@ ProtocolFacade* system_cloud_protocol_instance(void);

int spark_set_connection_property(unsigned property_id, unsigned data, void* datap, void* reserved);

// minimal udp public server key
const unsigned char backup_udp_public_server_key[] = {

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 4, 2017

Member

These should be defined in a source file instead of a header.

This comment has been minimized.

Copy link
@technobly

technobly May 4, 2017

Author Member

Good point, I'll make that change.

@technobly technobly force-pushed the feature/psk-servaddr-repair branch from 6199308 to 3d66a69 May 5, 2017

@technobly technobly requested a review from avtolstoy May 5, 2017

@@ -68,7 +68,7 @@ static sock_handle_t sparkSocket = socket_handle_invalid();

ProtocolFacade* sp;

static Flags<ParticleKeyErrorFlag> particle_key_errors;
static uint32_t particle_key_errors;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 5, 2017

Member

Just to be on a safe side I would initialize it to 0.

This comment has been minimized.

Copy link
@technobly

technobly May 5, 2017

Author Member

Done!

@technobly technobly merged commit 37116a5 into develop May 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/psk-servaddr-repair branch May 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.