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

ports/esp32 add api support for ota updates #3576

Closed
wants to merge 11 commits into from

Conversation

andynd
Copy link

@andynd andynd commented Jan 21, 2018

add the following APIs:
esp.partition_find_first
esp.ota_set_boot_partition
esp.ota_get_boot_partition
esp.ota_get_running_partition
esp.ota_get_next_update_partition

move the vfs to its own partition if it exists
use an ota capable partition layout

add the following APIs:
	esp.partition_find_first
	esp.ota_set_boot_partition
	esp.ota_get_boot_partition

move the vfs to its own partition if it exists
use an ota capable partition layout
@andynd andynd changed the title add support for ota updates for the esp32 port ports/esp32 add api support for ota updates Feb 4, 2018
@andynd
Copy link
Author

andynd commented Feb 27, 2018

Any plans to merge this? Pull request was created more than a month ago.

@ThomasWaldmann
Copy link

ThomasWaldmann commented Mar 19, 2018

Having OTA capability would be nice!

Do I understand it correctly that it would update the partition with micropython, but not the one with the filesystem?

@@ -5,6 +5,7 @@
try:
if bdev:
uos.mount(bdev, '/')
print("Mounted /")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation looks wrong. tab? one space missing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh.. yes. thx Will fix it.

if not offset:
self.START_SEC = esp.flash_user_start() // self.SEC_SIZE
else:
self.START_SEC = offset // self.SEC_SIZE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again: indentation looks wrong.

@andynd
Copy link
Author

andynd commented Mar 19, 2018

Do I understand it correctly that it would update the partition with micropython, but not the one with the filesystem?

It moves the fs to its own partition, right now micropython ignores the partition table and uses space after micropython. This would corrupt a 2nd partition. You can update the fs too, but you do not need any code in mp to do it, it can be done in pure python code so this pull doesn't contain any code for it

Is the flash big enough for this? How would a typical layout with mp and fs look like?

Depends on your esp32 module. Most I work with have 4mb flash, enought for two mp partitions with 1.7mb each and a 512kb fs partition. My folder struktur is two folders at / one named "factory" and one named "ota_0". My boot.py then moves in the folder named after the boot partition and executes the boot.py and main.py there. Like that i can update the fs too, using a downloaded tar.

@ThomasWaldmann
Copy link

Thanks for explaining. C&P that to some docs file?

@andynd
Copy link
Author

andynd commented Mar 25, 2018

Updated to current master, fixed indentation. I will try to get our internal ota updater written in python added to the docu, just have to get an ok for it first. What more is needed to merge this?

app_update was included multiple times
@andynd
Copy link
Author

andynd commented Aug 11, 2018

I did not yet have time to publish our python side for this, but are there any plans to merge this?

@Lisa999
Copy link

Lisa999 commented Aug 11, 2018

@andynd: can you fix the makefile error? It's the ULP which has been added to the makefile.
Some examples of use would be nice, like:
esp.partition_find_first(1,129,None)
esp.ota_get_boot_partition()

@andynd
Copy link
Author

andynd commented Aug 12, 2018

Merged upstream master. Any idea where to put the examples? I don't want to add yet another readme file like I had to for the ulp.

Andreas Valder added 3 commits August 12, 2018 14:39
esp.ota_get_running_partition()
esp.ota_get_next_update_partition(start_from=None)
@andynd
Copy link
Author

andynd commented Aug 12, 2018

I've added a README.ota.md on how to use this APIs. The code (in the README) is extracted from our product source code and some (irrelevant) parts removed. I have not tested it after I removed this parts but the production code is working so it should at most have some minor errors.

@Lisa999
Copy link

Lisa999 commented Aug 12, 2018

I've integrated it into my micropython tree, works just fine (after fixing the makefile merge)!
Thx for the good work, this is just what i needed!

Ps, you can't use a 512K fatvfs! The minimum for 4K sectors is 512K, but.... an additional 16K is being used for the WL (wear level). Here's what espressif said:

With 4k sector size, the smallest FAT partition size is 512K (128 sectors), plus 4 sectors used by WL, makes the minimum of 528K.

https://www.esp32.com/viewtopic.php?t=1897

@andynd
Copy link
Author

andynd commented Aug 12, 2018

Well the micropython implementation doesn't seem to have this limitation. Micropython doesn't use any of the esp-idf filesystem code.

@andynd
Copy link
Author

andynd commented Aug 16, 2018

Merged upstream master again. What is missing to finally merge this? The original PR is 7 months old.

@ThomasWaldmann
Copy link

@andynd guess it is the usual "many contributions, few reviewers/maintainers / little time" issue.

@@ -60,6 +60,8 @@ INC += -I$(BUILD)

INC_ESPCOMP += -I$(ESPCOMP)/bootloader_support/include
INC_ESPCOMP += -I$(ESPCOMP)/bootloader_support/include_priv
INC_ESPCOMP += -I$(ESPCOMP)/bootloader_support/include_bootloader
>>>>>>> upstream/master
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a rest of a merge conflict, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it was, thx for pointing it out. Funny how it still was able to built without errors. Fixed.

@andynd
Copy link
Author

andynd commented Aug 27, 2018

poke

@andynd
Copy link
Author

andynd commented Sep 5, 2018

more poke

@andynd
Copy link
Author

andynd commented Jan 29, 2019

Yes, because this pull request is over a year old. Unless somebody tells me he is going to merge it I will not again spend the time to fix it.

@ThomasWaldmann
Copy link

@dpgeorge ^^^

@Lisa999
Copy link

Lisa999 commented Mar 18, 2019

@dpgeorge: Wake up call...

@Lisa999
Copy link

Lisa999 commented Mar 18, 2019

@andynd : i've build my work on your pr, could you please fix the problem? I'll stalk @dpgeorge

@Lisa999
Copy link

Lisa999 commented Mar 26, 2019

@dpgeorge: what does it take to get this merged?

@Lisa999
Copy link

Lisa999 commented Apr 1, 2019

@dpgeorge ^^^

@Lisa999
Copy link

Lisa999 commented Apr 13, 2019

@dpgeorge : as long as this doesn’t getting merged, i won’t create the PR requested in #4436 ...
What’s the point in making PR’s if they are open more then a year?

@ThomasWaldmann
Copy link

I am watching this since quite a while and I think this project needs more core developers who are trusted enough to review and merge PRs into this project.

I don't have personal interest to become a core dev (I don't like coding in C and also do not use micropython that much, I like micropython, but basically just played with it yet), but I know there are people who would qualify (just look at the PRs and forks).

Also, considering that revision control makes it easy to also revert stuff in case something is found to be problematic, maybe PRs should be accepted more easily. New stuff can be marked experimental for a while, without any guarantees (not even for staying).

Open a ticket about this?

@Lisa999
Copy link

Lisa999 commented Apr 14, 2019

@ThomasWaldmann: I suspect that @dpgeorge wants to be in control and therefor doesn’t want to delegate the PR merge rights. Maybe that’s because of the breakup with pfalcon.
I don’t have the rights, so i can’t do it myself. My company is happy to pay to get this, and other PR’s, merged...
There are already three PR’s i need to get merged, so the question is who can do the merge and how much is that going to cost (in donation, contract work or whatever).

@spacemanspiff2007
Copy link
Contributor

@Lisa999: The lack of progress really is super frustrating and it's a shame that pfalcon is not working at this repo any more. Maybe you can team up with pfalcon (or someone else) on his repo or find a fork where there are more activity (maybe LoBoris)?
I'd really like to see the project advancing so it would be nice to have people working together.

@dpgeorge
Copy link
Member

Sorry all for neglecting this. I'll try to make some progress here.

I think it's a good feature to have, so thanks @andynd for contributing it. But I think the way it's done, the way it is exposed at the Python level, needs to be changed.

The reason is because it looks like the approach here cannot support encrypted partitions (but please correct me if I'm wrong). That's because encrypted read/write must go through esp_partition_read/esp_partition_write (again, correct me if I'm wrong about this). If in the future encryption support was added to uPy we don't want to have to change everything, so it's good to think now how encryption would fit in.

As such my proposal would be to make the API more object-based, rather than pure function calls. In particular put emphasis on the concept of a Partition object. A Partition object would:

  • contain a pointer (at the C level) to a esp_partition_t struct
  • contain methods to retrieve the metadata (type, subtype, address, size, etc)
  • contain methods to read/write/erase the partition

Partition objects would be created by esp_partition_find_first(), esp_ota_get_boot_partition() and esp_ota_get_running_partition(), and be passed to esp_ota_set_boot_partition() and esp_ota_get_next_update_partition().

For example the API could be:

class Partition:
    @staticmethod
    def find_first(type, subtype, label) -> Partition: ...

    def metadata(self) -> (type, subtype, address, size, label, encrypted): ...

    def readinto(self, offset, buf): ....
    def write(self, offset, buf): ....
    def erase(self, offset, size): ....

def ota_get_boot_partition() -> Partition: ...
def ota_get_running_partition() -> Partition: ...
def ota_set_boot_partition(part): ...
def ota_get_next_update_partition(part) -> Partition: ...

By having a Partition object you don't need to worry about passing the correct offsets to esp.write_flash etc. And Partition could in the future support the block protocol allowing it to have filesystems on it and be mounted, etc (so it'd replace the FlashBdev class).

import esp
import sys

def bytecompare(a,b):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this function necessary, isn't it enough to just do a == b? Or do I miss something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not, (a == b) can be = false while bytecompare(a,b) returns true, because its compare by referenz and compare by value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example where it's not the same? MicroPython compares by value, eg:

>>> bytearray(b'\x01\x02\x03')==b'\x01\x02\x03'
True
>>> bytearray(b'\x01\x02\x03')==bytearray(b'\x01\x02\x03')
True
>>> b'\x01\x02\x03'==bytearray(b'\x01\x02\x03')
True

@dpgeorge
Copy link
Member

dpgeorge commented Jul 9, 2019

See #4910 for a proposed Partition class.

@Lisa999
Copy link

Lisa999 commented Jul 19, 2019

@andynd: #4910 partitions support is great, is this PR still needed?

@waneksx
Copy link

waneksx commented Oct 17, 2019

Is it posible to resolve those conflicts?

@Lisa999
Copy link

Lisa999 commented Oct 17, 2019

Is it posible to resolve those conflicts?

#4910 is integrated into the master branch, #3576 is obsolete now...

@mrwhy-orig
Copy link

Where are we on this one? I got a bit lost with all those cross references... Is there documentation on how to make OTA possible?


debug()
run(host='0.0.0.0', port=8000)
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python code style looks bad, isn't it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very small example server, it's fine for that but obviosly nobody should use that in production.

@tve
Copy link
Contributor

tve commented Jan 23, 2020

Why is this PR still open? Is it hoping for someone to turn pieces of the README.ota.md into docs?

@mrwhy-orig
Copy link

My guess this README.ota.md is outdated. @dpgeorge implemented the Partition Class which serves as a replacement for another Class which is used in this pull request.
I think there is no Updated README.ota.md yet.

@@ -0,0 +1,405 @@
# Over the air updates

This is an example of how to do ota-updates using micropython and a very simple bottle server application. It assumes you have some way of notifying your esp of updates and transmit the update hashe securely (for example using ssl with certificate pinning).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "hashe"

how about cryptographically signed updates rather? the esp-idf / esp32 support ecdsa signatures and can verify that on update (flashing) / on boot (kind of "secure boot"). they even support encrypted flash images, but there is no requirement to use encryption if you just want to have signatures.

that way, the security of the server having the update files and that of the transmission channel does not matter.
if the signature is ok, you know the file is authentic, complete, correct (as released / signed by the author).
at least as long as you manage to keep your private signing key private.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. As long as there are no secrets or otherwise sensitive info embedded in the MP firmware being updated it should be fine to verify a hash over the firmware assuming the hash got transmitted securely. A signature is more flexible but much more of a hassle in many use-cases.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has nothing to do with whether it has secrets inside, but whether you can authenticate the image. you can't authenticate with just a hash from same source.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's save as long as you have a secure way to transfer this hash. We did not want signed updates because the signature verification was too complex and it was not needed for our usecase. We use certificate pinning and would have gained nothing except protection against a attacker controlled server, but that was not our attack scenario.


def bytecompare(a,b):
if (len(a) != len(b)):
return False

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabs?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabs > spaces.

def trackerprobe():
if do_update:
res['update'] = {}
res['update']['partition'] = {'url': '{}/chunk/part'.format(otahost), 'size': int(os.stat(partfile).st_size), 'hash': hashlib.sha256(open(partfile, 'rb').read()).hexdigest()}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem with this is that if somebody hacks your server and puts some tampered update binaries on it (instead the original good ones), the code will compute the hash for the tampered ones and the updater code on the esp32 will happily flash the bad update.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see above how to avoid this)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was not the attack vector we cared about. We wanted verified updates without confidentiality, thats why no encryption.

Copy link
Contributor

@tve tve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very interested in OTA (or rather, will be when I'm through with my current TODO list :-). But I'm not understanding what this PR is proposing. It's great to have a long readme, but I would prefer to first have a concise statement about how OTA is supposed to work and which use-cases it covers.

For example, it seems this PR intends the entire MP filesystem to be swapped with the OTA, as opposed to separating the filesystem from the MP firmware. Also, it seems this PR intends there to be 3 slots for firmware: factory, ota1, and ota2. Yet only 2 are really required, the factory one is optional (I wouldn't want it, for example).

I have used the Arduino Update class on the esp32 successfully implementing my own update and thought it abstracted the minutiae nicely without adding too many constraints. It would be worth taking a look at what it does: https://github.com/espressif/arduino-esp32/blob/master/libraries/Update/src/Updater.cpp

STATIC mp_obj_t mp_esp_ota_get_boot_partition (void) {
const esp_partition_t* part = esp_ota_get_boot_partition();
void* partptr = ∂
return mp_obj_new_bytes(partptr, sizeof(partptr));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this returning in Python-speak? An opaque bunch of bytes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The pointer as bytes.

@@ -5,6 +5,7 @@
try:
if bdev:
uos.mount(bdev, '/')
print("Mounted /")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

nvs, data, nvs, 0x9000, 0x6000,
phy_init, data, phy, 0xf000, 0x1000,
factory, app, factory, 0x10000, 0x180000,
nvs, data, nvs, 0x9000, 0x4000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gratuitous whitespace change?
Why the size reduction?


## Folders

We are using the following folder structure:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the host or on the device?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

device

└─ ota_1
```

The folders "factory", "ota_o" and "ota_1" musst have corresponding partitions. "VERSION" contains our currently running software version identifier, for example a git commit hash.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 3 partitions, i.e. copies of the firmware? Or am I misunderstanding?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the product where this code is from, we used "factory" with the factory image that never got updated and was the fallback code for some specific scenarios. You can use only two partitions as well.

@andynd
Copy link
Author

andynd commented Jan 26, 2020

In general: please understand that this code was written for a specific product with specific tradeoffs and making it open source was just an afterthought. Now that there is a Partition class, you can write some parts different and in a nicer way.

Please understand as well that we wanted secure (as in someone who controlles the network can not tamper with them) and not confidential (an attacker can read firmware images, they are not secret and we do not care if someone gets them). An attacker who attacks our server and steals the SSL certificate or replaces the binary was not our concern. For this scenario it is fine to transfere a hash of the update over a ssl protected connection with certificate pinning and not to do signature verification and to transmit the update chunks unencrypted.

@andynd
Copy link
Author

andynd commented Feb 22, 2020

#4910 and #5027 implemented all changes I did here in a better way, closing this PR.

@andynd andynd closed this Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet