Skip to content

Conversation

rjwats
Copy link
Owner

@rjwats rjwats commented Jun 28, 2020

Hey @kasedy,

Thx for this, nice little change. The templates weren't working for me as PlatformIO was expanding them to empty strings before handing them off to the build process. I couldn't even get it working by escaping the dollars - strange. Removing the dollar from our placeholders trick though, look OK?

I opted to restore the settings ifndefs. IMO it's nice to have the source code build without any external defines needing to be provided (especially useful if you want to produce a project from the source code which won't be built with PIO).

Just finishing off testing this under various *nix configurations.

kasedy and others added 19 commits May 31, 2020 21:21
Header files contains duplicates of factory values defined in factory_settings.ini Removed them to simplify the code.
Header files contains duplicates of factory values defined in factory_settings.ini Removed them to simplify the code.
Updated documentation
Header files contains duplicates of factory values defined in factory_settings.ini Removed them to simplify the code.
 Use text formatting for default factory values to produce dynamic names.
clang formatting
use 2 spaces in ini files
use ${platform}-${chip_id} for hostname
put chip id in brackets in AP SSID
- this is so src can be built without an exaustive set build-time defines
- standardize ordering of defines: factory settings, paths, config
experiment with removing $'s from our format strings (they are being substituted with empty values by pio)
rename FactoryValue to SettingValue, put in own header
give example of direct usage of FactorySetting::format in README.md
@kasedy
Copy link
Contributor

kasedy commented Jun 28, 2020

We can replace dollar sign ($) with a hash (#) to avoid bash variable expansion.

@rjwats
Copy link
Owner Author

rjwats commented Jul 6, 2020

Updated to use #{value} seems to work

rjwats added 2 commits July 7, 2020 09:18
# Conflicts:
#	factory_settings.ini
#	lib/framework/APSettingsService.h
@rjwats
Copy link
Owner Author

rjwats commented Jul 7, 2020

Have been struggling with odd issues while testing this branch on one of my ESP8266 devices (a nodemcuv2 board).

It was exhibiting an unresponsive access point when erasing flash before uploading (effectively a factory reset). Oddly the culprit appears to be the use of flash string helpers and the following change solves the issue (my older nodemcuv0.9 board works fine without this change):

image

I'm not entirely sure why it solves the problem and would love to know if anyone is more enlightened.

@proddy
Copy link

proddy commented Jul 8, 2020

I'm facing a similar problem, esptool.py erase_flash -> upload firmware brings up an empty Captive Portal page on an ESP8266. I'll try the workaround.

@rjwats
Copy link
Owner Author

rjwats commented Jul 8, 2020 via email

@proddy
Copy link

proddy commented Jul 17, 2020

Haven't been able to reproduce it unfortunately. I'm back on master now.

@kasedy
Copy link
Contributor

kasedy commented Dec 30, 2020

Do we plan to merge this feature into main?

@rjwats
Copy link
Owner Author

rjwats commented Dec 30, 2020 via email

# Conflicts:
#	factory_settings.ini
#	lib/framework/WiFiSettingsService.h
@rjwats
Copy link
Owner Author

rjwats commented Dec 31, 2020

I couldn't re-create the issue I saw last time around so this is probs OK to merge. Have caught it up with master and applied the fix from before for good measure. Have run out of time tonight.. will review the code and merge this tomorrow

@kasedy
Copy link
Contributor

kasedy commented Dec 31, 2020

I also had this issue in the past. It is not related with the change. I believe the issue is in esp8266-Arduino SDK as it has lots of open tickets. For a long time I used ESPAsyncWiFiManager as it is the most issue free WiFi manager for ESP8266. It has many workarounds to make WiFi work properly. For example it resets wifi before connecting to it [1]. Also it disconnects wifi on reset [2]. I think we should do something similar. By the way I skimmed the other projects. JustWifi and Tasmota have a workaround for WiFi connectivity issue [3] and [4].

[1] https://github.com/alanswx/ESPAsyncWiFiManager/blob/master/ESPAsyncWiFiManager.cpp#L575
[2] https://github.com/alanswx/ESPAsyncWiFiManager/blob/master/ESPAsyncWiFiManager.cpp#L691
[3] https://github.com/xoseperez/justwifi/blob/master/src/JustWifi.cpp#L65
[4] https://github.com/arendst/Tasmota/blob/development/tasmota/support_wifi.ino#L121

@rjwats rjwats linked an issue Jan 1, 2021 that may be closed by this pull request
@rjwats
Copy link
Owner Author

rjwats commented Jan 1, 2021

Just need to replace "chipId" with "salt" to secure the JWT secret properly. It's a pretty significant security hole as it stands. I'm sure I can get PIO to generate a random value for a salt at build time... will look this eve.

Arduino uses the ESP random number generator for "true random" numbers on both esp32 and esp8266
This makes a better JWT secret and may be useful for other factory defaults too
In addition a modification has been made to force the FSPersistance to save the file if applying defaults
@rjwats
Copy link
Owner Author

rjwats commented Jan 2, 2021

I think this might be ready now, another user of the framework spotted an issue with using efuse/chipId for the hostname which this PR also resolves:

#212

In addition I have resolved a security issue, where the default JWT secret was previously derived from the station MAC address which may be sniffed from the WiFi network (not great if you care about security!)

Thankfully the Arduino random() function was adapted for these devices to use the built-in hardware true random number generator so it offers a good solution for the default JWT secret (with added bonus that it's random rather than derrived and is re-generated on factory reset). Nice.

@rjwats rjwats merged commit e771ab1 into master Jan 3, 2021
@rjwats rjwats deleted the ft-placeholders branch January 16, 2021 19:12
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.

Chip ID is not unique enough to be used as the secret or the hostname
3 participants