search

joeycastillo / Sensor-Watch

A board replacement for the classic Casio F-91W wristwatch
Other
1.02k stars 210 forks source link

totp_face_lfs - reduce memory usage and add secmod #393

Open wryun opened 2 months ago

wryun commented 2 months ago

This should fix #392

I've refactored this so it doesn't allocate the secrets on startup, but instead reads them from lfs each time. Not a fan of the filesystem API here... (would be so much more efficient if I had easy access to the underlying seek/read and could just leave the file open, but putting all that in this PR seemed like overkill).

I've also added a 'secure mode' which lets you remove from the secrets from the flash (see doco in header).

@madhogs - I haven't tried this on a real watch yet, so if you want to be a guinea pig that would be awesome :)

madhogs commented 2 months ago

Thanks for looking into this :smile: Tested on my physical watch and the normal mode works great :partying_face:, tested with lots of very long 128 character secrets and again with my real secrets and everything worked well. Codes produced were correct and no memory issues, I also didn't see any filesystem issues at all with this code (previously i had to reformat it each time anything went wrong).

The 'secure mode' however did not work as expected, even with just 2 short example codes when activated it would work whilst remaining on that face however when moving away from the totp_lfs face and back again the watch would reset, which would then load 'no codes' as the file has been deleted.

I hope you don't mind me saying as it is a nice idea but considering the memory restrictions on the physical watch do you think it is worth keeping this 'secure mode'? Even in the best case I imagine it could only handle a few secrets before the watch would run out of memory.

madhogs commented 2 months ago

I've been running the fix (I removed the secure mode code) on my physical watch for a week or so now and using the codes. All is working well, not had any issues :smile:

In the interest of getting this merged would you be willing to separate the secure mode changes from the fix? I would also be happy to do that with your name on the commit if you like.

wryun commented 2 months ago

Feel free to raise PRs with this code with or without my name; at the moment I don't feel like splitting them ;)

Were you worried about having secure mode? It should be pretty hard to get into by accident.

madhogs commented 2 months ago

I wasn't worried about activating it by accident, I just like to remove any features I'm not going to use on my own personal watch. For the PR i just thought it makes sense to separate the fix from the new feature, can then merge the fix first whilst any issues in secure mode can be worked out :smile: